SONARJAVA-7215 Do not report S2695 when part of the query cannot be resolved

java-checks-test-sources/default/src/main/java/checks/PreparedStatementAndResultSetCheckSample.java

@@ -155,10 +155,10 @@ public void updateCoffeeSales(HashMap<String, Integer> salesForWeek, Connection
       updateString = "update " + param + ".COFFEES set SALES = ?";
 
       PreparedStatement testParam = con.prepareStatement(updateString);
-      testParam.setInt(3, 0); // Noncompliant
+      testParam.setInt(3, 0); // Compliant - "param" is unknown and may itself contain placeholders
 
       testParam = con.prepareStatement(param + " update");
-      testParam.setInt(3, 0); // Noncompliant
+      testParam.setInt(3, 0); // Compliant - "param" is unknown and may itself contain placeholders
 
       testParam = con.prepareStatement(param + param);
       testParam.setInt(3, 0); // Compliant
@@ -257,4 +257,152 @@ void concatenation3(Connection con, String password, String name, int position)
     statement.setString(3, password); // compliant
     statement.setString(4, password); // Noncompliant
   }
+
+  static class SqlConstants {
+    static final String SELECT = "SELECT * FROM t WHERE a = ?";
+    static final String AND_CLAUSE = " AND b = ?";
+  }
+
+  private static final String COUNT_WRAPPER_BEGIN = "select count(*) from (";
+  private static final String COUNT_WRAPPER_END = ")";
+
+  void unknownMethodCallInConcatenation(Connection con) throws SQLException {
+    String sql = getQuery();
+    sql = COUNT_WRAPPER_BEGIN + sql + COUNT_WRAPPER_END;
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setInt(1, 1); // Compliant - part of the query is unknown
+  }
+
+  void unknownMethodCallInPlusAssignment(Connection con) throws SQLException {
+    String sql = "select * from t where ";
+    sql += getQuery();
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setInt(1, 1); // Compliant - part of the query is unknown
+  }
+
+  void ternaryInPlusAssignment(Connection con, boolean isRecall) throws SQLException {
+    String sql = "SELECT * FROM t WHERE ";
+    sql += isRecall ? "LENDER_REF_ID = ? AND SENDER_ID = ?" : "BORROWER_REF_ID = ? AND RECIPIENT_ID = ?";
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setString(1, "a"); // Compliant
+    ps.setLong(2, 1L); // Compliant
+    ps.setLong(3, 1L); // Noncompliant
+  }
+
+  void ternaryWithDifferentBranches(Connection con, boolean withImage) throws SQLException {
+    String sql = "INSERT INTO Product (a, b" + (withImage ? ", c" : "") + ") VALUES (?, ?" + (withImage ? ", ?" : "") + ")";
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setString(1, "a"); // Compliant
+    ps.setString(2, "b"); // Compliant
+    ps.setString(3, "c"); // Compliant - the third parameter exists when the condition holds
+    ps.setString(4, "d"); // Noncompliant
+  }
+
+  void ternaryWithNonConstantFragment(Connection con, String tenant, int maxTurns) throws SQLException {
+    String sql = "SELECT * FROM m WHERE m.tenant = ? AND m.session_id = ? "
+      + (maxTurns > 0 ? "AND t.id IN (SELECT id FROM t WHERE tenant=? AND session=? LIMIT " + maxTurns + ") " : "")
+      + "ORDER BY m.seq";
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setString(1, tenant); // Compliant
+    if (maxTurns > 0) {
+      ps.setString(3, tenant); // Compliant
+      ps.setString(4, tenant); // Compliant
+    }
+    // The whole query resolves: the int fragment contributes no placeholder and the branch taken contributes two.
+    ps.setString(5, tenant); // Noncompliant {{This "PreparedStatement" only has 4 parameters.}}
+  }
+
+  void ternaryWithUnresolvableBranch(Connection con, boolean cond) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE a = ? " + (cond ? getQuery() : ""));
+    ps.setString(5, "x"); // Compliant - one branch of the conditional is unknown, so the count is too
+  }
+
+  void assignedInBothBranches(Connection con, boolean hasPassword) throws SQLException {
+    String sql;
+    if (hasPassword) {
+      sql = "UPDATE Account SET email = ?, name = ?, password = ? WHERE id = ?";
+    } else {
+      sql = "UPDATE Account SET email = ?, name = ? WHERE id = ?";
+    }
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setString(1, "a"); // Compliant
+    if (hasPassword) {
+      // FALSE POSITIVE: only the closest assignment is resolved, while the branch taken here has 4 parameters.
+      // Can't be fixed without dataflow analysis.
+      ps.setString(4, "b"); // Noncompliant
+    }
+  }
+
+  void nonConstantNumericFragment(Connection con, int limit, Integer offset) throws SQLException {
+    String sql = "SELECT * FROM t WHERE id = ? LIMIT " + limit + " OFFSET " + offset;
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setInt(1, 1); // Compliant
+    // A number concatenated into the query can not add a placeholder, so the count stays exact.
+    ps.setInt(2, 2); // Noncompliant
+  }
+
+  void nonConstantNumericFragmentFromMethod(Connection con) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? LIMIT " + getIntValue());
+    ps.setInt(2, 2); // Noncompliant
+  }
+
+  void boxedCharFragment(Connection con, Character separator) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + separator);
+    ps.setInt(2, 2); // Compliant - the character may be '?'
+    ps.setInt(3, 2); // Noncompliant {{This "PreparedStatement" only has 2 parameters.}}
+  }
+
+  void nonConstantCharFragment(Connection con, char separator) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + separator);
+    ps.setInt(2, 2); // Compliant - the character may be '?'
+    // A character adds at most one placeholder, so anything past that is still reportable.
+    ps.setInt(3, 2); // Noncompliant {{This "PreparedStatement" only has 2 parameters.}}
+  }
+
+  private static final char QUESTION_MARK = '?';
+
+  void charConstantFragment(Connection con) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + QUESTION_MARK);
+    // FALSE POSITIVE: index 1 is valid, the character constant is the placeholder. Constant folding renders a char as
+    // its numeric value, so the query resolves to "... WHERE id = 63" and the placeholder is lost. Fix belongs to the
+    // frontend, tracked by SONARJAVA-7228; remove this annotation once it lands.
+    ps.setInt(1, 1); // Noncompliant
+  }
+
+  void charLiteralIsThePlaceholder(Connection con) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + '?');
+    ps.setInt(1, 1); // Compliant
+    ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}}
+  }
+
+  void charLiteralIsNotAPlaceholder(Connection con) throws SQLException {
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = ? AND flag = " + 'y');
+    ps.setInt(1, 1); // Compliant
+    ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}}
+  }
+
+  void charLocalVariableFragment(Connection con) throws SQLException {
+    // the char-typed fragment is bounded at one placeholder
+    final char questionMark = '?';
+    PreparedStatement ps = con.prepareStatement("SELECT * FROM t WHERE id = " + questionMark);
+    ps.setInt(1, 1); // Compliant
+    ps.setInt(2, 2); // Noncompliant {{This "PreparedStatement" only has 1 parameters.}}
+  }
+
+  void constantsFromAnotherClass(Connection con) throws SQLException {
+    String sql = SqlConstants.SELECT;
+    sql += SqlConstants.AND_CLAUSE;
+
+    PreparedStatement ps = con.prepareStatement(sql);
+    ps.setString(1, "a"); // Compliant
+    ps.setString(2, "b"); // Compliant
+    ps.setString(3, "c"); // Noncompliant
+  }
 }

