You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Check for other consumers of the old error strings
Review test coverage
Post inline comments
Good change overall. Leaking ${e.cause?.cause} into a user-facing 422 was the actual bug in #7106, and routing everything through one parser fixes submission, revision and the FASTA path together. Two details I specifically checked and can confirm are correct:
The regex's (upload_id, X) shape holds for all three constraints that can fire here — V1__init.sql:144, V1.16__unique_loculus_accession_for_revisions.sql:3, V1.22__refactor_aux_tables.sql:20.
Postgres does not quote or escape values in the Key (…)=(…) detail, so the [^,]+ / greedy (.+) split is the right way to handle IDs containing commas and parens. The SAMPLE,C(1) test cases pin that well — nice.
No website, e2e or docs code matches on the old message strings, so the wording change is safe.
Four inline comments, none blocking:
Where
Point
UploadDatabaseService.kt:117-124
takeUnless drops the stack trace on unparseable unique violations — the one case that needs it. e.message is gone from the log too.
DuplicateRecordId.kt:9-13
Regex depends on English lc_messages and on column ordering; both degrade silently and neither degraded path is tested. A DB-free unit test over the parser would pin it.
DuplicateRecordId.kt:16-21
SQLException.iterator() already walks the cause chain, so the outer generateSequence { it.cause } is redundant.
SubmitModel.kt:340-343
Both call sites discard DuplicateRecordId.field and hardcode the label; the ": $it" suffix is duplicated.
One note on the changed error semantics that is worth a conscious decision rather than a code change: the revision path reports one duplicate (whichever constraint Postgres hits first), where the removed extractDuplicateColumns built a combined message across accession and submission_id. In practice only one constraint fires per statement so this isn't a regression, but a file with both kinds of duplicate now needs two round trips to surface both. The "at least one" wording already sets that expectation, so I'd leave it.
I did not run the test suite — the diff is message-shape only and the assertions are exact-match, so the local ./gradlew test ktlintCheck run reported in the description covers it.
· branch fix/7106-duplicate-upload-id-errors
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
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.
Shows duplicate IDs instead of raw Postgres errors, using one helper for submission and revision.
Closes #7106
PR Checklist
All necessary documentation has been adapted../gradlew test ktlintChecklocally, 638 tests passed and 1 skipped🚀 Preview: Add
previewlabel to enable