Correction to try/finally

Question

I'm trying to close my resultset, connection and statement with a try/finally but Sonar doesn't seem to like it. What mistake am I doing and why aren't they closing? Thanks.

public static List<String> findByName(String firstName, String lastName) throws SQLException {

    /*Connects  to  table and contexts a statement to this connection. Creates the appropriate statement (query)
    according to the first name and last name nulls, builds an array from the ResultSet that is created.*/
    List<String> nameList = new ArrayList();
    String query;

    Connection conn = null;
    Statement preparedStatement = null;
    ResultSet allNames = null;

    try {
        conn = DriverManager.getConnection(getHost(), getuName(), getuPass());
        preparedStatement = conn.createStatement();
        if (firstName == null && lastName == null) {
            query = "SELECT * FROM person_c";
        } else if (firstName == null) {
            query = "SELECT * FROM person_c WHERE LAST_NAME= '" + lastName + "'";
        } else if (lastName == null) {
            query = "SELECT * FROM person_c where FIRST_NAME= '" + firstName + "'";
        } else {
            query = "SELECT * FROM person_c where FIRST_NAME =  '" + firstName + "'" + "AND LAST_NAME= '" + lastName + "'";
        }
        allNames = preparedStatement.executeQuery(query);
        while (allNames.next()) {
            nameList.add(allNames.getString("FIRST_NAME") + "  " + allNames.getString("LAST_NAME"));
        }
    } finally {
        if (allNames != null) allNames.close();
        if (preparedStatement!=null)  preparedStatement.close();
        if (conn!=null) conn.close();
    }
    return nameList;
}

*doesn't seem to like it.* - That is not very technical is it? What is the error? — Scary Wombat, Jun 17 '19 at 08:15
from the security aspect i would say this sql query is vulnerable to sql injection. Dont use `"SELECT * FROM person_c WHERE LAST_NAME= '" + lastName + "'";`. Use parameterized statements instead. [More Information to SQL Injections](https://software-security.sans.org/developer-how-to/fix-sql-injection-in-java-using-prepared-callable-statement) — Jonas, Jun 17 '19 at 08:19
Thank you MapReduce - something I will keep in mind in future. At the moment we're just doing exercises as I'm an apprentice. I've completed the code but tidying up the edges. — Hywel Griffiths, Jun 17 '19 at 08:25
@MapReduce - OMG, how did I miss the fact there was string concat in those queries?! — T.J. Crowder, Jun 17 '19 at 08:49

T.J. Crowder · Answer 1 · 2019-06-17T08:52:21.167

There are three unrelated problems in that code:

In theory, close can throw. If any of your previous close calls throws, later ones won't be done. Also, if an exception is already underway, an exception from those close calls will override it, hiding the true problem. The solution is to use try-with-resources.
You're calling executeQuery(String) on a PreparedStatement. Never do that, it's treating PreparedStatement as though it were just a Statement. (It's an API design mistake.) Pass the query into prepareStatement instead.
Don't use string concatenation to put values into a query. That's the point of PreparedStatement: To parameterize the query safely. (Let me introduce you to my friend Bobby...)

So handling all three of those:

public static List<String> findByName(String firstName, String lastName) throws SQLException {

    /*Connects  to  table and contexts a statement to this connection. Creates the appropriate statement (query)
    according to the first name and last name nulls, builds an array from the ResultSet that is created.*/
    List<String> nameList = new ArrayList();
    String query;

    try (
        Connection conn = DriverManager.getConnection(getHost(), getuName(), getuPass());
    ) {
        if (firstName == null && lastName == null) {
            query = "SELECT * FROM person_c";
        } else if (firstName == null) {
            query = "SELECT * FROM person_c WHERE LAST_NAME = ?";
        } else if (lastName == null) {
            query = "SELECT * FROM person_c where FIRST_NAME = ?";
        } else {
            query = "SELECT * FROM person_c where FIRST_NAME = ? AND LAST_NAME = ?";
        }
        try (
            Statement preparedStatement = conn.createStatement(query);
        ) {
            int n = 1;
            if (firstName != null) {
                preparedStatement.setString(n++, firstName);
            }
            if (lastName != null) {
                preparedStatement.setString(n++, lastName);
            }
            try (
                ResultSet allNames = preparedStatement.executeQuery();
            ) {
                while (allNames.next()) {
                    nameList.add(allNames.getString("FIRST_NAME") + "  " + allNames.getString("LAST_NAME"));
                }
            }
        }
    }
    return nameList;
}

Thank you Crowder - you've been very helpful. Will give it a go :-) — Hywel Griffiths, Jun 17 '19 at 08:17
@HywelGriffiths - No worries. I'm afraid I missed the string concatenation in your code earlier, and I also forgot to remove `query` from `executeQuery` in my updated example Both fixed now, sorry about that. — T.J. Crowder, Jun 17 '19 at 08:52

Correction to try/finally

1 Answers1