Transaction Support #1487

mikereiche · 2022-06-28T17:33:40Z

Closes #1145.

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

See #1257

Closes #1270. Co-authored-by: mikereiche <[email protected]>

Closes #1286. Co-authored-by: mikereiche <[email protected]>

See #1298

See #1301

Closes #1277.

See #1307

Closes #1352

Closes #1356.

Closes #1358.

…ty class. (#1366) Motivation: Currently an abstract entity class specified in the repository definition can be used for the predicate typeKey = typeAlias(of abstract entity class) in queries. Since documents are stored with typeKey = typeAlias(of concrete class) those queries will never match any documents. To allow this to work, all of the abstract entity class an all concrete subclasses must use the same typeAlias. Once those documents are found, regardless of their concrete class, they will all have the same typeKey = typeAlias, instead of having the typeAlias specific to the concrete class. Additional information in the stored document is needed to identify the concrete class (subtype in the example test case), as well as a TypeMapper to interpret that information. Changes: This allows a common TypeAlias to be used for the purpose of the predicate typeKey = typeAlias, and the determination of the concrete type by implementing an AbstractingMappingCouchbaseConverter that inspects the 'subtype' property. Closes #1365. Co-authored-by: Michael Reiche <[email protected]>

@transactional

The fluent operations are common for options that are common to operations with and without transactions. Once there is a transaction(ctx), or an option specific to without-transactions (such as scanConsistency), the interfaces are bifurcated, so that an non-transaction option cannot be applied to a transaction operation, and a transaction(ctx) cannot be applied where a non-transaction option has already been applied. Closes #1145. Support for @transactional for blocking and reactive Transactionmanager. Closes 1145. Transaction Support. Transaction Support. Porting to SDK-integrated version of transactions The transactions logic exists in the Java SDK as of 3.3.0, with a slightly different API. This is the first effort at the port, which literally just compiles. It will not run as crucial code has been commented and todo-ed. There is work remaining to figure out how to complete the port, as some crucial parts (such as ctx.commit() and ctx.rollback()) have been intentionally removed. Continuing work to get the ExtSDKIntegration port working Trying to transition to CallbackPreferring manager. Added CouchbaseSimpleCallbackTransactionManager, the simplest possible implementation of CallbackPreferringTransactionManager, combined with a simpler approach to ThreadLocal storage in ReactiveInsertByIdSupport. Test 'commitShouldPersistTxEntriesOfTxAnnotatedMethod' is now passing. Adding WIP get-and-replace @transactional support (Not yet working as CAS/version field in Person is not populated correctly.) Datacouch 1145 transaction support (#1423) * Porting to SDK-integrated version of transactions The transactions logic exists in the Java SDK as of 3.3.0, with a slightly different API. This is the first effort at the port, which literally just compiles. It will not run as crucial code has been commented and todo-ed. There is work remaining to figure out how to complete the port, as some crucial parts (such as ctx.commit() and ctx.rollback()) have been intentionally removed. * Continuing work to get the ExtSDKIntegration port working Trying to transition to CallbackPreferring manager. * Added CouchbaseSimpleCallbackTransactionManager, the simplest possible implementation of CallbackPreferringTransactionManager, combined with a simpler approach to ThreadLocal storage in ReactiveInsertByIdSupport. Test 'commitShouldPersistTxEntriesOfTxAnnotatedMethod' is now passing. * Adding WIP get-and-replace @transactional support (Not yet working as CAS/version field in Person is not populated correctly.) Commit before pulling Graham's changes. Merge branch 'datacouch_1145_transaction_support' of github.com:spring-projects/spring-data-couchbase into datacouch_1145_transaction_support Transitioning to use CoreTransactionAttemptContext. Tests may fail. Removing AttemptContextReactiveAccessor Don't think we need this, as we can pass around CoreTransactionAttemptContext instead, which gives access to a lot of internals. Removing TransactionsReactive Would prefer not to C&P a huge class out of the transaction internals, and don't think we need it. Removing some files not currently used To reduce & simplify the amount of code to look at. Some don't seem to be used in any branch, some just aren't used in this branch. Removing CouchbaseTransactionInterceptor As per offline discussion, CallbackPreferringPlatformTransactionManager is perhaps the optimal solution. Copying @transactional tests out into separate class Tidyup Tidyup test names Verify GenericSupport is on same thread before and after transactional operation Refactoring CouchbaseSimpleCallbackTransactionManager ThreadLocalStorage management Using latest java-client ReactiveReplaceByIdSupport - Fixing use of CAS now have CoreTransactionAttemptContext. Removing unused code. ReactiveInsertByIdSupport - fixing use of reactive vs non-reactive, and CAS Merging upstream Remove incorrect thread check (.doOnNext could execute on a different thread) Get scope and collection from pseudoArgs and some cleanup. Completing merge from upstream Removing unused classes Give GenericSupport a better name Reject at runtime options that aren't supported in a transaction Fixing some small todos, partly by removing unused coe Fix runtime option checks Simplifying CouchbaseSimpleCallbackTransactionManager ThreadLocalStorage Standardising on ReactiveCouchbaseResourceHolder rather than holding CoreTransactionAttemptContext too Removing version from CouchbaseDocument Can't recall why I added this, and tests pass without it Improving CouchbaseTransactionalIntegrationTests and adding more tests Reject operations that aren't allowed in a transaction (upsertById etc.) Improve handling of CAS mismatch By calling CoreTransactionAttemptContext.operationFailed, it ensures that internal state is set. So even if the user catches the exception, the transaction still behaves as it should. Removing a now-redundant non-transactional check on upsertById I missed this when adding TransactionalSupport.verifyNotInTransaction here. Support @transactional options timeout and isolation level Add ReactiveTransactionWrapper/TransactionWrapper and a bunch of cleanup. Merge branch 'datacouch_1145_transaction_support' of https://github.com/programmatix/spring-data-couchbase into programmatix-datacouch_1145_transaction_support Fixed up merge issues. Datacouch 1145 transaction support (#1447) * Move CouchbaseTransactionalOperator to use SLF4J, same as rest of the code. * Handle all propagation levels * Adding new tests for repository calls inside @transactional One test is failure due to what looks like a bug elsewhere. * Rename CouchbaseTransactionalIntegrationTests, and check after each test that we're not in a transaction. Remove unnecessary methods From ReactiveCouchbaseClientFactory. Also rationalized naming of methods and other changes. Cleanup of test classes. Datacouch 1145 transaction support (#1448) * Move CouchbaseTransactionalOperator to use SLF4J, same as rest of the code. * Handle all propagation levels * Adding new tests for repository calls inside @transactional One test is failure due to what looks like a bug elsewhere. * Rename CouchbaseTransactionalIntegrationTests, and check after each test that we're not in a transaction. * Remove unnecessary methods From ReactiveCouchbaseClientFactory. Also rationalized naming of methods and other changes. * Cleanup of test classes. * Removing unused classes (Reducing the cognitive burden) * Removing version from CouchbaseDocument This change was done previously - it must have slipped back in a merge. * Adding and removing TODOs * Adding and removing TODOs * DRYing CouchbaseTransactionalPropagationIntegrationTests * Check propagation tests retry as expected * Tidy up PersonWithoutVersion to the minimum required * Removing unused code This should all be non-destructive. Just removing code IntelliJ declares unused. Intent is to make it easier to figure out how the CoreTransactionAttemptContext TLS is working. * Adding tests for @transactional removeByQuery and findByQuery Failing as they aren't being executed transactionally - investigating why. Co-authored-by: Michael Reiche <[email protected]> change references to resource holder change refs to resource holder Merge branch 'programmatix-datacouch_1145_transaction_support' of github.com:spring-projects/spring-data-couchbase into programmatix-datacouch_1145_transaction_support Removing core transaction attempt context bound couchbase client factory rebased (#1449) * Move ReactiveTransactionsWrapper tests into a new file * (Temporarily?) disabling tests using CouchbaseTransactionOperation or TransactionalOperator As I feel we should be removing/not-supporting these, and on this branch I've broken them. * Make all transaction tests call assertNotInTransaction * Removing unused code * Instead of binding the transaction AttemptContext to a CouchbaseClientFactory, fetch it from ThreadLocalStorage (or the reactive context) instead. This allows a lot of simplifying: * The non-trivial ReactiveCouchbaseClientUtils can be removed * As can CoreTransactionAttemptContextBoundCouchbaseClientFactory Also removing TransactionalSupport.one as it wasn't providing as much DRY utility as I thought it would - only used in two places. This change won't compile on its own. To reduce the complexity of this patchset, the Reactive*OperationSupport changes will go into a separate commit. * Reactive*OperationSupport changes to support the previous commit. * Fixing ReactiveRemoveByQuerySupport. Both to support the changes to TransactionalSupport. And to fix the TODO where the query resuls were not being handled. * Disabling a test * Adding CouchbaseTransactionsWrapperTemplateIntegrationTests * Another advantage of removing CoreTransactionAttemptContextBoundCouchbaseClientFactory is we can remove Cluster and ClusterInterface. * Adding CouchbaseReactiveTransactionsWrapperTemplateIntegrationTests Some of these tests are currently failing - tracking down where the issue is. Merge branch 'programmatix-datacouch_1145_transaction_support' of github.com:spring-projects/spring-data-couchbase into programmatix-datacouch_1145_transaction_support manual merges for PR manual merges for PR Fix a bunch of test cases and remove unused bits. Removing CouchbaseTransactionInterceptor As discussed on Slack. Reenabling some tests that are passing (Unclear why these were disabled?) Verified that CallbackPreferringPlatformTransactionManager getTransaction/commit/rollback are never called Adding tests for TransactionTemplate, which works fine with CouchbaseSimpleCallbackTransactionManager Whether we actually document this support is another matter - it's yet another way of doing transactions. Small fixes to support TransactionTemplate * Handle if the user has set isRollbackOnly on the TransactionStatus (which is only available - I think - when using TransactionTemplate) * Supply a `transaction` object to CouchbaseTransactionStatus so that status.isNewTransaction() correctly returns true. (This method requires that transaction to be set.) Clarifying that direct use of PlatformTransactionManager is not supported Adding further TransactionTemplateIntegrationTests tests Fixing removeByQuery queryOptions creation Removing now-fixed TODO (no longer key off Cluster) Adding overload to CouchbaseSimpleCallbackTransactionManager to allow it to be constructed without a TransactionOptions. Adding CouchbaseSimpleTransactionalOperator, the simplest possible implementation of TransactionalOperator. Adding retry tests Just making sure that error handling and retries are done correctly throughout. Updating and adding some TODOs Have CouchbaseTransactionManager support CouchbaseResourceHolder.class binding Adding more tests for CouchbaseTransactionManager. These tests fail, and are known to fail. I'm adding them as a solid demonstration of why I don't feel we can have this CouchbaseTransactionManager: it doesn't provide the crucial 'core loop' functionality, including error handling and retries. We should standardise on CouchbaseSimpleCallbackTransactionManager instead. CouchbaseSimpleCallbackTransactionManager.executeNewReactiveTransaction now buffers results rather than trying to stream a Flux from out of a completed lambda (which I doubt is even possible.) Removing comment that has been resolved. (As per Slack, we will live with this limitation.) Adding CouchbaseSimpleTransactionInterceptor, a very simple TransactionInterceptor implemention that defers to CouchbaseSimpleCallbackTransactionManager if that is the provided TransactionManager, and otherwise just calls super. This allows reactive @transactional - though all @transactional methods including blocking will now flow through it. There are two rather divergent approaches in the code currently: 1. CouchbaseTransactionManager, ReactiveTransactionManager, CouchbaseTransactionalOperator, CouchbaseTransactionInterceptor 2. CouchbaseSimpleCallbackTransactionManager, CouchbaseSimpleTransactionalOperator, CouchbaseSimpleTransactionInterceptor I know the intent is to remove some aspects of (1), but until that's done it's proving tricky to have tests for both concurrently - I've hit several issues on adding CouchbaseSimpleTransactionInterceptor, with 'multiple transaction manager beans in config' being common. So, temporarily moving some beans from AbstractCouchbaseConfiguration into the test Config class, renaming it, and having two separately TransactionsConfig classes for the two approaches. Once we've aligned the approaches more, can move what beans survive back into AbstractCouchbaseConfiguration. Safety check in CouchbaseSimpleCallbackTransactionManager that the blocking run is not accidentally running a reactive @transactional somehow. Adding tests for reactive @transactional, which now works (including error handling and retries) as of the CouchbaseSimpleTransactionInterceptor. With TransactionsConfigCouchbaseSimpleTransactionManager change, can now simplify @transactional(transactionManager = ...) to just @transactional. Tidying TODOs I saw in a PR comment that getResources no longer uses TransactionOptions Removing configureTransactions from config As per PR discussion Removing some code that has now been refactored into 3.3.1 SDK Removing TODOs that are TODONE already Removing transactionsOptions() bean from AbstractCouchbaseConfiguration. As per comment, this feels unnecessary: any options you'd configure at this config level, you'd surely provide at the global (Cluster) level instead? Reinstating some commented-out code This looks pretty crucial - can't recall why I commented it in first place Removing TransactionResult This was from a now-abandoned idea of storing transactional metadata in the entity class. Fix recent removal of TransactionOptions bean Switch some IllegalStateException for more accurate UnsupportedOperationException Tidying tests to remove old GenericApplicationContext method Tidying some TODOs (todo gp == in code that I think we should remove) (todo gpx == needs looking at) Tidying TransactionsConfigCouchbaseSimpleTransactionManager Provide SpringTransactionAttemptContext and ReactiveSpringTransactionAttemptContext wrappers For use by TransactionsWrapper and reactive equivalent. Pro: it's an abstraction layer. It lets us add Spring-specific functionality, or hide functionality that for whatever reason doesn't work with Spring (which is why it's composition rather than inheritance, beyond that being a best practice anyway). Con: any new API added will also have to be added to these wrappers. But that's a small amount of work and API is added very infrequently. Cleanup tests mostly. Temporary fix for CouchbaseSimpleCallbackTransactionManager. Merge branch 'programmatix-datacouch_1145_transaction_support' of github.com:spring-projects/spring-data-couchbase into programmatix-datacouch_1145_transaction_support Tidying up test cases. Fixes to over-zealous manual merging. - keep CouchbaseTransactionManager as Graham is still using it. - fix tests that were expecting SimulateFailure to be nested. Add ReactiveTransactionWrapper and enable tests that use it. More tidying. Removed CouchbaseTransactionManager. Only wrap exceptions in CouchbaseSimpleTransactionInterceptor if they are not RuntimeExceptions. Removed CouchbaseTransactionalOperator and ReactiveCouchbaseTransactionalOperator. Fixed non-transactions regression introduced in previous commit. Removing the two transactions config classes Now we've landed on a single agreed approach there's no need for this separation any more, and any beans can be moved back into AbstractCouchbaseConfiguration. Rename CouchbaseSimpleTransactionInterceptor The "Simple" moniker is no longer useful since we now only have one of these. Rename CouchbaseSimpleCallbackTransactionManager The "Simple" moniker is no longer useful since we now only have one of these. Rename CouchbaseSimpleTransactionalOperator The "Simple" moniker is no longer useful since we now only have one of these. Change CouchbaseTransactionalOperator construction to static As per Slack discussion. Remove comments related to another database Removing a test comment that doesn't seem to apply anymore Test passes for me at least Simplify TransactionsWrapper and AttemptContextReactiveAccessor newCoreTranactionAttemptContext is reimplementing some code from core plus has some config bugs to resolve. I think it's simpler to remove it and replace TransactionsWrapper (the only code still using this method) with the simple code seen now. (Note this is similar to how I had it before 1701183 - not sure if that commit intentionally reverted things?) This change also petmits a lot of tidyup & simplification throughout the codebase. We can just create CouchbaseResourceHolders directly now. Tidying a test No longer needs retryWhen now using the new approaches Remove ReactiveCouchbaseClientFactory. It was added to support getting the transaction from the TransactionSynchronizationManager.forCurrent() and providing a template with a session containing the transaction. Fixing some code warnings Mostly removing unused code Mark internal classes @Stability.Internal Removing some tests that have already been previously moved into another file. Removing the transaction wrappers Requires JVMCBC-1105 and 3.3.2 Add more tests for native SDK transactions Move all tests related to native SDK transactions into their own package It makes it easier to test just that functionality while iterating. Removing CouchbaseTransactionManagerTransactionalTemplateIntegrationTests This test is now redundant. It was created to show why the original CouchbaseTransactionManager couldn't work (no retries). Now that has been replaced, this test is just duplicating others. Adding some minimal JavaDocs and comments. Tidying up after moving ThreadLocalStorage into SDK Deleting CouchbaseTemplateTransactionIntegrationTests As this relies on Spring test @transactional, which we do not support as that Spring logic is not aware of CallbackPreferringTransactionManager. Removed some redundant bean names Tidying up tests and comments Removing some now-unused reflection code Starting with mapping TransactionFailedException and TransactionCommitAmbiguousException, into new errors TransactionSystemUnambiguousException and TransactionSystemAmbiguousException. These will be raised from an @transactional transaction. E.g. to do error handling the user would do: ``` try { service.transactionalMethod(); } catch (TransactionSystemAmbiguousException ex) { // app-specific handling } catch (TransactionSystemUnambiguousException ex) { // app-specific handling } class Service { @transactional void transactionalMethod() { // ... } } ``` Mapping TransactionOperationFailedException, which is an opaque signal raised from transaction operations, to new exception UncategorizedTransactionDataAccessException. This depends on some new functionality added into Java SDK 3.3.2, WrappedTransactionOperationFailedException. Minor tidyuo Improving tests for correct operation-level errors

mikereiche · 2022-06-28T18:41:16Z

src/main/java/org/springframework/data/couchbase/SimpleCouchbaseClientFactory.java

@@ -97,9 +97,9 @@ public Scope getScope() {
 	@Override
 	public Collection getCollection(final String collectionName) {
 		final Scope scope = getScope();
-		if (collectionName == null) {
+		if (collectionName == null || CollectionIdentifier.DEFAULT_COLLECTION.equals(collectionName)) {
 			if (!scope.name().equals(CollectionIdentifier.DEFAULT_SCOPE)) {


better checking for illegal scope/collection combination.

mikereiche · 2022-06-28T18:44:02Z

src/main/java/org/springframework/data/couchbase/core/AbstractTemplateSupport.java

@@ -0,0 +1,233 @@
+/*


factored out common methods from reactive/non-reactive template support.

Also - applyUpdatedCas() and applyUpdatedId() have been combined into applyResult().

And decodeEntity() has two extra args - TransactionResultHolder and CouchbaseResourceHolder - that are not currently used (but might be in the future).

mikereiche · 2022-06-28T18:46:07Z

...main/java/org/springframework/data/couchbase/core/ExecutableFindByQueryOperationSupport.java

@@ -136,7 +136,7 @@ public FindByQueryWithProjection<T> project(String[] fields) {
 		}

 		@Override
-		public FindByQueryWithProjection<T> distinct(final String[] distinctFields) {
+		public FindByQueryWithProjecting<T> distinct(final String[] distinctFields) {
 			Assert.notNull(distinctFields, "distinctFields must not be null");


fix an inconsistency in the fluent api.

mikereiche · 2022-06-28T18:47:17Z

src/main/java/org/springframework/data/couchbase/core/ExecutableRemoveByIdOperation.java

+		 * @return result of the remove
+		 */
+		RemoveResult oneEntity(Object entity);
+


remove operations in a transaction must have a cas - which comes from the entity. one(id) is not sufficient in a transaction.

mikereiche · 2022-06-28T18:48:21Z

src/main/java/org/springframework/data/couchbase/core/NonReactiveSupportWrapper.java

@@ -17,13 +17,16 @@



changes to go with factoring in AbstractTemplateSupport

mikereiche · 2022-06-28T20:55:24Z

src/main/java/org/springframework/data/couchbase/core/ExecutableRemoveByIdOperationSupport.java

-			return new ExecutableRemoveByIdSupport(template, domainType, scope, collection != null ? collection : this.collection, options, persistTo, replicateTo,
-					durabilityLevel, cas);
+			return new ExecutableRemoveByIdSupport(template, domainType, scope,
+					collection != null ? collection : this.collection, options, persistTo, replicateTo, durabilityLevel, cas);


reformatted only.

Dang, I missed durabilityLevel throughout when writing rejectInvalidTransactionalOptions. Could you please add? We don't support overriding the durability level on a per-operation basis inside a transaction.

I created an issue for this.

mikereiche · 2022-06-28T20:55:39Z

src/main/java/org/springframework/data/couchbase/core/ExecutableRemoveByIdOperationSupport.java

-			return new ExecutableRemoveByIdSupport(template, domainType, scope != null ? scope : this.scope, collection, options, persistTo, replicateTo,
-					durabilityLevel, cas);
+			return new ExecutableRemoveByIdSupport(template, domainType, scope != null ? scope : this.scope, collection,
+					options, persistTo, replicateTo, durabilityLevel, cas);


reformatted only.

mikereiche · 2022-06-28T20:59:36Z

src/main/java/org/springframework/data/couchbase/core/ReactiveFindByQueryOperationSupport.java

@@ -101,7 +106,7 @@ public FindByQueryWithQuery<T> matching(Query query) {
 		}

 		@Override
-		public TerminatingFindByQuery<T> withOptions(final QueryOptions options) {
+		public FindByQueryWithQuery<T> withOptions(final QueryOptions options) {


fix fluent api inconsistencies

mikereiche · 2022-06-28T21:03:12Z

...main/java/org/springframework/data/couchbase/core/ReactiveRemoveByQueryOperationSupport.java

@@ -103,7 +125,7 @@ public TerminatingRemoveByQuery<T> matching(final Query query) {
 		}

 		@Override
-		public RemoveByQueryWithConsistency<T> inCollection(final String collection) {
+		public RemoveByQueryWithOptions<T> inCollection(final String collection) {


fix fluent api inconsistency

mikereiche · 2022-06-28T21:06:54Z

src/main/java/org/springframework/data/couchbase/transaction/CouchbaseResourceHolder.java

@@ -0,0 +1,62 @@
+/*


Currently holds only the CoreTransactionAttemptContext.
In the future, it could also hold a map of getResults for the current transaction, such that subsequent operation could lookup the TransactionResult and use that instead of refetching with a ctx.get().

daschl · 2022-06-29T05:59:30Z

pom.xml

-        <couchbase>3.3.0</couchbase>
-        <couchbase.osgi>3.3.0</couchbase.osgi>
+        <couchbase>3.3.2-SNAPSHOT</couchbase>
+        <couchbase.osgi>3.3.1</couchbase.osgi>


would this also have to be 3.3.2-SNAPSHOT?

Yes, it should be. I'm not sure why we have couchbase.osgi separate from couchbase. I think it's referenced only in template.mf and must be the version of the couchbase sdk.

template.mf: com.couchbase.client.*;version="${couchbase.osgi:[=.=.=,+1.0.0)}",

daschl · 2022-06-29T06:00:37Z

src/main/java/org/springframework/data/couchbase/SimpleCouchbaseClientFactory.java

@@ -97,9 +97,9 @@ public Scope getScope() {
 	@Override
 	public Collection getCollection(final String collectionName) {
 		final Scope scope = getScope();
-		if (collectionName == null) {
+		if (collectionName == null || CollectionIdentifier.DEFAULT_COLLECTION.equals(collectionName)) {
 			if (!scope.name().equals(CollectionIdentifier.DEFAULT_SCOPE)) {


scope could be null here and throw a npe too?

Yes. Fixed the checking.

daschl · 2022-06-29T06:02:01Z

src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java

+	 * @return
+	 */
+	@Bean(BeanNames.COUCHBASE_TRANSACTION_MANAGER)
+	CouchbaseCallbackTransactionManager callbackTransactionManager(CouchbaseClientFactory clientFactory) {


should the name of this bean be couchbaseTransactionManager? (the callback in here, at least in the bean name, does not feel right..) (leaks impl detail and gives no hint to what actual transaction manager it is -- and the bean is also named differently)

I'll change the method name.
The actual bean name is already couchbaseTransactionManager.
COUCHBASE_TRANSACTION_MANAGER = "couchbaseTransactionManager";

daschl · 2022-06-29T06:02:33Z

src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java

+	 * @return
+	 */
+	@Bean(BeanNames.COUCHBASE_TRANSACTIONAL_OPERATOR)
+	public CouchbaseTransactionalOperator transactionalOperator(


should it be couchbaseTransactionalOperator ? Our other beans also ave the couchbase prefix I think

Yes. Changing the method name. Bean name is already couchbaseTransactionalOperator.

daschl · 2022-06-29T06:04:31Z

src/main/java/org/springframework/data/couchbase/core/AbstractTemplateSupport.java

+ *
+ * @author Michael Reiche
+ */
+public abstract class AbstractTemplateSupport {


is this stuff @internal?

added @Stability.Internal

daschl · 2022-06-29T06:10:07Z

src/main/java/org/springframework/data/couchbase/core/ReactiveFindByQueryOperationSupport.java

@@ -172,65 +177,95 @@ public Mono<T> first() {
 		public Flux<T> all() {
 			PseudoArgs<QueryOptions> pArgs = new PseudoArgs(template, scope, collection, options, domainType);


Same as above, I think this needs Flux.defer() wrapping

daschl · 2022-06-29T06:11:11Z

src/main/java/org/springframework/data/couchbase/core/ReactiveFindByQueryOperationSupport.java

+			TransactionQueryOptions opts = OptionsBuilder.buildTransactionQueryOptions(buildOptions(options));
+			return opts;
+		}
+
 		@Override
 		public Mono<Long> count() {
 			PseudoArgs<QueryOptions> pArgs = new PseudoArgs(template, scope, collection, options, domainType);


Actually this seems to be in place in many methods, maybe doing the .defer methods should be in a follow-up changeset?

checkForTransactionInThreadLocalStorage() returns a Mono.deferContextual() - is that sufficient?
wrapping in another Mono.defer() just makes the the setup - determining the scope/collection and args to also be deferred. And it also makes the stack deeper :)

daschl · 2022-06-29T06:11:35Z

...va/org/springframework/data/couchbase/core/ReactiveFindFromReplicasByIdOperationSupport.java

-			LOG.trace("getAnyReplica key={} {}", id, pArgs);
-			return Mono.just(id)
+			if (LOG.isDebugEnabled()) {
+				LOG.debug("getAnyReplica key={} {}", id, pArgs);


any reason this is debug now vs trace?

Yes. The auditing is also logged at trace - so if one wanted to see the operations logged, they would also have to see the auditing logged - which is noisy. Now one can set logging to debug and see the operations without the audit.
The operations are org.springframework.data.couchbase.core.* and the AuditEventListener is. org.springframework.data.couchbase.core.mapping.event.AuditingEventListener. So configuring a logger for org.springframework.data.couchbase.core for operations would also get the audit logging. And I never thought of putting a separate org.springframework.data.couchbase.core.mapping.event.AuditingEventListener logger at debug to not get the audit events.

daschl · 2022-06-29T06:15:04Z

...ain/java/org/springframework/data/couchbase/repository/support/DynamicInvocationHandler.java

 		}
 		this.options = options;
 		this.collection = collection;
 		this.scope = scope;
 		this.repositoryClass = target.getClass();
 	}

+	void printInterfaces(Class clazz, String tab) {


is this a leftover (with the print above) from development? if log, then to the logger or into a string?

changed to construct a string, then include in thrown exception.

daschl · 2022-06-29T06:15:44Z

...main/java/org/springframework/data/couchbase/repository/support/TransactionResultHolder.java

@@ -0,0 +1,56 @@
+/*


If it's not used, can we get rid of it?

programmatix · 2022-06-30T13:50:13Z

src/main/java/org/springframework/data/couchbase/core/CouchbaseOperations.java

+	<T> T save(T entity, String... scopeAndCollection);
+
+	<T> Long count(Query query, Class<T> domainType);
+


May be worth JavaDoc-ing the semantics of save()? E.g. as a user I'd be curious when should I call it (is there any automatic flush or do I have to save), does it internally do a replace or an upsert or an insert, does it respect CAS, etc.

programmatix · 2022-06-30T13:53:23Z

src/main/java/org/springframework/data/couchbase/core/ExecutableRemoveByIdOperationSupport.java

-			return new ExecutableRemoveByIdSupport(template, domainType, scope, collection != null ? collection : this.collection, options, persistTo, replicateTo,
-					durabilityLevel, cas);
+			return new ExecutableRemoveByIdSupport(template, domainType, scope,
+					collection != null ? collection : this.collection, options, persistTo, replicateTo, durabilityLevel, cas);


Dang, I missed durabilityLevel throughout when writing rejectInvalidTransactionalOptions. Could you please add? We don't support overriding the durability level on a per-operation basis inside a transaction.

programmatix · 2022-06-30T13:57:23Z

src/main/java/org/springframework/data/couchbase/core/ReactiveFindByIdOperationSupport.java

+										pArgs.getScope(), pArgs.getCollection(), null, null));
+					}
+				} else {
+					return ctxOpt.get().getCore().get(makeCollectionIdentifier(rc.async()), id)


Oops, there should be a rejectNonTransactionalOptions line here

programmatix · 2022-06-30T13:59:18Z

src/main/java/org/springframework/data/couchbase/core/ReactiveRemoveByIdOperation.java

+		/**
+		 * Remove the documents in the collection. Requires whole entity for transaction to have the cas.
+		 *
+		 * @param ids the document IDs.


javadoc and parameter name isn't correct

should be "entities"
#1493

In addition to transaction support, some inconsitencies in fluent APIs were remedied. Documentation to follow. Closes #1145.

yyfMichaelYan and others added 18 commits June 24, 2022 10:49

fix flaky test writesAndReadsCustomFieldsConvertedClass (#1264)

6ec1381

Prepare 4.3 GA (2021.1.0).

af66ada

See #1257

After release cleanups.

00e05e4

See #1257

Handle Collection<> parameters to repository query methods. (#1271)

ef22b3d

Closes #1270. Co-authored-by: mikereiche <[email protected]>

Incorporate changes from 5.0.x and bump Couchbase SDK. (#1287)

5dbf182

Closes #1286. Co-authored-by: mikereiche <[email protected]>

Prepare 4.4 M1 (2021.2.0).

ecadff6

See #1298

After release cleanups.

ebe33cf

See #1298

Prepare 4.4 M2 (2021.2.0).

da7d241

See #1301

After release cleanups.

1fdbf6d

See #1301

Add mechanism for save to do one of insert, replace or upsert. (#1316)

4a04419

Closes #1277.

Prepare 4.4 M3 (2021.2.0).

13d266e

See #1307

After release cleanups.

e83bd41

See #1307

Use Java 17 to build snapshots for Artifactory.

9af8ece

Closes #1352

Reinstate CouchbaseCache documentation. (#1357)

d5ca390

Closes #1356.

Fix update to cache documentation. (#1359)

5f03d1e

Closes #1358.

Rebase with main.

e20ed08

mikereiche force-pushed the programmatix-datacouch_1145_transaction_support branch 2 times, most recently from 895f988 to 0db6a4a Compare June 28, 2022 20:52

mikereiche commented Jun 28, 2022

View reviewed changes

mikereiche requested a review from daschl June 28, 2022 21:09

mikereiche force-pushed the programmatix-datacouch_1145_transaction_support branch 2 times, most recently from 83877e2 to bde2d7f Compare June 29, 2022 00:52

daschl suggested changes Jun 29, 2022

View reviewed changes

mikereiche force-pushed the programmatix-datacouch_1145_transaction_support branch 2 times, most recently from 69ccd3f to 7abc594 Compare June 29, 2022 23:06

programmatix suggested changes Jun 30, 2022

View reviewed changes

Rebased and tidied transaction branch.

e2df8e7

In addition to transaction support, some inconsitencies in fluent APIs were remedied. Documentation to follow. Closes #1145.

mikereiche force-pushed the programmatix-datacouch_1145_transaction_support branch from 7abc594 to e2df8e7 Compare June 30, 2022 19:08

mikereiche mentioned this pull request Jul 11, 2022

reject durability as a transaction option #1492

Closed

mikereiche closed this Jul 12, 2022

mikereiche deleted the programmatix-datacouch_1145_transaction_support branch July 28, 2022 22:21

jakov222 mentioned this pull request Jan 24, 2025

Transaction fails when entity has set @Durability #2027

Closed

		@@ -172,65 +177,95 @@ public Mono<T> first() {
		public Flux<T> all() {
		PseudoArgs<QueryOptions> pArgs = new PseudoArgs(template, scope, collection, options, domainType);

		<T> T save(T entity, String... scopeAndCollection);

		<T> Long count(Query query, Class<T> domainType);

Transaction Support #1487

Transaction Support #1487

Conversation

mikereiche commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment