From 3bda079328f9baffdfc13cbbf5bbc9198d43e51d Mon Sep 17 00:00:00 2001 From: Giuseppe Nespolino Date: Fri, 19 Jul 2024 17:23:11 +0200 Subject: [PATCH] fix: repeated fields in TE query [DHIS2-17745] (#18142) * fix: repeated fields in TE query [DHIS2-17745] * test: unit test [DHIS2-17745] * test: unit test [DHIS2-17745] --- .../dhis/analytics/common/query/Field.java | 18 ++++- .../context/querybuilder/TeiQueryBuilder.java | 34 +------- .../query/context/sql/RenderableSqlQuery.java | 21 ++++- .../query/context/sql/SqlQueryBuilders.java | 50 ++++++++++++ .../context/sql/RenderableSqlQueryTest.java | 78 +++++++++++++++++++ 5 files changed, 165 insertions(+), 36 deletions(-) create mode 100644 dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQueryTest.java diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/query/Field.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/query/Field.java index ca13252bc71c..1afc5227b147 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/query/Field.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/common/query/Field.java @@ -35,17 +35,23 @@ import static org.hisp.dhis.commons.util.TextUtils.doubleQuote; import lombok.AccessLevel; +import lombok.AllArgsConstructor; import lombok.Getter; import lombok.RequiredArgsConstructor; import lombok.With; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; import org.hisp.dhis.analytics.common.params.dimension.DimensionIdentifier; import org.hisp.dhis.analytics.common.params.dimension.DimensionParam; +import org.springframework.lang.NonNull; /** * This class represents a {@link Renderable} field. It's mainly used for SQL query rendering and * headers display. */ @RequiredArgsConstructor(staticName = "of", access = AccessLevel.PRIVATE) +@AllArgsConstructor(access = AccessLevel.PRIVATE) +@Slf4j public class Field extends BaseRenderable { private final String tableAlias; @@ -63,6 +69,9 @@ public class Field extends BaseRenderable { /** virtual fields won't be added to the select clause */ @With @Getter private final boolean virtual; + // A cached version of the rendered field. + private String renderedField; + public Field asVirtual() { return withVirtual(true); } @@ -161,7 +170,14 @@ public String getDimensionIdentifier() { } @Override - public String render() { + public @NonNull String render() { + if (StringUtils.isBlank(renderedField)) { + renderedField = renderField(); + } + return renderedField; + } + + private String renderField() { String rendered = EMPTY; if (isNotBlank(tableAlias)) { diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/TeiQueryBuilder.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/TeiQueryBuilder.java index 38459f0d8fa9..39a1720ab018 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/TeiQueryBuilder.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/querybuilder/TeiQueryBuilder.java @@ -65,37 +65,6 @@ @RequiredArgsConstructor @org.springframework.core.annotation.Order(1) public class TeiQueryBuilder extends SqlQueryBuilderAdaptor { - private static final String JSON_AGGREGATION_QUERY = - """ - coalesce( (select json_agg(json_build_object('programUid', pr.uid, - 'enrollmentUid', en.programinstanceuid, - 'enrollmentDate', en.enrollmentdate, - 'incidentDate', en.incidentdate, - 'endDate', en.enddate, - 'orgUnitUid', en.ou, - 'orgUnitName', en.ouname, - 'orgUnitCode', en.oucode, - 'orgUnitNameHierarchy', en.ounamehierarchy, - 'enrollmentStatus', en.enrollmentstatus, - 'events', - coalesce( (select json_agg(json_build_object('programStageUid', ps.uid, - 'eventUid', ev.programstageuid, - 'occurredDate', ev.occurreddate, - 'dueDate', ev.scheduleddate, - 'orgUnitUid', ev.ou, - 'orgUnitName', ev.ouname, - 'orgUnitCode', ev.oucode, - 'orgUnitNameHierarchy', ev.ounamehierarchy, - 'eventStatus', ev.status, - 'eventDataValues', ev.eventdatavalues)) - from analytics_tei_events_%s ev, - programstage ps - where ev.programinstanceuid = en.programinstanceuid - and ps.uid = ev.programstageuid), '[]'::json))) - from analytics_tei_enrollments_%s en, - program pr - where en.trackedentityinstanceuid = t_1.trackedentityinstanceuid - and pr.uid = en.programuid), '[]'::json)"""; private final IdentifiableObjectManager identifiableObjectManager; @@ -121,8 +90,7 @@ public boolean alwaysRun() { @Override protected Stream getSelect(QueryContext queryContext) { String aggregationQuery = - JSON_AGGREGATION_QUERY.formatted( - queryContext.getTetTableSuffix(), queryContext.getTetTableSuffix()); + SqlQueryBuilders.getJsonAggregationQuery(queryContext.getTetTableSuffix()); Field aggregatedEnrollments = Field.ofUnquoted(EMPTY, () -> aggregationQuery, "enrollments").withUsedInHeaders(false); diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQuery.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQuery.java index 9944c5efc481..6987a69cf757 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQuery.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQuery.java @@ -34,6 +34,9 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; import lombok.Builder; @@ -52,6 +55,7 @@ import org.hisp.dhis.analytics.common.query.Select; import org.hisp.dhis.analytics.common.query.Table; import org.hisp.dhis.analytics.common.query.Where; +import org.springframework.lang.NonNull; /** * This class is responsible for rendering a SQL query. Each instance of this class will only render @@ -100,7 +104,7 @@ public RenderableSqlQuery forCount() { } @Override - public String render() { + public @NonNull String render() { if (countRequested) { return renderSqlCountQuery(); } @@ -141,7 +145,20 @@ private String select() { } private List nonVirtualSelectFields() { - return selectFields.stream().filter(f -> !f.isVirtual()).toList(); + return selectFields.stream() + .filter(Predicate.not(Field::isVirtual)) + // We need to filter out fields that have already been rendered. + // It might happen that a field can be added multiple times (for example + // TE static fields, which are always added to the query, and TE period fields) + // see: DHIS2-17745 + .filter(distinctByRendered()) + .toList(); + } + + /** Returns a predicate that filters out fields that have already been rendered. */ + private static Predicate distinctByRendered() { + Set seen = ConcurrentHashMap.newKeySet(); + return f -> seen.add(f.render()); } private String getIfPresentOrElse(String key, Supplier supplier) { diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/SqlQueryBuilders.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/SqlQueryBuilders.java index f5ef8645d6cf..11e783c79a2e 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/SqlQueryBuilders.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/tei/query/context/sql/SqlQueryBuilders.java @@ -27,6 +27,9 @@ */ package org.hisp.dhis.analytics.tei.query.context.sql; +import static org.apache.commons.text.StringSubstitutor.replace; + +import java.util.Map; import lombok.AccessLevel; import lombok.NoArgsConstructor; import org.hisp.dhis.analytics.common.params.dimension.DimensionIdentifier; @@ -36,6 +39,49 @@ /** Utility class of common methods used in the SQL query builders. */ @NoArgsConstructor(access = AccessLevel.PRIVATE) public class SqlQueryBuilders { + + private static final String EVENT_QUERY = + """ + select json_agg(json_build_object( + 'programStageUid', ev.programstageuid, + 'eventUid', ev.programstageinstanceuid, + 'occurredDate', ev.occurreddate, + 'dueDate', ev.scheduleddate, + 'orgUnitUid', ev.ou, + 'orgUnitName', ev.ouname, + 'orgUnitCode', ev.oucode, + 'orgUnitNameHierarchy', ev.ounamehierarchy, + 'eventStatus', ev.status, + 'eventDataValues', ev.eventdatavalues)) + from analytics_tei_events_${trackedEntityType} ev + where ev.programinstanceuid = en.programinstanceuid"""; + + private static final String ENROLLMENT_QUERY = + replace( + """ + select json_agg( + json_build_object( + 'programUid', en.programuid, + 'enrollmentUid', en.programinstanceuid, + 'enrollmentDate', en.enrollmentdate, + 'incidentDate', en.incidentdate, + 'endDate', en.enddate, + 'orgUnitUid', en.ou, + 'orgUnitName', en.ouname, + 'orgUnitCode', en.oucode, + 'orgUnitNameHierarchy', en.ounamehierarchy, + 'enrollmentStatus', en.enrollmentstatus, + 'events', ${eventQuery})) + from analytics_tei_enrollments_${trackedEntityType} en + where en.trackedentityinstanceuid = t_1.trackedentityinstanceuid""", + Map.of("eventQuery", coalesceToEmptyArray(EVENT_QUERY))); + + private static final String JSON_AGGREGATION_QUERY = coalesceToEmptyArray(ENROLLMENT_QUERY); + + private static String coalesceToEmptyArray(String query) { + return replace("coalesce((${query}), '[]'::json)", Map.of("query", query)); + } + public static boolean isNotPeriodDimension( DimensionIdentifier dimensionIdentifier) { return !dimensionIdentifier.getDimension().isPeriodDimension(); @@ -50,4 +96,8 @@ public static boolean isOfType( DimensionParamObjectType type) { return dimensionParamDimensionIdentifier.getDimension().isOfType(type); } + + public static String getJsonAggregationQuery(String tetTableSuffix) { + return replace(JSON_AGGREGATION_QUERY, Map.of("trackedEntityType", tetTableSuffix)); + } } diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQueryTest.java b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQueryTest.java new file mode 100644 index 000000000000..08ffacc1e9b1 --- /dev/null +++ b/dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/tei/query/context/sql/RenderableSqlQueryTest.java @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2004-2024, 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. + */ +package org.hisp.dhis.analytics.tei.query.context.sql; + +import java.util.stream.Stream; +import org.hisp.dhis.analytics.common.query.Field; +import org.hisp.dhis.analytics.common.query.GroupableCondition; +import org.hisp.dhis.analytics.common.query.IndexedOrder; +import org.hisp.dhis.analytics.common.query.LimitOffset; +import org.hisp.dhis.analytics.common.query.Order; +import org.hisp.dhis.analytics.common.query.Table; +import org.hisp.dhis.common.SortDirection; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class RenderableSqlQueryTest { + + @ParameterizedTest + @MethodSource("provideTestCases") + void testRenderQueryMatches(String expected, RenderableSqlQuery query) { + Assertions.assertEquals(expected, query.render()); + } + + private static Stream provideTestCases() { + return Stream.of( + Arguments.of( + """ + select t1."field1" as "alias1", t2."field2" as "alias2" from table1 t1 where condition order by orderField nulls last limit 10 offset 20""", + RenderableSqlQuery.builder() + .selectField(Field.of("t1", () -> "field1", "alias1")) + .selectField(Field.of("t2", () -> "field2", "alias2")) + .selectField(Field.of("t1", () -> "field1", "alias1")) + .mainTable(Table.of(() -> "table1", () -> "t1")) + .limitOffset(LimitOffset.of(10, 20, false)) + .orderClause(IndexedOrder.of(1, Order.of(() -> "orderField", SortDirection.ASC))) + .groupableCondition(GroupableCondition.of("group", () -> "condition")) + .build()), + Arguments.of( + """ + select t1."field1" as "alias1" from table1 t1 where condition1 order by orderField nulls last limit 10 offset 20""", + RenderableSqlQuery.builder() + .selectField(Field.of("t1", () -> "field1", "alias1")) + .selectField(Field.of("t2", () -> "field2", "alias2").asVirtual()) + .selectField(Field.of("t1", () -> "field1", "alias1")) + .mainTable(Table.of(() -> "table1", () -> "t1")) + .limitOffset(LimitOffset.of(10, 20, false)) + .orderClause(IndexedOrder.of(1, Order.of(() -> "orderField", SortDirection.ASC))) + .groupableCondition(GroupableCondition.of("group", () -> "condition1")) + .build())); + } +}