DATACOUCH-963 - Scope and collection api. #1071

mikereiche · 2021-02-03T06:19:30Z

Draft of changes for scope and collection API to share with Michael Nitschinger.
Some of the mechanisms in this change-set have been ruled out (the proliferation
of all the method signatures, for instance).

Closes #963
Original pull request: #1071.

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

mp911de

All repository-related changes violate the core ideas of Spring Data repositories to keep these as pure as possible. Introducing another 50 method overloads clutters the API and pulls in a level of abstraction that duplicates the Template API.

Specifying store-specific options can be done on the Template API.

mp911de · 2021-02-03T08:28:27Z

src/main/java/org/springframework/data/couchbase/core/support/MapList.java

+import java.util.LinkedList;
+import java.util.List;
+
+public class MapList<K,V> extends HashMap<K, List<V>> {


There's no need for this one as Spring Framework ships with MultiValueMap.

mp911de · 2021-02-03T08:28:49Z

src/main/java/org/springframework/data/couchbase/core/support/ScopeName.java

@@ -0,0 +1,14 @@
+package org.springframework.data.couchbase.core.support;
+
+public class ScopeName {


Where possible, use immutable value objects.

mp911de · 2021-02-03T08:28:54Z

src/main/java/org/springframework/data/couchbase/core/support/CollectionName.java

+
+public class CollectionName {
+  String name;
+  public CollectionName(String name){


Where possible, use immutable value objects.

mp911de · 2021-02-03T08:31:50Z

src/main/java/org/springframework/data/couchbase/repository/CouchbaseRepository.java

+
+	@Override
+	Optional<T> findById(ID id);
+	Optional<T> findById(ID id, QueryOptions... options);


That is exactly what we want to avoid. The repository must not accept store-specific types as we do not want the calling code to interact with store specifics.

Store specifics at the Template API level are fine.

mp911de · 2021-02-03T08:32:39Z

src/main/java/org/springframework/data/couchbase/repository/CouchbaseRepository.java

+	List<T> findAll(CollectionName collectionName, ScopeName... scopeNames);
+
+
+	List<T> findAll(org.springframework.data.couchbase.core.query.Query query);


Introducing Query makes the Repository somewhat pointless as this method duplicates what's possible on the Template API level.

…name. Queries on field annotated properties have original name. They should use getFieldName() which will use the annotated if it exists.

Draft of changes for scope and collection API to share with Michael Nitschinger. Some of the mechanisms in this change-set have been ruled out (the proliferation of all the method signatures, for instance). Closes #963. Original pull request: #1071.

mikereiche · 2021-03-08T20:52:44Z

src/test/java/org/springframework/data/couchbase/domain/AirportRepository.java

 *
 * @author Michael Nitschinger
 * @author Michael Reiche
 */
 @Repository
-public interface AirportRepository extends PagingAndSortingRepository<Airport, String> {
+@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
+public interface AirportRepository extends CouchbaseRepository<Airport, String>, DynamicProxyable<AirportRepository> {



PagingAndSortingRepository is from spring-data-common
CouchbaseRepository extends PagingAndSortingRepository. AirportRepository needs to extend DynamicProxyable to have the AirportRepository type.

mikereiche · 2021-03-08T21:58:19Z

...in/java/org/springframework/data/couchbase/core/ExecutableRemoveByQueryOperationSupport.java

+		@Override
+		public RemoveByQueryInCollection<T> inScope(String scope) {
+			return null;
+		}


fixed to return new ExecutableRemoveByQuerySupport<>()

Draft of changes for scope and collection API to share with Michael Nitschinger. Some of the mechanisms in this change-set have been ruled out (the proliferation of all the method signatures, for instance). Closes #963. Original pull request: #1071.

daschl · 2021-03-09T14:17:17Z

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

@@ -74,7 +74,7 @@ private SimpleCouchbaseClientFactory(final Supplier<Cluster> cluster, final Stri

 	@Override
 	public CouchbaseClientFactory withScope(final String scopeName) {
-		return new SimpleCouchbaseClientFactory(cluster, bucket.name(), scopeName);
+		return new SimpleCouchbaseClientFactory(cluster, bucket.name(), scopeName != null ? scopeName : getScope().name());


why is that change needed? Wouldn't this return the exact same instance?

Edit: This is getting handled by the caller, so I've reverted this change.

If scopeName is null,
the old behavior would return a clientFactory with scope = _default
the new behavior returns a clientFactory with the same scope as the "this". (yes, it could actually return "this" instead of a new object - that is a better implementation).
This is to allow just using withScope(scopeName) without checking if scopeName is null.

daschl · 2021-03-09T14:17:50Z

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

+	/**
+	 * @param pseudoArgs - the metaArgs to set on the ThreadLocal field of the CouchbaseOperations
+	 */
+	void setThrdLocalArgs(PseudoArgs<?> pseudoArgs);


can we name this setThreadLocalArgs ? The two chars seem like a typo :)

Definitely.

daschl · 2021-03-10T14:02:16Z

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

@@ -59,7 +59,7 @@ ClusterType type() {

 	@Override
 	TestClusterConfig _start() throws Exception {
-		bucketname = UUID.randomUUID().toString();
+		bucketname = "my_bucket"; // UUID.randomUUID().toString();


why is this hardcoded now?

With the creation of multiple scopes and collections and the indexing, the initialization is getting long. This now reuses the bucket, scopes, collections etc. if they already exist.

daschl · 2021-03-10T14:02:38Z

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

-		if (postResponse.code() != 202) {
-			throw new Exception("Could not create bucket: " + postResponse + ", Reason: " + postResponse.body().string());
+		String reason = postResponse.body().string();
+		if (postResponse.code() != 202 && !(reason.contains("Bucket with given name already exists"))) {


this shouldn't exist with the uuid change above, right?

When reusing "my_bucket", 202(created) and "already exists" is acceptable, don't throw an exception.

daschl · 2021-03-10T14:05:25Z

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

@@ -107,6 +107,7 @@ public static void beforeAll() {
 		ApplicationContext ac = new AnnotationConfigApplicationContext(Config.class);
 		couchbaseTemplate = (CouchbaseTemplate) ac.getBean(COUCHBASE_TEMPLATE);
 		reactiveCouchbaseTemplate = (ReactiveCouchbaseTemplate) ac.getBean(REACTIVE_COUCHBASE_TEMPLATE);
+		System.err.println("JavaIntegrationTests.beforeAll() couchbaseTemplate: " + couchbaseTemplate);


should these be logged instead? because then you can turn it on off with the logger config, vs these will show up all the time

Yes. I'll look for other System.out and change to log.debug

daschl · 2021-03-10T14:26:45Z

src/main/java/org/springframework/data/couchbase/core/support/WithScope.java

+ * @author Michael Reiche
+ * @param <T> - the entity class
+ */
+public interface WithScope<T> {


the naming difference is odd - with vs. in

Yes - it should match - InScope. Changing these an all the FindByQueryWithScope etc. interfaces to FindByQueryInScope etc.

daschl · 2021-03-10T14:28:11Z

src/main/java/org/springframework/data/couchbase/core/support/PseudoArgs.java

+import org.springframework.data.couchbase.core.query.Query;
+
+public class PseudoArgs<OPTS> {
+	OPTS options;


these should be private final?

still outstanding?

fixed in local, needs push.

daschl · 2021-03-10T14:30:57Z

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

 		if (getParameters() != null) {
 			if (getParameters() instanceof JsonArray) {
 				options.parameters((JsonArray) getParameters());
 			} else {
 				options.parameters((JsonObject) getParameters());
 			}
 		}
+		if (scanConsistency == null) {
+			if (getScanConsistency() != null) {


can be combined with &&

still outstanding?

fixed locally. Need to push.

daschl · 2021-03-10T14:32:24Z

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

@@ -48,16 +52,18 @@

 	}

-	interface ExistsByIdWithCollection extends TerminatingExistsById, WithCollection {
+	interface ExistsByIdWithOptions<T> extends TerminatingExistsById, WithGetOptions<T> {
+		TerminatingExistsById withOptions(GetOptions options);


shouldn't that be ExistsOptions?

daschl · 2021-03-10T14:34:29Z

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

 	}

 	static class ExecutableExistsByIdSupport implements ExecutableExistsById {

 		private final CouchbaseTemplate template;
+		private final String scope;
+		private final String collection;
+		private final GetOptions options;


ExistsOptions?

Yes. Similar issue with RemoveByQuery - it had RemoveOptions instead of QueryOptions - and in both cases the options were not passed in execution. Going through all the Reactive*OperationSupport classes to check for usage of options.

I added "options" tests that pass an options with timeout of 10 nanoseconds, resulting in a timeout.

Draft of changes for scope and collection API to share with Michael Nitschinger. Some of the mechanisms in this change-set have been ruled out (the proliferation of all the method signatures, for instance). Closes #963. Original pull request: #1071.

daschl

found a couple things - also I noticed many of my questions did not have comments/answers, would be good to get clarifications on those as well (just look through the change they will pop up)

daschl · 2021-04-09T07:41:54Z

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

+/**
+ * Insert Operations
+ *
+ * @author Christoph Strobl


copy/paste?

No - I tried to fill in some of the missing javadoc. From looking at javadoc on other classes that would have been created at the same time, I assumed the same author.

daschl · 2021-04-09T07:43:40Z

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

-			Assert.hasText(collection, "Collection must not be null nor empty.");
-			return new ExecutableExistsByIdSupport(template, collection);
+		public ExistsByIdWithOptions inCollection(final String collection) {
+			return new ExecutableExistsByIdSupport(template, scope, collection, options);


was the assertion removed by accident? I wonder if it makes sense to validate those as well (also inScope)

At one point I removed the assertion to allow just calling without checking if the caller was using collections or not. That said - I put the assertion back in and all the tests pass - so it seems I'm not doing that anymore. I'll put it back for scope and collection.

daschl · 2021-04-09T07:44:21Z

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

+/**
+ * FindByAnalytics Operations
+ *
+ * @author Christoph Strobl


looks like some copy paste, you can put your name there if we want that at all

I was trying to "back-date" the javadoc with the correct author.

daschl · 2021-04-09T08:22:05Z

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

+/**
+ * Get Operations
+ *
+ * @author Christoph Strobl


same here, I'll stop pointing them out below

daschl · 2021-04-09T08:23:49Z

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

 		if (getParameters() != null) {
 			if (getParameters() instanceof JsonArray) {
 				options.parameters((JsonArray) getParameters());
 			} else {
 				options.parameters((JsonObject) getParameters());
 			}
 		}
+		if (scanConsistency == null) {
+			if (getScanConsistency() != null) {


still outstanding?

daschl · 2021-04-09T08:24:01Z

src/main/java/org/springframework/data/couchbase/core/support/PseudoArgs.java

+import org.springframework.data.couchbase.core.query.Query;
+
+public class PseudoArgs<OPTS> {
+	OPTS options;


still outstanding?

daschl · 2021-04-09T08:25:53Z

src/main/java/org/springframework/data/couchbase/core/support/PseudoArgs.java

+	/**
+	 * @@return the options from the ThreadLocal field of the template
+	 */
+	private OPTS getTlOptions(ReactiveCouchbaseTemplate template) {


can those be also renamed to getThreadLocalOptions so it's not as confusing?

fixed local, needs push.

daschl · 2021-04-09T08:27:19Z

...in/java/org/springframework/data/couchbase/repository/query/N1qlRepositoryQueryExecutor.java

@@ -47,7 +47,7 @@ public N1qlRepositoryQueryExecutor(final CouchbaseOperations operations, final C
 		this.queryMethod = queryMethod;
 		this.namedQueries = namedQueries;
 		this.evaluationContextProvider = evaluationContextProvider;
-		throw new RuntimeException("Deprecated");


daschl · 2021-04-09T08:27:48Z

...va/org/springframework/data/couchbase/repository/query/ReactiveCouchbaseRepositoryQuery.java

-		this.evaluationContextProvider = evaluationContextProvider;
-		throw new RuntimeException("Deprecated");
+		this.evaluationContextProvider = QueryMethodEvaluationContextProvider.DEFAULT;
+		//throw new RuntimeException("Deprecated");


daschl · 2021-04-09T08:28:02Z

...org/springframework/data/couchbase/repository/query/ReactiveN1qlRepositoryQueryExecutor.java

@@ -40,7 +40,7 @@ public ReactiveN1qlRepositoryQueryExecutor(final ReactiveCouchbaseOperations ope
 		this.queryMethod = queryMethod;
 		this.namedQueries = namedQueries;
 		this.evaluationContextProvider = evaluationContextProvider;
-		throw new RuntimeException("Deprecated");
+		//throw new RuntimeException("Deprecated");


Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

mikereiche · 2021-03-11T02:28:50Z

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

 	}

 	static class ExecutableFindByAnalyticsSupport<T> implements ExecutableFindByAnalytics<T> {

 		private final CouchbaseTemplate template;
-		private final Class<T> domainType;
+		private final Class<?> domainType;
+		private final Class<T> returnType;
 		private final ReactiveFindByAnalyticsSupport<T> reactiveSupport;


I'm not sure if returnType (for producing an object other than domainType) makes sense here, but it is in Query apis.

mikereiche · 2021-04-22T15:49:04Z

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

+/**
+ * Insert Operations
+ *
+ * @author Christoph Strobl


No - I tried to fill in some of the missing javadoc. From looking at javadoc on other classes that would have been created at the same time, I assumed the same author.

mikereiche · 2021-04-22T16:10:41Z

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

-			Assert.hasText(collection, "Collection must not be null nor empty.");
-			return new ExecutableExistsByIdSupport(template, collection);
+		public ExistsByIdWithOptions inCollection(final String collection) {
+			return new ExecutableExistsByIdSupport(template, scope, collection, options);


At one point I removed the assertion to allow just calling without checking if the caller was using collections or not. That said - I put the assertion back in and all the tests pass - so it seems I'm not doing that anymore. I'll put it back for scope and collection.

mikereiche · 2021-04-22T16:11:32Z

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

+/**
+ * FindByAnalytics Operations
+ *
+ * @author Christoph Strobl


I was trying to "back-date" the javadoc with the correct author.

mikereiche · 2021-04-22T16:13:33Z

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

+/**
+ * Get Operations
+ *
+ * @author Christoph Strobl


mikereiche · 2021-04-22T16:21:55Z

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

 		if (getParameters() != null) {
 			if (getParameters() instanceof JsonArray) {
 				options.parameters((JsonArray) getParameters());
 			} else {
 				options.parameters((JsonObject) getParameters());
 			}
 		}
+		if (scanConsistency == null) {
+			if (getScanConsistency() != null) {


fixed locally. Need to push.

mikereiche · 2021-04-22T19:41:35Z

src/main/java/org/springframework/data/couchbase/core/support/PseudoArgs.java

+import org.springframework.data.couchbase.core.query.Query;
+
+public class PseudoArgs<OPTS> {
+	OPTS options;


fixed in local, needs push.

mikereiche · 2021-04-22T19:43:48Z

src/main/java/org/springframework/data/couchbase/core/support/PseudoArgs.java

+	/**
+	 * @@return the options from the ThreadLocal field of the template
+	 */
+	private OPTS getTlOptions(ReactiveCouchbaseTemplate template) {


fixed local, needs push.

Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

Scope and Collection API for template. Closes #963. Original pull request: #1071.

mikereiche requested a review from daschl February 3, 2021 06:20

mikereiche marked this pull request as draft February 3, 2021 06:20

mp911de requested changes Feb 3, 2021

View reviewed changes

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch 2 times, most recently from 81b6bb6 to ce42488 Compare February 26, 2021 22:05

DATACOUCH-1055 - Queries on field annotated properties have original …

c303bdc

…name. Queries on field annotated properties have original name. They should use getFieldName() which will use the annotated if it exists.

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch 2 times, most recently from 9f6b2c7 to 5050cf0 Compare March 5, 2021 01:39

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from 5050cf0 to afe0bd2 Compare March 9, 2021 01:20

mikereiche commented Mar 9, 2021

View reviewed changes

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from afe0bd2 to e974a8c Compare March 9, 2021 23:50

daschl suggested changes Mar 10, 2021

View reviewed changes

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from e974a8c to d0da3a1 Compare March 11, 2021 02:20

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from d0da3a1 to d045e5e Compare March 11, 2021 02:41

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from d045e5e to 7a9d533 Compare March 11, 2021 03:09

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from 7a9d533 to 71af4c7 Compare March 22, 2021 15:56

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from 71af4c7 to cf60490 Compare March 29, 2021 21:56

mikereiche force-pushed the master branch 4 times, most recently from 9af0782 to 3f1cea1 Compare April 6, 2021 15:59

daschl suggested changes Apr 9, 2021

View reviewed changes

gregturn changed the base branch from master to main April 15, 2021 17:47

mikereiche added a commit that referenced this pull request Apr 23, 2021

DATACOUCH-963 - Scope and Collection API.

5c88c3c

Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from cf60490 to 5c88c3c Compare April 23, 2021 14:36

DATACOUCH-963 - Scope and Collection API.

d2b0b92

Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

mikereiche force-pushed the datacouch_963_scope_and_collection_api branch from 5c88c3c to d2b0b92 Compare April 23, 2021 14:37

mikereiche commented Apr 23, 2021

View reviewed changes

mikereiche added a commit that referenced this pull request Apr 26, 2021

DATACOUCH-963 - Scope and Collection API.

a11a3ff

Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

mikereiche mentioned this pull request Apr 26, 2021

Scope and collection API for template. #1133

Merged

5 tasks

mikereiche added a commit that referenced this pull request Apr 26, 2021

DATACOUCH-963 - Scope and Collection API.

bb8e690

Draft of changes for scope and collection API to share with Michael Nitschinger. Closes #963. Original pull request: #1071.

mikereiche added a commit that referenced this pull request Apr 26, 2021

Scope and Collection API for template.

46072dc

Scope and Collection API for template. Closes #963. Original pull request: #1071.

mikereiche closed this May 6, 2021

mikereiche added a commit that referenced this pull request May 18, 2021

Scope and Collection API for template.

0d1d779

Scope and Collection API for template. Closes #963. Original pull request: #1071.

mikereiche added a commit that referenced this pull request Jun 2, 2021

Scope and collection API for template. (#1133)

43889e6

Scope and Collection API for template. Closes #963. Original pull request: #1071.

mikereiche added a commit that referenced this pull request Aug 9, 2021

Scope and collection API for template. (#1133)

42b7fb3

Scope and Collection API for template. Closes #963. Original pull request: #1071.

mikereiche mentioned this pull request Aug 10, 2021

Add Debug logging of couchbase operations - query statements etc. #1045

Closed

mikereiche deleted the datacouch_963_scope_and_collection_api branch August 11, 2021 22:08

		@@ -0,0 +1,14 @@
		package org.springframework.data.couchbase.core.support;

		public class ScopeName {

		List<T> findAll(CollectionName collectionName, ScopeName... scopeNames);


		List<T> findAll(org.springframework.data.couchbase.core.query.Query query);

DATACOUCH-963 - Scope and collection api. #1071

DATACOUCH-963 - Scope and collection api. #1071

Conversation

mikereiche commented Feb 3, 2021 • edited Loading

mp911de 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

mikereiche Mar 10, 2021 • edited Loading

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

daschl 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

mikereiche commented Feb 3, 2021 •

edited

Loading

mikereiche Mar 10, 2021 •

edited

Loading