java-checks/src/main/java/org/sonar/java/checks/PreparedStatementAndResultSetCheck.java

@@ -26,9 +26,12 @@
 import org.sonar.java.model.ExpressionUtils;
 import org.sonar.plugins.java.api.semantic.MethodMatchers;
 import org.sonar.plugins.java.api.semantic.Symbol;
+import org.sonar.plugins.java.api.semantic.Type;
+import org.sonar.plugins.java.api.semantic.Type.Primitives;
 import org.sonar.plugins.java.api.tree.Arguments;
 import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
 import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
+import org.sonar.plugins.java.api.tree.ConditionalExpressionTree;
 import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.IdentifierTree;
 import org.sonar.plugins.java.api.tree.LiteralTree;
@@ -107,48 +110,105 @@ private static Integer getPreparedStatementNumberOfParameters(@Nullable Expressi
   }
 
   /**
-   * Count the number of query in the given expression. The method will reconstruct the string, if created in multiples steps.
-   * During the process, we are looking for closest reassignment of a variable. We will look for any reassignment before
-   * the first token of the argument "startingPointForReassignment". It is used to avoid infinite recursion in case the variable
-   * is reassigned and read in the same statement.
+   * Count the number of query parameters in the given expression. The method will reconstruct the string, if created in
+   * multiples steps. During the process, we are looking for closest reassignment of a variable. We will look for any
+   * reassignment before the first token of the argument "startingPointForReassignment". It is used to avoid infinite
+   * recursion in case the variable is reassigned and read in the same statement.
+   * Returns {@code null} as soon as any part of the query can not be resolved: an unresolved fragment may itself hold
+   * parameter placeholders, so the total count is unknown rather than zero.
    */
   @CheckForNull
   private static Integer getNumberQuery(ExpressionTree expression, Tree startingPointForReassignment) {
     ExpressionTree expr = ExpressionUtils.skipParentheses(expression);
+    Integer unresolvedBound = unresolvedPlaceholderBound(expr);
+    if (unresolvedBound != null) {
+      return unresolvedBound;
+    }
+    Integer constantCount = countQuery(expr);
+    if (constantCount != null) {
+      return constantCount;
+    }
     if (expr.is(Tree.Kind.IDENTIFIER)) {
       return handleVariableUsedAsQuery((IdentifierTree) expr, startingPointForReassignment);
-    } else if (expr.is(Tree.Kind.PLUS)) {
+    }
+    if (expr.is(Tree.Kind.PLUS)) {
       return handleStringConcatenation((BinaryExpressionTree) expr, startingPointForReassignment);
     }
-    return countQuery(expr);
+    if (expr.is(Tree.Kind.CONDITIONAL_EXPRESSION)) {
+      return handleConditionalExpression((ConditionalExpressionTree) expr, startingPointForReassignment);
+    }
+    return null;
   }
 
