This is an automated email from the ASF dual-hosted git repository.
sergehuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/master by this push:
new 1991f3503 UNOMI-983: Refuse to save an item that carries no itemId
(#865)
1991f3503 is described below
commit 1991f3503f2c723509e85fe88cf670bb51b3c942
Author: Jonathan SINOVASSIN-NAIK <[email protected]>
AuthorDate: Mon Sep 28 16:29:41 2026 +0200
UNOMI-983: Refuse to save an item that carries no itemId (#865)
Co-authored-by: Serge Huber <[email protected]>
---
.../org/apache/unomi/itests/ProfileServiceIT.java | 19 ++++---------
.../ElasticSearchPersistenceServiceImpl.java | 8 ++++++
.../OpenSearchPersistenceServiceImpl.java | 8 ++++++
.../rest/endpoints/ProfileServiceEndPoint.java | 3 ++
.../services/impl/profiles/ProfileServiceImpl.java | 5 ++++
.../impl/profiles/ProfileServiceImplTest.java | 33 ++++++++++++++++++++++
6 files changed, 63 insertions(+), 13 deletions(-)
diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileServiceIT.java
b/itests/src/test/java/org/apache/unomi/itests/ProfileServiceIT.java
index e256c7004..3f9473236 100644
--- a/itests/src/test/java/org/apache/unomi/itests/ProfileServiceIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/ProfileServiceIT.java
@@ -21,7 +21,6 @@ import org.apache.unomi.api.*;
import org.apache.unomi.api.conditions.Condition;
import org.apache.unomi.api.query.Query;
import org.junit.After;
-import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -89,20 +88,14 @@ public class ProfileServiceIT extends BaseIT {
public void testProfileWithoutItemId() throws Exception {
Profile profile = new Profile();
profile.setProperty("name", "testProfileWithoutItemId");
- profileService.saveOrMerge(profile);
- Profile testProfile = keepTrying("Profile not found in the required
time", () -> profileService.findProfilesByPropertyValue("properties.name",
"testProfileWithoutItemId", 0, 10, null), Objects::nonNull,
- DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES).get(0);
- LOGGER.info("Ensure an itemId as been set...");
- Assert.assertNotNull(testProfile.getItemId());
+ assertNull("saveOrMerge must refuse a profile that carries no itemId",
profileService.saveOrMerge(profile));
- LOGGER.info("Delete profile...");
- profileService.delete(testProfile.getItemId(), false);
-
- waitForNullValue("Profile still present after deletion", () ->
profileService.load(testProfile.getItemId()), DEFAULT_TRYING_TIMEOUT,
- DEFAULT_TRYING_TRIES);
-
- LOGGER.info("Profile deleted successfully.");
+ // Stay empty for the same window a successful save would have become
visible in.
+ shouldBeTrueUntilEnd("A profile with no itemId must not be stored",
+ () ->
profileService.findProfilesByPropertyValue("properties.name",
"testProfileWithoutItemId", 0, 10, null),
+ found -> found != null && found.getTotalSize() == 0 &&
found.getList() != null && found.getList().isEmpty(),
+ DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES);
}
@Test
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;
+ });
+ }
+
}
