From 0194470c1fcead05e1e3da3f9105bb58a2c9d4a3 Mon Sep 17 00:00:00 2001 From: Zakaria El Orche Date: Wed, 12 Aug 2026 23:33:05 +0000 Subject: [PATCH] feat(data): actually invoke TxSynchronization.beforeCommit, and document the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit beforeCommit(boolean) has been part of the TxSynchronization API from the start and was invoked by nothing, in either manager — anyone implementing it got silence, the same failure class as the dropped afterCommit callbacks in the previous commit. Left out of that one because fixing it is a design decision rather than a restored behaviour; this is that decision, written down. It now runs immediately before the real commit, with the transaction still active and its session/connection still bound, which is the whole reason to have a hook on this side of the commit: it can still write through the same resource and land in the same atomic unit. Skipped when the transaction is already rollback-only, since there is no commit to precede. Throwing from it vetoes the commit: the transaction rolls back, the surviving callbacks hear ROLLED_BACK, and the exception propagates. Without that, a hook running before the commit would be strictly less useful than one running after. A commit that fails on its own now takes the same path instead of completing silently with no callback at all, and a rollback that also fails is attached as a suppressed exception rather than replacing the one that explains the failure. Documented on the interface itself and in flash-ext-data-core/docs/README.md: which hook sits on which side of the commit, what each may still touch, what throwing does, and the per-transaction scoping rule from the previous commit. Tests: 6 more (3 per manager) — runs inside the transaction with the resource still bound, receives the read-only flag, skipped on a rollback-only transaction, and vetoes the commit when it throws. 37 across the two managers now, all green, reactor verify passes the 80% Jacoco gate. Co-Authored-By: Claude Opus 5 --- .../flash-ext-data-core/docs/README.md | 45 ++++++++- .../flash/ext/data/core/ResourceRegistry.java | 22 +++++ .../ext/data/core/TxSynchronization.java | 50 ++++++++++ .../data/hibernate/HibernateTxManager.java | 18 ++++ ...HibernateTxManagerSynchronizationTest.java | 93 ++++++++++++++++--- .../flash/ext/data/jdbc/JdbcTxManager.java | 26 +++++- .../JdbcTxManagerSynchronizationTest.java | 76 +++++++++++++-- 7 files changed, 304 insertions(+), 26 deletions(-) diff --git a/flash-extensions/flash-ext-data-core/docs/README.md b/flash-extensions/flash-ext-data-core/docs/README.md index 8e19234..0454c12 100644 --- a/flash-extensions/flash-ext-data-core/docs/README.md +++ b/flash-extensions/flash-ext-data-core/docs/README.md @@ -21,7 +21,7 @@ Non parla con Hibernate o JDBC direttamente: espone solo astrazioni e un runtime - `RepositorySupport`: helper interno condiviso. - `TransactionPropagation`: semantica di propagazione. - `TransactionIsolation`: livello di isolamento. -- `TxSynchronization`: hook lifecycle. +- `TxSynchronization`: hook lifecycle (vedi sotto). ## Modello di esecuzione @@ -42,6 +42,46 @@ Il flusso è: - `NOT_SUPPORTED`: sospende la tx corrente ed esegue senza tx. - `MANDATORY`: richiede una tx attiva. +## Synchronization (`TxSynchronization`) + +Hook sul ciclo di vita di **una** transazione, registrati con `Data.afterCommit(...)` (o +direttamente con `ResourceRegistry.addSynchronization(...)`). + +Ogni callback appartiene esattamente alla transazione più interna attiva al momento della +registrazione, e scatta una volta sola quando *quella* transazione completa: + +- una tx interna **joined** (`REQUIRED`) non è una transazione a sé, quindi i callback registrati + al suo interno aspettano il commit più esterno; +- una tx `REQUIRES_NEW` lo è, quindi completarla fa scattare solo i propri callback e lascia in + sospeso quelli della transazione esterna sospesa. + +### Posizione rispetto al commit + +| hook | quando | risorsa | +| --- | --- | --- | +| `beforeCommit(readOnly)` | subito **prima** del commit reale | sessione/connection ancora **bound**, tx ancora attiva | +| `afterCommit()` / `afterRollback()` | dopo il completamento | risorsa già **sganciata** | +| `afterCompletion(outcome)` | dopo i due precedenti | risorsa già sganciata | + +`beforeCommit` è l'unico hook che può ancora scrivere sulla stessa risorsa e finire nella stessa +unità atomica: flush di un buffer, riga di audit, valore derivato. Non viene eseguito se la +transazione è già `rollback-only`, perché non c'è nessun commit da precedere. + +I callback post-completamento girano invece a risorsa sganciata: uno che apre una propria +transazione ne ottiene una **nuova** invece di agganciarsi a quella appena conclusa. È questo che +li rende il posto giusto per invalidare una cache, accodare un messaggio o notificare qualcosa +fuori dal database. + +### Fallimenti + +Lanciare da `beforeCommit` **veta il commit**: la transazione va in rollback, scattano +`afterRollback`/`afterCompletion(ROLLED_BACK)` e l'eccezione arriva al chiamante. È il motivo per +cui l'hook gira prima del commit e non dopo — può ancora rifiutare. + +Gli hook post-completamento non hanno questo potere: la transazione è già chiusa quando girano, +quindi un'eccezione si propaga ma non cambia nulla di ciò che è stato committato, e blocca i +callback in coda dietro di lei. + ## Uso di `Repository` `Repository` è la base comune per le repository concrete. @@ -79,3 +119,6 @@ Questo rende il layer dati componibile con il sistema di extension di Flash senz - Lo stack transazionale è thread-local e viene ripulito quando torna vuoto. - Le risorse backend sono sospese e ripristinate per `REQUIRES_NEW` e `NOT_SUPPORTED`. - `TxSynchronization` è il punto di aggancio per hook di commit/rollback/completion. +- Le synchronization sono in una lista thread-local; ogni transazione nuova registra quante ne + esistevano già alla sua apertura e fa scattare solo la propria coda, così una `REQUIRES_NEW` + non trascina con sé quelle della transazione sospesa. diff --git a/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/ResourceRegistry.java b/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/ResourceRegistry.java index 9052d7d..1af31ce 100644 --- a/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/ResourceRegistry.java +++ b/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/ResourceRegistry.java @@ -62,6 +62,28 @@ public final class ResourceRegistry { return SYNCHRONIZATIONS.get().size(); } + /** + * Runs {@link TxSynchronization#beforeCommit} on the synchronizations registered from + * {@code fromIndex} onward, while their transaction is still active and its resource still + * bound. Unlike {@link #fireSynchronizations} this leaves them registered: they still have + * their post-completion callbacks to come. Exceptions propagate on purpose — a beforeCommit + * that throws vetoes the commit, see {@link TxSynchronization}. + * + *

