jsinovassin opened a new pull request, #865: URL: https://github.com/apache/unomi/pull/865
Fixes UNOMI-983, at https://issues.apache.org/jira/browse/UNOMI-983 ### What this changes 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 failure is silent. Unomi logs no warning and no error, and a caller that reads the response back sees a saved item. ### Where the behaviour comes from `ElasticSearchPersistenceServiceImpl.getDocumentIDForItemType` prefixes the tenant: ```java private String getDocumentIDForItemType(String itemId, String itemType) { String tenantId = getTenantId(); String baseId = systemItems.contains(itemType) ? (itemId + "_" + itemType.toLowerCase()) : itemId; return tenantId + "_" + baseId; } ``` 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. `ProfileServiceImpl.save(Profile, boolean)` already refuses a null item id. The REST endpoint never reaches that guard, because it calls `saveOrMerge`, which carries no such check. ### Mechanism 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. Refusing the write is the answer rather than generating a UUID. A generated id restores the 3.0 outcome, and it hides a caller that never states the identity of what it writes. ### Effect Measured on Apache Unomi 3.1.0-SNAPSHOT. Before, two calls with no `itemId` both answered 200, and one document held the second item: ``` _id= default_null {"firstName": "Second", "lastName": "Item"} ``` Now: ``` POST /cxs/profiles (no itemId) -> 400 {"errorMessage":"A profile states its own identity, so the body must carry an itemId"} POST /cxs/profiles (with itemId) -> 200 ``` ### Tests `mvn test` on `persistence-spi`, `services`, `rest` and `persistence-elasticsearch/core` reports no failure. `ProfileServiceImplTest` gains two tests: `saveOrMerge` refuses a profile with no item id, and two such profiles cannot overwrite each other. ### Notes for the reviewer The guard sits on the save path only. `getDocumentIDForItemType` still accepts a null item id on the read path, where a null answers "not found" and destroys nothing. This pull request is independent of https://github.com/apache/unomi/pull/864, and the two branches touch no common file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
