sergehuber commented on PR #865: URL: https://github.com/apache/unomi/pull/865#issuecomment-5732311346
Thank you for catching this, Jonathan — this is a real 3.1 footgun and I am glad you tracked it down. Refusing a missing `itemId` is the right call: persistence now prefixes `<tenant>_`, so a null id becomes `default_null` and later saves silently overwrite each other. Rejecting the write is much better than minting a UUID, which would change the old “server assigns an id” behaviour in a different way. The persistence unit tests you added are also very welcome. The only merge blocker I can see is that the existing integration test still describes the old behaviour. ES and OS ITs fail on `ProfileServiceIT.testProfileWithoutItemId`: it saves a `Profile` with no `itemId`, then does `.get(0)` on an empty search result (`IndexOutOfBoundsException` at line 95). A small update to that test should be enough — for example, after `saveOrMerge`, assert that no profile with that name is found (or that `save` / `saveOrMerge` returns `null` / does not persist), instead of waiting for a row and reading `.get(0)`. A non-blocking question, only if you want to keep thinking about it in this PR: REST 400 is currently on `POST /cxs/profiles` only, and other item types rely on persistence `save` returning `false`. If you would rather keep this PR focused on the persistence guard plus the profile endpoint, that is completely fine; Session / Persona 400s can be a follow-up. Thank you again for the careful diagnosis. Happy to re-review quickly once the IT is updated — and thank you for contributing this. -- 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]
