fix(admin): 409 (not 500) on duplicate menu / group code#27
Merged
Conversation
Consistency gap found by auditing every create endpoint after the
menu duplicate surfaced during integration testing.
GlobalExceptionHandler maps IllegalStateException -> 409 CONFLICT,
and User / Role / Permission / Tenant services all do a
findBy...isPresent() pre-check and throw it on a duplicate. But
MenuAdminService.create and GroupService.create had no such check —
they went straight to repository.save(), hit the
uq_platform_{menu,group}_tenant_code unique constraint, and bubbled
a DataIntegrityViolationException that no handler caught, producing
a 500 instead of a clean 409.
Both tables already declare the unique constraint, so this only adds
the friendly pre-check that the other four resources already had:
- JpaPlatformGroupRepository / JpaPlatformMenuRepository:
new findByTenantIdAndCode(tenantId, code).
- GroupService.create / MenuAdminService.create:
throw IllegalStateException("... already exists ...") when a row
with the same (tenantId, code) exists.
Verified end-to-end against the running sample-app:
POST /menus {code: dup.menu.test} -> 201 then 409
POST /groups {code: dup-grp} -> 201 then 409
POST /roles {code: PLATFORM_ADMIN}-> 409 (unchanged, sanity)
Note: there's still a small race between the pre-check and the
insert under concurrent creates — the DB constraint is the real
guard and would 500 in that rare window. Making that path also
return 409 would mean catching DataIntegrityViolationException in
GlobalExceptionHandler; left out here to keep the change minimal
and consistent with the existing four services, which have the
same theoretical race.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consistency gap found by auditing every create endpoint after the menu duplicate surfaced during integration testing — you asked me to check whether other resources had the same problem, and two did.
The gap
GlobalExceptionHandlermapsIllegalStateException → 409 CONFLICT, and User / Role / Permission / Tenant services all do afindBy…isPresent()pre-check and throw it on a duplicate. ButMenuAdminService.createandGroupService.createhad no such check — they went straight torepository.save(), hit theuq_platform_{menu,group}_tenant_codeunique constraint, and bubbled aDataIntegrityViolationExceptionthat no handler caught → 500 instead of 409.The fix
Both tables already declare the unique constraint; this just adds the friendly pre-check the other four resources already had:
JpaPlatformGroupRepository/JpaPlatformMenuRepository: newfindByTenantIdAndCode(tenantId, code).GroupService.create/MenuAdminService.create: throwIllegalStateException("… already exists …")when a row with the same(tenantId, code)exists.Verified end-to-end (running sample-app)
Known limitation (documented, not fixed)
There's still a narrow race between the pre-check and the insert under concurrent creates — the DB constraint is the real guard and would 500 in that rare window. Catching
DataIntegrityViolationExceptioninGlobalExceptionHandlerwould close it, but I left that out to keep the change minimal and consistent with the existing four services, which share the same theoretical race. Worth a follow-up if you want belt-and-suspenders.Context
5th item from integration verification. The full audit of create endpoints confirmed only menu + group were affected; the not-found path (
IllegalArgumentException → 400) is uniform across all resources and was left as-is.