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-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)) {

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<Field> 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);

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<Field> 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<Field> distinctByRendered() {
+    Set<String> seen = ConcurrentHashMap.newKeySet();
+    return f -> seen.add(f.render());
   }
 
   private String getIfPresentOrElse(String key, Supplier<String> supplier) {

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<DimensionParam> 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));
+  }
 }

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<Arguments> 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()));
+  }
+}