Bump couchbase sdk to 3_4_3. #1663

mikereiche · 2023-02-14T01:50:17Z

Requires some refactoring around @Stability.Internal APIs. Also fixed a test to get it to pass.

mikereiche

dnault · 2023-02-14T21:59:14Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		JsonObject jo = JsonObject.create();
-		optsBuilt.injectParams(jo);
-		return jo;
+	private static ClassicCoreQueryOps kvOps = new ClassicCoreQueryOps(Core.create(CoreEnvironment.builder().build(),


final?
kvOps -> queryOps ?

fixed - it's a static methods not needed.

dnault · 2023-02-14T23:08:39Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 				options.parameters((JsonArray) query.getParameters());
-			} else {
+			} else if( query.getParameters() instanceof JsonObject && !((JsonObject)query.getParameters()).isEmpty()){


NIT: formatting

IntelliJ tip: Command-Option-Shift-L, then select "Only changes uncommitted to VCS"

That's sorcery!!

dnault · 2023-02-14T23:12:52Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		optsBuilt.injectParams(jo);
-		return jo;
+  public static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {
+    return JsonObject.fromJson(ClassicCoreQueryOps.convertOptions(optsBuilt).toString().getBytes());


getBytes(StandardCharsets.UTF_8)

I've removed getQueryOpts() method completely as it is only needed in one place. (to transform GetOptions into TransactionGetOptions)

dnault · 2023-02-14T23:15:47Z

src/test/java/org/springframework/data/couchbase/util/UnmanagedTestCluster.java

@@ -93,7 +94,7 @@ TestClusterConfig _start() throws Exception {
 		// no means to create a bucket on Capella
 		// have not created config() yet.
 		boolean usingCloud = seedHost.endsWith("cloud.couchbase.com");
-		bucketname = usingCloud ? "my_bucket" : UUID.randomUUID().toString();
+		bucketname = "my_bucket"; // usingCloud ? "my_bucket" : UUID.randomUUID().toString();


NIT: remove commented-out code?

Oops. This shouldn't be in the change.

dnault · 2023-02-14T23:19:54Z

.../java/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java

+					couchbaseTemplate.removeByQuery(User.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).all();
+					break;
+				} catch (Exception e) {
+					e.printStackTrace(); // gives IndexFailureException if the lock is still active


Does this hide other exceptions that aren't expected? (Sure, it logs the stack trace, but nobody's going to look at it if the test "passes".)

Does this hide other exceptions that aren't expected?

It prevents an exception in the finally() from showing instead of the actual test failure showing.

dnault · 2023-02-14T23:27:08Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 import java.util.Map;
 import java.util.Optional;

+import com.couchbase.client.core.Core;


Unused import? (not just a NIT, because Core is potentially on the chopping block)

removed. Along with the other related, unused imports.

dnault · 2023-02-14T23:28:25Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

+			txOptions.parameters((JsonObject)value);
+		}else if(value instanceof JsonArray) {
+			txOptions.parameters((JsonArray) value);
+		}


else throw if not null?

programmatix · 2023-02-16T11:58:07Z

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

@@ -100,11 +101,10 @@ public Flux<RemoveResult> all() {
 				} else {
 					TransactionQueryOptions opts = OptionsBuilder
 							.buildTransactionQueryOptions(buildQueryOptions(pArgs.getOptions()));
-					ObjectNode convertedOptions = com.couchbase.client.java.transactions.internal.OptionsUtil
-							.createTransactionOptions(pArgs.getScope() == null ? null : rs, statement, opts);
+					CoreQueryContext queryContext = CollectionIdentifier.DEFAULT_SCOPE.equals(rs.name()) ? null : CoreQueryContext.of(rs.bucketName(), rs.name());


It's always necessary to send a CoreQueryContext to satisfy The Platform That Shall Not Be Publically Named. This is what ultimately causes a query_context to be sent.

Noted. I opened a separate ticket for this #1671.

programmatix · 2023-02-16T12:00:13Z

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

 					return transactionContext.get().getCore()
-							.queryBlocking(statement, template.getBucketName(), pArgs.getScope(), convertedOptions, false)


I forgot that Spring was using these internals directly. Let's get these methods annotated with the new annotation so breakage like this doesn't happen again.

programmatix · 2023-02-16T12:06:26Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 		for (Map.Entry<String, Object> entry : optsJson.toMap().entrySet()) {
-			txOptions.raw(entry.getKey(), entry.getValue());
+			if(!entry.getKey().equals("args")) {
+				txOptions.raw(entry.getKey(), entry.getValue());


Continue to not feel good about this approach to clone a QueryOptions into a TransactionQueryOptions... It won't handle things like the JsonSerializer, for example.

programmatix · 2023-02-16T12:14:21Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		JsonObject jo = JsonObject.create();
-		optsBuilt.injectParams(jo);
-		return jo;
+  public static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {


It doesn't feel great turning QueryOptions into JSON, and it also calls into the internal method ClassicCoreQueryOps.convertOptions, Luckily, I don't think this getQueryOpts is necessary anymore. It's called from two places:

buildQueryOptions. Which only uses it for getScanConsistency. But that's not needed anymore. QueryOptions.Built now implements CoreQueryOptions so you can call scanConsistency() directly on it.

buildTransactionQueryOptions. As mentioned above I already feel a bit queasy about this solution of converting a QueryOptions into JSON and then using that to set raw parameters on a TransactionQueryOptions. And now with CoreQueryOptions you don't need it - you can copy over the parameters one by one (now including the serializer).

I removed the getQueryOptions() method as it is only needed in buildTransactionQueryOptions.
I opened a new ticket #1672 for better handling of TransactionGetOptions.

Requires some refactoring around @Stability.Internal APIs. Also fixed a test to get it to pass. Closes #1661,#1662.

mikereiche · 2023-02-14T22:57:20Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 				options.parameters((JsonArray) query.getParameters());
-			} else {
+			} else if( query.getParameters() instanceof JsonObject && !((JsonObject)query.getParameters()).isEmpty()){
 				options.parameters((JsonObject) query.getParameters());
 			}


This was previously broken - it was overwriting options.parameters() with empty query.parameters()

mikereiche · 2023-02-16T19:20:06Z

src/test/java/org/springframework/data/couchbase/util/UnmanagedTestCluster.java

@@ -93,7 +94,7 @@ TestClusterConfig _start() throws Exception {
 		// no means to create a bucket on Capella
 		// have not created config() yet.
 		boolean usingCloud = seedHost.endsWith("cloud.couchbase.com");
-		bucketname = usingCloud ? "my_bucket" : UUID.randomUUID().toString();
+		bucketname = "my_bucket"; // usingCloud ? "my_bucket" : UUID.randomUUID().toString();


Oops. This shouldn't be in the change.

mikereiche · 2023-02-17T18:38:42Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		JsonObject jo = JsonObject.create();
-		optsBuilt.injectParams(jo);
-		return jo;
+	private static ClassicCoreQueryOps kvOps = new ClassicCoreQueryOps(Core.create(CoreEnvironment.builder().build(),


fixed - it's a static methods not needed.

mikereiche · 2023-02-17T18:39:38Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		optsBuilt.injectParams(jo);
-		return jo;
+  public static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {
+    return JsonObject.fromJson(ClassicCoreQueryOps.convertOptions(optsBuilt).toString().getBytes());


I've removed getQueryOpts() method completely as it is only needed in one place. (to transform GetOptions into TransactionGetOptions)

mikereiche · 2023-02-17T18:41:40Z

.../java/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java

+					couchbaseTemplate.removeByQuery(User.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).all();
+					break;
+				} catch (Exception e) {
+					e.printStackTrace(); // gives IndexFailureException if the lock is still active


Does this hide other exceptions that aren't expected?

It prevents an exception in the finally() from showing instead of the actual test failure showing.

mikereiche · 2023-02-17T18:43:07Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 import java.util.Map;
 import java.util.Optional;

+import com.couchbase.client.core.Core;


removed. Along with the other related, unused imports.

mikereiche · 2023-02-17T18:47:14Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

+			txOptions.parameters((JsonObject)value);
+		}else if(value instanceof JsonArray) {
+			txOptions.parameters((JsonArray) value);
+		}


mikereiche · 2023-02-17T18:48:08Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

 				options.parameters((JsonArray) query.getParameters());
-			} else {
+			} else if( query.getParameters() instanceof JsonObject && !((JsonObject)query.getParameters()).isEmpty()){


That's sorcery!!

mikereiche · 2023-02-17T18:52:06Z

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

@@ -100,11 +101,10 @@ public Flux<RemoveResult> all() {
 				} else {
 					TransactionQueryOptions opts = OptionsBuilder
 							.buildTransactionQueryOptions(buildQueryOptions(pArgs.getOptions()));
-					ObjectNode convertedOptions = com.couchbase.client.java.transactions.internal.OptionsUtil
-							.createTransactionOptions(pArgs.getScope() == null ? null : rs, statement, opts);
+					CoreQueryContext queryContext = CollectionIdentifier.DEFAULT_SCOPE.equals(rs.name()) ? null : CoreQueryContext.of(rs.bucketName(), rs.name());


Noted. I opened a separate ticket for this #1671.

mikereiche · 2023-02-17T18:54:54Z

src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java

-		JsonObject jo = JsonObject.create();
-		optsBuilt.injectParams(jo);
-		return jo;
+  public static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {


I removed the getQueryOptions() method as it is only needed in buildTransactionQueryOptions.
I opened a new ticket #1672 for better handling of TransactionGetOptions.

Requires some refactoring around @Stability.Internal APIs. Also fixed a test to get it to pass. Closes #1661,#1662.

mikereiche commented Feb 14, 2023

View reviewed changes

mikereiche force-pushed the datacouch_1661_bump_couchbase_sdk_to_3_4_3 branch 2 times, most recently from 78f8f29 to 78feaaf Compare February 14, 2023 23:04

dnault suggested changes Feb 14, 2023

View reviewed changes

programmatix suggested changes Feb 16, 2023

View reviewed changes

mikereiche force-pushed the datacouch_1661_bump_couchbase_sdk_to_3_4_3 branch from 78feaaf to 05bd415 Compare February 17, 2023 18:56

Bump couchbase sdk to 3_4_3.

4b527eb

Requires some refactoring around @Stability.Internal APIs. Also fixed a test to get it to pass. Closes #1661,#1662.

mikereiche force-pushed the datacouch_1661_bump_couchbase_sdk_to_3_4_3 branch from 05bd415 to 4b527eb Compare February 17, 2023 19:42

mikereiche commented Feb 17, 2023

View reviewed changes

dnault approved these changes Feb 17, 2023

View reviewed changes

mikereiche merged commit 02afaee into main Feb 17, 2023

mikereiche deleted the datacouch_1661_bump_couchbase_sdk_to_3_4_3 branch February 27, 2023 18:06

mikereiche mentioned this pull request Feb 28, 2023

ReactiveCouchbaseTemplateKeyValueIntegrationTests.findByIdWithLock needs longer duration #1662

Closed

mikereiche added a commit that referenced this pull request Feb 28, 2023

Bump couchbase sdk to 3_4_3. (#1663)

6924392

Requires some refactoring around @Stability.Internal APIs. Also fixed a test to get it to pass. Closes #1661,#1662.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump couchbase sdk to 3_4_3. #1663

Bump couchbase sdk to 3_4_3. #1663

mikereiche commented Feb 14, 2023

mikereiche left a comment

dnault Feb 14, 2023

mikereiche Feb 17, 2023

dnault Feb 14, 2023

mikereiche Feb 17, 2023

dnault Feb 14, 2023

mikereiche Feb 17, 2023

dnault Feb 14, 2023

mikereiche Feb 16, 2023

dnault Feb 14, 2023

mikereiche Feb 17, 2023

dnault Feb 14, 2023

mikereiche Feb 17, 2023

dnault Feb 14, 2023

mikereiche Feb 17, 2023

programmatix Feb 16, 2023

mikereiche Feb 17, 2023

programmatix Feb 16, 2023

programmatix Feb 16, 2023

programmatix Feb 16, 2023

mikereiche Feb 17, 2023

mikereiche Feb 14, 2023

mikereiche Feb 16, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

mikereiche Feb 17, 2023

		return transactionContext.get().getCore()
		.queryBlocking(statement, template.getBucketName(), pArgs.getScope(), convertedOptions, false)

Bump couchbase sdk to 3_4_3. #1663

Bump couchbase sdk to 3_4_3. #1663

Conversation

mikereiche commented Feb 14, 2023

mikereiche left a 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