diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks.yaml b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks.yaml index b9daa00ecfef..434387c465ce 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks.yaml +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks.yaml @@ -44,6 +44,7 @@ checks: - programs/tracker_geometry_invalid_srid.yaml - programs/program_stages_no_programs.yaml - programs/single_events_null_occurred_date.yaml + - programs/tracked_entity_attribute_null_value.yaml - programs/tracker_associate_is_deprecated.yaml - programs/programs_inconsistent_tracked_entity_type.yaml - program_indicators/program_indicators_without_expression.yaml diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/single_events_null_occurred_date.yaml b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/single_events_null_occurred_date.yaml index 0d21e7392daf..d141292a5f93 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/single_events_null_occurred_date.yaml +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/single_events_null_occurred_date.yaml @@ -28,7 +28,7 @@ name: single_events_null_occurred_date description: Single events without an occurred date section: Programs -section_order: 3 +section_order: 6 summary_sql: >- WITH rs as ( SELECT e.uid diff --git a/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/tracked_entity_attribute_null_value.yaml b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/tracked_entity_attribute_null_value.yaml new file mode 100644 index 000000000000..7b143e68147e --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-administration/src/main/resources/data-integrity-checks/programs/tracked_entity_attribute_null_value.yaml @@ -0,0 +1,102 @@ +# Copyright (c) 2004-2025, University of Oslo +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# +# Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# Neither the name of the HISP project nor the names of its contributors may +# be used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR +# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON +# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# +--- +name: tracked_entity_attribute_null_value +description: Tracked entity attribute value with no plain text value +section: Programs +section_order: 7 +summary_sql: >- + WITH rs as ( + SELECT teav.trackedentityid, teav.trackedentityattributeid + FROM trackedentityattributevalue teav + WHERE teav.value IS NULL OR teav.value = '' + ) + SELECT COUNT(*) as value, + 100.0 * COUNT(*) / NULLIF( (SELECT COUNT(*) from trackedentityattributevalue),0) as percent + from rs; +details_sql: >- + SELECT + te.uid as uid, + te.uid || ' / ' || tea.name as name, + CASE + WHEN teav.encryptedvalue IS NOT NULL + THEN 'Attribute "' || tea.name || '" (' || tea.uid || + ') has an encrypted value but no plain text value' + ELSE 'Attribute "' || tea.name || '" (' || tea.uid || + ') has no value' + END AS comment + FROM trackedentityattributevalue teav + INNER JOIN trackedentity te ON te.trackedentityid = teav.trackedentityid + INNER JOIN trackedentityattribute tea ON tea.trackedentityattributeid = teav.trackedentityattributeid + WHERE teav.value IS NULL OR teav.value = ''; +severity: SEVERE +introduction: > + A tracked entity attribute value should always have a plain text value stored in the `value` column. + Some values only have an `encryptedvalue` and an empty or null `value`. This happened for attributes + that were marked as confidential, whose values were stored encrypted only. + + Support for marking tracked entity attributes as confidential will be removed in version 2.44, and + such attributes are automatically migrated to `skipAnalytics = true`. That migration cannot recover the + plain text of values that were stored encrypted only, so any tracked entity attribute value that has an + encrypted value but no plain text value must be resolved before upgrading. This check must pass in order + to be able to upgrade to v44 and later. +details_id_type: tracker +recommendation: >- + For each flagged tracked entity attribute value you need to either restore a plain text value or delete + the value. How you proceed depends on whether an encrypted value is present (see the comment for each row). + + Case 1 - the value has an encrypted value but no plain text value (previously confidential): + The plain text cannot be recovered by DHIS2 itself. You have two options. + + Option A - recover the plain text value. This requires access to the encryption key that was configured + in `dhis.conf` (the `encryption.password` property) at the time the value was stored, and decrypting the + `encryptedvalue` outside of DHIS2. If you need the historical values preserved, please reach out on the + DHIS2 Community of Practice forum at https://community.dhis2.org for assistance before upgrading. + + Option B - delete the encrypted-only values if you do not need them: + + DELETE FROM trackedentityattributevalue + WHERE encryptedvalue IS NOT NULL + AND (value IS NULL OR value = ''); + + Case 2 - the value has neither a plain text value nor an encrypted value: + These are empty rows that carry no information and can be deleted: + + DELETE FROM trackedentityattributevalue + WHERE encryptedvalue IS NULL + AND (value IS NULL OR value = ''); + + As is always the case with direct database manipulation, you should ensure that you have a backup of your + database before running these commands, ensure that DHIS2 is not running, and perform the change in a test + environment first. + + After applying the fix, restart DHIS2 and re-run the integrity check to ensure that no tracked entity + attribute values are flagged. + + If you are unsure how to proceed or encounter issues, please feel free to reach out on the DHIS2 Community + of Practice forum at https://community.dhis2.org. +is_slow: true diff --git a/dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/dataintegrity/DataIntegrityYamlReaderTest.java b/dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/dataintegrity/DataIntegrityYamlReaderTest.java index d95c72c53edc..d719a92bc3cf 100644 --- a/dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/dataintegrity/DataIntegrityYamlReaderTest.java +++ b/dhis-2/dhis-services/dhis-service-administration/src/test/java/org/hisp/dhis/dataintegrity/DataIntegrityYamlReaderTest.java @@ -93,7 +93,7 @@ void testReadDataIntegrityYaml() { List checks = new ArrayList<>(); readYaml(checks, "data-integrity-checks.yaml", "data-integrity-checks", CLASS_PATH); - assertEquals(95, checks.size()); + assertEquals(96, checks.size()); // Names should be unique List allNames = checks.stream().map(DataIntegrityCheck::getName).toList(); diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/resources/i18n_global.properties b/dhis-2/dhis-services/dhis-service-core/src/main/resources/i18n_global.properties index e1d8dc4d55a3..0dc4dbfdb621 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/resources/i18n_global.properties +++ b/dhis-2/dhis-services/dhis-service-core/src/main/resources/i18n_global.properties @@ -2124,6 +2124,7 @@ data_integrity.program_rules_no_action.name=Program rules without actions data_integrity.program_rules_no_expression.name=Program rules with no expression. data_integrity.program_rules_no_priority.name=Program rules without priorities data_integrity.single_events_null_occurred_date.name=Single events without an occurred date +data_integrity.tracked_entity_attribute_null_value.name=Tracked entity attribute value with no plain text value data_integrity.tracker_associate_is_deprecated.name=TRACKER_ASSOCIATE value type is deprecated data_integrity.validation_rules_missing_value_strategy_null.name=Validation rule expressions without missing value strategies data_integrity.dashboards_not_viewed_one_year.name=Dashboards which have not been actively viewed in the last 12 months diff --git a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/AbstractTrackerPersister.java b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/AbstractTrackerPersister.java index d49f6e93b976..e5286449b68d 100644 --- a/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/AbstractTrackerPersister.java +++ b/dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/imports/bundle/persister/AbstractTrackerPersister.java @@ -408,8 +408,16 @@ protected void handleTrackedEntityAttributeValues( String previousValue = isNew ? null : currentValue.getPlainValue(); boolean valueChanged = isNew || !Objects.equals(previousValue, attribute.getValue()); - if (isDelete && !isNew) { - delete(entityManager, preheat, currentValue, trackedEntity, user, changeLogs); + if (isDelete) { + if (!isNew) { + delete(entityManager, preheat, currentValue, trackedEntity, user, changeLogs); + + // Leave the entry in the map: the DELETE is not flushed until the end of + // the run, so a later occurrence of the same TE+attribute in this run must + // still see it as existing (matching the pre-batch DB-read behaviour). + } + + // If the value doesn't exist yet, deleting it is a no-op. } else if (valueChanged) { saveOrUpdateAttributeValue( entityManager, diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityAttributeTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityAttributeTest.java index 0ceb5c253bac..6e56082c0cae 100644 --- a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityAttributeTest.java +++ b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityAttributeTest.java @@ -29,12 +29,15 @@ */ package org.hisp.dhis.tracker.imports.bundle; +import static org.hisp.dhis.test.utils.Assertions.assertContainsOnly; +import static org.hisp.dhis.tracker.Assertions.assertNoErrors; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import java.io.IOException; import java.util.List; import org.hisp.dhis.common.IdentifiableObjectManager; +import org.hisp.dhis.common.UID; import org.hisp.dhis.organisationunit.OrganisationUnit; import org.hisp.dhis.test.integration.PostgresIntegrationTestBase; import org.hisp.dhis.trackedentity.TrackedEntity; @@ -42,7 +45,13 @@ import org.hisp.dhis.trackedentity.TrackedEntityType; import org.hisp.dhis.trackedentityattributevalue.TrackedEntityAttributeValue; import org.hisp.dhis.tracker.TestSetup; +import org.hisp.dhis.tracker.TrackerIdSchemeParam; import org.hisp.dhis.tracker.TrackerIdSchemeParams; +import org.hisp.dhis.tracker.imports.TrackerImportParams; +import org.hisp.dhis.tracker.imports.TrackerImportService; +import org.hisp.dhis.tracker.imports.TrackerImportStrategy; +import org.hisp.dhis.tracker.imports.domain.Attribute; +import org.hisp.dhis.tracker.imports.domain.MetadataIdentifier; import org.hisp.dhis.tracker.imports.domain.TrackerObjects; import org.hisp.dhis.tracker.imports.preheat.TrackerPreheat; import org.hisp.dhis.tracker.imports.preheat.TrackerPreheatService; @@ -64,6 +73,8 @@ class TrackedEntityAttributeTest extends PostgresIntegrationTestBase { @Autowired private TrackerPreheatService trackerPreheatService; + @Autowired private TrackerImportService trackerImportService; + @Autowired private TrackedEntityAttributeValueService trackedEntityAttributeValueService; @Autowired private IdentifiableObjectManager manager; @@ -111,6 +122,118 @@ void shouldSetStoredByToAuthenticatedUserForTrackedEntityAttributeValue() throws TrackedEntity trackedEntity = manager.getAll(TrackedEntity.class).get(0); List attributeValues = trackedEntityAttributeValueService.getTrackedEntityAttributeValues(trackedEntity); + attributeValues.forEach(av -> assertEquals(importUser.getUsername(), av.getStoredBy())); } + + @Test + void shouldUpdateExistingAttributeValueWhenImportingWithNonUidIdScheme() throws IOException { + testSetup.importTrackerData("tracker/te_with_tea_data.json"); + + // The persister must recognize the attribute value as existing (and route it to an UPDATE + // instead of a duplicate INSERT, which would violate the composite PK) regardless of the + // idScheme the import resolves attributes with. + TrackerObjects update = + TrackerObjects.builder() + .trackedEntities( + List.of( + org.hisp.dhis.tracker.imports.domain.TrackedEntity.builder() + .trackedEntity(UID.of("CLR1fvPj4ic")) + .trackedEntityType(MetadataIdentifier.ofName("Person")) + .orgUnit(MetadataIdentifier.ofName("Country")) + .attributes( + List.of( + Attribute.builder() + .attribute(MetadataIdentifier.ofName("Attribute_Text")) + .value("updated value") + .build())) + .build())) + .build(); + TrackerImportParams params = + TrackerImportParams.builder() + .importStrategy(TrackerImportStrategy.UPDATE) + .idSchemes(TrackerIdSchemeParams.builder().idScheme(TrackerIdSchemeParam.NAME).build()) + .build(); + + assertNoErrors(trackerImportService.importTracker(params, update)); + + TrackedEntity trackedEntity = manager.getAll(TrackedEntity.class).get(0); + List attributeValues = + trackedEntityAttributeValueService.getTrackedEntityAttributeValues(trackedEntity); + assertEquals(3, attributeValues.size()); + TrackedEntityAttributeValue updatedValue = + attributeValues.stream() + .filter(av -> "TsfP85GKsU5".equals(av.getAttribute().getUid())) + .findFirst() + .orElseThrow(); + assertEquals("updated value", updatedValue.getValue()); + } + + @Test + void shouldNotPersistAttributeValueWhenImportingEmptyValueForAttributeNotInDb() { + UID te = UID.generate(); + TrackerObjects trackerObjects = + TrackerObjects.builder() + .trackedEntities( + List.of( + org.hisp.dhis.tracker.imports.domain.TrackedEntity.builder() + .trackedEntity(te) + .trackedEntityType(MetadataIdentifier.ofUid("KrYIdvLxkMb")) + .orgUnit(MetadataIdentifier.ofUid("cNEZTkdAvmg")) + .attributes( + List.of( + Attribute.builder() + .attribute(MetadataIdentifier.ofUid("sYn3tkL3XKa")) + .value("123") + .build(), + Attribute.builder() + .attribute(MetadataIdentifier.ofUid("TsfP85GKsU5")) + .value("") + .build())) + .build())) + .build(); + + assertNoErrors( + trackerImportService.importTracker(TrackerImportParams.builder().build(), trackerObjects)); + + TrackedEntity trackedEntity = manager.get(TrackedEntity.class, te.getValue()); + List attributeValues = + trackedEntityAttributeValueService.getTrackedEntityAttributeValues(trackedEntity); + assertContainsOnly( + List.of("sYn3tkL3XKa"), + attributeValues.stream().map(av -> av.getAttribute().getUid()).toList()); + } + + @Test + void shouldDeleteAttributeValueWhenImportingEmptyValueForExistingAttribute() throws IOException { + testSetup.importTrackerData("tracker/te_with_tea_data.json"); + + TrackerObjects update = + TrackerObjects.builder() + .trackedEntities( + List.of( + org.hisp.dhis.tracker.imports.domain.TrackedEntity.builder() + .trackedEntity(UID.of("CLR1fvPj4ic")) + .trackedEntityType(MetadataIdentifier.ofUid("KrYIdvLxkMb")) + .orgUnit(MetadataIdentifier.ofUid("cNEZTkdAvmg")) + .attributes( + List.of( + Attribute.builder() + .attribute(MetadataIdentifier.ofUid("TsfP85GKsU5")) + .value("") + .build())) + .build())) + .build(); + TrackerImportParams params = + TrackerImportParams.builder().importStrategy(TrackerImportStrategy.UPDATE).build(); + + assertNoErrors(trackerImportService.importTracker(params, update)); + + TrackedEntity trackedEntity = manager.get(TrackedEntity.class, "CLR1fvPj4ic"); + List attributeValues = + trackedEntityAttributeValueService.getTrackedEntityAttributeValues(trackedEntity); + assertContainsOnly( + List.of("sYn3tkL3XKa", "sTGqP5JNy6E"), + attributeValues.stream().map(av -> av.getAttribute().getUid()).toList()); + } } diff --git a/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/dataintegrity/DataIntegrityTrackedEntityAttributeNullValueControllerTest.java b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/dataintegrity/DataIntegrityTrackedEntityAttributeNullValueControllerTest.java new file mode 100644 index 000000000000..ab7aac89557f --- /dev/null +++ b/dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/dataintegrity/DataIntegrityTrackedEntityAttributeNullValueControllerTest.java @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2004-2025, University of Oslo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.hisp.dhis.webapi.controller.dataintegrity; + +import org.hisp.dhis.organisationunit.OrganisationUnit; +import org.hisp.dhis.trackedentity.TrackedEntity; +import org.hisp.dhis.trackedentity.TrackedEntityAttribute; +import org.hisp.dhis.trackedentity.TrackedEntityType; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Tests the {@code tracked_entity_attribute_null_value} data integrity check, which flags tracked + * entity attribute values that have no plain text value stored in the {@code value} column. This + * includes values that only have an {@code encryptedvalue} (previously confidential attributes) and + * values that are completely empty. Anomalous rows are inserted with plain SQL because the service + * layer refuses to persist a null value. + */ +class DataIntegrityTrackedEntityAttributeNullValueControllerTest + extends AbstractDataIntegrityIntegrationTest { + + private static final String CHECK = "tracked_entity_attribute_null_value"; + + private static final String DETAILS_ID_TYPE = "tracker"; + + @Autowired private JdbcTemplate jdbcTemplate; + + private TrackedEntity trackedEntity; + + private TrackedEntityAttribute attribute; + + @BeforeEach + void setUp() { + OrganisationUnit organisationUnit = createOrganisationUnit('A'); + manager.save(organisationUnit); + + TrackedEntityType trackedEntityType = createTrackedEntityType('A'); + manager.save(trackedEntityType); + + trackedEntity = createTrackedEntity(organisationUnit, trackedEntityType); + manager.save(trackedEntity); + + attribute = createTrackedEntityAttribute('A'); + manager.save(attribute); + + // flush so the parent rows exist in the DB before the raw JDBC inserts below + manager.flush(); + } + + @Test + void shouldFindNoIssuesWhenThereIsNoData() { + assertHasNoDataIntegrityIssues(DETAILS_ID_TYPE, CHECK, false); + } + + @Test + void shouldFindNoIssuesWhenAttributeValueHasPlainTextValue() { + insertAttributeValue("some value", null); + + assertHasNoDataIntegrityIssues(DETAILS_ID_TYPE, CHECK, true); + } + + @Test + void shouldFindIssueWhenAttributeValueHasNeitherPlainTextNorEncryptedValue() { + insertAttributeValue(null, null); + + assertHasDataIntegrityIssues( + DETAILS_ID_TYPE, + CHECK, + 100, + trackedEntity.getUid(), + trackedEntity.getUid(), + "has no value", + true); + } + + @Test + void shouldFindIssueWhenAttributeValueHasEncryptedValueButNoPlainTextValue() { + insertAttributeValue(null, "encrypted-cipher-text"); + + assertHasDataIntegrityIssues( + DETAILS_ID_TYPE, + CHECK, + 100, + trackedEntity.getUid(), + trackedEntity.getUid(), + "has an encrypted value but no plain text value", + true); + } + + private void insertAttributeValue(String value, String encryptedValue) { + jdbcTemplate.update( + "INSERT INTO trackedentityattributevalue " + + "(trackedentityid, trackedentityattributeid, created, lastupdated, value, encryptedvalue) " + + "VALUES (?, ?, now(), now(), ?, ?)", + trackedEntity.getId(), + attribute.getId(), + value, + encryptedValue); + } +}