From eb6106b1e960dc6f0dc82ac6486ba491db7f430b Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Thu, 10 Oct 2024 16:00:26 +0200 Subject: [PATCH] refactor: Proper wrap retryable exceptions in TransactionSystemException. This avoids an attempted rollback when the commit already failed which will lead to an illegal state exception otherwise, which will completely throw off the transaction system. Discovered through an internal card. --- .../core/support/RetryExceptionPredicate.java | 7 ++++++- .../transaction/Neo4jTransactionManager.java | 17 +++++++++++++---- .../ReactiveNeo4jTransactionManager.java | 3 +++ .../QuerydslNeo4jPredicateExecutorIT.java | 2 +- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/springframework/data/neo4j/core/support/RetryExceptionPredicate.java b/src/main/java/org/springframework/data/neo4j/core/support/RetryExceptionPredicate.java index b2a0c88d5..aa7907261 100644 --- a/src/main/java/org/springframework/data/neo4j/core/support/RetryExceptionPredicate.java +++ b/src/main/java/org/springframework/data/neo4j/core/support/RetryExceptionPredicate.java @@ -25,6 +25,7 @@ import org.neo4j.driver.exceptions.ServiceUnavailableException; import org.neo4j.driver.exceptions.SessionExpiredException; import org.neo4j.driver.exceptions.TransientException; import org.springframework.dao.TransientDataAccessResourceException; +import org.springframework.transaction.TransactionSystemException; /** @@ -52,13 +53,17 @@ public final class RetryExceptionPredicate implements Predicate { return true; } + if (throwable instanceof TransactionSystemException && throwable.getCause() != null) { + throwable = throwable.getCause(); + } + if (throwable instanceof IllegalStateException) { String msg = throwable.getMessage(); return msg != null && RETRYABLE_ILLEGAL_STATE_MESSAGES.contains(msg); } Throwable ex = throwable; - if (throwable instanceof TransientDataAccessResourceException) { + if (throwable instanceof TransientDataAccessResourceException || throwable instanceof TransactionSystemException) { ex = throwable.getCause(); } diff --git a/src/main/java/org/springframework/data/neo4j/core/transaction/Neo4jTransactionManager.java b/src/main/java/org/springframework/data/neo4j/core/transaction/Neo4jTransactionManager.java index 3ea3ea4a2..51fcdde70 100644 --- a/src/main/java/org/springframework/data/neo4j/core/transaction/Neo4jTransactionManager.java +++ b/src/main/java/org/springframework/data/neo4j/core/transaction/Neo4jTransactionManager.java @@ -23,6 +23,8 @@ import org.neo4j.driver.Driver; import org.neo4j.driver.Session; import org.neo4j.driver.Transaction; import org.neo4j.driver.TransactionConfig; +import org.neo4j.driver.exceptions.Neo4jException; +import org.neo4j.driver.exceptions.RetryableException; import org.springframework.beans.BeansException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; @@ -338,10 +340,17 @@ public final class Neo4jTransactionManager extends AbstractPlatformTransactionMa @Override protected void doCommit(DefaultTransactionStatus status) throws TransactionException { - Neo4jTransactionObject transactionObject = extractNeo4jTransaction(status); - Neo4jTransactionHolder transactionHolder = transactionObject.getRequiredResourceHolder(); - Collection newBookmarks = transactionHolder.commit(); - this.bookmarkManager.resolve().updateBookmarks(transactionHolder.getBookmarks(), newBookmarks); + try { + Neo4jTransactionObject transactionObject = extractNeo4jTransaction(status); + Neo4jTransactionHolder transactionHolder = transactionObject.getRequiredResourceHolder(); + Collection newBookmarks = transactionHolder.commit(); + this.bookmarkManager.resolve().updateBookmarks(transactionHolder.getBookmarks(), newBookmarks); + } catch (Neo4jException ex) { + if (ex instanceof RetryableException) { + throw new TransactionSystemException(ex.getMessage(), ex); + } + throw ex; + } } @Override diff --git a/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveNeo4jTransactionManager.java b/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveNeo4jTransactionManager.java index f8c3223f0..1152eb0fa 100644 --- a/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveNeo4jTransactionManager.java +++ b/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveNeo4jTransactionManager.java @@ -21,6 +21,7 @@ import reactor.util.function.Tuples; import org.apiguardian.api.API; import org.neo4j.driver.Driver; import org.neo4j.driver.TransactionConfig; +import org.neo4j.driver.exceptions.RetryableException; import org.neo4j.driver.reactivestreams.ReactiveSession; import org.neo4j.driver.reactivestreams.ReactiveTransaction; import org.springframework.beans.BeansException; @@ -35,6 +36,7 @@ import org.springframework.lang.Nullable; import org.springframework.transaction.NoTransactionException; import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.TransactionException; +import org.springframework.transaction.TransactionSystemException; import org.springframework.transaction.reactive.AbstractReactiveTransactionManager; import org.springframework.transaction.reactive.GenericReactiveTransaction; import org.springframework.transaction.reactive.TransactionSynchronizationManager; @@ -326,6 +328,7 @@ public final class ReactiveNeo4jTransactionManager extends AbstractReactiveTrans .getRequiredResourceHolder(); return holder.commit() .doOnNext(bookmark -> bookmarkManager.resolve().updateBookmarks(holder.getBookmarks(), bookmark)) + .onErrorMap(e -> e instanceof RetryableException, e -> new TransactionSystemException(e.getMessage(), e)) .then(); } diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/QuerydslNeo4jPredicateExecutorIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/QuerydslNeo4jPredicateExecutorIT.java index a6f8ce835..125cab78e 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/imperative/QuerydslNeo4jPredicateExecutorIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/imperative/QuerydslNeo4jPredicateExecutorIT.java @@ -323,7 +323,7 @@ class QuerydslNeo4jPredicateExecutorIT { Predicate predicate = Expressions.predicate(Ops.EQ, firstNamePath, Expressions.asString("Helge")) .or(Expressions.predicate(Ops.EQ, lastNamePath, Expressions.asString("B."))); List people = repository.findBy(predicate, - q -> q.limit(1)).all(); + q -> q.sortBy(Sort.by("firstName").descending()).limit(1)).all(); assertThat(people).hasSize(1); assertThat(people).extracting(Person::getFirstName).containsExactly("Helge");