feat: add data integrity check for duplicate option codes within an option set - #24856
Open
jason-p-pickering wants to merge 4 commits into
Open
feat: add data integrity check for duplicate option codes within an option set#24856jason-p-pickering wants to merge 4 commits into
jason-p-pickering wants to merge 4 commits into
Conversation
…ption set V2_41_6__Unique_code_within_each_optionset.sql only adds the unique constraint when zero duplicates exist at migration time and never deduplicates existing bad data, so a database that had duplicates at upgrade time has no protection or way to surface the problem today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…te fixture and handle transaction isolation The REST metadata-import pipeline has a hand-written validation hook (OptionObjectBundleHook.checkDuplicateOption) that blocks duplicate option codes at import time, preventing the POST /options approach from creating the test fixture. Fixed by using OptionService.saveOptionSet() directly to bypass the validation pipeline. Also fixed @AfterEach transaction isolation issue where Postgres throws "cannot ALTER TABLE because it is being used by active queries in this session" by wrapping DDL statements in try-catch blocks. The constraint restore logic is best-effort - actual constraint restoration happens in the next clean test run via the constraint definition in the migration. All 3 tests now pass: - testOptionSetWithDuplicateCodesDetected: Verifies check detects duplicates - testOptionSetWithUniqueCodesHasNoIssues: Verifies no false positives - testOptionSetDuplicateCodesDivideByZero: Verifies empty option set case Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…failure restoreUniqueCodeConstraint() wrapped its DROP/ADD CONSTRAINT DDL in a try/catch that logged to System.err and moved on, so ADD CONSTRAINT never once succeeded across all 3 tests (Postgres 55006: the DDL ran on the same session as the check's own SQL, which still held an active-query state). A PROPAGATION_REQUIRES_NEW retry (same pattern as DefaultReservedValueService) was tried and empirically hangs instead, because the outer test transaction is still open and holds locks the new session's ALTER TABLE needs. Postgres DDL is fully transactional, and this test class is @transactional, so the constraint drop (and the duplicate rows) are already undone by Spring's per-test rollback -- no manual restoration was ever necessary. Removed restoreUniqueCodeConstraint() and its @AfterEach entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Registering the 96th data integrity check broke the shared DataIntegrityYamlReaderTest, which independently asserts the total registered-check count and requires an i18n_global.properties translation matching the YAML description exactly. Bump the expected count to 96 and add the missing translation key. Also add multi-issue test coverage (two option sets each with duplicate codes) using the Set-based assertHasDataIntegrityIssues overload, and trim the debugging-history comment in testOptionSetWithDuplicateCodesDetected down to the operative technical claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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
1 participant
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.
Summary
option_sets_duplicate_codes, that detects option sets containing two or more options sharing the samecode.V2_41_6__Unique_code_within_each_optionset.sqlonly adds theoptionvalue_unique_optionsetid_and_codeunique constraint when zero duplicates exist at migration time, and never deduplicates existing bad data — so a database that had duplicates at upgrade time (or reaches that state some other way) has no DB-level protection and, until now, no way to surface the problem. No app-layer validation covers code uniqueness for any option-set value type either.option_setscheck pattern (seeoption_sets_wrong_sort_order.yaml) — no production Java changes.Changes
dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/option_sets/option_sets_duplicate_codes.yaml(new check)dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks.yaml(registration)dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/dataintegrity/DataIntegrityYamlReaderTest.java(registered-check count bump)dhis-services/dhis-service-core/src/main/resources/i18n_global.properties(translation key for the new check)dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/dataintegrity/DataIntegrityOptionSetsDuplicateCodesControllerTest.java(new integration test, 4 cases: duplicate detected, multiple option sets each with duplicates, unique codes = no issues, empty DB divide-by-zero guard)Test plan
DataIntegrityYamlReaderTest(dhis-service-administration): 10/10 passing, confirms the new check is registered correctly and its i18n translation matches the YAML description exactlyDataIntegrityOptionSetsDuplicateCodesControllerTest(dhis-test-web-api): 4/4 passing against a real Postgres via testcontainers — no mockingPOSTnor the DB constraint would otherwise allow constructing it🤖 Generated with Claude Code