Added workaround for children out of order while adding statements. (#351)

- Added a new method to add statements to nodes that preserves children
order. This unfortunately causes some small formatting changes.
- Enabled cleanup step that removes unused variables for some
parameterization codemods.
Closes #340.

Author: andrecsilva

core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java

@@ -37,9 +37,8 @@ private Optional<CodemodChange> onNodeFound(
         var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
         // Cleanup, removes empty string concatenations and unused variables
         maybeMethodDecl.ifPresent(cd -> ASTTransforms.removeEmptyStringConcatenation(cd));
-        // TODO hits a bug with javaparser, where adding nodes won't result in the correct children
-        // order. This causes the following to remove actually used variables
-        // maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
+        // Remove potential unused variables left after transform
+        maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
         return Optional.of(CodemodChange.from(methodCallExpr.getRange().get().begin.line));
       }
     }

core-codemods/src/main/java/io/codemodder/codemods/SQLParameterizerWithCleanup.java

@@ -13,11 +13,11 @@ public static boolean checkAndFix(final MethodCallExpr methodCallExpr) {
     if (maybeFixed.isPresent()) {
       // Cleanup
       var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
+
       // Remove concatenation with empty strings e.g "first" +  "" -> "first";
       maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation);
-      // TODO hits a bug with javaparser, where adding nodes won't result in the correct children
-      // order. This causes the following to remove actually used variables
-      // maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
+      // Remove potential unused variables left after transform
+      maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
 
       // Merge concatenated literals, e.g. "first" + " and second" -> "first and second"
       maybeFixed

core-codemods/src/test/resources/defectdojo-sql-injection/SqlInjectionChallenge/SqlInjectionChallenge.java.after

@@ -69,7 +69,6 @@ public class SqlInjectionChallenge extends AssignmentEndpoint {
         PreparedStatement statement = connection.prepareStatement(checkUserQuery);
         statement.setString(1, username_reg);
         ResultSet resultSet = statement.execute();
-
         if (resultSet.next()) {
           if (username_reg.contains("tom'")) {
             attackResult = success(this).feedback("user.exists").build();
@@ -85,6 +84,7 @@ public class SqlInjectionChallenge extends AssignmentEndpoint {
           preparedStatement.execute();
           attackResult = success(this).feedback("user.created").feedbackArgs(username_reg).build();
         }
+
       } catch (SQLException e) {
         attackResult = failed(this).output("Something went wrong").build();
       }

core-codemods/src/test/resources/defectdojo-sql-injection/SqlInjectionLesson8/SqlInjectionLesson8.java.after

@@ -75,7 +75,6 @@ query,                 ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDAT
         statement.setString(1, name);
         statement.setString(2, auth_tan);
         ResultSet results = statement.execute();
-
         if (results.getStatement() != null) {
           if (results.first()) {
             output.append(generateTable(results));
@@ -99,6 +98,7 @@ query,                 ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDAT
         } else {
           return failed(this).build();
         }
+
       } catch (SQLException e) {
         return failed(this)
             .output("<br><span class='feedback-negative'>" + e.getMessage() + "</span>")
@@ -147,7 +147,6 @@ query,                 ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDAT
     action = action.replace('\'', '"');
     Calendar cal = Calendar.getInstance();
     SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
-    String time = sdf.format(cal.getTime());
 
     String logQuery =
         "INSERT INTO access_log (time, action) VALUES (?, ?)";
@@ -157,7 +156,7 @@ query,                 ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDAT
       statement.setString(1, sdf.format(cal.getTime()));
       statement.setString(2, action);
       statement.execute();
-    } catch (SQLException e) {
+        } catch (SQLException e) {
       System.err.println(e.getMessage());
     }
   }

core-codemods/src/test/resources/hql-parameterizer/Test.java.after

@@ -22,7 +22,6 @@ public final class Test {
     }
 
     void indirectMultipeString(String tainted, String tainted2) {
-      String third = "";
       String second = " and p.surname like :parameter1";
       String first = "select p from Person p where p.name like :parameter0" + second;
       Query<User> hqlQuery = session.createQuery(first).setParameter(":parameter0", tainted).setParameter(":parameter1", tainted2);

core-codemods/src/test/resources/hql-parameterizer/Test.java.before

@@ -9,22 +9,26 @@ public final class Test {
 
     void direct(String tainted) {
       Query<User> hqlQuery = session.createQuery("select p from Person p where p.name like '" + tainted + "'");
+      List l = hqlQuery.list();
     }
 
     void indirect(String tainted) {
       String query = "select p from Person p where p.name like '" + tainted + "'";
       Query<User> hqlQuery = session.createQuery(query);
+      List l = hqlQuery.list();
     }
 
     void indirectMultiple(String tainted, String tainted2) {
       String query = "select p from Person p where p.name like '" + tainted + "' and p.surname like '" + tainted2 + "'";
       Query<User> hqlQuery = session.createQuery(query);
+      List l = hqlQuery.list();
     }
 
     void indirectMultipeString(String tainted, String tainted2) {
       String third = "'";
       String second = "' and p.surname like '" + tainted2 + third;
       String first = "select p from Person p where p.name like '" + tainted + second;
       Query<User> hqlQuery = session.createQuery(first);
+      List l = hqlQuery.list();
     }
 }

core-codemods/src/test/resources/jexl-expression-injection/Test.java.after

@@ -27,6 +27,7 @@ public final class Test {
       JexlExpression expression = jexl.createExpression(input);
       JexlContext context = new MapContext();
       expression.evaluate(context);
+
     }
   }
 
@@ -41,6 +42,7 @@ public final class Test {
           sandbox.block(cls);
       }
       new JexlBuilder().sandbox(sandbox).create().createExpression(input).evaluate(context);
+
     }
   }
 

