Skip to content

Commit

Permalink
Address more SQ issues
Browse files Browse the repository at this point in the history
  • Loading branch information
luciano-fiandesio committed Jan 20, 2025
1 parent 43a02c4 commit 24e7ebf
Show file tree
Hide file tree
Showing 10 changed files with 368 additions and 83 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ public void getEnrollments(EventQueryParams params, Grid grid, int maxLimit) {
? buildEnrollmentQueryWithCte(params)
: getAggregatedEnrollmentsSql(params, maxLimit);
}
System.out.println(sql); // FIXME remove
if (params.analyzeOnly()) {
withExceptionHandling(
() -> executionPlanStore.addExecutionPlan(params.getExplainOrderId(), sql));
Expand Down Expand Up @@ -593,7 +592,7 @@ private Condition buildCteConditions(
private List<Condition> buildItemConditions(QueryItem item, CteDefinition cteDef) {
return item.getFilters().stream()
.map(filter -> buildFilterCondition(filter, item, cteDef))
.collect(Collectors.toList());
.toList();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

/**
Expand All @@ -46,6 +47,10 @@ public sealed interface Condition
Condition.Raw,
NotCondition,
SimpleCondition {

Pattern WHERE_AND_PATTERN = Pattern.compile("^(?i)(where|and)\\b.*");
Pattern WHERE_AND_REPLACE_PATTERN = Pattern.compile("^(?i)(where|and)\\s+");

/**
* Converts the condition to its SQL string representation.
*
Expand Down Expand Up @@ -83,8 +88,8 @@ public String toSql() {

// Remove only the first occurrence of WHERE or AND
String cleaned = sql.trim();
if (cleaned.toLowerCase().matches("^(where|and)\\b.*")) {
cleaned = cleaned.replaceFirst("(?i)^(where|and)\\s+", "");
if (WHERE_AND_PATTERN.matcher(cleaned.toLowerCase()).matches()) {
cleaned = WHERE_AND_REPLACE_PATTERN.matcher(cleaned).replaceFirst("");
}

return cleaned.trim();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright (c) 2004-2025, 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.util.sql;

import lombok.experimental.UtilityClass;

@UtilityClass
public class QuoteUtils {

/**
* Removes surrounding quotes (double quotes <code>"</code> or backticks <code>`</code>) from a
* string. If the string is not quoted or the quotes do not match, the original string is returned
* unchanged.
*
* <p><b>Behavior:</b>
*
* <ul>
* <li>Returns an empty string if the input is <code>null</code> or empty.
* <li>Returns the original string if it is too short to have matching quotes (less than 2
* characters).
* <li>Removes quotes only if the first and last characters are matching quotes (either <code>"
* </code> or <code>`</code>).
* <li>Returns the original string if the quotes do not match or if the string is not quoted.
* </ul>
*
* <p><b>Examples:</b>
*
* <ul>
* <li><code>unquote("\"quoted\"")</code> returns <code>"quoted"</code>
* <li><code>unquote("`backticked`")</code> returns <code>"backticked"</code>
* <li><code>unquote("not quoted")</code> returns <code>"not quoted"</code>
* <li><code>unquote("\"mismatch`")</code> returns <code>"\"mismatch`"</code>
* <li><code>unquote("")</code> returns <code>""</code>
* <li><code>unquote(null)</code> returns <code>""</code>
* </ul>
*
* @param quoted The string from which to remove quotes. Can be <code>null</code> or empty.
* @return The string without surrounding quotes, or the original string if it is not quoted.
* Returns an empty string if the input is <code>null</code> or empty.
*/
static String unquote(String quoted) {
// Handle null or empty
if (quoted == null || quoted.isEmpty()) {
return "";
}

// Check minimum length (needs at least 2 chars for quotes)
if (quoted.length() < 2) {
return quoted;
}

char firstChar = quoted.charAt(0);
char lastChar = quoted.charAt(quoted.length() - 1);

// Check if quotes match
if ((firstChar == '"' && lastChar == '"') || (firstChar == '`' && lastChar == '`')) {
return quoted.substring(1, quoted.length() - 1);
}

return quoted;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@
*/
package org.hisp.dhis.analytics.util.sql;

import static org.hisp.dhis.analytics.util.sql.QuoteUtils.unquote;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

/**
Expand Down Expand Up @@ -68,6 +71,7 @@ public class SelectBuilder {
private final List<OrderByClause> orderByClauses = new ArrayList<>();
private Integer limit;
private Integer offset;
private static final Pattern ORDER_BY_PATTERN = Pattern.compile("^(?i)order\\s+by\\s+");

/**
* Represents a column in the SELECT clause of a SQL query. Handles column expressions with
Expand Down Expand Up @@ -428,7 +432,7 @@ public SelectBuilder orderBy(String rawSortClause) {
}

// Remove "order by" prefix if present
String cleaned = rawSortClause.trim().replaceFirst("(?i)^order\\s+by\\s+", "");
String cleaned = ORDER_BY_PATTERN.matcher(rawSortClause.trim()).replaceFirst("");

// Split by commas, but not commas within CASE statements
List<String> parts = splitPreservingCaseStatements(cleaned);
Expand Down Expand Up @@ -728,26 +732,4 @@ private String sanitizeFromClause(String input) {

return sanitized;
}

private static String unquote(String quoted) {
// Handle null or empty
if (quoted == null || quoted.isEmpty()) {
return "";
}

// Check minimum length (needs at least 2 chars for quotes)
if (quoted.length() < 2) {
return quoted;
}

char firstChar = quoted.charAt(0);
char lastChar = quoted.charAt(quoted.length() - 1);

// Check if quotes match
if ((firstChar == '"' && lastChar == '"') || (firstChar == '`' && lastChar == '`')) {
return quoted.substring(1, quoted.length() - 1);
}

return quoted;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,13 @@ public static String replaceTableAliases(String whereClause, List<String> column
expr.accept(visitor);
return expr.toString();
} catch (Exception e) {
throw new RuntimeException("Error parsing SQL where clause: " + e.getMessage(), e);
throw new IllegalArgumentException("Error parsing SQL where clause: " + e.getMessage(), e);
}
}

private static class ColumnReplacementVisitor extends ExpressionVisitorAdapter {
private final Set<String> columns;
private static final Table PLACEHOLDER_TABLE = new Table("%s");
private static final Table OUTER_REFERENCE_TABLE = new Table("%z"); // New constant

private boolean inSubQuery = false;

Expand All @@ -120,7 +119,6 @@ public ColumnReplacementVisitor(List<String> columns) {
public void visit(Column column) {
String columnName = column.getColumnName();
String rawColumnName = stripQuotes(columnName);
Table currentTable = column.getTable();

if (columns.contains(rawColumnName.toLowerCase())) {
String quoteType = getQuoteType(columnName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
*/
package org.hisp.dhis.analytics.util.sql;

import static org.hisp.dhis.analytics.util.sql.QuoteUtils.unquote;

import lombok.experimental.UtilityClass;
import net.sf.jsqlparser.expression.Expression;
import net.sf.jsqlparser.parser.CCJSqlParserUtil;
Expand Down Expand Up @@ -60,30 +62,7 @@ public static String removeTableAlias(String columnReference) {
// Extract the column name
return unquote(column.getColumnName());
} catch (Exception e) {
throw new RuntimeException("Error parsing SQL: " + e.getMessage(), e);
}
}

// FIXME - this method is duplicated in SqlWhereClauseExtractor
private static String unquote(String quoted) {
// Handle null or empty
if (quoted == null || quoted.isEmpty()) {
return "";
}

// Check minimum length (needs at least 2 chars for quotes)
if (quoted.length() < 2) {
return quoted;
throw new IllegalArgumentException("Error parsing SQL: " + e.getMessage(), e);
}

char firstChar = quoted.charAt(0);
char lastChar = quoted.charAt(quoted.length() - 1);

// Check if quotes match
if ((firstChar == '"' && lastChar == '"') || (firstChar == '`' && lastChar == '`')) {
return quoted.substring(1, quoted.length() - 1);
}

return quoted;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,36 +27,75 @@
*/
package org.hisp.dhis.analytics.util.sql;

import java.util.Arrays;
import java.util.stream.Collectors;
import lombok.experimental.UtilityClass;

/**
* A utility class for joining multiple SQL conditions into a single WHERE clause. This class is
* designed to handle conditions that may or may not include the "WHERE" keyword and ensures proper
* formatting of the final SQL WHERE clause.
*/
@UtilityClass
public class SqlConditionJoiner {

/**
* Joins multiple SQL conditions into a single WHERE clause, separated by the <code>AND</code>
* operator. Conditions that are null, empty, or consist solely of whitespace are ignored.
*
* <p><b>Behavior:</b>
*
* <ul>
* <li>Returns an empty string if no valid conditions are provided (null, empty, or all
* conditions are blank).
* <li>Removes leading "WHERE" or " where" from individual conditions before joining.
* <li>Joins valid conditions with the <code>AND</code> operator.
* <li>Adds a "WHERE" prefix to the final result if at least one valid condition is present.
* </ul>
*
* <p><b>Examples:</b>
*
* <ul>
* <li><code>joinSqlConditions("column1 = 1", "column2 = 2")</code> returns <code>
* "where column1 = 1 and column2 = 2"</code>
* <li><code>joinSqlConditions("where column1 = 1", "where column2 = 2")</code> returns <code>
* "where column1 = 1 and column2 = 2"</code>
* <li><code>joinSqlConditions("", null, "column1 = 1")</code> returns <code>"where column1 = 1"
* </code>
* <li><code>joinSqlConditions()</code> returns <code>""</code>
* <li><code>joinSqlConditions(null, "", " ")</code> returns <code>""</code>
* </ul>
*
* @param conditions The SQL conditions to join. Can be null, empty, or contain null/blank
* strings.
* @return A single WHERE clause combining the valid conditions with <code>AND</code> operators.
* Returns an empty string if no valid conditions are provided.
*/
public static String joinSqlConditions(String... conditions) {

if (conditions == null || conditions.length == 0) {
return "";
}

StringBuilder result = new StringBuilder("where ");
boolean firstCondition = true;

for (String condition : conditions) {
if (condition == null || condition.trim().isEmpty()) {
continue;
}

// Remove leading "where" or " where" and trim
String cleanedCondition = condition.trim();
if (cleanedCondition.toLowerCase().startsWith("where")) {
cleanedCondition = cleanedCondition.substring(5).trim();
}

if (!cleanedCondition.isEmpty()) {
if (!firstCondition) {
result.append(" and ");
}
result.append(cleanedCondition);
firstCondition = false;
}
}
// Filter out null, empty, or blank conditions
String joinedConditions =
Arrays.stream(conditions)
.filter(condition -> condition != null && !condition.trim().isEmpty())
.map(
condition -> {
// Remove leading "where" or " where" and trim
String cleanedCondition = condition.trim();
if (cleanedCondition.toLowerCase().startsWith("where")) {
cleanedCondition = cleanedCondition.substring(5).trim();
}
return cleanedCondition;
})
.filter(
cleanedCondition ->
!cleanedCondition.isEmpty()) // Filter out empty strings after cleaning
.collect(Collectors.joining(" and "));

return result.toString();
// Return the result with "where" prefix if there are valid conditions
return joinedConditions.isEmpty() ? "" : "where " + joinedConditions;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,13 @@
package org.hisp.dhis.analytics.util.sql;

import java.util.Set;
import java.util.regex.Pattern;
import lombok.experimental.UtilityClass;

@UtilityClass
public class SqlFormatter {
private static final Pattern WHITESPACE_PATTERN = Pattern.compile("\\s+");

private static final Set<String> MAIN_CLAUSES =
Set.of(
"with",
Expand Down Expand Up @@ -121,7 +126,7 @@ public static String lowercase(String sql) {
}

// Replace all whitespace sequences (including newlines) with a single space
result = result.replaceAll("\\s+", " ");
result = WHITESPACE_PATTERN.matcher(result).replaceAll(" ");

return result.trim();
}
Expand All @@ -132,9 +137,7 @@ private static String formatParentheses(String sql) {
boolean inString = false;
char[] chars = sql.toCharArray();

for (int i = 0; i < chars.length; i++) {
char c = chars[i];

for (char c : chars) {
// Handle string literals
if (c == '\'') {
inString = !inString;
Expand Down
Loading

0 comments on commit 24e7ebf

Please sign in to comment.