Skip to content

Commit

Permalink
feat: handle Doris JDBC exceptions same as Postgres errors (#19370)
Browse files Browse the repository at this point in the history
  • Loading branch information
@luciano-fiandesio
luciano-fiandesio authored Dec 3, 2024
1 parent dc0fa5e commit 142bf3c
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 38 deletions.
12 changes: 11 additions & 1 deletion dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/SqlExceptionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@

@NoArgsConstructor(access = AccessLevel.PRIVATE)
public class SqlExceptionUtils {

private static final String PG_TABLE_NOT_EXISTING = "42P01";

public static final String ERR_MSG_TABLE_NOT_EXISTING =
"Query failed, likely because the requested analytics table does not exist: ";

Expand All @@ -52,7 +55,14 @@ public class SqlExceptionUtils {
*/
public static boolean relationDoesNotExist(SQLException ex) {
if (ex != null) {
return Optional.of(ex).map(SQLException::getSQLState).filter("42P01"::equals).isPresent();
return Optional.of(ex)
.map(SQLException::getSQLState)
.filter(PG_TABLE_NOT_EXISTING::equals)
.isPresent()
|| Optional.of(ex)
.map(SQLException::getMessage)
.filter(m -> m.contains("Table") && m.contains("does not exist in database"))
.isPresent();
}

return false;
Expand Down
31 changes: 21 additions & 10 deletions ...ces/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/util/AnalyticsUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.math.BigDecimal;
import java.sql.SQLException;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Date;
Expand Down Expand Up @@ -115,9 +116,11 @@
import org.hisp.dhis.system.grid.ListGrid;
import org.hisp.dhis.util.DateUtils;
import org.joda.time.DateTime;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.DataAccessResourceFailureException;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.jdbc.BadSqlGrammarException;
import org.springframework.jdbc.UncategorizedSQLException;
import org.springframework.util.Assert;

/**
Expand Down Expand Up @@ -1154,16 +1157,8 @@ public static <T> Optional<T> withExceptionHandling(
Supplier<T> supplier, boolean isMultipleQueries) {
try {
return Optional.ofNullable(supplier.get());
} catch (BadSqlGrammarException ex) {
if (relationDoesNotExist(ex.getSQLException())) {
log.info(ERR_MSG_TABLE_NOT_EXISTING, ex);
throw ex;
}
if (!isMultipleQueries) {
log.error(ERR_MSG_SQL_SYNTAX_ERROR, ex);
throw ex;
}
log.info(ERR_MSG_SILENT_FALLBACK, ex);
} catch (UncategorizedSQLException | BadSqlGrammarException usq) {
handleDataAccessException(usq, isMultipleQueries);
} catch (QueryRuntimeException ex) {
log.error("Internal runtime exception", ex);
throw ex;
Expand All @@ -1178,6 +1173,22 @@ public static <T> Optional<T> withExceptionHandling(
return Optional.empty();
}

private static void handleDataAccessException(DataAccessException ex, boolean isMultipleQueries) {
if (ex.getCause() instanceof SQLException sqlexception) {
if (relationDoesNotExist(sqlexception)) {
log.info(ERR_MSG_TABLE_NOT_EXISTING, ex);
throw ex;
}
if (!isMultipleQueries) {
log.error(ERR_MSG_SQL_SYNTAX_ERROR, ex);
throw ex;
}
log.info(ERR_MSG_SILENT_FALLBACK, ex);
} else {
throw ex;
}
}

/**
* Retrieves the sql string with content replacement between for example select and from
*
Expand Down
6 changes: 5 additions & 1 deletion ...tics/src/test/java/org/hisp/dhis/analytics/event/data/EnrollmentAnalyticsManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.sql.SQLException;
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
Expand Down Expand Up @@ -379,7 +380,10 @@ void testBadGrammarExceptionWithMultipleQueries() {
// Given
mockEmptyRowSet();
EventQueryParams params = createRequestParamsWithMultipleQueries();
when(jdbcTemplate.queryForRowSet(anyString())).thenThrow(BadSqlGrammarException.class);
SQLException sqlException = new SQLException("Some exception", "HY000");
BadSqlGrammarException badSqlGrammarException =
new BadSqlGrammarException("task", "select * from nothing", sqlException);
when(jdbcTemplate.queryForRowSet(anyString())).thenThrow(badSqlGrammarException);

// Then
assertDoesNotThrow(() -> subject.getEnrollments(params, new ListGrid(), 10000));
Expand Down
126 changes: 100 additions & 26 deletions ...dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/util/AnalyticsUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,14 @@
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.google.common.collect.Lists;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Supplier;
import org.apache.commons.lang3.EnumUtils;
import org.hisp.dhis.analytics.DataQueryParams;
import org.hisp.dhis.category.Category;
Expand All @@ -64,6 +67,7 @@
import org.hisp.dhis.common.DisplayProperty;
import org.hisp.dhis.common.Grid;
import org.hisp.dhis.common.GridHeader;
import org.hisp.dhis.common.QueryRuntimeException;
import org.hisp.dhis.common.ValueType;
import org.hisp.dhis.dataelement.DataElement;
import org.hisp.dhis.dataelement.DataElementOperand;
Expand All @@ -87,12 +91,18 @@
import org.hisp.dhis.program.ProgramIndicator;
import org.hisp.dhis.system.grid.ListGrid;
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;

/**
* @author Lars Helge Overland
*/
class AnalyticsUtilsTest extends TestBase {

private static final String DUMMY_SQL = "SELECT * FROM dummy";

@Test
void testGetByDataDimensionType() {
Program prA = createProgram('A');
Expand Down Expand Up @@ -182,7 +192,7 @@ void testConvertDxToOperandNone() {
void testGetRoundedValueObject() {
DataQueryParams paramsA = DataQueryParams.newBuilder().build();
DataQueryParams paramsB = DataQueryParams.newBuilder().withSkipRounding(true).build();
assertEquals(null, AnalyticsUtils.getRoundedValueObject(paramsA, null), "Should be null");
assertNull(AnalyticsUtils.getRoundedValueObject(paramsA, null), "Should be null");
assertEquals(
"Car", AnalyticsUtils.getRoundedValueObject(paramsA, "Car"), "Should be a String: Car");
assertEquals(
Expand Down Expand Up @@ -229,15 +239,15 @@ void testEndsWithZeroDecimal() {
void testGetRoundedValueDouble() {
DataQueryParams paramsA = DataQueryParams.newBuilder().build();
DataQueryParams paramsB = DataQueryParams.newBuilder().withSkipRounding(true).build();
assertEquals(null, AnalyticsUtils.getRoundedValue(paramsA, null, (Double) null));
assertNull(AnalyticsUtils.getRoundedValue(paramsA, null, (Double) null));
assertEquals(3d, AnalyticsUtils.getRoundedValue(paramsA, null, 3d).doubleValue(), 0.01);
assertEquals(3.12, AnalyticsUtils.getRoundedValue(paramsA, null, 3.123).doubleValue(), 0.01);
assertEquals(3.1, AnalyticsUtils.getRoundedValue(paramsA, 1, 3.123).doubleValue(), 0.01);
assertEquals(3.12, AnalyticsUtils.getRoundedValue(paramsA, 2, 3.123).doubleValue(), 0.01);
assertEquals(3.123, AnalyticsUtils.getRoundedValue(paramsB, 3, 3.123).doubleValue(), 0.01);
assertEquals(3l, AnalyticsUtils.getRoundedValue(paramsB, 0, 3.123).longValue());
assertEquals(12l, AnalyticsUtils.getRoundedValue(paramsB, 0, 12.34).longValue());
assertEquals(13l, AnalyticsUtils.getRoundedValue(paramsB, 0, 13.999).longValue());
assertEquals(3L, AnalyticsUtils.getRoundedValue(paramsB, 0, 3.123).longValue());
assertEquals(12L, AnalyticsUtils.getRoundedValue(paramsB, 0, 12.34).longValue());
assertEquals(13L, AnalyticsUtils.getRoundedValue(paramsB, 0, 13.999).longValue());
}

@Test
Expand Down Expand Up @@ -403,23 +413,23 @@ void testHandleGridForDataValueSet() {
assertEquals("deabcdefghB", grid.getRow(1).get(0));
assertNull(grid.getRow(1).get(3));
assertNull(grid.getRow(1).get(4));
assertEquals((Double) grid.getRow(1).get(5), 0.01, 2d);
assertEquals(0.01, (Double) grid.getRow(1).get(5), 2d);
assertEquals("deabcdefghC", grid.getRow(2).get(0));
assertNull(grid.getRow(2).get(3));
assertEquals("ceabcdefghA", grid.getRow(2).get(4));
assertEquals(3, grid.getRow(2).get(5));
assertEquals("deabcdefghD", grid.getRow(3).get(0));
assertEquals("ceabcdefghB", grid.getRow(3).get(3));
assertNull(grid.getRow(3).get(4));
assertEquals((Double) grid.getRow(3).get(5), 0.01, 4d);
assertEquals(0.01, (Double) grid.getRow(3).get(5), 4d);
assertEquals("deabcdefghA", grid.getRow(4).get(0));
assertEquals("ceabcdefghA", grid.getRow(4).get(3));
assertNull(grid.getRow(4).get(4));
assertEquals(5, grid.getRow(4).get(5));
assertEquals("deabcdefghB", grid.getRow(5).get(0));
assertEquals("ceabcdefghA", grid.getRow(5).get(3));
assertNull(grid.getRow(5).get(4));
assertEquals((Double) grid.getRow(5).get(5), 0.01, 6d);
assertEquals(0.01, (Double) grid.getRow(5).get(5), 6d);
}

@Test
Expand Down Expand Up @@ -613,20 +623,20 @@ void testIsPeriodOverApprovalThreshold() {
Integer twoYearsAgo = currentYear - 2;
Integer threeYearsAgo = currentYear - 3;
// maxYears = 0 should always return false
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 0));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(oneYearAgo, 0));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(twoYearsAgo, 0));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(threeYearsAgo, 0));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 0));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(oneYearAgo, 0));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(twoYearsAgo, 0));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(threeYearsAgo, 0));
// maxYears = 1 should only return true for years other than thisYear
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 1));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 1));
assertTrue(AnalyticsUtils.periodIsOutsideApprovalMaxYears(oneYearAgo, 1));
assertTrue(AnalyticsUtils.periodIsOutsideApprovalMaxYears(twoYearsAgo, 1));
assertTrue(AnalyticsUtils.periodIsOutsideApprovalMaxYears(threeYearsAgo, 1));
// maxYears = 4 should only return false for all three years defined
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 5));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(oneYearAgo, 5));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(twoYearsAgo, 5));
assertTrue(!AnalyticsUtils.periodIsOutsideApprovalMaxYears(threeYearsAgo, 5));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(thisYear, 5));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(oneYearAgo, 5));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(twoYearsAgo, 5));
assertFalse(AnalyticsUtils.periodIsOutsideApprovalMaxYears(threeYearsAgo, 5));
}

