This is an automated email from the ASF dual-hosted git repository. jsinovassin pushed a commit to branch UNOMI-983-refuse-item-without-itemid in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 5824a059719328edc2702d87799c4f0a9d290923 Author: jsinovassin <[email protected]> AuthorDate: Wed Sep 16 17:18:45 2026 +0200 UNOMI-983: Refuse to save an item that carries no itemId An item states its own identity. Apache Unomi 3.1 builds the Elasticsearch document id by prefixing the tenant, and it guards no null. An item with no itemId therefore lands on the document "<tenant>_null". Every id-less item of that type shares one document, and each save destroys the item the previous save wrote. The call answers 200 and returns the literal string "null" as the itemId. The same method on unomi-2.7.x and on unomi-3.0.x returns the item id unchanged, which left Elasticsearch to generate a distinct document id for each item. Three guards now refuse the write, one per layer that can reach it: - ProfileServiceEndPoint.save answers 400, because a body with no itemId cannot be served. - ProfileServiceImpl.saveOrMerge returns null, the rule save(Profile, boolean) already applied. There is nothing to look up and nothing to merge into without an item id. - ElasticSearchPersistenceServiceImpl.save and OpenSearchPersistenceServiceImpl.save answer false and log a warning, so no other caller can reach the shared document by another route. Measured on Apache Unomi 3.1.0-SNAPSHOT. A POST to /cxs/profiles with no itemId answered 200 and overwrote the profile the previous call created. It now answers 400 with an explanatory message, and a POST that carries an itemId still answers 200. ProfileServiceImplTest gains two tests: saveOrMerge refuses a profile with no item id, and two such profiles cannot overwrite each other. --- .../ElasticSearchPersistenceServiceImpl.java | 8 ++++++ .../OpenSearchPersistenceServiceImpl.java | 8 ++++++ .../rest/endpoints/ProfileServiceEndPoint.java | 3 ++ .../services/impl/profiles/ProfileServiceImpl.java | 5 ++++ .../impl/profiles/ProfileServiceImplTest.java | 33 ++++++++++++++++++++++ 5 files changed, 57 insertions(+) diff --git a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java index 69ca79dc9..139b87b92 100644 --- a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java +++ b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java @@ -931,6 +931,14 @@ public class ElasticSearchPersistenceServiceImpl implements PersistenceService, } @Override public boolean save(final Item item, final Boolean useBatchingOption, final Boolean alwaysOverwriteOption) { + // An item states its own identity. Without an itemId the document id below would read + // "<tenant>_null", so every id-less item of the same type would land on one shared document + // and overwrite the item the previous save wrote. Refuse the save and say so. + if (item.getItemId() == null) { + LOGGER.warn("Refusing to save an item of type {} that carries no itemId", item.getItemType()); + return false; + } + String finalTenantId = validateTenantAndGetId(SecurityServiceConfiguration.PERMISSION_SAVE); item.setTenantId(finalTenantId); diff --git a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java index 823c33d3f..c4221ec4d 100644 --- a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java +++ b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/OpenSearchPersistenceServiceImpl.java @@ -854,6 +854,14 @@ public class OpenSearchPersistenceServiceImpl implements PersistenceService, Syn @Override public boolean save(final Item item, final Boolean useBatchingOption, final Boolean alwaysOverwriteOption) { + // An item states its own identity. Without an itemId the document id below would read + // "<tenant>_null", so every id-less item of the same type would land on one shared document + // and overwrite the item the previous save wrote. Refuse the save and say so. + if (item.getItemId() == null) { + LOGGER.warn("Refusing to save an item of type {} that carries no itemId", item.getItemType()); + return false; + } + String finalTenantId = validateTenantAndGetId(SecurityServiceConfiguration.PERMISSION_SAVE); item.setTenantId(finalTenantId); diff --git a/rest/src/main/java/org/apache/unomi/rest/endpoints/ProfileServiceEndPoint.java b/rest/src/main/java/org/apache/unomi/rest/endpoints/ProfileServiceEndPoint.java index 3c43745f9..305a7bb55 100644 --- a/rest/src/main/java/org/apache/unomi/rest/endpoints/ProfileServiceEndPoint.java +++ b/rest/src/main/java/org/apache/unomi/rest/endpoints/ProfileServiceEndPoint.java @@ -249,6 +249,9 @@ public class ProfileServiceEndPoint { @POST @Path("/") public Profile save(Profile profile) { + if (profile == null || profile.getItemId() == null) { + throw new BadRequestException("A profile states its own identity, so the body must carry an itemId"); + } Profile savedProfile = profileService.saveOrMerge(profile); if (savedProfile != null) { Event profileUpdated = new Event("profileUpdated", null, savedProfile, null, null, savedProfile, new Date()); diff --git a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java index f24636ca9..8bb24622a 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java @@ -677,6 +677,11 @@ public class ProfileServiceImpl extends AbstractMultiTypeCachingService implemen } public Profile saveOrMerge(Profile profile) { + // Same rule as save(Profile, boolean): a profile states its own identity, and there is + // nothing to look up or to merge into without it. + if (profile == null || profile.getItemId() == null) { + return null; + } Profile previousProfile = persistenceService.load(profile.getItemId(), Profile.class); profile.setSystemProperty("lastUpdated", new Date()); if (previousProfile == null) { diff --git a/services/src/test/java/org/apache/unomi/services/impl/profiles/ProfileServiceImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/profiles/ProfileServiceImplTest.java index 2df2cd516..32280b264 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/profiles/ProfileServiceImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/profiles/ProfileServiceImplTest.java @@ -698,4 +698,37 @@ public class ProfileServiceImplTest { }); } + // A profile states its own identity. Without an itemId there is nothing to look up and nothing + // to merge into, and the persistence layer would write every such profile to one shared + // document, so the second call would destroy what the first one wrote. + @Test + public void testSaveOrMergeRefusesAProfileWithNoItemId() { + executionContextManager.executeAsTenant(TENANT_1, () -> { + Profile profile = new Profile(); + profile.setProperty("firstName", "Ada"); + + assertNull(profileService.saveOrMerge(profile), "saveOrMerge must refuse a profile that carries no itemId"); + return null; + }); + } + + // Two profiles with no itemId must not end up on the same document. + @Test + public void testSaveOrMergeKeepsTwoIdLessProfilesApart() { + executionContextManager.executeAsTenant(TENANT_1, () -> { + Profile first = new Profile(); + first.setProperty("firstName", "First"); + Profile second = new Profile(); + second.setProperty("firstName", "Second"); + + profileService.saveOrMerge(first); + profileService.saveOrMerge(second); + + Query query = new Query(); + assertEquals(0L, profileService.search(query, Profile.class).getTotalSize(), + "Neither profile is stored, so no profile can overwrite the other"); + return null; + }); + } + }
