refactor: Remove unnecessary data filter clause [DHIS2-16705] (#19381)

dhis2 · Dec 4, 2024 · 43d4175 · 43d4175
1 parent 8fcf3bf
commit 43d4175
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 73 deletions.
diff --git a/...-analytics/src/main/java/org/hisp/dhis/analytics/table/AbstractEventJdbcTableManager.java b/...-analytics/src/main/java/org/hisp/dhis/analytics/table/AbstractEventJdbcTableManager.java
@@ -27,18 +27,17 @@
  */
 package org.hisp.dhis.analytics.table;
 
-import static org.apache.commons.lang3.StringUtils.EMPTY;
 import static org.hisp.dhis.analytics.table.model.Skip.SKIP;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.getClosingParentheses;
 import static org.hisp.dhis.analytics.util.AnalyticsUtils.getColumnType;
 import static org.hisp.dhis.db.model.DataType.GEOMETRY;
 import static org.hisp.dhis.db.model.DataType.TEXT;
-import static org.hisp.dhis.system.util.MathUtils.NUMERIC_LENIENT_REGEXP;
 
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
+import org.apache.commons.lang3.Validate;
 import org.hisp.dhis.analytics.AnalyticsTableHookService;
 import org.hisp.dhis.analytics.partition.PartitionManager;
 import org.hisp.dhis.analytics.table.model.AnalyticsDimensionType;
@@ -99,14 +98,6 @@ public AbstractEventJdbcTableManager(
 
   public static final String OU_NAME_COL_SUFFIX = "_name";
 
-  protected final String getNumericClause() {
-    return " and " + sqlBuilder.regexpMatch("value", "'" + NUMERIC_LENIENT_REGEXP + "'");
-  }
-
-  protected final String getDateClause() {
-    return " and " + sqlBuilder.regexpMatch("value", DATE_REGEXP);
-  }
-
   /**
    * Indicates whether creating an index should be skipped.
    *
@@ -148,22 +139,6 @@ protected String getColumnExpression(ValueType valueType, String columnExpressio
     }
   }
 
-  /**
-   * For numeric and date value types, returns a data filter clause for checking whether the value
-   * is valid according to the value type. For other value types, returns the empty string.
-   *
-   * @param attribute the {@link TrackedEntityAttribute}.
-   * @return a data filter clause.
-   */
-  protected String getDataFilterClause(TrackedEntityAttribute attribute) {
-    if (attribute.isNumericType()) {
-      return getNumericClause();
-    } else if (attribute.isDateType()) {
-      return getDateClause();
-    }
-    return EMPTY;
-  }
-
   /**
    * Returns a cast expression which includes a value filter for the given value type.
    *
@@ -225,11 +200,10 @@ protected List<AnalyticsTableColumn> getColumnForAttribute(TrackedEntityAttribut
     String valueColumn = String.format("%s.%s", quote(attribute.getUid()), "value");
     DataType dataType = getColumnType(attribute.getValueType(), isSpatialSupport());
     String selectExpression = getColumnExpression(attribute.getValueType(), valueColumn);
-    String dataFilterClause = getDataFilterClause(attribute);
     Skip skipIndex = skipIndex(attribute.getValueType(), attribute.hasOptionSet());
 
     if (attribute.getValueType().isOrganisationUnit()) {
-      columns.addAll(getColumnForOrgUnitAttribute(attribute, dataFilterClause));
+      columns.addAll(getColumnForOrgUnitAttribute(attribute));
     }
 
     columns.add(
@@ -248,19 +222,19 @@ protected List<AnalyticsTableColumn> getColumnForAttribute(TrackedEntityAttribut
    * Returns a list of columns based on the given attribute.
    *
    * @param attribute the {@link TrackedEntityAttribute}.
-   * @param dataFilterClause the data filter clause.
    * @return a list of {@link AnalyticsTableColumn}.
    */
   private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
-      TrackedEntityAttribute attribute, String dataFilterClause) {
+      TrackedEntityAttribute attribute) {
+    Validate.isTrue(attribute.getValueType().isOrganisationUnit());
     List<AnalyticsTableColumn> columns = new ArrayList<>();
 
     String fromClause =
         qualifyVariables("from ${organisationunit} ou where ou.uid = (select value");
 
     if (isSpatialSupport()) {
       String selectExpression = "ou.geometry " + fromClause;
-      String ouGeoSql = getSelectSubquery(attribute, selectExpression, dataFilterClause);
+      String ouGeoSql = getSelectSubquery(attribute, selectExpression);
       columns.add(
           AnalyticsTableColumn.builder()
               .name((attribute.getUid() + OU_GEOMETRY_COL_SUFFIX))
@@ -272,7 +246,7 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
     }
 
     String selectExpression = "ou.name " + fromClause;
-    String ouNameSql = getSelectSubquery(attribute, selectExpression, dataFilterClause);
+    String ouNameSql = getSelectSubquery(attribute, selectExpression);
 
     columns.add(
         AnalyticsTableColumn.builder()
@@ -291,20 +265,17 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
    *
    * @param attribute the {@link TrackedEntityAttribute}.
    * @param selectExpression the select expression.
-   * @param dataFilterClause the data filter clause.
    * @return a select statement.
    */
-  private String getSelectSubquery(
-      TrackedEntityAttribute attribute, String selectExpression, String dataFilterClause) {
+  private String getSelectSubquery(TrackedEntityAttribute attribute, String selectExpression) {
     return replaceQualify(
         """
         (select ${selectExpression} from ${trackedentityattributevalue} \
         where trackedentityid=en.trackedentityid \
-        and trackedentityattributeid=${attributeId}${dataFilterClause})\
+        and trackedentityattributeid=${attributeId})\
         ${closingParentheses} as ${attributeUid}""",
         Map.of(
             "selectExpression", selectExpression,
-            "dataFilterClause", dataFilterClause,
             "attributeId", String.valueOf(attribute.getId()),
             "closingParentheses", getClosingParentheses(selectExpression),
             "attributeUid", quote(attribute.getUid())));

diff --git a/...analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManager.java b/...analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManager.java
@@ -39,6 +39,7 @@
 import static org.hisp.dhis.db.model.DataType.GEOMETRY;
 import static org.hisp.dhis.db.model.DataType.INTEGER;
 import static org.hisp.dhis.db.model.DataType.TEXT;
+import static org.hisp.dhis.system.util.MathUtils.NUMERIC_LENIENT_REGEXP;
 import static org.hisp.dhis.util.DateUtils.toLongDate;
 import static org.hisp.dhis.util.DateUtils.toMediumDate;
 
@@ -502,7 +503,7 @@ private List<AnalyticsTableColumn> getColumnForDataElement(
     }
 
     if (dataElement.getValueType().isOrganisationUnit()) {
-      columns.addAll(getColumnForOrgUnitDataElement(dataElement, dataFilterClause));
+      columns.addAll(getColumnForOrgUnitDataElement(dataElement));
     }
 
     columns.add(
@@ -524,8 +525,7 @@ private List<AnalyticsTableColumn> getColumnForDataElement(
    * @param dataFilterClause the data filter SQL clause.
    * @return a list of {@link AnalyticsTableColumn}.
    */
-  private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(
-      DataElement dataElement, String dataFilterClause) {
+  private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(DataElement dataElement) {
     List<AnalyticsTableColumn> columns = new ArrayList<>();
 
     String columnExpression =
@@ -535,20 +535,20 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(
 
     if (isSpatialSupport()) {
       String fromType = "ou.geometry " + fromClause;
-      String geoSql = getOrgUnitSelectExpression(dataElement, fromType, dataFilterClause);
+      String geoExpression = getOrgUnitSelectExpression(dataElement, fromType);
 
       columns.add(
           AnalyticsTableColumn.builder()
               .name((dataElement.getUid() + OU_GEOMETRY_COL_SUFFIX))
               .dimensionType(AnalyticsDimensionType.DYNAMIC)
               .dataType(GEOMETRY)
-              .selectExpression(geoSql)
+              .selectExpression(geoExpression)
               .indexType(IndexType.GIST)
               .build());
     }
 
     String fromTypeSql = "ou.name " + fromClause;
-    String ouNameSql = getOrgUnitSelectExpression(dataElement, fromTypeSql, dataFilterClause);
+    String ouNameSql = getOrgUnitSelectExpression(dataElement, fromTypeSql);
 
     columns.add(
         AnalyticsTableColumn.builder()
@@ -592,7 +592,7 @@ private List<AnalyticsTableColumn> getAttributeColumns(Program program) {
   private List<AnalyticsTableColumn> getColumnForAttributeWithLegendSet(
       TrackedEntityAttribute attribute) {
     String columnExpression = getColumnExpression(attribute.getValueType(), "value");
-    String numericClause = getNumericClause();
+    String numericClause = getNumericClause("value");
     String query =
         """
         \s(select l.uid from ${maplegend} l \
@@ -630,18 +630,15 @@ private List<AnalyticsTableColumn> getColumnForAttributeWithLegendSet(
    *
    * @param dataElement the data element to create the select statement for.
    * @param selectExpression the select expression.
-   * @param dataFilterClause the data filter clause.
    * @return a select expression.
    */
-  private String getOrgUnitSelectExpression(
-      DataElement dataElement, String selectExpression, String dataFilterClause) {
+  private String getOrgUnitSelectExpression(DataElement dataElement, String selectExpression) {
     Validate.isTrue(dataElement.getValueType().isOrganisationUnit());
     String prts = getClosingParentheses(selectExpression);
     return replaceQualify(
-        "(select ${selectExpression} ${dataFilterClause})${closingParentheses} as ${uid}",
+        "(select ${selectExpression})${closingParentheses} as ${uid}",
         Map.of(
             "selectExpression", selectExpression,
-            "dataFilterClause", dataFilterClause,
             "closingParentheses", prts,
             "uid", quote(dataElement.getUid())));
   }
@@ -756,6 +753,16 @@ private List<Integer> getDataYears(
     return jdbcTemplate.queryForList(sql, Integer.class);
   }
 
+  /**
+   * Returns a numeric regexp match expression for the given value.
+   *
+   * @param value the value.
+   * @return a numeric regexp match expression.
+   */
+  private final String getNumericClause(String value) {
+    return " and " + sqlBuilder.regexpMatch(value, "'" + NUMERIC_LENIENT_REGEXP + "'");
+  }
+
   /**
    * Retrieve years for partition tables. Year will become a partition key. The default return value
    * is the list with the recent year.

diff --git a/.../src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerDorisTest.java b/.../src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerDorisTest.java
@@ -169,7 +169,7 @@ void verifyGetTableWithDataElements() {
         "case when json_unquote(json_extract(eventdatavalues, '$.%s.value')) regexp '^\\d{4}-\\d{2}-\\d{2}(\\s|T)?((\\d{2}:)(\\d{2}:)?(\\d{2}))?(|.(\\d{3})|.(\\d{3})Z)?$' then cast(json_unquote(json_extract(eventdatavalues, '$.%s.value')) as datetime) else null end as `%s`";
     String aliasE = "json_unquote(json_extract(eventdatavalues, '$.%s.value')) as `%s`";
     String aliasF =
-        "(select ou.name from dhis2.public.`organisationunit` ou where ou.uid = json_unquote(json_extract(eventdatavalues, '$.%s.value')) ) as `%s`";
+        "(select ou.name from dhis2.public.`organisationunit` ou where ou.uid = json_unquote(json_extract(eventdatavalues, '$.%s.value'))) as `%s`";
     String aliasG =
         "case when json_unquote(json_extract(eventdatavalues, '$.%s.value')) regexp '^(-?[0-9]+)(\\.[0-9]+)?$' then cast(json_unquote(json_extract(eventdatavalues, '$.%s.value')) as bigint) else null end as `%s`";
 

diff --git a/...ytics/src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerTest.java b/...ytics/src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerTest.java
@@ -399,15 +399,13 @@ void verifyGetTableWithDataElements() {
     String aliasD5_geo =
         "(select ou.geometry from \"organisationunit\" ou where ou.uid = eventdatavalues #>> '{"
             + d5.getUid()
-            + ", value}' "
-            + ") as \""
+            + ", value}') as \""
             + d5.getUid()
             + "\"";
     String aliasD5_name =
         "(select ou.name from \"organisationunit\" ou where ou.uid = eventdatavalues #>> '{"
             + d5.getUid()
-            + ", value}' "
-            + ") as \""
+            + ", value}') as \""
             + d5.getUid()
             + "\"";
     AnalyticsTableUpdateParams params =
@@ -599,7 +597,7 @@ void verifyDataElementTypeOrgUnitFetchesOuNameWhenPopulatingEventAnalyticsTable(
         String.format(
             """
             (select ou.name from "organisationunit" ou where ou.uid = \
-            eventdatavalues #>> '{%s, value}' ) as %s""",
+            eventdatavalues #>> '{%s, value}') as %s""",
             d5.getUid(), quote(d5.getUid()));
 
     assertThat(sql.getValue(), containsString(ouUidQuery));

diff --git a/...dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/util/AnalyticsUtilsTest.java b/...dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/util/AnalyticsUtilsTest.java
@@ -93,7 +93,6 @@
 import org.hisp.dhis.test.TestBase;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
-import org.springframework.jdbc.BadSqlGrammarException;
 import org.springframework.jdbc.UncategorizedSQLException;
 
 /**
@@ -763,31 +762,14 @@ void whenUncategorizedSQLException_withTableNotExisting_thenThrowException() {
         () -> AnalyticsUtils.withExceptionHandling(supplier, false));
   }
 
-  @Test
-  void whenBadSqlGrammarException_withMultipleQueries_thenReturnEmpty() {
-    SQLException sqlException = new SQLException("syntax error");
-    BadSqlGrammarException badSqlException =
-        new BadSqlGrammarException("task", DUMMY_SQL, sqlException);
-    Supplier<String> supplier =
-        () -> {
-          throw badSqlException;
-        };
-
-    Optional<String> result = AnalyticsUtils.withExceptionHandling(supplier, true);
-
-    assertFalse(result.isPresent());
-  }
-
   @Test
   void whenQueryRuntimeException_thenRethrow() {
-    // Arrange
     QueryRuntimeException queryException = new QueryRuntimeException("Test error");
     Supplier<String> supplier =
         () -> {
           throw queryException;
         };
 
-    // Act & Assert
     QueryRuntimeException thrown =
         assertThrows(
             QueryRuntimeException.class,