+  @CheckForNull
   private static Integer handleVariableUsedAsQuery(IdentifierTree identifier, Tree startingPointForReassignment) {
     ExpressionTree lastAssignmentExpr = ReassignmentFinder.getClosestReassignmentOrDeclarationExpression(startingPointForReassignment, identifier.symbol());
     if (lastAssignmentExpr != null) {
       Tree lastAssignment = lastAssignmentExpr.parent();
       if (lastAssignment.is(Tree.Kind.PLUS_ASSIGNMENT)) {
-        return zeroIfNull(getNumberQuery(lastAssignmentExpr, lastAssignment))
-          + zeroIfNull(getNumberQuery(((AssignmentExpressionTree) lastAssignment).variable(), lastAssignment));
+        return sumOrNull(
+          getNumberQuery(((AssignmentExpressionTree) lastAssignment).variable(), lastAssignment),
+          getNumberQuery(lastAssignmentExpr, lastAssignment));
       }
       // If the current assignment contains the identifier, we have to update the starting point for reassignment to avoid infinite recursion.
       return getNumberQuery(lastAssignmentExpr, lastAssignment);
     }
     return null;
   }
 
+  @CheckForNull
   private static Integer handleStringConcatenation(BinaryExpressionTree expr, Tree startingPointForReassignment) {
-    Integer left = getNumberQuery(expr.leftOperand(), startingPointForReassignment);
-    Integer right = getNumberQuery(expr.rightOperand(), startingPointForReassignment);
-    return (left == null && right == null) ? null : (zeroIfNull(left) + zeroIfNull(right));
+    return sumOrNull(
+      getNumberQuery(expr.leftOperand(), startingPointForReassignment),
+      getNumberQuery(expr.rightOperand(), startingPointForReassignment));
+  }
+
+  /**
+   * A conditional expression contributes as many placeholders as its most parameterized branch: reporting against the
+   * smaller branch would flag indices which are valid whenever the other branch is taken.
+   */
+  @CheckForNull
+  private static Integer handleConditionalExpression(ConditionalExpressionTree expr, Tree startingPointForReassignment) {
+    Integer trueBranch = getNumberQuery(expr.trueExpression(), startingPointForReassignment);
+    Integer falseBranch = getNumberQuery(expr.falseExpression(), startingPointForReassignment);
+    if (trueBranch == null || falseBranch == null) {
+      return null;
+    }
+    return Math.max(trueBranch, falseBranch);
+  }
+
+  /**
+   * Upper bound on the placeholders an operand can contribute when its value is unknown: none for a number or a
+   * boolean, whatever their runtime value, and at most one for a character, since '?' is a valid one. Returns
+   * {@code null} for anything that can hold an arbitrary number of them.
+   */
+  @CheckForNull
+  private static Integer unresolvedPlaceholderBound(ExpressionTree expression) {
+    if (expression.is(Tree.Kind.CHAR_LITERAL)) {
+      return "'?'".equals(((LiteralTree) expression).value()) ? 1 : 0;
+    }
+    Type type = expression.symbolType();
+    if (type.isPrimitive(Primitives.CHAR) || type.is("java.lang.Character")) {
+      return 1;
+    }
+    if (type.isPrimitive() || type.isPrimitiveWrapper()) {
+      return 0;
+    }
+    return null;
   }
 
-  private static int zeroIfNull(@Nullable Integer intValue) {
-    return intValue == null ? 0 : intValue;
+  @CheckForNull
+  private static Integer sumOrNull(@Nullable Integer left, @Nullable Integer right) {
+    if (left == null || right == null) {
+      return null;
+    }
+    return left + right;
   }
 
   @CheckForNull
   private static Integer countQuery(ExpressionTree expression) {
-    return expression.is(Tree.Kind.STRING_LITERAL) ? StringUtils.countMatches(((LiteralTree) expression).value(), "?") : null;
+    return expression.asConstant(String.class)
+      .map(query -> StringUtils.countMatches(query, "?"))
+      .orElse(null);
   }
 }