fix(db): add CHECK constraint for userId XOR guestToken invariant (#23) #26
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/order-user-guest-check-constraint"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
Issue #23: The Order Javadoc states "Either userId or guestToken is set; never both, never neither" but only the
@PrePersistlifecycle callback enforced this in Java. A stray INSERT (admin tooling, manual SQL) could violate it silently.Changes
CHECK ((user_id IS NULL) <> (guest_token IS NULL))on theorderstableTest plan
./gradlew :backend:test— BUILD SUCCESSFULCloses #23
The Order entity Javadoc states "Either userId or guestToken is set; never both, never neither" but only the @PrePersist lifecycle callback enforced this in Java. A stray INSERT (admin tooling, manual SQL) could violate it silently — creating an order with neither set (invisible to both JWT and guest lookup paths) or both set (ambiguous ownership). Changes: - V13 Flyway migration adds a CHECK constraint on the orders table: CHECK ((user_id IS NULL) <> (guest_token IS NULL)) This evaluates TRUE when exactly one column is NULL (the other is set), and FALSE when both are NULL or both are set. - Standard SQL CHECK constraint, supported by both H2 (tests/dev) and PostgreSQL (prod). Verified: - Flyway migration check passes (V13 is next available version) - ./gradlew :backend:test — BUILD SUCCESSFUL (all tests pass) - H2 accepts the constraint at Flyway migration time Closes #23Verdict: Looks good — clean, correct, well-documented migration. Ship it.
Verified against the codebase before commenting:
user_id/guest_tokenmatch theOrderentity@Columnmappings (Order.java:29 and :75).masteris V12 (V12__add_guest_order_columns.sql, which addedguest_token), so V13 is the next free slot — consistent with the repo'snext-flyway-version.shconvention.guest_tokenis aUUIDcolumn (from V12), so there is no empty-string-vs-NULL edge case to worry about — theIS NULLcheck is sufficient.Critical
None.
Warnings
None.
Suggestions
:backend:testis green, but nothing asserts the constraint actually rejects invalid rows. A small@DataJpaTest(or raw-JDBC test against H2) verifying that persisting an Order with bothuserIdandguestTokenset — and one with neither set — throws aDataIntegrityViolationExceptionwould lock the invariant in.OrderServiceTest/GuestOrderControllerTestcover service/controller paths but not the constraint itself. The highest-value assertion is the both-set rejection, since@PrePersistalready handles neither-set at the app layer. (See inline comment.)Looks Good
(user_id IS NULL) <> (guest_token IS NULL):IS NULLalways yields a non-NULL boolean, so<>can never hit SQL three-valued-logic —TRUE<>FALSE/FALSE<>TRUEallow exactly-one-set,FALSE<>FALSE(both null) andTRUE<>TRUE(both set) reject. Truth table in the comment is accurate.Order.onCreate()(the@PrePersist, Order.java:85-87) only guards the neither-set case — it auto-assigns aguestTokenwhen both are null but does not reject the both-set case. This constraint closes that gap, so the PR body is, if anything, understating its value.user_idset,guest_tokenNULL) and guest orders (guest_tokenset,user_idNULL) both satisfy the constraint, so theALTER TABLEwill not fail on existing rows.Advisory review only — no changes required to merge.
💡 Suggestion (non-blocking): worth a
@DataJpaTestthat asserts this constraint rejects invalid rows — persist an Order with bothuserIdandguestTokenset, expecting aDataIntegrityViolationExceptionreferencingchk_user_or_guest. The@PrePersistinOrder.onCreate()already prevents the neither-set case at the application layer, so the both-set rejection is the untested, highest-value path.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.