6
\$\begingroup\$

I have a library of news articles. Each story is unique by id.

 public String getStory(String id) throws Exception {
        Connection conn = MysqlDb.getConn(dbName);

        try {
            for(Publisher p : Publishers) {
                PreparedStatement prpStmt = conn.prepareStatement("SELECT story FROM " + p + " WHERE id='" + id + "'");
                ResultSet rs = prpStmt.executeQuery();

                while (rs.next()) {
                    String story = rs.getString(1);
                    rs.close();
                    return story;
                }
                rs.close();
            }   
        } finally {
            conn.close();
        }
        return null;
    }

Come what may (like an exception, or something), I want to make sure to release the JDBC resources used by the Connection and ResultSet objects. Does finally always execute in Java?

  1. Closing the database connection in a finally block makes sense, right? In the code I've looked at, I've not seen finally really used for anything.
  2. Maybe making sure that the ResultSet closes is not absolutely necessary? But, the close() method must exist for some reason... But, in my method above, the scope is too narrow to be closed by the finally block. What to do? Create another finally with adequate scope for the ResultSet?
\$\endgroup\$
  • \$\begingroup\$ I sure hope Publisher.toString() sanitizes its output properly to prevent SQL injection when you go "SELECT story FROM " + p.... \$\endgroup\$ – h.j.k. Dec 15 '15 at 0:18
  • \$\begingroup\$ The proper way to handle for id is mentioned in @janos's answer. I'm actually referring to how the concatenation is done with " ... " + p + " ... ", as theoretically that allows for this to be true: p.toString() == " publisher_table where id = '?'; TRUNCATE some_tables; --". So, as long as your Publisher class is not doing something silly for this to happen, you should be all right after taking in @janos's answer. \$\endgroup\$ – h.j.k. Dec 15 '15 at 3:56
  • 1
    \$\begingroup\$ @h.j.k. thanks. I'm reading about sql injection now. \$\endgroup\$ – david Dec 15 '15 at 4:13
7
\$\begingroup\$

The best way to close a Connection is... not doing it yourself, but using try-with-resources, which is the best practice as of Java 7:

public String getStory(String id) throws Exception {
    try (Connection conn = MysqlDb.getConn(dbName)) {
        for (Publisher p : Publishers) {
            PreparedStatement prpStmt = conn.prepareStatement("SELECT story FROM " + p + " WHERE id='" + id + "'");
            ResultSet rs = prpStmt.executeQuery();

            while (rs.next()) {
                String story = rs.getString(1);
                rs.close();
                return story;
            }
            rs.close();
        }
    }
    return null;
}

There are a couple of other issues in your code:

  • You're not using the prepared statement properly and to its full advantage. Don't create a query string as a concatenation of variables used in the WHERE clause. Those values should be set via the prepared statement. In its current form, your query may be vulnerable to SQL injection attacks.

  • The inner while loop is not actually looping. You should replace the while with an if.

  • You should use try-with-resources for the prepared statement and the result set too

  • "Publishers" looks like a collection, a variable, but by the naming conventions of Java only class names should start with a capital letter. So this should be renamed to publishers.

With the above suggestions applied, the code becomes:

public String getStory(String id) throws Exception {
    try (Connection conn = MysqlDb.getConn(dbName)) {
        for (Publisher p : publishers) {
            String sql = "SELECT story FROM " + p + " WHERE id = ?";
            try (PreparedStatement prpStmt = conn.prepareStatement(sql)) {
                prpStmt.setString(1, id);
                try (ResultSet rs = prpStmt.executeQuery()) {
                    if (rs.next()) {
                        return rs.getString(1);
                    }
                }
            }
        }
    }
    return null;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.