Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.2, Nightly Builds
    • Fix Version/s: None
    • Labels:
      None

      Description

      It would be great (and fairly easy to do) to provide a way to get autogenerated keys from QueryRunner.update. There was an email thread about this in 2004 but it seems it never was actually implemented.
      http://mail-archives.apache.org/mod_mbox/commons-dev/200406.mbox/%3C20040602024627.21604.qmail@web50606.mail.yahoo.com%3E

      The thought is to provide an ability to recover generated keys, for instance by providing a result set handler, in which case prepared statement would be generated with RETURN_GENERATED_KEYS and getGeneratedKeys() would be passed to the result handler.

      It seems that in 1.2 there is a way to get PreparedStatement and work with QueryRunner more as a support to JDBC but IMO it would be cool to add this feature.

      example solution:

       
          public int update(Connection conn, String sql, Object... params)
              throws SQLException {
                update(sql, null, params);
          }
          protected PreparedStatement prepareStatement(Connection conn, String sql, int autoGeneratedKeys)
              throws SQLException {
              return conn.prepareStatement(sql, autoGeneratedKeys);
          }
          public int update(Connection conn, String sql, ResultSetHandler<?> rsh, Object... params)
              throws SQLException {
      
              PreparedStatement stmt = null;
              int rows = 0;
      
              try {
                  stmt = this.prepareStatement(conn, sql, rsh==null?Statement.NO_GENERATED_KEY:Statement.RETURN_GENERATED_KEYS);
                  this.fillStatement(stmt, params);
                  rows = stmt.executeUpdate();
                  if(rsh!=null)
                       rsh.handle(stmt.getGeneratedKeys());
              } catch (SQLException e) {
                  this.rethrow(e, sql, params);
              } finally {
                  close(stmt);
              }
      
              return rows;
          }
      

      Thanks!

      1. QueryRunner.patch
        22 kB
        Julien Aymé
      2. GenKeyQueryRunner.java
        3 kB
        eugen p.
      3. GenKeyQueryRunner.java
        14 kB
        Julien Aymé
      4. GeneratedKeysHandler.java
        1 kB
        eugen p.

        Activity

        Hide
        xakep eugen p. added a comment -

        oh yes, highly appreciated

        with one remark: the factory method for preparedStatement shall be implemented by caller, because in oracle for instance, you may want to provide the column to retrieve the mapped key from, like

        {{
        stmt = con.prepareStatement(sql,*new String[]

        {"personID"}

        *); or even just providing the column index like described in JDBC API
        }}

        Show
        xakep eugen p. added a comment - oh yes, highly appreciated with one remark: the factory method for preparedStatement shall be implemented by caller, because in oracle for instance, you may want to provide the column to retrieve the mapped key from, like {{ stmt = con.prepareStatement(sql,*new String[] {"personID"} *); or even just providing the column index like described in JDBC API }}
        Hide
        gverig Michael V added a comment -

        I'll have to admit my ignorance, I never worked with Oracle and never needed anything more advanced than a single generated key. Not sure how that even works
        Now, (ignorance, remember?), could you clarify, do you feel this feature is not worth adding (because of many possible use cases and ways keys could be retrieved), do you want me to update provided code or will you tweak it yourself? I'm not opposed to any of the three, just wanted to plug what I found to be a limitation.

        Thanks.

        Show
        gverig Michael V added a comment - I'll have to admit my ignorance, I never worked with Oracle and never needed anything more advanced than a single generated key. Not sure how that even works Now, (ignorance, remember?), could you clarify, do you feel this feature is not worth adding (because of many possible use cases and ways keys could be retrieved), do you want me to update provided code or will you tweak it yourself? I'm not opposed to any of the three, just wanted to plug what I found to be a limitation. Thanks.
        Hide
        xakep eugen p. added a comment -

        well, my comment was actually aimed to guy(s) who gonna implement it - if at all. So I can understand that it is totally valueless for you, since your database driver can handle the generated key retrieval even without the hints. The code is not worth updating as solution its pretty straightforward, and again - the guy(s) implemeting it, for sure, have already got an idea.

        Show
        xakep eugen p. added a comment - well, my comment was actually aimed to guy(s) who gonna implement it - if at all. So I can understand that it is totally valueless for you, since your database driver can handle the generated key retrieval even without the hints. The code is not worth updating as solution its pretty straightforward, and again - the guy(s) implemeting it, for sure, have already got an idea.
        Hide
        julien.ayme@gmail.com Julien Aymé added a comment - - edited

        We should think about it, since there is three methods to indicate that the generated keys should be returned:

        The first one just tells that the generated keys should be returned:

        Connection#prepareStatement(String, int)

        http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20int%29

        The second one specifies the names of the columns from which the keys should be generated

        Connection#prepareStatement(String, String[])

        http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20java.lang.String[]%29

        and the last one specifies the indexes of the columns from which the keys should be generated

        Connection#prepareStatement(String, int[])

        http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20int[]%29

        I'm willing to contribute a patch with all three methods (this will however add a lot of code into QueryRunner).

        Show
        julien.ayme@gmail.com Julien Aymé added a comment - - edited We should think about it, since there is three methods to indicate that the generated keys should be returned: The first one just tells that the generated keys should be returned: Connection#prepareStatement( String , int ) http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20int%29 The second one specifies the names of the columns from which the keys should be generated Connection#prepareStatement( String , String []) http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20java.lang.String[]%29 and the last one specifies the indexes of the columns from which the keys should be generated Connection#prepareStatement( String , int []) http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#prepareStatement%28java.lang.String,%20int[]%29 I'm willing to contribute a patch with all three methods (this will however add a lot of code into QueryRunner).
        Hide
        julien.ayme@gmail.com Julien Aymé added a comment -

        Patch proposal.

        It contains some new methods, named "insertAndGetKeys", copied from the update methods, with an aditionnal parameter: the ResultSetHandler, which is used to transform the generated keys into a java object.

        The return type as been changed from int (number of rows updated) to the result of the ResultSetHandler.

        The name has been changed from update to insert since only INSERT statement can generate keys (obviously).

        I had to add an additional prepareStatement method in order to maintain backward comptability, as well as code simplicity.

        Comments and reviews are greatly appreciated.

        Show
        julien.ayme@gmail.com Julien Aymé added a comment - Patch proposal. It contains some new methods, named "insertAndGetKeys", copied from the update methods, with an aditionnal parameter: the ResultSetHandler, which is used to transform the generated keys into a java object. The return type as been changed from int (number of rows updated) to the result of the ResultSetHandler. The name has been changed from update to insert since only INSERT statement can generate keys (obviously). I had to add an additional prepareStatement method in order to maintain backward comptability, as well as code simplicity. Comments and reviews are greatly appreciated.
        Hide
        dfabulich Dan Fabulich added a comment -

        I have to -1 this patch as-is. It's just too much code to add to QueryRunner. The patch is 500 lines long (the file today is only 723 lines) and it adds 21 methods, more than doubling the size of the API. This will make the class harder to read and understand.

        QueryRunner adds very little of value in INSERT/UPDATE statements except for its ability to fill out the parameters of prepared statements. But you can use QueryRunner#fillStatement today in 1.2 without having QueryRunner actually prepare your statement.

        Users that need to prepare their own statements (as eugen does) or needs to do something with the resulting statement can prepare the statement themselves just as easily as we can (if not easier) without complicating the API.

        Fleshing out eugen's example above:

        stmt = con.prepareStatement(sql,new String[]{"personID"}); 
        queryRunner.fillStatement(stmt, "foo", "bar", "baz");
        stmt.executeUpdate();
        rs = stmt.getGeneratedKeys();
        rsh.handle(rs);
        

        This is easy code to write. (That was just five lines of code.) I don't think adding lots of code on our end makes it any easier; if anything, it makes it harder to understand and use the API.

        Show
        dfabulich Dan Fabulich added a comment - I have to -1 this patch as-is. It's just too much code to add to QueryRunner. The patch is 500 lines long (the file today is only 723 lines) and it adds 21 methods, more than doubling the size of the API. This will make the class harder to read and understand. QueryRunner adds very little of value in INSERT/UPDATE statements except for its ability to fill out the parameters of prepared statements. But you can use QueryRunner#fillStatement today in 1.2 without having QueryRunner actually prepare your statement. Users that need to prepare their own statements (as eugen does) or needs to do something with the resulting statement can prepare the statement themselves just as easily as we can (if not easier) without complicating the API. Fleshing out eugen's example above: stmt = con.prepareStatement(sql, new String []{ "personID" }); queryRunner.fillStatement(stmt, "foo" , "bar" , "baz" ); stmt.executeUpdate(); rs = stmt.getGeneratedKeys(); rsh.handle(rs); This is easy code to write. (That was just five lines of code.) I don't think adding lots of code on our end makes it any easier; if anything, it makes it harder to understand and use the API.
        Hide
        xakep eugen p. added a comment -

        @Julien, thanks for submission
        @Dan, I agree that blowing API is worse than writing 5 more lines in the code - yet, the most valueable at dbUtils is not the fillStatement() method, rather it seems to be the fact of auto-releasing of resources ( the annoying close() invocation

        However, I would provide a slightly different solution - two more classes derived from base. Please find in file attachments. Sample usage would look

            GenKeyQueryRunner runner = new GenKeyQueryRunner(new GeneratedKeysHandler() {
              public String [] getKeyColName() {
                return new String[]{"personID"};
              }
              public void handle(ResultSet rs) throws SQLException {
                System.out.println("generated key " + rs.getInt(1));
              }
            });
            
            runner.update(someConnection, "INSERT INTO FOO(NAME) VALUES ('BAR')");
        

        This would keep up backwards compabitility. Of course the update() method also might be able to return the resulting object, like query() does with ResultSetHandler, but this would end up in incompatible APIs and yet another QueryRunner derivate.

        Show
        xakep eugen p. added a comment - @Julien, thanks for submission @Dan, I agree that blowing API is worse than writing 5 more lines in the code - yet, the most valueable at dbUtils is not the fillStatement() method, rather it seems to be the fact of auto-releasing of resources ( the annoying close() invocation However, I would provide a slightly different solution - two more classes derived from base. Please find in file attachments. Sample usage would look GenKeyQueryRunner runner = new GenKeyQueryRunner( new GeneratedKeysHandler() { public String [] getKeyColName() { return new String []{ "personID" }; } public void handle(ResultSet rs) throws SQLException { System .out.println( "generated key " + rs.getInt(1)); } }); runner.update(someConnection, "INSERT INTO FOO(NAME) VALUES ('BAR')" ); This would keep up backwards compabitility. Of course the update() method also might be able to return the resulting object, like query() does with ResultSetHandler, but this would end up in incompatible APIs and yet another QueryRunner derivate.
        Hide
        julien.ayme@gmail.com Julien Aymé added a comment - - edited

        Attaching updated version of GenKeyQueryRunner, no need of GeneratedKeysHandler.java anymore.
        The new version is used simply this way:

        GenKeyQueryRunner<Object> runner = new GenKeyQueryRunner(ds, new ScalarHandler(), "id");
        // The key column to use for auto-generated key retrieval will be the column named "id"
        
        int updates = runner.update("INSERT INTO person(name, height) VALUES (?, ?)", "John Doe", 1.80);
        // This will insert a John Doe into the Person table,
        // and the RDBMS will generate an auto-incremented id
         
        Long key = (Long) runner.getGeneratedKeys(); 
        // Use the key here
        ...
        

        This allow the reuse of existing ResultSetHandler implementations, and the users clearly see that either keyColsByIndex or keyColsByName can be used, but not both in the same time.

        Show
        julien.ayme@gmail.com Julien Aymé added a comment - - edited Attaching updated version of GenKeyQueryRunner, no need of GeneratedKeysHandler.java anymore. The new version is used simply this way: GenKeyQueryRunner< Object > runner = new GenKeyQueryRunner(ds, new ScalarHandler(), "id" ); // The key column to use for auto-generated key retrieval will be the column named "id" int updates = runner.update( "INSERT INTO person(name, height) VALUES (?, ?)" , "John Doe" , 1.80); // This will insert a John Doe into the Person table, // and the RDBMS will generate an auto-incremented id Long key = ( Long ) runner.getGeneratedKeys(); // Use the key here ... This allow the reuse of existing ResultSetHandler implementations, and the users clearly see that either keyColsByIndex or keyColsByName can be used, but not both in the same time.
        Hide
        julien.ayme@gmail.com Julien Aymé added a comment -

        Note that the proposed GenKeyQueryRunner patch looses the thread safety of QueryRunner (due to the field generatedKeys used to store the generated keys within the update method).
        The new GenKeyQueryRunner is indeed statefull, while QueryRunner was stateless (except for the pmdKnownBroken volatile field).

        Show
        julien.ayme@gmail.com Julien Aymé added a comment - Note that the proposed GenKeyQueryRunner patch looses the thread safety of QueryRunner (due to the field generatedKeys used to store the generated keys within the update method). The new GenKeyQueryRunner is indeed statefull, while QueryRunner was stateless (except for the pmdKnownBroken volatile field).

          People

          • Assignee:
            Unassigned
            Reporter:
            gverig Michael V
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development