From 3bebfdac80daadb78bd7eba22ef5bd3266b0cf6c Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Wed, 6 Dec 2023 10:22:40 -0500 Subject: [PATCH 1/4] fix(jdbc): reintroduce improved support for Statement#getGeneratedKeys Adds back support for the getGeneratedKeys functionality previously removed in commit 332edfb4dc3f4dcb463f0d1d8417f9c92519e609. The new implementation attaches the generated keys directly to the Statement so that it is safe to use in an interleaved manner and accross threads. This does not address tracking inserts of multiple values in the same statement (#468) but that functionality was never previously available. It could potentially be added in future enhancements. Resolves: #329, #963, #965, #980, #1018, #1028 --- .../java/org/sqlite/core/CoreStatement.java | 50 ++++++++++++++++ .../sqlite/jdbc3/JDBC3DatabaseMetaData.java | 2 +- .../sqlite/jdbc3/JDBC3PreparedStatement.java | 19 +++++-- .../java/org/sqlite/jdbc3/JDBC3Statement.java | 40 ++++++------- src/test/java/org/sqlite/DBMetaDataTest.java | 1 - src/test/java/org/sqlite/PrepStmtTest.java | 7 ++- src/test/java/org/sqlite/StatementTest.java | 57 ++++++++++++++++++- 7 files changed, 143 insertions(+), 33 deletions(-) diff --git a/src/main/java/org/sqlite/core/CoreStatement.java b/src/main/java/org/sqlite/core/CoreStatement.java index e63325799..450c72dab 100644 --- a/src/main/java/org/sqlite/core/CoreStatement.java +++ b/src/main/java/org/sqlite/core/CoreStatement.java @@ -17,6 +17,8 @@ import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Statement; + import org.sqlite.SQLiteConnection; import org.sqlite.SQLiteConnectionConfig; import org.sqlite.jdbc3.JDBC3Connection; @@ -33,6 +35,9 @@ public abstract class CoreStatement implements Codes { protected Object[] batch = null; protected boolean resultsWaiting = false; + private Statement generatedKeysStat = null; + private ResultSet generatedKeysRs = null; + protected CoreStatement(SQLiteConnection c) { conn = c; rs = new JDBC4ResultSet(this); @@ -149,4 +154,49 @@ protected void checkIndex(int index) throws SQLException { throw new SQLException("Parameter index is invalid"); } } + + protected void clearGeneratedKeys() throws SQLException { + if(generatedKeysRs != null && !generatedKeysRs.isClosed()) { + generatedKeysRs.close(); + } + generatedKeysRs = null; + if (generatedKeysStat != null && !generatedKeysStat.isClosed()) { + generatedKeysStat.close(); + } + generatedKeysStat = null; + } + + /** + * SQLite's last_insert_rowid() function is DB-specific. However, in this implementation + * we ensure the Generated Key result set is statement-specific by executing the query + * immediately after an insert operation is performed. The caller is simply responsible for + * calling updateGeneratedKeys on the statement object right after execute in + * a synchronized(connection) block. + */ + public void updateGeneratedKeys() throws SQLException { + clearGeneratedKeys(); + if(sql != null && sql.toLowerCase().startsWith("insert")) { + generatedKeysStat = conn.createStatement(); + generatedKeysRs = generatedKeysStat.executeQuery("SELECT last_insert_rowid();"); + } + } + + + /** + * This implementation uses SQLite's last_insert_rowid function to obtain the row ID. It cannot + * provide multiple values when inserting multiple rows. Suggestion is to use a RETURNING clause instead. + * + * @see java.sql.Statement#getGeneratedKeys() + */ + public ResultSet getGeneratedKeys() throws SQLException { + // getGeneratedKeys is required to return an EmptyResult set if the statement + // did not generate any keys. Thus, if the generateKeysResultSet is NULL, spin + // up a new result set without any contents by issuing a query with a false where condition + if(generatedKeysRs == null) { + generatedKeysStat = conn.createStatement(); + generatedKeysRs = generatedKeysStat.executeQuery("SELECT last_insert_rowid() WHERE 0 <> 0;"); + } + return generatedKeysRs; + } } diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java b/src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java index d91daa661..6277ba8aa 100644 --- a/src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java +++ b/src/main/java/org/sqlite/jdbc3/JDBC3DatabaseMetaData.java @@ -559,7 +559,7 @@ public boolean supportsFullOuterJoins() throws SQLException { /** @see java.sql.DatabaseMetaData#supportsGetGeneratedKeys() */ public boolean supportsGetGeneratedKeys() { - return false; + return true; } /** @see java.sql.DatabaseMetaData#supportsGroupBy() */ diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java b/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java index 2f771b841..0eb7a2eba 100644 --- a/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java +++ b/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java @@ -54,10 +54,13 @@ public boolean execute() throws SQLException { () -> { boolean success = false; try { - resultsWaiting = - conn.getDatabase().execute(JDBC3PreparedStatement.this, batch); - success = true; - updateCount = getDatabase().changes(); + synchronized (conn) { + resultsWaiting = + conn.getDatabase().execute(JDBC3PreparedStatement.this, batch); + updateGeneratedKeys(); + success = true; + updateCount = getDatabase().changes(); + } return 0 != columnCount; } finally { if (!success && !pointer.isClosed()) pointer.safeRunConsume(DB::reset); @@ -119,7 +122,13 @@ public long executeLargeUpdate() throws SQLException { } return this.withConnectionTimeout( - () -> conn.getDatabase().executeUpdate(JDBC3PreparedStatement.this, batch)); + () -> { + synchronized (conn) { + long rc = conn.getDatabase().executeUpdate(JDBC3PreparedStatement.this, batch); + updateGeneratedKeys(); + return rc; + } + }); } /** @see java.sql.PreparedStatement#addBatch() */ diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java b/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java index 30210228a..8d8be4bdd 100644 --- a/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java +++ b/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java @@ -32,6 +32,7 @@ protected JDBC3Statement(SQLiteConnection conn) { /** @see java.sql.Statement#close() */ public void close() throws SQLException { + clearGeneratedKeys(); internalClose(); } @@ -49,12 +50,14 @@ public boolean execute(final String sql) throws SQLException { } JDBC3Statement.this.sql = sql; - - conn.getDatabase().prepare(JDBC3Statement.this); - boolean result = exec(); - updateCount = getDatabase().changes(); - exhaustedResults = false; - return result; + synchronized (conn) { + conn.getDatabase().prepare(JDBC3Statement.this); + boolean result = exec(); + updateGeneratedKeys(); + updateCount = getDatabase().changes(); + exhaustedResults = false; + return result; + } }); } @@ -126,13 +129,15 @@ public long executeLargeUpdate(String sql) throws SQLException { ext.execute(db); } else { try { - changes = db.total_changes(); - - // directly invokes the exec API to support multiple SQL statements - int statusCode = db._exec(sql); - if (statusCode != SQLITE_OK) throw DB.newSQLException(statusCode, ""); + synchronized (db) { + changes = db.total_changes(); + // directly invokes the exec API to support multiple SQL statements + int statusCode = db._exec(sql); + if (statusCode != SQLITE_OK) throw DB.newSQLException(statusCode, ""); + updateGeneratedKeys(); + changes = db.total_changes() - changes; + } - changes = db.total_changes() - changes; } finally { internalClose(); } @@ -350,17 +355,6 @@ public void setFetchDirection(int direction) throws SQLException { } } - /** - * SQLite's last_insert_rowid() function is DB-specific, not statement specific, and cannot - * provide multiple values when inserting multiple rows. Suggestion is to use a RETURNING clause instead. - * - * @see java.sql.Statement#getGeneratedKeys() - */ - public ResultSet getGeneratedKeys() throws SQLException { - throw unsupported(); - } - /** * SQLite does not support multiple results from execute(). * diff --git a/src/test/java/org/sqlite/DBMetaDataTest.java b/src/test/java/org/sqlite/DBMetaDataTest.java index ab6661c06..03c7a2b95 100644 --- a/src/test/java/org/sqlite/DBMetaDataTest.java +++ b/src/test/java/org/sqlite/DBMetaDataTest.java @@ -51,7 +51,6 @@ public void close() throws SQLException { public void getTables() throws SQLException { ResultSet rs = meta.getTables(null, null, null, null); assertThat(rs).isNotNull(); - stat.close(); assertThat(rs.next()).isTrue(); diff --git a/src/test/java/org/sqlite/PrepStmtTest.java b/src/test/java/org/sqlite/PrepStmtTest.java index 403603cb0..ee2e77ec8 100644 --- a/src/test/java/org/sqlite/PrepStmtTest.java +++ b/src/test/java/org/sqlite/PrepStmtTest.java @@ -67,8 +67,13 @@ public void update() throws SQLException { assertThat(prep.executeUpdate()).isEqualTo(1); prep.setInt(1, 7); assertThat(prep.executeUpdate()).isEqualTo(1); - prep.close(); + ResultSet rsgk = prep.getGeneratedKeys(); + assertThat(rsgk.next()).isTrue(); + assertThat(rsgk.getInt(1)).isEqualTo(3); + rsgk.close(); + + prep.close(); // check results with normal statement ResultSet rs = stat.executeQuery("select sum(c1) from s1;"); assertThat(rs.next()).isTrue(); diff --git a/src/test/java/org/sqlite/StatementTest.java b/src/test/java/org/sqlite/StatementTest.java index dc7a7aa93..a3d3ec4f6 100644 --- a/src/test/java/org/sqlite/StatementTest.java +++ b/src/test/java/org/sqlite/StatementTest.java @@ -307,8 +307,61 @@ public void getGeneratedKeys() throws SQLException { stat.executeUpdate("create table t1 (c1 integer primary key, v);"); stat.executeUpdate("insert into t1 (v) values ('red');"); - assertThatExceptionOfType(SQLFeatureNotSupportedException.class) - .isThrownBy(() -> stat.getGeneratedKeys()); + rs = stat.getGeneratedKeys(); + assertThat(rs.next()).isTrue(); + assertThat(rs.getInt(1)).isEqualTo(1); + rs.close(); + stat.executeUpdate("insert into t1 (v) values ('blue');"); + rs = stat.getGeneratedKeys(); + assertThat(rs.next()).isTrue(); + assertThat(rs.getInt(1)).isEqualTo(2); + rs.close(); + + // generated keys are now attached to the statement. calling getGeneratedKeys + // on a statement that has not generated any should return an empty result set + stat.close(); + Statement stat2 = conn.createStatement(); + rs = stat2.getGeneratedKeys(); + assertThat(rs).isNotNull(); + assertThat(rs.next()).isFalse(); + stat2.close(); + } + + @Test + public void getGeneratedKeysIsStatementSpecific() throws SQLException { + /* this test ensures that the results of getGeneratedKeys are tied to + a specific statement. To verify this, we create two separate Statement + objects and then execute inserts on both. We then make getGeneratedKeys() + calls and verify that the two separate expected values are returned. + + Note that the old implementation of getGeneratedKeys was called lazily, so + the result of both getGeneratedKeys calls would be the same value, the row ID + of the last insert on the connection. As a result it was unsafe to use + with multiple statements or in a multithreaded application. + */ + stat.executeUpdate("create table t1 (c1 integer primary key, v);"); + + ResultSet rs1; + Statement stat1 = conn.createStatement(); + ResultSet rs2; + Statement stat2 = conn.createStatement(); + + stat1.executeUpdate("insert into t1 (v) values ('red');"); + stat2.executeUpdate("insert into t1 (v) values ('blue');"); + + rs2 = stat2.getGeneratedKeys(); + rs1 = stat1.getGeneratedKeys(); + + assertThat(rs1.next()).isTrue(); + assertThat(rs1.getInt(1)).isEqualTo(1); + rs1.close(); + + assertThat(rs2.next()).isTrue(); + assertThat(rs2.getInt(1)).isEqualTo(2); + rs2.close(); + + stat1.close(); + stat2.close(); } @Test From 72605a1155a467462871a945abd0f50ccf26e16f Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Thu, 7 Dec 2023 15:35:43 -0500 Subject: [PATCH 2/4] apply spotless code formatting on getGeneratedKeys code --- .../java/org/sqlite/core/CoreStatement.java | 21 +++++++++---------- .../sqlite/jdbc3/JDBC3PreparedStatement.java | 4 +++- .../java/org/sqlite/jdbc3/JDBC3Statement.java | 3 ++- src/test/java/org/sqlite/StatementTest.java | 18 ++++++++-------- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/sqlite/core/CoreStatement.java b/src/main/java/org/sqlite/core/CoreStatement.java index 450c72dab..cf9f03569 100644 --- a/src/main/java/org/sqlite/core/CoreStatement.java +++ b/src/main/java/org/sqlite/core/CoreStatement.java @@ -18,7 +18,6 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; - import org.sqlite.SQLiteConnection; import org.sqlite.SQLiteConnectionConfig; import org.sqlite.jdbc3.JDBC3Connection; @@ -156,7 +155,7 @@ protected void checkIndex(int index) throws SQLException { } protected void clearGeneratedKeys() throws SQLException { - if(generatedKeysRs != null && !generatedKeysRs.isClosed()) { + if (generatedKeysRs != null && !generatedKeysRs.isClosed()) { generatedKeysRs.close(); } generatedKeysRs = null; @@ -167,21 +166,20 @@ protected void clearGeneratedKeys() throws SQLException { } /** - * SQLite's last_insert_rowid() function is DB-specific. However, in this implementation - * we ensure the Generated Key result set is statement-specific by executing the query - * immediately after an insert operation is performed. The caller is simply responsible for - * calling updateGeneratedKeys on the statement object right after execute in - * a synchronized(connection) block. + * SQLite's last_insert_rowid() function is DB-specific. However, in this implementation we + * ensure the Generated Key result set is statement-specific by executing the query immediately + * after an insert operation is performed. The caller is simply responsible for calling + * updateGeneratedKeys on the statement object right after execute in a synchronized(connection) + * block. */ public void updateGeneratedKeys() throws SQLException { clearGeneratedKeys(); - if(sql != null && sql.toLowerCase().startsWith("insert")) { + if (sql != null && sql.toLowerCase().startsWith("insert")) { generatedKeysStat = conn.createStatement(); generatedKeysRs = generatedKeysStat.executeQuery("SELECT last_insert_rowid();"); } } - /** * This implementation uses SQLite's last_insert_rowid function to obtain the row ID. It cannot * provide multiple values when inserting multiple rows. Suggestion is to use a 0;"); + generatedKeysRs = + generatedKeysStat.executeQuery("SELECT last_insert_rowid() WHERE 0 <> 0;"); } return generatedKeysRs; } diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java b/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java index 0eb7a2eba..6c2c34340 100644 --- a/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java +++ b/src/main/java/org/sqlite/jdbc3/JDBC3PreparedStatement.java @@ -124,7 +124,9 @@ public long executeLargeUpdate() throws SQLException { return this.withConnectionTimeout( () -> { synchronized (conn) { - long rc = conn.getDatabase().executeUpdate(JDBC3PreparedStatement.this, batch); + long rc = + conn.getDatabase() + .executeUpdate(JDBC3PreparedStatement.this, batch); updateGeneratedKeys(); return rc; } diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java b/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java index 8d8be4bdd..69cadf72b 100644 --- a/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java +++ b/src/main/java/org/sqlite/jdbc3/JDBC3Statement.java @@ -133,7 +133,8 @@ public long executeLargeUpdate(String sql) throws SQLException { changes = db.total_changes(); // directly invokes the exec API to support multiple SQL statements int statusCode = db._exec(sql); - if (statusCode != SQLITE_OK) throw DB.newSQLException(statusCode, ""); + if (statusCode != SQLITE_OK) + throw DB.newSQLException(statusCode, ""); updateGeneratedKeys(); changes = db.total_changes() - changes; } diff --git a/src/test/java/org/sqlite/StatementTest.java b/src/test/java/org/sqlite/StatementTest.java index a3d3ec4f6..ee08f4c90 100644 --- a/src/test/java/org/sqlite/StatementTest.java +++ b/src/test/java/org/sqlite/StatementTest.java @@ -330,15 +330,15 @@ public void getGeneratedKeys() throws SQLException { @Test public void getGeneratedKeysIsStatementSpecific() throws SQLException { /* this test ensures that the results of getGeneratedKeys are tied to - a specific statement. To verify this, we create two separate Statement - objects and then execute inserts on both. We then make getGeneratedKeys() - calls and verify that the two separate expected values are returned. - - Note that the old implementation of getGeneratedKeys was called lazily, so - the result of both getGeneratedKeys calls would be the same value, the row ID - of the last insert on the connection. As a result it was unsafe to use - with multiple statements or in a multithreaded application. - */ + a specific statement. To verify this, we create two separate Statement + objects and then execute inserts on both. We then make getGeneratedKeys() + calls and verify that the two separate expected values are returned. + + Note that the old implementation of getGeneratedKeys was called lazily, so + the result of both getGeneratedKeys calls would be the same value, the row ID + of the last insert on the connection. As a result it was unsafe to use + with multiple statements or in a multithreaded application. + */ stat.executeUpdate("create table t1 (c1 integer primary key, v);"); ResultSet rs1; From c90bdba4e9177dbe3c66bb9e3678fdde34237301 Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Wed, 17 Jan 2024 10:36:04 -0500 Subject: [PATCH 3/4] simplify empty ResultSet case in getGeneratedKeys() --- src/main/java/org/sqlite/core/CoreStatement.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/sqlite/core/CoreStatement.java b/src/main/java/org/sqlite/core/CoreStatement.java index cf9f03569..9357fe908 100644 --- a/src/main/java/org/sqlite/core/CoreStatement.java +++ b/src/main/java/org/sqlite/core/CoreStatement.java @@ -194,7 +194,7 @@ public ResultSet getGeneratedKeys() throws SQLException { if (generatedKeysRs == null) { generatedKeysStat = conn.createStatement(); generatedKeysRs = - generatedKeysStat.executeQuery("SELECT last_insert_rowid() WHERE 0 <> 0;"); + generatedKeysStat.executeQuery("SELECT 1 WHERE 1 = 2;"); } return generatedKeysRs; } From afffec43bbd4d7708586199c57aadfe0ab3fdbaa Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Thu, 18 Jan 2024 10:45:41 -0500 Subject: [PATCH 4/4] re-apply spotless code formatting on getGeneratedKeys --- src/main/java/org/sqlite/core/CoreStatement.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/sqlite/core/CoreStatement.java b/src/main/java/org/sqlite/core/CoreStatement.java index 9357fe908..b5834b0dd 100644 --- a/src/main/java/org/sqlite/core/CoreStatement.java +++ b/src/main/java/org/sqlite/core/CoreStatement.java @@ -193,8 +193,7 @@ public ResultSet getGeneratedKeys() throws SQLException { // up a new result set without any contents by issuing a query with a false where condition if (generatedKeysRs == null) { generatedKeysStat = conn.createStatement(); - generatedKeysRs = - generatedKeysStat.executeQuery("SELECT 1 WHERE 1 = 2;"); + generatedKeysRs = generatedKeysStat.executeQuery("SELECT 1 WHERE 1 = 2;"); } return generatedKeysRs; }