0

I have a small problem. I wrote a method in which I have an SQL query that should output a correct string after 2 parameters. When debugging, however, the result is not the right element. I don't know why this happens.

public static String findRightTemplate(String user_name, int template_id)
  throws Exception {


        Connection conn = DriverManager.getConnection(
                "xxx", "xxx", "xxx");
        Statement st = conn.createStatement();

        st = conn.createStatement();
        ResultSet rs = st.executeQuery(
          "SELECT template FROM templates " +
          "where template_id=template_id AND user_name=user_name"
          );

        String temp="";
        while(rs.next())
        {
            temp=rs.getString("template");
        }


        rs.close();
        st.close();
        conn.close();

I ask for the username and template_id and I just want to get an element out of the template column. The SQL query is correct. I've already tested that. But it seems that the query runs through all elements with the same username. As a result, I only get the last element and not the right one.

UPDATE

enter image description here

Doncarlito87
  • 359
  • 1
  • 8
  • 25
  • Is this a generic JDBC question, or specific to one database engine? The latter (use `LIMIT`) is far more optimal than the former (reading all rows, discarding most of them). – The Impaler Jan 15 '20 at 20:46
  • 2
    Your SQL server will see the following query: `SELECT template FROM templates where template_id=template_id AND user_name=user_name` Your `WHERE` condition will **always** be true (well... with the exception of NULL). – PM 77-1 Jan 15 '20 at 20:49
  • Its specific. I tryed with LIMIT but. But as result i got the first element from the User – Doncarlito87 Jan 15 '20 at 20:50
  • 1
    "As a result, I only get the last element and not the right one." -- This doesn't make sense. Relational tables do not have inherent row ordering; in the absence of an `ORDER BY` clause the query can return the rows in any order, and this can also change over time. – The Impaler Jan 15 '20 at 20:51
  • 2
    Use `PreparedStaement` and parametrize your query. – PM 77-1 Jan 15 '20 at 20:51
  • @ The Impaler i want not a row as result. – Doncarlito87 Jan 15 '20 at 20:53
  • @Doncarlito87 `SELECT` statements return "result sets" composed of rows and columns. What are you expecting? – The Impaler Jan 15 '20 at 20:54
  • You better show the sample data, and the expected result. I'm voting to close this question, since it's not well formulated. – The Impaler Jan 15 '20 at 20:56
  • You should be using prepared statements, but for a quick fix it may be enough to change `user_name=user_name"` to `user_name = '" + user_name + "'"` – Steve Lovell Jan 15 '20 at 21:02
  • 2
    @SteveLovell Why do you even suggest that? – stevecross Jan 15 '20 at 21:03
  • I added my table now. Wie man sehen kann. gibt es 4 Spalten. Id, template (String) username und template_id – Doncarlito87 Jan 15 '20 at 21:05
  • @Steve Lovell I tried your solution but I still get the wrong template – Doncarlito87 Jan 15 '20 at 21:10
  • Ok Problem Solved. "SELECT template FROM templates WHERE template_id='" + template_id + "' AND user_name = '" + user_name + "'" was right. Thx a lot – Doncarlito87 Jan 15 '20 at 21:15
  • 1
    Please, please tell me that this isn't production code, then. Assembling your query string like that leaves you wide open to [SQL injection attacks](https://stackoverflow.com/questions/9516625/prevent-sql-injection-attacks-in-a-java-program). (Obligatory [xkcd](https://xkcd.com/327/)) – azurefrog Jan 15 '20 at 21:21
  • No its not. Dont worry – Doncarlito87 Jan 15 '20 at 21:24
  • @stevecross, your answer is much better than my "hack". However, the dangers of SQL injection are often overstated. We do not know the context in which this code is being used or how the variables are populated. My comment also has the virtue of hinting why the code from the OP didn't work. – Steve Lovell Jan 15 '20 at 21:50
  • 1
    @SteveLovell That may be true sometimes but I would argue that there are no downsides when using a `PreparedStatement`. In my opinion one should understand the risks of SQL injections before he can decide if building a query 'by hand ' is okay. And I don't think that this is the case here. – stevecross Jan 15 '20 at 21:58
  • You make a good point @stevecross. – Steve Lovell Jan 15 '20 at 22:00

1 Answers1

3

Currently you do not use the method parameters inside your query. As already suggested you should use a PreparedStatement to fix that. You should basically do the following:

public static String findRightTemplate(String userName, int templateId) throws SQLException {
    try (final Connection connection = DriverManager.getConnection("...")) {
        final PreparedStatement preparedStatement = connection.prepareStatement(
                "SELECT template " +
                "FROM templates " +
                "WHERE user_name = ? " +
                "AND template_id = ? " +
                "LIMIT 1"
        );

        preparedStatement.setString(1, userName);
        preparedStatement.setInt(2, templateId);

        final ResultSet resultSet = preparedStatement.executeQuery();

        if (resultSet.next()) {
            return resultSet.getString(1);
        }
    }

    return null;
}

If you do not use a PreparedStatement and build the query manually as suggested in the comments your application could be vulnerable to SQL injection attacks.

stevecross
  • 5,588
  • 7
  • 47
  • 85
  • 1
    1 and 2 are swiapped. And connection, preparedStatement and resultSet should be `close()`d. Best with `try (Connection connection = ...) {` etcetera. – Joop Eggen Jan 15 '20 at 21:38
  • 1
    Thanks, good catch! You are absolutely right, but in favor of a simpler example I left out the JDBC boilerplate. Personally I would not even use JDBC directly but something like Spring's JDBCTemplate. – stevecross Jan 15 '20 at 21:42