Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump couchbase sdk to 3_4_3. #1663

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Bump couchbase sdk to 3_4_3. #1663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
</parent>

<properties>
<couchbase>3.4.1</couchbase>
<couchbase.osgi>3.4.1</couchbase.osgi>
<couchbase>3.4.3</couchbase>
<couchbase.osgi>3.4.3</couchbase.osgi>
<springdata.commons>3.1.0-SNAPSHOT</springdata.commons>
<java-module-name>spring.data.couchbase</java-module-name>
<hibernate.validator>7.0.1.Final</hibernate.validator>
Expand Down
10 changes: 5 additions & 5 deletions ...n/java/org/springframework/data/couchbase/core/ReactiveRemoveByQueryOperationSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package org.springframework.data.couchbase.core;

import com.couchbase.client.core.api.query.CoreQueryContext;
import com.couchbase.client.core.io.CollectionIdentifier;
import reactor.core.publisher.Flux;

import java.util.Optional;
Expand All @@ -28,7 +30,6 @@
import org.springframework.data.couchbase.core.support.TemplateUtils;
import org.springframework.util.Assert;

import com.couchbase.client.core.deps.com.fasterxml.jackson.databind.node.ObjectNode;
import com.couchbase.client.java.ReactiveScope;
import com.couchbase.client.java.json.JsonObject;
import com.couchbase.client.java.query.QueryOptions;
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.flatMapIterable(result -> result.rows).map(row -> {
.queryBlocking(statement, queryContext, opts.builder().build(), false)
.flatMapIterable(result -> result.collectRows()).map(row -> {
JsonObject json = JsonObject.fromJson(row.data());
return new RemoveResult(json.getString(TemplateUtils.SELECT_ID), json.getLong(TemplateUtils.SELECT_CAS),
Optional.empty());
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/springframework/data/couchbase/core/query/N1QLQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ public QueryOptions getOptions() {
return options;
}

// for logging only
public JsonObject n1ql() {
JsonObject query = JsonObject.create().put("statement", expression.toString());
options.build().injectParams(query);
query.put("options", OptionsBuilder.getQueryOpts(options.build()));
return query;
}

Expand Down
61 changes: 40 additions & 21 deletions src/main/java/org/springframework/data/couchbase/core/query/OptionsBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.springframework.data.couchbase.core.query;

import static com.couchbase.client.core.util.Validators.notNull;
import static org.springframework.data.couchbase.core.query.Meta.MetaKey.RETRY_STRATEGY;
import static org.springframework.data.couchbase.core.query.Meta.MetaKey.SCAN_CONSISTENCY;
import static org.springframework.data.couchbase.core.query.Meta.MetaKey.TIMEOUT;
Expand All @@ -24,9 +25,13 @@
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

import com.couchbase.client.core.api.query.CoreQueryScanConsistency;
import com.couchbase.client.core.classic.query.ClassicCoreQueryOps;
import com.couchbase.client.core.error.InvalidArgumentException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.core.annotation.AnnotatedElementUtils;
Expand Down Expand Up @@ -67,21 +72,21 @@ public class OptionsBuilder {
static QueryOptions buildQueryOptions(Query query, QueryOptions options, QueryScanConsistency scanConsistency) {
options = options != null ? options : QueryOptions.queryOptions();
if (query.getParameters() != null) {
if (query.getParameters() instanceof JsonArray) {
if (query.getParameters() instanceof JsonArray && !((JsonArray) query.getParameters()).isEmpty()) {
options.parameters((JsonArray) query.getParameters());
} else {
} else if( query.getParameters() instanceof JsonObject && !((JsonObject)query.getParameters()).isEmpty()){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: formatting

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's sorcery!!

options.parameters((JsonObject) query.getParameters());
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Meta meta = query.getMeta() != null ? query.getMeta() : new Meta();
QueryOptions.Built optsBuilt = options.build();
JsonObject optsJson = getQueryOpts(optsBuilt);

QueryScanConsistency metaQueryScanConsistency = meta.get(SCAN_CONSISTENCY) != null
? ((ScanConsistency) meta.get(SCAN_CONSISTENCY)).query()
: null;
QueryScanConsistency qsc = fromFirst(QueryScanConsistency.NOT_BOUNDED, query.getScanConsistency(),
getScanConsistency(optsJson), scanConsistency, metaQueryScanConsistency);
scanConsistency(optsBuilt), scanConsistency, metaQueryScanConsistency);
Duration timeout = fromFirst(Duration.ofSeconds(0), getTimeout(optsBuilt), meta.get(TIMEOUT));
RetryStrategy retryStrategy = fromFirst(null, getRetryStrategy(optsBuilt), meta.get(RETRY_STRATEGY));

Expand All @@ -100,6 +105,21 @@ static QueryOptions buildQueryOptions(Query query, QueryOptions options, QuerySc
return options;
}

private static QueryScanConsistency scanConsistency(QueryOptions.Built optsBuilt){
CoreQueryScanConsistency scanConsistency = optsBuilt.scanConsistency();
if (scanConsistency == null){
return null;
}
switch (scanConsistency) {
case NOT_BOUNDED:
return QueryScanConsistency.NOT_BOUNDED;
case REQUEST_PLUS:
return QueryScanConsistency.REQUEST_PLUS;
default:
throw new InvalidArgumentException("Unknown scan consistency type " + scanConsistency, null, null);
}
}

public static TransactionQueryOptions buildTransactionQueryOptions(QueryOptions options) {
QueryOptions.Built built = options.build();
TransactionQueryOptions txOptions = TransactionQueryOptions.queryOptions();
Expand All @@ -110,8 +130,21 @@ public static TransactionQueryOptions buildTransactionQueryOptions(QueryOptions
throw new IllegalArgumentException("QueryOptions.flexIndex is not supported in a transaction");
}

Object value = optsJson.get("args");
if(value instanceof JsonObject){
txOptions.parameters((JsonObject)value);
}else if(value instanceof JsonArray) {
txOptions.parameters((JsonArray) value);
} else if(value != null) {
throw InvalidArgumentException.fromMessage(
"non-null args property was neither JsonObject(namedParameters) nor JsonArray(positionalParameters) "
+ value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else throw if not null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.


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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

if (LOG.isDebugEnabled()) {
Expand Down Expand Up @@ -370,10 +403,8 @@ static String toString(MutateInOptions o) {
return s.toString();
}

private static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {
JsonObject jo = JsonObject.create();
optsBuilt.injectParams(jo);
return jo;
public static JsonObject getQueryOpts(QueryOptions.Built optsBuilt) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return JsonObject.fromJson(ClassicCoreQueryOps.convertOptions(optsBuilt).toString().getBytes());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBytes(StandardCharsets.UTF_8)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand All @@ -396,18 +427,6 @@ public static <T> T fromFirst(T deflt, Object... choice) {
return chosen;
}

private static QueryScanConsistency getScanConsistency(JsonObject opts) {
String str = opts.getString("scan_consistency");
if ("at_plus".equals(str)) {
return null;
}
return str == null ? null : QueryScanConsistency.valueOf(str.toUpperCase());
}

private static JsonObject getScanVectors(JsonObject opts) {
return opts.getObject("scan_vectors");
}

private static Duration getTimeout(QueryOptions.Built optsBuilt) {
Optional<Duration> timeout = optsBuilt.timeout();
return timeout.isPresent() ? timeout.get() : null;
Expand Down
17 changes: 12 additions & 5 deletions ...va/org/springframework/data/couchbase/core/CouchbaseTemplateKeyValueIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
import com.couchbase.client.java.query.QueryScanConsistency;

/**
* KV tests Theses tests rely on a cb server running.
* KV test - these tests rely on a cb server running.
*
* @author Michael Nitschinger
* @author Michael Reiche
Expand All @@ -81,11 +81,11 @@ public void beforeEach() {
@Test
void findByIdWithLock() {
try {
User user = new User(UUID.randomUUID().toString(), "user1", "user1");
User user = new User("1", "user1", "user1");

couchbaseTemplate.upsertById(User.class).one(user);

User foundUser = couchbaseTemplate.findById(User.class).withLock(Duration.ofSeconds(2)).one(user.getId());
User foundUser = couchbaseTemplate.findById(User.class).withLock(Duration.ofSeconds(5)).one(user.getId());
user.setVersion(foundUser.getVersion());// version will have changed
assertEquals(user, foundUser);

Expand All @@ -94,8 +94,15 @@ void findByIdWithLock() {
);
assertTrue(exception.retryReasons().contains(RetryReason.KV_LOCKED), "should have been locked");
} finally {
sleepSecs(2);
couchbaseTemplate.removeByQuery(User.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).all();
for(int i=0; i< 10; i++) {
sleepSecs(2);
try {
couchbaseTemplate.removeByQuery(User.class).withConsistency(QueryScanConsistency.REQUEST_PLUS).all();
break;
} catch (Exception e) {
e.printStackTrace(); // gives IndexFailureException if the lock is still active
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}

}
Expand Down
34 changes: 18 additions & 16 deletions src/test/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@
import java.util.List;
import java.util.Locale;

import com.couchbase.client.core.deps.com.fasterxml.jackson.databind.node.ArrayNode;
import com.couchbase.client.java.query.QueryOptions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.data.couchbase.core.convert.CouchbaseConverter;
import org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter;
import org.springframework.data.couchbase.core.mapping.CouchbaseMappingContext;
import org.springframework.data.couchbase.core.mapping.CouchbasePersistentEntity;
import org.springframework.data.couchbase.core.mapping.CouchbasePersistentProperty;
import org.springframework.data.couchbase.core.query.OptionsBuilder;
import org.springframework.data.couchbase.core.query.Query;
import org.springframework.data.couchbase.domain.Person;
import org.springframework.data.couchbase.domain.PersonRepository;
Expand Down Expand Up @@ -119,18 +122,17 @@ void queryParametersArray() throws Exception {
QueryMethod queryMethod = new QueryMethod(method, new DefaultRepositoryMetadata(UserRepository.class),
new SpelAwareProxyProjectionFactory());
Query expected = (new Query()).addCriteria(where(i("firstname")).in("Oliver", "Charles"));
JsonArray parameters = JsonArray.create().add(JsonArray.create().add("Oliver").add("Charles"));
QueryOptions expectedQOptions = QueryOptions.queryOptions().parameters(parameters);
N1qlQueryCreator creator = new N1qlQueryCreator(tree,
getAccessor(getParameters(method), new Object[] { new Object[] { "Oliver", "Charles" } }), queryMethod,
converter, bucketName);
Query query = creator.createQuery();

// Query expected = (new Query()).addCriteria(where("firstname").in("Oliver", "Charles"));
assertEquals(" WHERE `firstname` in $1", query.export(new int[1]));
JsonObject expectedOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(expectedOptions);
JsonObject actualOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(actualOptions);
assertEquals(expectedOptions.removeKey("client_context_id"), actualOptions.removeKey("client_context_id"));
ArrayNode expectedOptions = expected.buildQueryOptions(expectedQOptions, null).build().positionalParameters();
ArrayNode actualOptions = query.buildQueryOptions(null, null).build().positionalParameters();
assertEquals(expectedOptions.toString(), actualOptions.toString());
}

@Test
Expand All @@ -148,12 +150,12 @@ void queryParametersJsonArray() throws Exception {
Query query = creator.createQuery();

Query expected = (new Query()).addCriteria(where(i("firstname")).in("Oliver", "Charles"));
JsonArray parameters = JsonArray.create().add(JsonArray.create().add("Oliver").add("Charles"));
QueryOptions expectedQOptions = QueryOptions.queryOptions().parameters(parameters);
assertEquals(" WHERE `firstname` in $1", query.export(new int[1]));
JsonObject expectedOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(expectedOptions);
JsonObject actualOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(actualOptions);
assertEquals(expectedOptions.removeKey("client_context_id"), actualOptions.removeKey("client_context_id"));
ArrayNode expectedOptions = expected.buildQueryOptions(expectedQOptions, null).build().positionalParameters();
ArrayNode actualOptions = query.buildQueryOptions(null, null).build().positionalParameters();
assertEquals(expectedOptions.toString(), actualOptions.toString());
}

@Test
Expand All @@ -171,13 +173,13 @@ void queryParametersList() throws Exception {
Query query = creator.createQuery();

Query expected = (new Query()).addCriteria(where(i("firstname")).in("Oliver", "Charles"));
JsonArray parameters = JsonArray.create().add(JsonArray.create().add("Oliver").add("Charles"));
QueryOptions expectedQOptions = QueryOptions.queryOptions().parameters(parameters);

assertEquals(" WHERE `firstname` in $1", query.export(new int[1]));
JsonObject expectedOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(expectedOptions);
JsonObject actualOptions = JsonObject.create();
expected.buildQueryOptions(null, null).build().injectParams(actualOptions);
assertEquals(expectedOptions.removeKey("client_context_id"), actualOptions.removeKey("client_context_id"));
ArrayNode expectedOptions = expected.buildQueryOptions(expectedQOptions, null).build().positionalParameters();
ArrayNode actualOptions = query.buildQueryOptions(null, null).build().positionalParameters();
assertEquals(expectedOptions.toString(), actualOptions.toString());
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/springframework/data/couchbase/util/JavaIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public static CompletableFuture<Void> createPrimaryIndex(Cluster cluster, String
options.timeout(Duration.ofSeconds(300));
options.ignoreIfExists(true);
final CreatePrimaryQueryIndexOptions.Built builtOpts = options.build();
final String indexName = builtOpts.indexName().orElse(null);
final String indexName = builtOpts.indexName();

String keyspace = "default:`" + bucketName + "`.`" + scopeName + "`.`" + collectionName + "`";
String statement = "CREATE PRIMARY INDEX ";
Expand Down