core-codemods/src/test/resources/sql-parameterizer/Test.java.after

@@ -23,16 +23,17 @@ public final class Test {
     stmt.setString(1, input);
     var rs = stmt.execute();
     return rs;
-  }
+    }
 
   public ResultSet nameConflict(String input) throws SQLException {
     int stmt = 0;
     String sql = "SELECT * FROM USERS WHERE USER = ?";
     PreparedStatement statement = conn.prepareStatement(sql);
     statement.setString(1, input);
     ResultSet rs = statement.execute();
+    stmt++;
     return rs;
-  }
+    }
 
   public ResultSet doubleNameConflict(String input) throws SQLException {
     int stmt = 0;
@@ -41,8 +42,9 @@ public final class Test {
     PreparedStatement stmt1 = conn.prepareStatement(sql);
     stmt1.setString(1, input);
     ResultSet rs = stmt1.execute();
+    stmt = stmt + statement;
     return rs;
-  }
+    }
 
   public ResultSet tryResource(String input) throws SQLException {
     String sql = "SELECT * FROM USERS WHERE USER = ?";
@@ -60,7 +62,7 @@ public final class Test {
     stmt.setString(1, "user_" + input + "_name");
     stmt.setString(2, input2);
     return stmt.execute();
-  }
+    }
 
   public ResultSet referencesAfterExecute(String input) throws SQLException {
     String sql = "SELECT * FROM USERS WHERE USER = ?";

core-codemods/src/test/resources/sql-parameterizer/Test.java.before

@@ -25,6 +25,7 @@ public final class Test {
     int stmt = 0;
     String sql = "SELECT * FROM USERS WHERE USER = '" + input + "'";
     ResultSet rs = conn.createStatement().executeQuery(sql);
+    stmt++;
     return rs;
   }
 
@@ -33,6 +34,7 @@ public final class Test {
     int statement = 0;
     String sql = "SELECT * FROM USERS WHERE USER = '" + input + "'";
     ResultSet rs = conn.createStatement().executeQuery(sql);
+    stmt = stmt + statement;
     return rs;
   }
 

framework/codemodder-base/src/main/java/io/codemodder/ast/ASTTransforms.java

@@ -15,6 +15,7 @@
 import com.github.javaparser.ast.nodeTypes.NodeWithBody;
 import com.github.javaparser.ast.nodeTypes.NodeWithName;
 import com.github.javaparser.ast.nodeTypes.NodeWithSimpleName;
+import com.github.javaparser.ast.nodeTypes.NodeWithStatements;
 import com.github.javaparser.ast.stmt.BlockStmt;
 import com.github.javaparser.ast.stmt.ExpressionStmt;
 import com.github.javaparser.ast.stmt.IfStmt;
@@ -23,7 +24,9 @@
 import com.github.javaparser.ast.stmt.TryStmt;
 import com.github.javaparser.ast.type.ClassOrInterfaceType;
 import com.github.javaparser.resolution.types.ResolvedType;
+import java.util.ArrayList;
 import java.util.Optional;
+import java.util.stream.IntStream;
 
 public final class ASTTransforms {
   /** Add an import in alphabetical order. */
@@ -76,6 +79,34 @@ public static void addStaticImportIfMissing(final CompilationUnit cu, final Stri
     cu.addImport(method, true, false);
   }
 
+  /**
+   * Adds a statement to a node at a given index. Mostly a workaround for a weird behavior of
+   * JavaParser. See https://github.com/javaparser/javaparser/issues/4370.
+   */
+  public static <N extends Node> void addStatementAt(
+      final NodeWithStatements<N> node, final Statement stmt, final int index) {
+    var newStatements = new ArrayList<Statement>();
+    int i = 0;
+    for (var s : node.getStatements()) {
+      if (i == index) {
+        newStatements.add(stmt);
+      }
+      newStatements.add(s);
+      i++;
+    }
+
+    // rebuilds the whole statements list as to preserve proper children order.
+
+    // workaround for maintaining indent, removes all but the first
+    IntStream.range(0, node.getStatements().size() - 1)
+        .forEach(j -> node.getStatements().removeLast());
+    // replace the first with the new statement if needed
+    if (index == 0) {
+      node.getStatements().get(0).replace(stmt);
+    }
+    newStatements.stream().skip(1).forEach(node.getStatements()::add);
+  }
+
   /**
    * Adds an {@link Statement} before another {@link Statement}. Single {@link Statement}s in the
    * body of For/Do/While/If/Labeled Statements are replaced with a {@link BlockStmt} containing
@@ -105,14 +136,9 @@ public static void addStatementBeforeStatement(
       // NodeWithBody, MethodDeclaration, NodeWithBlockStmt, SwitchStmt(?)
       // No way (or reason) to add a Statement before those, maybe throw an Error?
       final var block = (BlockStmt) parent;
-      // Workaround for a bug on LexicalPreservingPrinter (see
-      // https://github.com/javaparser/javaparser/issues/3746)
-      if (existingStatement.equals(block.getStatement(0))) {
-        existingStatement.replace(newStatement);
-        block.addStatement(1, existingStatement);
-      } else {
-        block.getStatements().addBefore(newStatement, existingStatement);
-      }
+
+      int existingIndex = block.getStatements().indexOf(existingStatement);
+      addStatementAt(block, newStatement, existingIndex);
     }
   }