diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/ProgramIndicatorService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/ProgramIndicatorService.java index 424da674da97..218c1f44ce23 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/ProgramIndicatorService.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/ProgramIndicatorService.java @@ -174,6 +174,23 @@ String getAnalyticsSql( Date startDate, Date endDate); + /** + * Gets the analytics SQL clause of an expression. The SQL does not substitute null values. + * + * @param expression the expression. + * @param dataType the data type to return. + * @param programIndicator the program indicator to evaluate. + * @param startDate the start date. + * @param endDate the end date. + * @return the SQL string. + */ + String getAnalyticsSqlAllowingNulls( + String expression, + DataType dataType, + ProgramIndicator programIndicator, + Date startDate, + Date endDate); + /** * Gets the the analytics SQL clause of an expression. Does not ignore missing numeric values for * data elements and attributes. @@ -194,6 +211,25 @@ String getAnalyticsSql( Date endDate, String tableAlias); + /** + * Gets the analytics SQL clause of an expression. The SQL does not substitute null values. + * + * @param expression the expression. + * @param dataType the data type to return. + * @param programIndicator the program indicator to evaluate. + * @param startDate the start date. + * @param endDate the end date. + * @param tableAlias use this table alias for expression returning a inner query + * @return the SQL string. + */ + String getAnalyticsSqlAllowingNulls( + String expression, + DataType dataType, + ProgramIndicator programIndicator, + Date startDate, + Date endDate, + String tableAlias); + /** * Returns a SQL clause which matches any value for the data elements and attributes in the given * expression. diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java index 96e394bb171b..7736fea29728 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java @@ -433,7 +433,7 @@ protected String getWhereClause(EventQueryParams params) { if (params.hasProgramIndicatorDimension() && params.getProgramIndicator().hasFilter()) { String filter = - programIndicatorService.getAnalyticsSql( + programIndicatorService.getAnalyticsSqlAllowingNulls( params.getProgramIndicator().getFilter(), BOOLEAN, params.getProgramIndicator(), diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEventAnalyticsManager.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEventAnalyticsManager.java index 4a0bb99c07e0..b08961736442 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEventAnalyticsManager.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEventAnalyticsManager.java @@ -516,7 +516,7 @@ protected String getWhereClause(EventQueryParams params) { if (params.hasProgramIndicatorDimension() && params.getProgramIndicator().hasFilter()) { String filter = - programIndicatorService.getAnalyticsSql( + programIndicatorService.getAnalyticsSqlAllowingNulls( params.getProgramIndicator().getFilter(), BOOLEAN, params.getProgramIndicator(), diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/programindicator/DefaultProgramIndicatorSubqueryBuilder.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/programindicator/DefaultProgramIndicatorSubqueryBuilder.java index 710e8bf3276a..9eab6345445f 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/programindicator/DefaultProgramIndicatorSubqueryBuilder.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/programindicator/DefaultProgramIndicatorSubqueryBuilder.java @@ -125,7 +125,7 @@ private String getAggregateClauseForPIandRelationshipType( aggregateSql += (where.isBlank() ? " WHERE " : " AND ") + "(" - + getProgramIndicatorSql( + + getProgramIndicatorFilterSql( programIndicator.getFilter(), BOOLEAN, programIndicator, @@ -205,4 +205,19 @@ private String getProgramIndicatorSql( latestDate, SUBQUERY_TABLE_ALIAS); } + + private String getProgramIndicatorFilterSql( + String expression, + DataType dataType, + ProgramIndicator programIndicator, + Date earliestStartDate, + Date latestDate) { + return this.programIndicatorService.getAnalyticsSqlAllowingNulls( + expression, + dataType, + programIndicator, + earliestStartDate, + latestDate, + SUBQUERY_TABLE_ALIAS); + } } diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/ProgramIndicatorQueryBuilder.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/ProgramIndicatorQueryBuilder.java index 85150e74ff52..281f8619a1f4 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/ProgramIndicatorQueryBuilder.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/ProgramIndicatorQueryBuilder.java @@ -230,7 +230,7 @@ private ProgramIndicatorQueryParts getProgramIndicatorQueryParts( null, SUBQUERY_TABLE_ALIAS), // filter - programIndicatorService.getAnalyticsSql( + programIndicatorService.getAnalyticsSqlAllowingNulls( programIndicator.getFilter(), DataType.BOOLEAN, programIndicator, diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java index aa8b5ceb6b04..4192c3023030 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/AbstractJdbcEventAnalyticsManagerTest.java @@ -61,6 +61,8 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -1004,6 +1006,43 @@ private QueryItem buildQueryItemWithGroupAndFilters( return queryItem; } + /** + * DHIS2-20929: PI filters such as {@code #{stage.de} == 0} must not be wrapped in {@code + * coalesce(..., 0)} — otherwise events where the DE is NULL (or where the row belongs to a + * different program stage) incorrectly match the equality to zero. The filter must therefore be + * compiled with NULL-allowing semantics. + */ + @Test + void verifyProgramIndicatorFilterCompiledAllowingNulls() { + ProgramIndicator programIndicator = + createProgramIndicator('A', programA, "V{event_count}", "#{ProgrmStagA.DataElmentA} == 0"); + + EventQueryParams params = + new EventQueryParams.Builder(createRequestParams()) + .withProgramIndicator(programIndicator) + .build(); + + lenient() + .when( + programIndicatorService.getAnalyticsSqlAllowingNulls( + eq(programIndicator.getFilter()), + eq(org.hisp.dhis.analytics.DataType.BOOLEAN), + eq(programIndicator), + any(Date.class), + any(Date.class))) + .thenReturn("ax.\"DataElmentA\" = 0"); + + eventSubject.getWhereClause(params); + + verify(programIndicatorService) + .getAnalyticsSqlAllowingNulls( + eq(programIndicator.getFilter()), + eq(org.hisp.dhis.analytics.DataType.BOOLEAN), + eq(programIndicator), + any(Date.class), + any(Date.class)); + } + private EventQueryParams getEventQueryParamsForCoordinateFieldsTest( List coordinateFields) { DataElement deA = createDataElement('A', TEXT, AggregationType.NONE); diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/programindicator/ProgramIndicatorSubqueryBuilderTest.java b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/programindicator/ProgramIndicatorSubqueryBuilderTest.java index 4f9cbd8863c6..bfc4203c4a75 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/programindicator/ProgramIndicatorSubqueryBuilderTest.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/event/data/programindicator/ProgramIndicatorSubqueryBuilderTest.java @@ -185,7 +185,7 @@ void verifyProgramIndicatorWithFilter() { when(programIndicatorService.getAnalyticsSql( DUMMY_EXPRESSION, NUMERIC, pi, startDate, endDate, "subax")) .thenReturn("distinct psi"); - when(programIndicatorService.getAnalyticsSql( + when(programIndicatorService.getAnalyticsSqlAllowingNulls( DUMMY_FILTER_EXPRESSION, BOOLEAN, pi, startDate, endDate, "subax")) .thenReturn("a = b"); diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramIndicatorService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramIndicatorService.java index 673d023106c4..83a3a2875cd1 100644 --- a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramIndicatorService.java +++ b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramIndicatorService.java @@ -90,6 +90,7 @@ import org.hisp.dhis.parser.expression.CommonExpressionVisitor; import org.hisp.dhis.parser.expression.ExpressionItem; import org.hisp.dhis.parser.expression.ExpressionItemMethod; +import org.hisp.dhis.parser.expression.ExpressionState; import org.hisp.dhis.parser.expression.ProgramExpressionParams; import org.hisp.dhis.parser.expression.function.RepeatableProgramStageOffset; import org.hisp.dhis.parser.expression.function.VectorAvg; @@ -333,7 +334,20 @@ public String getAnalyticsSql( ProgramIndicator programIndicator, Date startDate, Date endDate) { - return getAnalyticsSqlCached(expression, dataType, programIndicator, startDate, endDate, null); + return getAnalyticsSqlCached( + expression, dataType, programIndicator, startDate, endDate, null, true); + } + + @Override + @Transactional(readOnly = true) + public String getAnalyticsSqlAllowingNulls( + String expression, + DataType dataType, + ProgramIndicator programIndicator, + Date startDate, + Date endDate) { + return getAnalyticsSqlCached( + expression, dataType, programIndicator, startDate, endDate, null, false); } @Override @@ -346,29 +360,49 @@ public String getAnalyticsSql( Date endDate, String tableAlias) { return getAnalyticsSqlCached( - expression, dataType, programIndicator, startDate, endDate, tableAlias); + expression, dataType, programIndicator, startDate, endDate, tableAlias, true); } - private String getAnalyticsSqlCached( + @Override + @Transactional(readOnly = true) + public String getAnalyticsSqlAllowingNulls( String expression, DataType dataType, ProgramIndicator programIndicator, Date startDate, Date endDate, String tableAlias) { + return getAnalyticsSqlCached( + expression, dataType, programIndicator, startDate, endDate, tableAlias, false); + } + + private String getAnalyticsSqlCached( + String expression, + DataType dataType, + ProgramIndicator programIndicator, + Date startDate, + Date endDate, + String tableAlias, + boolean replaceNulls) { if (expression == null) { return null; } String cacheKey = getAnalyticsSqlCacheKey( - expression, dataType, programIndicator, startDate, endDate, tableAlias); + expression, dataType, programIndicator, startDate, endDate, tableAlias, replaceNulls); return analyticsSqlCache.get( cacheKey, k -> getAnalyticsSqlInternal( - expression, dataType, programIndicator, startDate, endDate, tableAlias)); + expression, + dataType, + programIndicator, + startDate, + endDate, + tableAlias, + replaceNulls)); } private String getAnalyticsSqlCacheKey( @@ -377,7 +411,8 @@ private String getAnalyticsSqlCacheKey( ProgramIndicator programIndicator, Date startDate, Date endDate, - String tableAlias) { + String tableAlias, + boolean replaceNulls) { return expression + "|" + dataType.name() @@ -386,7 +421,8 @@ private String getAnalyticsSqlCacheKey( + dateIfPresent(startDate) + dateIfPresent(endDate) + "|" - + (tableAlias == null ? "" : tableAlias); + + (tableAlias == null ? "" : tableAlias) + + replaceNulls; } /** @@ -408,7 +444,8 @@ private String getAnalyticsSqlInternal( ProgramIndicator programIndicator, Date startDate, Date endDate, - String tableAlias) { + String tableAlias, + boolean replaceNulls) { // Get the uids from the expression even if this is the filter Set uids = getDataElementAndAttributeIdentifiers( @@ -424,7 +461,7 @@ private String getAnalyticsSqlInternal( .dataElementAndAttributeIdentifiers(uids) .build(); - CommonExpressionVisitor visitor = newVisitor(ITEM_GET_SQL, params, progParams); + CommonExpressionVisitor visitor = newVisitor(ITEM_GET_SQL, params, progParams, replaceNulls); visitor.setExpressionLiteral(new SqlLiteral()); @@ -511,6 +548,14 @@ private CommonExpressionVisitor newVisitor( ExpressionItemMethod itemMethod, ExpressionParams params, ProgramExpressionParams progParams) { + return newVisitor(itemMethod, params, progParams, true); + } + + private CommonExpressionVisitor newVisitor( + ExpressionItemMethod itemMethod, + ExpressionParams params, + ProgramExpressionParams progParams, + boolean replaceNulls) { return CommonExpressionVisitor.builder() .idObjectManager(idObjectManager) .dimensionService(dimensionService) @@ -522,6 +567,7 @@ private CommonExpressionVisitor newVisitor( .itemMethod(itemMethod) .params(params) .progParams(progParams) + .state(ExpressionState.builder().replaceNulls(replaceNulls).build()) .build(); } diff --git a/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/aggregate/AnalyticsQueryDv8AutoTest.java b/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/aggregate/AnalyticsQueryDv8AutoTest.java index 4fbba7ae888e..84b6c0795e68 100644 --- a/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/aggregate/AnalyticsQueryDv8AutoTest.java +++ b/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/aggregate/AnalyticsQueryDv8AutoTest.java @@ -331,14 +331,12 @@ public void queryChildHealthAndInpatientIndicators() throws JSONException { validateRow(response, List.of("htr2mMY515K", "202101", "141.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202102", "136.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202103", "163.0", "", "", "", "", "")); - validateRow(response, List.of("htr2mMY515K", "202104", "163.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202105", "126.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202106", "162.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202107", "158.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202108", "154.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202109", "149.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202110", "156.0", "", "", "", "", "")); - validateRow(response, List.of("htr2mMY515K", "202111", "181.0", "", "", "", "", "")); validateRow(response, List.of("htr2mMY515K", "202112", "139.0", "", "", "", "", "")); validateRow(response, List.of("tUdBD1JDxpn", "202101", "42.82", "", "", "", "", "")); validateRow(response, List.of("tUdBD1JDxpn", "202102", "43.78", "", "", "", "", "")); @@ -391,14 +389,11 @@ public void queryChildHealthAndInpatientIndicators() throws JSONException { validateRow(response, List.of("ToQVD4irW3Q", "202101", "49.19", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202102", "50.39", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202103", "48.95", "", "", "", "", "")); - validateRow(response, List.of("ToQVD4irW3Q", "202104", "48.24", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202105", "49.01", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202106", "49.26", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202107", "49.89", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202108", "48.66", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202109", "48.86", "", "", "", "", "")); - validateRow(response, List.of("ToQVD4irW3Q", "202110", "48.23", "", "", "", "", "")); - validateRow(response, List.of("ToQVD4irW3Q", "202111", "49.09", "", "", "", "", "")); validateRow(response, List.of("ToQVD4irW3Q", "202112", "51.3", "", "", "", "", "")); validateRow(response, List.of("ReQEl5V3z6p", "202101", "49.93", "", "", "", "", "")); validateRow(response, List.of("ReQEl5V3z6p", "202102", "49.49", "", "", "", "", "")); @@ -427,14 +422,11 @@ public void queryChildHealthAndInpatientIndicators() throws JSONException { validateRow(response, List.of("vDdRoZYybP2", "202101", "295.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202102", "288.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202103", "320.0", "", "", "", "", "")); - validateRow(response, List.of("vDdRoZYybP2", "202104", "317.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202105", "298.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202106", "307.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202107", "316.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202108", "299.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202109", "310.0", "", "", "", "", "")); - validateRow(response, List.of("vDdRoZYybP2", "202110", "303.0", "", "", "", "", "")); - validateRow(response, List.of("vDdRoZYybP2", "202111", "316.0", "", "", "", "", "")); validateRow(response, List.of("vDdRoZYybP2", "202112", "282.0", "", "", "", "", "")); validateRow(response, List.of("p2Zxg0wcPQ3", "202107", "482.0", "", "", "", "", "")); validateRow(response, List.of("p2Zxg0wcPQ3", "202103", "493.0", "", "", "", "", "")); @@ -535,14 +527,12 @@ public void queryChildHealthAndInpatientIndicators() throws JSONException { validateRow(response, List.of("Y7hKDSuqEtH", "202101", "75.51", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202102", "60.76", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202103", "65.06", "", "", "", "", "")); - validateRow(response, List.of("Y7hKDSuqEtH", "202104", "61.91", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202105", "63.63", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202106", "69.18", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202107", "68.54", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202108", "65.73", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202109", "69.79", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202110", "69.22", "", "", "", "", "")); - validateRow(response, List.of("Y7hKDSuqEtH", "202111", "61.1", "", "", "", "", "")); validateRow(response, List.of("Y7hKDSuqEtH", "202112", "71.79", "", "", "", "", "")); }