Snapshots before iterating, so a callback that registers further synchronizations (a + * nested {@code Data#afterCommit}) doesn't mutate the list mid-loop. Those new ones join the + * transaction's post-completion callbacks without getting a {@code beforeCommit} of their own, + * which is the only coherent answer once the pass is already running. + */ + public static void fireBeforeCommit(boolean readOnly, int fromIndex) { + List pending = SYNCHRONIZATIONS.get(); + if (fromIndex >= pending.size()) { + return; + } + for (TxSynchronization sync : List.copyOf(pending.subList(fromIndex, pending.size()))) { + sync.beforeCommit(readOnly); + } + } + /** * Fires (and removes) the synchronizations registered from {@code fromIndex} onward — the ones * belonging to the transaction now completing. Everything before that index was registered by diff --git a/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/TxSynchronization.java b/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/TxSynchronization.java index 380d51a..57ce363 100644 --- a/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/TxSynchronization.java +++ b/flash-extensions/flash-ext-data-core/src/main/java/dev/relism/flash/ext/data/core/TxSynchronization.java @@ -1,8 +1,58 @@ package dev.relism.flash.ext.data.core; +/** + * Lifecycle hooks for one transaction, registered through {@code Data#afterCommit} (or + * {@link ResourceRegistry#addSynchronization} directly) and fired by the {@link TxManager} that + * owns the transaction they were registered in. + * + *

Every callback belongs to exactly one transaction — the innermost one active at registration + * time — and fires exactly once, when that transaction completes. A joined + * ({@code REQUIRED}) inner transaction is not a transaction of its own, so callbacks registered + * inside one wait for the outermost commit; a {@code REQUIRES_NEW} transaction is, so completing + * it fires only its own callbacks and leaves the suspended outer transaction's alone. + * + *

Where each hook sits relative to the commit

+ *
    + *
  • {@link #beforeCommit(boolean)} — immediately before the real commit, with the + * transaction still active and its session/connection still bound. This is the only hook + * that can still write through that same resource and have the write land in the same atomic + * unit: flush a buffer, stamp an audit row, materialize a derived value. Skipped entirely + * when the transaction is already rollback-only, since there is no commit to precede.
    • + *
  • {@link #afterCommit()} / {@link #afterRollback()}, then {@link #afterCompletion(TxOutcome)} + * — after the transaction has completed and its resource has been unbound. Nothing + * done here is part of the transaction: a callback that opens its own transaction gets a + * fresh one instead of joining the one that just finished, which is what makes this the + * right place to refresh a cache, enqueue a message, or notify anything outside the + * database.
    • + *
+ * + *

Failure

+ * Throwing from {@link #beforeCommit(boolean)} vetoes the commit: the transaction is rolled + * back, {@link #afterRollback()}/{@link #afterCompletion(TxOutcome)} fire with + * {@link TxOutcome#ROLLED_BACK}, and the exception propagates to the caller. That is the point of + * this hook running before the commit rather than after — it can still refuse. + * + *

The post-completion hooks have no such power: the transaction is over by the time they run, + * so an exception from one propagates to the caller but changes nothing already committed, and + * stops the callbacks queued behind it. + */ public interface TxSynchronization { + + /** + * Runs inside the transaction, immediately before it commits — see the interface javadoc. + * Throwing from here rolls the transaction back instead of committing it. + * + * @param readOnly whether the transaction was opened read-only, so a callback that would + * otherwise write can skip work it is not allowed to do + */ default void beforeCommit(boolean readOnly) {} + + /** Runs after a successful commit, with the transaction's resource already unbound. */ default void afterCommit() {} + + /** Runs after a rollback, with the transaction's resource already unbound. */ default void afterRollback() {} + + /** Runs after {@link #afterCommit()}/{@link #afterRollback()}, whichever applied. */ default void afterCompletion(TxOutcome outcome) {} } diff --git a/flash-extensions/flash-ext-data-hibernate/src/main/java/dev/relism/flash/ext/data/hibernate/HibernateTxManager.java b/flash-extensions/flash-ext-data-hibernate/src/main/java/dev/relism/flash/ext/data/hibernate/HibernateTxManager.java index e642318..537893c 100644 --- a/flash-extensions/flash-ext-data-hibernate/src/main/java/dev/relism/flash/ext/data/hibernate/HibernateTxManager.java +++ b/flash-extensions/flash-ext-data-hibernate/src/main/java/dev/relism/flash/ext/data/hibernate/HibernateTxManager.java @@ -127,9 +127,27 @@ public class HibernateTxManager implements TxManager { s.session().getTransaction().rollback(); outcome = TxOutcome.ROLLED_BACK; } else { + // Still inside the transaction, session still bound: a beforeCommit callback can + // write through it and land in this same commit. Throwing from there vetoes the + // commit — see TxSynchronization. + ResourceRegistry.fireBeforeCommit(s.isReadOnly(), s.synchronizationBaseline()); s.session().getTransaction().commit(); outcome = TxOutcome.COMMITTED; } + } catch (RuntimeException e) { + // A vetoing beforeCommit, or a commit that failed outright: either way nothing was + // committed, so roll back and let the remaining callbacks hear ROLLED_BACK rather than + // nothing at all. A rollback failure here is swallowed deliberately — it would mask + // the exception that actually explains what went wrong, which is the one propagating. + if (s.session().getTransaction().isActive()) { + try { + s.session().getTransaction().rollback(); + } catch (RuntimeException suppressed) { + e.addSuppressed(suppressed); + } + } + outcome = TxOutcome.ROLLED_BACK; + throw e; } finally { // Order is load-bearing, and getting it wrong is silent: cleanupIfIdle() calls // ResourceRegistry.cleanup(), which removes the very ThreadLocal list of diff --git a/flash-extensions/flash-ext-data-hibernate/src/test/java/dev/relism/flash/ext/data/hibernate/HibernateTxManagerSynchronizationTest.java b/flash-extensions/flash-ext-data-hibernate/src/test/java/dev/relism/flash/ext/data/hibernate/HibernateTxManagerSynchronizationTest.java index d046a1e..6288ebe 100644 --- a/flash-extensions/flash-ext-data-hibernate/src/test/java/dev/relism/flash/ext/data/hibernate/HibernateTxManagerSynchronizationTest.java +++ b/flash-extensions/flash-ext-data-hibernate/src/test/java/dev/relism/flash/ext/data/hibernate/HibernateTxManagerSynchronizationTest.java @@ -14,9 +14,9 @@ import java.util.List; import static org.junit.jupiter.api.Assertions.*; /** - * Transaction synchronization semantics — the {@code afterCommit}/{@code afterRollback} callbacks - * {@code Data#afterCommit} exposes, and the contract callers build on: "my callback runs once, on - * the outermost commit, after the session is gone". + * Transaction synchronization semantics — the {@code beforeCommit}/{@code afterCommit}/ + * {@code afterRollback} callbacks {@code Data#afterCommit} exposes, and the contract callers build + * on: "my callback runs once, for my transaction, on the right side of the commit". * *

None of this was covered before, and the gap was not academic: {@link #afterCommit_fires_on_commit} * failed against the original {@code commit()}, which fired synchronizations only after @@ -52,11 +52,15 @@ class HibernateTxManagerSynchronizationTest { private static final class Recorder implements TxSynchronization { final List calls = new ArrayList<>(); + @Override public void beforeCommit(boolean readOnly) { calls.add("beforeCommit:" + readOnly); } @Override public void afterCommit() { calls.add("afterCommit"); } @Override public void afterRollback() { calls.add("afterRollback"); } @Override public void afterCompletion(TxOutcome outcome) { calls.add("afterCompletion:" + outcome); } } + private static final List COMMITTED = List.of("beforeCommit:false", "afterCommit", "afterCompletion:COMMITTED"); + private static final List ROLLED_BACK = List.of("afterRollback", "afterCompletion:ROLLED_BACK"); + @Test void afterCommit_fires_on_commit() { Recorder recorder = new Recorder(); @@ -65,7 +69,7 @@ class HibernateTxManagerSynchronizationTest { manager.commit(tx); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls); + assertEquals(COMMITTED, recorder.calls); } @Test @@ -76,12 +80,12 @@ class HibernateTxManagerSynchronizationTest { manager.rollback(tx); - assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls); + assertEquals(ROLLED_BACK, recorder.calls); } - /** A commit() call on a tx already marked rollback-only really rolls back — the callbacks must say so. */ + /** A commit() call on a tx already marked rollback-only really rolls back — and has no commit to precede. */ @Test - void commit_of_a_rollback_only_tx_fires_the_rollback_callbacks() { + void commit_of_a_rollback_only_tx_rolls_back_without_running_beforeCommit() { Recorder recorder = new Recorder(); TxStatus tx = manager.begin(TxDefinition.DEFAULTS); tx.markRollbackOnly(); @@ -89,13 +93,72 @@ class HibernateTxManagerSynchronizationTest { manager.commit(tx); - assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls); + assertEquals(ROLLED_BACK, recorder.calls); + } + + /** beforeCommit runs inside the transaction: session still bound, transaction still active. */ + @Test + void beforeCommit_runs_while_the_transaction_is_still_active() { + List stillActive = new ArrayList<>(); + List sessionSeen = new ArrayList<>(); + + TxStatus tx = manager.begin(TxDefinition.DEFAULTS); + Session session = tx.resource(Session.class); + ResourceRegistry.addSynchronization(new TxSynchronization() { + @Override + public void beforeCommit(boolean readOnly) { + stillActive.add(session.getTransaction().isActive()); + // MANDATORY only succeeds while a transaction is bound — proof this runs inside it. + TxStatus joined = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.MANDATORY)); + sessionSeen.add(joined.resource(Session.class)); + manager.commit(joined); + } + }); + + manager.commit(tx); + + assertEquals(List.of(true), stillActive, "the transaction must not have committed yet"); + assertSame(session, sessionSeen.get(0), "the same session must still be bound, so writes land in this commit"); + } + + @Test + void beforeCommit_is_told_whether_the_transaction_is_read_only() { + Recorder readWrite = new Recorder(); + TxStatus rw = manager.begin(TxDefinition.DEFAULTS); + ResourceRegistry.addSynchronization(readWrite); + manager.commit(rw); + + Recorder readOnly = new Recorder(); + TxStatus ro = manager.begin(TxDefinition.DEFAULTS.asReadOnly()); + ResourceRegistry.addSynchronization(readOnly); + manager.commit(ro); + + assertEquals("beforeCommit:false", readWrite.calls.get(0)); + assertEquals("beforeCommit:true", readOnly.calls.get(0)); + } + + /** Throwing from beforeCommit is a veto: no commit, the rollback callbacks run, the exception propagates. */ + @Test + void a_throwing_beforeCommit_vetoes_the_commit() { + Recorder recorder = new Recorder(); + TxStatus tx = manager.begin(TxDefinition.DEFAULTS); + Session session = tx.resource(Session.class); + ResourceRegistry.addSynchronization(new TxSynchronization() { + @Override public void beforeCommit(boolean readOnly) { throw new IllegalStateException("veto"); } + }); + ResourceRegistry.addSynchronization(recorder); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> manager.commit(tx)); + + assertEquals("veto", thrown.getMessage()); + assertFalse(session.getTransaction().isActive(), "the vetoed transaction must be rolled back, not left open"); + assertEquals(ROLLED_BACK, recorder.calls, "the surviving callbacks must hear ROLLED_BACK, not silence"); } /** - * The load-bearing ordering detail: callbacks run after the committed session is - * unbound, so a callback that opens its own transaction (a cache reload, an outbox drain) - * gets a fresh one instead of silently joining the transaction that just committed. + * The load-bearing ordering detail: post-completion callbacks run after the committed + * session is unbound, so a callback that opens its own transaction (a cache reload, an outbox + * drain) gets a fresh one instead of silently joining the transaction that just committed. */ @Test void a_synchronization_may_open_its_own_transaction() { @@ -133,7 +196,7 @@ class HibernateTxManagerSynchronizationTest { assertEquals(List.of(), recorder.calls, "the joined commit did not commit anything yet"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls); + assertEquals(COMMITTED, recorder.calls); } /** Each callback belongs to one transaction: a second transaction must not re-run the first's. */ @@ -169,11 +232,11 @@ class HibernateTxManagerSynchronizationTest { ResourceRegistry.addSynchronization(innerSync); manager.commit(inner); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), innerSync.calls); + assertEquals(COMMITTED, innerSync.calls); assertEquals(List.of(), outerSync.calls, "the outer transaction has not committed yet"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls); + assertEquals(COMMITTED, outerSync.calls); } /** A rolled-back inner REQUIRES_NEW must not fire the outer's callbacks either — same reason, opposite outcome. */ @@ -190,6 +253,6 @@ class HibernateTxManagerSynchronizationTest { assertEquals(List.of(), outerSync.calls, "the outer transaction is still open"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls); + assertEquals(COMMITTED, outerSync.calls); } } diff --git a/flash-extensions/flash-ext-data-jdbc/src/main/java/dev/relism/flash/ext/data/jdbc/JdbcTxManager.java b/flash-extensions/flash-ext-data-jdbc/src/main/java/dev/relism/flash/ext/data/jdbc/JdbcTxManager.java index afa2e09..08ad9b1 100644 --- a/flash-extensions/flash-ext-data-jdbc/src/main/java/dev/relism/flash/ext/data/jdbc/JdbcTxManager.java +++ b/flash-extensions/flash-ext-data-jdbc/src/main/java/dev/relism/flash/ext/data/jdbc/JdbcTxManager.java @@ -123,11 +123,20 @@ public class JdbcTxManager implements TxManager { s.connection().rollback(); outcome = TxOutcome.ROLLED_BACK; } else { + // Still inside the transaction, connection still bound: a beforeCommit callback + // can write through it and land in this same commit. Throwing from there vetoes + // the commit — see TxSynchronization. + ResourceRegistry.fireBeforeCommit(s.isReadOnly(), s.synchronizationBaseline()); s.connection().commit(); outcome = TxOutcome.COMMITTED; } } catch (SQLException e) { - throw new TxException(e); + TxException wrapped = new TxException(e); + outcome = rollbackAfterFailedCommit(s, wrapped); + throw wrapped; + } catch (RuntimeException e) { + outcome = rollbackAfterFailedCommit(s, e); + throw e; } finally { // Unbind the connection before firing, so a callback that opens its own transaction // gets a fresh one instead of joining the connection that just committed — and fire @@ -138,6 +147,21 @@ public class JdbcTxManager implements TxManager { } } + /** + * Nothing was committed — a vetoing {@code beforeCommit}, or a commit that failed outright — + * so undo whatever the transaction had done and report {@code ROLLED_BACK} to the callbacks + * still queued behind it. A failure to roll back is attached to the exception already on its + * way out rather than replacing it: that one explains what actually went wrong. + */ + private static TxOutcome rollbackAfterFailedCommit(JdbcTxStatus s, Throwable propagating) { + try { + s.connection().rollback(); + } catch (SQLException suppressed) { + propagating.addSuppressed(suppressed); + } + return TxOutcome.ROLLED_BACK; + } + @Override public void rollback(TxStatus status) { JdbcTxStatus s = (JdbcTxStatus) status; diff --git a/flash-extensions/flash-ext-data-jdbc/src/test/java/dev/relism/flash/ext/data/jdbc/JdbcTxManagerSynchronizationTest.java b/flash-extensions/flash-ext-data-jdbc/src/test/java/dev/relism/flash/ext/data/jdbc/JdbcTxManagerSynchronizationTest.java index 5465583..8a6e83e 100644 --- a/flash-extensions/flash-ext-data-jdbc/src/test/java/dev/relism/flash/ext/data/jdbc/JdbcTxManagerSynchronizationTest.java +++ b/flash-extensions/flash-ext-data-jdbc/src/test/java/dev/relism/flash/ext/data/jdbc/JdbcTxManagerSynchronizationTest.java @@ -29,11 +29,15 @@ class JdbcTxManagerSynchronizationTest { private static final class Recorder implements TxSynchronization { final List calls = new ArrayList<>(); + @Override public void beforeCommit(boolean readOnly) { calls.add("beforeCommit:" + readOnly); } @Override public void afterCommit() { calls.add("afterCommit"); } @Override public void afterRollback() { calls.add("afterRollback"); } @Override public void afterCompletion(TxOutcome outcome) { calls.add("afterCompletion:" + outcome); } } + private static final List COMMITTED = List.of("beforeCommit:false", "afterCommit", "afterCompletion:COMMITTED"); + private static final List ROLLED_BACK = List.of("afterRollback", "afterCompletion:ROLLED_BACK"); + @Test void afterCommit_fires_on_commit() { Recorder recorder = new Recorder(); @@ -42,7 +46,7 @@ class JdbcTxManagerSynchronizationTest { manager.commit(tx); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls); + assertEquals(COMMITTED, recorder.calls); } @Test @@ -53,11 +57,11 @@ class JdbcTxManagerSynchronizationTest { manager.rollback(tx); - assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls); + assertEquals(ROLLED_BACK, recorder.calls); } @Test - void commit_of_a_rollback_only_tx_fires_the_rollback_callbacks() { + void commit_of_a_rollback_only_tx_rolls_back_without_running_beforeCommit() { Recorder recorder = new Recorder(); TxStatus tx = manager.begin(TxDefinition.DEFAULTS); tx.markRollbackOnly(); @@ -65,10 +69,64 @@ class JdbcTxManagerSynchronizationTest { manager.commit(tx); - assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls); + assertEquals(ROLLED_BACK, recorder.calls); } - /** Callbacks run after the committed connection is unbound, so opening a transaction gets a fresh one. */ + /** beforeCommit runs inside the transaction: connection still bound, nothing committed yet. */ + @Test + void beforeCommit_runs_while_the_transaction_is_still_active() { + List connectionSeen = new ArrayList<>(); + + TxStatus tx = manager.begin(TxDefinition.DEFAULTS); + Connection connection = tx.resource(Connection.class); + ResourceRegistry.addSynchronization(new TxSynchronization() { + @Override + public void beforeCommit(boolean readOnly) { + // MANDATORY only succeeds while a transaction is bound — proof this runs inside it. + TxStatus joined = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.MANDATORY)); + connectionSeen.add(joined.resource(Connection.class)); + manager.commit(joined); + } + }); + + manager.commit(tx); + + assertSame(connection, connectionSeen.get(0), "the same connection must still be bound, so writes land in this commit"); + } + + @Test + void beforeCommit_is_told_whether_the_transaction_is_read_only() { + Recorder readWrite = new Recorder(); + TxStatus rw = manager.begin(TxDefinition.DEFAULTS); + ResourceRegistry.addSynchronization(readWrite); + manager.commit(rw); + + Recorder readOnly = new Recorder(); + TxStatus ro = manager.begin(TxDefinition.DEFAULTS.asReadOnly()); + ResourceRegistry.addSynchronization(readOnly); + manager.commit(ro); + + assertEquals("beforeCommit:false", readWrite.calls.get(0)); + assertEquals("beforeCommit:true", readOnly.calls.get(0)); + } + + /** Throwing from beforeCommit is a veto: no commit, the rollback callbacks run, the exception propagates. */ + @Test + void a_throwing_beforeCommit_vetoes_the_commit() { + Recorder recorder = new Recorder(); + TxStatus tx = manager.begin(TxDefinition.DEFAULTS); + ResourceRegistry.addSynchronization(new TxSynchronization() { + @Override public void beforeCommit(boolean readOnly) { throw new IllegalStateException("veto"); } + }); + ResourceRegistry.addSynchronization(recorder); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> manager.commit(tx)); + + assertEquals("veto", thrown.getMessage()); + assertEquals(ROLLED_BACK, recorder.calls, "the surviving callbacks must hear ROLLED_BACK, not silence"); + } + + /** Post-completion callbacks run after the committed connection is unbound, so opening a transaction gets a fresh one. */ @Test void a_synchronization_may_open_its_own_transaction() { List connectionsSeen = new ArrayList<>(); @@ -104,7 +162,7 @@ class JdbcTxManagerSynchronizationTest { assertEquals(List.of(), recorder.calls, "the joined commit did not commit anything yet"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls); + assertEquals(COMMITTED, recorder.calls); } @Test @@ -133,11 +191,11 @@ class JdbcTxManagerSynchronizationTest { ResourceRegistry.addSynchronization(innerSync); manager.commit(inner); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), innerSync.calls); + assertEquals(COMMITTED, innerSync.calls); assertEquals(List.of(), outerSync.calls, "the outer transaction has not committed yet"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls); + assertEquals(COMMITTED, outerSync.calls); } @Test @@ -153,6 +211,6 @@ class JdbcTxManagerSynchronizationTest { assertEquals(List.of(), outerSync.calls, "the outer transaction is still open"); manager.commit(outer); - assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls); + assertEquals(COMMITTED, outerSync.calls); } }