From b11270239f0d50de9860cf3cf7a3c3319e9f43cb Mon Sep 17 00:00:00 2001 From: Magnus Persson <1431685+magnusp@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:49:32 +0200 Subject: [PATCH] refactor(jdbc): name savepoint state transitions Fixes #210 Names the three savepoint state transitions in `LibsqlConnection` and moves savepoint-name resolution next to the type that owns it. - `beginSavepointTransaction` starts the transaction an auto-commit savepoint owns without sending `BEGIN`; a failed savepoint closes the fresh stream and leaves auto-commit on. - `discardSavepointsAfter` cancels the savepoints created after a rollback target, which stays active. - `closeTransactionOwnedBySavepoint` drops the released savepoint and its nested ones, and closes the stream only when the released savepoint is the outermost one that started the transaction. - `requireManualCommit(String action)` builds `Cannot when in autoCommit mode`, so commit, rollback, and savepoint rollback/release keep their distinct messages. - `savepointSqlName` moved to `LibsqlSavepoint.sqlNameOf`; messages unchanged. - `beginTransaction` folded into its only caller, `ensureTransactionStarted`, and the redundant second lock in `setSavepoint(String)` dropped. Preserved: auto-commit savepoint opens the stream and its outermost release commits and closes it; failed implicit savepoint leaves auto-commit on; rollback/release in auto-commit send nothing; duplicate names resolve by `lastIndexOf`. No test or `pom.xml` change. --- .../magnusp/libsql/jdbc/LibsqlConnection.java | 166 ++++++++---------- .../magnusp/libsql/jdbc/LibsqlSavepoint.java | 27 +++ 2 files changed, 103 insertions(+), 90 deletions(-) diff --git a/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlConnection.java b/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlConnection.java index 1c86980..fcf292d 100644 --- a/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlConnection.java +++ b/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlConnection.java @@ -211,9 +211,15 @@ private BatchResult sendBatchOnStream(Batch batch) throws SQLException { return sending(() -> client.executeBatchOnStream(batch)); } + /** Callers hold {@link #stateLock}. */ private void ensureTransactionStarted() throws SQLException { - if (beginPending()) { - beginTransaction(); + if (!beginPending()) return; + try { + sendOnStream(new Stmt(transactionMode.beginSql())); + } catch (SQLException e) { + // The begin failed, so the fresh stream holds no transaction state. + closeStreamQuietly(); + throw e; } } @@ -228,17 +234,6 @@ private boolean beginPending() { return !autoCommit && !client.hasOpenStream(); } - /** Callers hold {@link #stateLock}. */ - private void beginTransaction() throws SQLException { - try { - sendOnStream(new Stmt(transactionMode.beginSql())); - } catch (SQLException e) { - // The begin failed, so the fresh stream holds no transaction state. - closeStreamQuietly(); - throw e; - } - } - /** Callers hold {@link #stateLock}. */ private void closeStreamQuietly() { if (client == null) { @@ -327,9 +322,7 @@ public boolean getAutoCommit() throws SQLException { public void commit() throws SQLException { synchronized (stateLock) { checkClosed(); - if (autoCommit) { - throw new SQLException("Cannot commit when in autoCommit mode"); - } + requireManualCommit("commit"); endTransaction("COMMIT"); } } @@ -338,9 +331,7 @@ public void commit() throws SQLException { public void rollback() throws SQLException { synchronized (stateLock) { checkClosed(); - if (autoCommit) { - throw new SQLException("Cannot rollback when in autoCommit mode"); - } + requireManualCommit("rollback"); endTransaction("ROLLBACK"); } } @@ -516,12 +507,9 @@ private static void checkHoldability(int holdability) throws SQLException { } /** - * Creates an unnamed savepoint. When the connection is in auto-commit mode this starts a - * transaction, matching xerial sqlite-jdbc 3.53.4.0 and SQLite itself: a {@code SAVEPOINT} - * issued outside a transaction behaves as {@code BEGIN DEFERRED}, so the savepoint is sent on - * a new Hrana stream without a preceding {@code BEGIN} and releasing that outermost savepoint - * later commits. Inside a transaction begun by {@link #setAutoCommit(boolean)} the savepoint - * is nested in the lazily begun transaction instead. + * Creates a savepoint. When the connection is in auto-commit mode this starts a transaction, + * matching xerial sqlite-jdbc 3.53.4.0 and SQLite itself; inside a transaction begun by {@link + * #setAutoCommit(boolean)} the savepoint is nested in the lazily begun transaction instead. */ @Override public Savepoint setSavepoint() throws SQLException { @@ -530,11 +518,9 @@ public Savepoint setSavepoint() throws SQLException { @Override public Savepoint setSavepoint(String name) throws SQLException { - synchronized (stateLock) { - checkClosed(); - if (name == null || name.isBlank()) { - throw new SQLException("Savepoint name cannot be null or empty"); - } + checkClosed(); + if (name == null || name.isBlank()) { + throw new SQLException("Savepoint name cannot be null or empty"); } return setSavepointInternal(name); } @@ -543,44 +529,55 @@ private Savepoint setSavepointInternal(String name) throws SQLException { synchronized (stateLock) { checkClosed(); LibsqlSavepoint savepoint = new LibsqlSavepoint(++nextSavepointId, name); - String sql = "SAVEPOINT " + quoteSavepointName(savepoint.sqlName()); + String sqlName = savepoint.sqlName(); + Stmt stmt = new Stmt("SAVEPOINT " + quoteSavepointName(sqlName)); if (autoCommit) { - try { - sendOnStream(new Stmt(sql)); - } catch (SQLException e) { - // No transaction was started, so the fresh stream holds no state. - closeStreamQuietly(); - throw e; - } - autoCommit = false; - transactionStartedBySavepoint = true; - savepointNames.clear(); - savepointNames.add(savepoint.sqlName()); + beginSavepointTransaction(stmt, sqlName); } else { ensureTransactionStarted(); - sendOnStream(new Stmt(sql)); - savepointNames.add(savepoint.sqlName()); + sendOnStream(stmt); + savepointNames.add(sqlName); } return savepoint; } } + /** + * Starts the transaction an auto-commit savepoint owns. SQLite gives the savepoint the begin, + * so no {@code BEGIN} is sent; releasing this savepoint later commits the transaction and + * closes the stream. Callers hold {@link #stateLock}. + */ + private void beginSavepointTransaction(Stmt stmt, String sqlName) throws SQLException { + try { + sendOnStream(stmt); + } catch (SQLException e) { + // No transaction was started, so the fresh stream holds no state. + closeStreamQuietly(); + throw e; + } + autoCommit = false; + transactionStartedBySavepoint = true; + savepointNames.clear(); + savepointNames.add(sqlName); + } + @Override public void rollback(Savepoint savepoint) throws SQLException { synchronized (stateLock) { checkClosed(); - String sqlName = savepointSqlName(savepoint); - if (autoCommit) { - throw new SQLException("Cannot rollback to a savepoint when in autoCommit mode"); - } + String sqlName = LibsqlSavepoint.sqlNameOf(savepoint); + requireManualCommit("rollback to a savepoint"); ensureTransactionStarted(); sendOnStream(new Stmt("ROLLBACK TO SAVEPOINT " + quoteSavepointName(sqlName))); - int index = savepointNames.lastIndexOf(sqlName); - if (index >= 0) { - // Rolling back cancels the savepoints created after the target; the target - // itself stays active. - savepointNames.subList(index + 1, savepointNames.size()).clear(); - } + discardSavepointsAfter(sqlName); + } + } + + /** Cancels the savepoints created after the target, which stays active. Callers hold {@link #stateLock}. */ + private void discardSavepointsAfter(String sqlName) { + int index = savepointNames.lastIndexOf(sqlName); + if (index >= 0) { + savepointNames.subList(index + 1, savepointNames.size()).clear(); } } @@ -588,47 +585,36 @@ public void rollback(Savepoint savepoint) throws SQLException { public void releaseSavepoint(Savepoint savepoint) throws SQLException { synchronized (stateLock) { checkClosed(); - String sqlName = savepointSqlName(savepoint); - if (autoCommit) { - throw new SQLException("Cannot release a savepoint when in autoCommit mode"); - } + String sqlName = LibsqlSavepoint.sqlNameOf(savepoint); + requireManualCommit("release a savepoint"); ensureTransactionStarted(); sendOnStream(new Stmt("RELEASE SAVEPOINT " + quoteSavepointName(sqlName))); - int index = savepointNames.lastIndexOf(sqlName); - if (index >= 0) { - boolean outermost = index == 0; - savepointNames.subList(index, savepointNames.size()).clear(); - if (outermost && transactionStartedBySavepoint) { - // Releasing the savepoint that started the transaction commits it, so the - // stream holds no transaction state and the next statement lazily begins a - // new one, as after a commit. - transactionStartedBySavepoint = false; - savepointNames.clear(); - closeStreamQuietly(); - } - } + closeTransactionOwnedBySavepoint(sqlName); } } - private static String savepointSqlName(Savepoint savepoint) throws SQLException { - if (savepoint == null) { - throw new SQLException("Savepoint cannot be null"); - } - if (savepoint instanceof LibsqlSavepoint libsqlSavepoint) { - return libsqlSavepoint.sqlName(); - } - try { - String name = savepoint.getSavepointName(); - if (name != null && !name.isBlank()) { - return name; - } - } catch (SQLException ignored) { - // An unnamed foreign savepoint has no name; its id identifies it below. - } - try { - return "SQLITE_SAVEPOINT_" + savepoint.getSavepointId(); - } catch (SQLException e) { - throw new SQLException("Savepoint has no usable name or id", e); + /** + * Drops the released savepoint and the ones nested inside it. A duplicated name resolves to the + * later savepoint, so only releasing the outermost one closes the stream: that release commits + * the transaction the savepoint started. Callers hold {@link #stateLock}. + */ + private void closeTransactionOwnedBySavepoint(String sqlName) { + int index = savepointNames.lastIndexOf(sqlName); + if (index < 0) return; + savepointNames.subList(index, savepointNames.size()).clear(); + if (index > 0 || !transactionStartedBySavepoint) return; + transactionStartedBySavepoint = false; + closeStreamQuietly(); + } + + /** + * Rejects work JDBC allows only with auto-commit off. The action finishes the message, so + * commit, rollback, and savepoint release each keep their own wording. Callers hold {@link + * #stateLock}. + */ + private void requireManualCommit(String action) throws SQLException { + if (autoCommit) { + throw new SQLException("Cannot " + action + " when in autoCommit mode"); } } diff --git a/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlSavepoint.java b/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlSavepoint.java index f825909..9202004 100644 --- a/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlSavepoint.java +++ b/libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlSavepoint.java @@ -25,6 +25,33 @@ String sqlName() { return sqlName; } + /** + * The SQL name of any savepoint handed to this driver: its own name, else the name or generated + * id a foreign {@link Savepoint} reports. The messages are part of the JDBC contract; the + * connection keeps only the transitions. + */ + static String sqlNameOf(Savepoint savepoint) throws SQLException { + if (savepoint == null) { + throw new SQLException("Savepoint cannot be null"); + } + if (savepoint instanceof LibsqlSavepoint libsqlSavepoint) { + return libsqlSavepoint.sqlName(); + } + try { + String name = savepoint.getSavepointName(); + if (name != null && !name.isBlank()) { + return name; + } + } catch (SQLException ignored) { + // An unnamed foreign savepoint has no name; its id identifies it below. + } + try { + return "SQLITE_SAVEPOINT_" + savepoint.getSavepointId(); + } catch (SQLException e) { + throw new SQLException("Savepoint has no usable name or id", e); + } + } + @Override public int getSavepointId() throws SQLException { if (name != null) {