@Test
Expand Down Expand Up @@ -654,18 +664,18 @@ void testGetIntegerOrValue() {
@Test
void testCalculateYearlyWeightedAverage() {
double avg = AnalyticsUtils.calculateYearlyWeightedAverage(10D, 20D, 9D);
assertEquals(avg, 0, 17.5);
assertEquals(0, avg, 17.5);
avg = AnalyticsUtils.calculateYearlyWeightedAverage(10D, -20D, 9D);
assertEquals(avg, 0, 12.5);
assertEquals(0, avg, 12.5);
}

@Test
void testGetBaseMonth() {
assertEquals(AnalyticsUtils.getBaseMonth(new FinancialAprilPeriodType()), 0, 3);
assertEquals(AnalyticsUtils.getBaseMonth(new FinancialJulyPeriodType()), 0, 6);
assertEquals(AnalyticsUtils.getBaseMonth(new FinancialOctoberPeriodType()), 0, 9);
assertEquals(AnalyticsUtils.getBaseMonth(new FinancialNovemberPeriodType()), 0, 10);
assertEquals(AnalyticsUtils.getBaseMonth(new DailyPeriodType()), 0, 0);
assertEquals(0, AnalyticsUtils.getBaseMonth(new FinancialAprilPeriodType()), 3);
assertEquals(0, AnalyticsUtils.getBaseMonth(new FinancialJulyPeriodType()), 6);
assertEquals(0, AnalyticsUtils.getBaseMonth(new FinancialOctoberPeriodType()), 9);
assertEquals(0, AnalyticsUtils.getBaseMonth(new FinancialNovemberPeriodType()), 10);
assertEquals(0, AnalyticsUtils.getBaseMonth(new DailyPeriodType()), 0);
}

@Test
Expand All @@ -690,7 +700,7 @@ void testFindDimensionalItems() {
pi4.setUid(pi1.getUid());
List<DimensionalItemObject> dimensionalItems =
AnalyticsUtils.findDimensionalItems(pi1.getUid(), List.of(pi1, pi2, pi3, pi4));
assertEquals(dimensionalItems.size(), 2);
assertEquals(2, dimensionalItems.size());
}

@Test
Expand All @@ -717,7 +727,7 @@ void testHasPeriod() {

/** {@link EnumUtils} is case sensitive, not specified in the documentation. */
@Test
void testgetEnumCaseSensitivity() {
void testGetEnumCaseSensitivity() {
assertEquals(POSTGRESQL, EnumUtils.getEnum(Database.class, "POSTGRESQL"));
assertEquals(POSTGRESQL, EnumUtils.getEnum(Database.class, "PostgreSQL".toUpperCase()));
assertEquals(POSTGRESQL, EnumUtils.getEnum(Database.class, "postgresql".toUpperCase()));
Expand All @@ -737,4 +747,68 @@ void testGetClosingParentheses() {
assertEquals(")", AnalyticsUtils.getClosingParentheses("from(select(select (*))"));
assertEquals("))", AnalyticsUtils.getClosingParentheses("(("));
}

@Test
void whenUncategorizedSQLException_withTableNotExisting_thenThrowException() {
SQLException sqlException = new SQLException("relation does not exist", "42P01");
UncategorizedSQLException uncategorizedSQLException =
new UncategorizedSQLException("task", DUMMY_SQL, sqlException);
Supplier<String> supplier =
() -> {
throw uncategorizedSQLException;
};

assertThrows(
UncategorizedSQLException.class,
() -> 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,
() -> AnalyticsUtils.withExceptionHandling(supplier, false));
assertEquals("Test error", thrown.getMessage());
}

@Test
@DisplayName(
"When an UncategorizedSQLException is thrown with a column not existing and multiple queries, then return empty")
void whenUncategorizedSQLException_withColumnNotExisting_thenReturnEmpty() {
SQLException sqlException =
new SQLException("Unknown column 'cX5k9anHEHd' in 'ax' in FILTER clause", "HY000");
UncategorizedSQLException uncategorizedSQLException =
new UncategorizedSQLException("task", DUMMY_SQL, sqlException);
Supplier<String> supplier =
() -> {
throw uncategorizedSQLException;
};

Optional<String> result = AnalyticsUtils.withExceptionHandling(supplier, true);
assertFalse(result.isPresent());
}
}

0 comments on commit 142bf3c

Please sign in to comment.