DATACOUCH-588 - Refactoring part 1 of n. #278

mikereiche · 2020-11-12T06:50:20Z

This is part 1 of a multi-part refactoring effort.

The files below are the non-test files involved. The classes marked new and old are exactly that, and the files marked 'replace' are replacements for similar classes. To understand the changes, start with the resolveQuery() of the two repository factory classes, the *PartTeeCouchbaseQuery and StringBasedCouchbaseQuery that they created, then their execute() method ( AbstractCouchbaseQueryBase.execute() ) which calls the AbstractCouchbaseQuery.doExecute(), which creates a *CouchbaseQueryExecution and executes it.

org/springframework/data/couchbase/repository/query/AbstractCouchbaseQuery.java new
org/springframework/data/couchbase/repository/query/AbstractCouchbaseQueryBase.java new
org/springframework/data/couchbase/repository/query/AbstractReactiveCouchbaseQuery.java new
org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java new
org/springframework/data/couchbase/repository/query/CouchbaseQueryMethod.java old
org/springframework/data/couchbase/repository/query/DtoInstantiatingConverter.java new
org/springframework/data/couchbase/repository/query/PartTreeCouchbaseQuery.java replace
org/springframework/data/couchbase/repository/query/ReactiveCouchbaseQueryExecution.java new
org/springframework/data/couchbase/repository/query/ReactiveCouchbaseQueryMethod.java new
org/springframework/data/couchbase/repository/query/ReactivePartTreeCouchbaseQuery.java replace
org/springframework/data/couchbase/repository/query/ReactiveStringBasedCouchbaseQuery.java replace
org/springframework/data/couchbase/repository/query/ResultProcessingConverter.java new
org/springframework/data/couchbase/repository/query/StringBasedCouchbaseQuery.java replace
org/springframework/data/couchbase/repository/query/StringN1qlQueryCreator.java old
org/springframework/data/couchbase/repository/support/CouchbaseRepositoryFactory.java old
org/springframework/data/couchbase/repository/support/ReactiveCouchbaseRepositoryFactory.java old

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

daschl

Couple remarks. Also note that there are many "not yet implement" comments in the code - I'm okay with these as long as they are not degrading functionality we had before. If so, we should probably work on this in a feature branch until we are back to feature parity with the stable code, so we don't break folks.

daschl · 2020-12-03T09:00:49Z

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

@@ -39,6 +42,7 @@
 	Iterable<Airport> findAll();

 	@Override
+	@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
 	Airport save(Airport airport);


isn't that a KV method which does not need the scan consistency?

daschl · 2020-12-03T09:31:44Z

.../org/springframework/data/couchbase/repository/CouchbaseRepositoryQueryIntegrationTests.java

 			List<Airport> airports = airportRepository.findAllByIata("vie");
-			assertEquals(vie.getId(), airports.get(0).getId());
+			assertEquals(1, airports.size());
+			System.out.println("findAllByIata(0): " + airports.get(0));


are these printlns leftovers from debugging in the tests?

daschl · 2020-12-03T09:35:40Z

...ingframework/data/couchbase/repository/ReactiveCouchbaseRepositoryQueryIntegrationTests.java

+			vie = airportRepository.save(vie).block();
+			List<Airport> airports = airportRepository.findAllByIata("vie").collectList().block();
+			assertEquals(1, airports.size());
+			System.out.println("findAllByIata(0): " + airports.get(0));


looks like some debugging leftovers - I'd recommend to remove all those prints from tests since they make them very noisy in logfiles

daschl · 2020-12-03T09:36:15Z

...ingframework/data/couchbase/repository/ReactiveCouchbaseRepositoryQueryIntegrationTests.java

+				PageRequest pageable = PageRequest.of(page++, 2);
+				System.out.println("findAllByIataLike: " + "%S");
+				// java.lang.IndexOutOfBoundsException: Source emitted more than one item
+				// Flux<Airport> airportFlux = airportRepository.findAllByIataLike("S%");


can this be removed or does this needs to be handled?

I changed this to actually use the paging. Removed the commented-out lines and added assertions.

daschl · 2020-12-03T09:36:37Z

...ingframework/data/couchbase/repository/ReactiveCouchbaseRepositoryQueryIntegrationTests.java

@@ -124,6 +143,23 @@ void count() {
 				airportRepository.save(airport).block();
 			}

+			int page = 0;
+			for (; page < 10;) {


a while(page < 10) is probably better here

I've flattened out the loop to better verify the results.

daschl · 2020-12-03T09:53:21Z

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

+ * @author Michael Reiche
+ * @since 4.1 
+ */
+public class ReactivePartTreeCouchbaseQuery extends AbstractReactiveCouchbaseQuery {


Same comment as with the string equivalent, is there a common base class for reactive and non-reactive that encapsulates the commonalities maybe?

It seems there should be a way to make this tidier, but it eludes me.

daschl · 2020-12-03T09:54:45Z

src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryMethod.java

+	 * @return is this a 'delete'?
+	 */
+	public boolean isDeleteQuery() {
+		return getName().toLowerCase().startsWith("delete");


should we use Locale.ROOT in the toLowerCase?

done on exists, delete and count. Mark suggested an annotation. I will (eventually) see what mongo does.

daschl · 2020-12-03T09:54:49Z

src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryMethod.java

+	 * @return is this an 'exists' query?
+	 */
+	public boolean isExistsQuery() {
+		return getName().toLowerCase().startsWith("exists");


should we use Locale.ROOT in the toLowerCase?

daschl · 2020-12-03T09:56:26Z

...ava/org/springframework/data/couchbase/repository/query/ReactiveCouchbaseQueryExecution.java

+		 */
+		@Override
+		public Object execute(Query query, Class<?> type, String collection) {
+			return operations.removeByQuery(type)/*.inCollection(collection)*/.matching(query).all();


does the collection string needs to be handled?

The do eventually. The support of an inCollection(collection) was one of the things I backed out to keep the (initial) changes to a minium.

daschl · 2020-12-03T09:56:58Z

src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java

+		 */
+		@Override
+		public Object execute(Query query, Class<?> type, String collection) {
+			return operations.removeByQuery(type).matching(query).all();


the collection string here is not handled at all - should we fail explicitly if it is present or even better handle it?

As above - we are going to handle it, but I wanted to keep that out of the initial branch.

daschl · 2020-12-11T06:54:15Z

pom.xml

+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <configuration>
+                    <source>9</source>


We are targeting java 8, not 9 right?

Also, I would think this is part of the parent pom?

I vaguely recall adding this when prompted by intellij to facilate some .of(...) call. Will check

removed. It was for Set.of(...) in a test.

daschl · 2020-12-11T06:55:38Z

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

+   * @author Christoph Strobl
+   * @since 1.10
+   */
+  public enum CursorOption {


we do not have cursors, does this apply at all to the impl?

daschl · 2020-12-11T06:56:30Z

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

+   * @return {@literal 0 (zero)} by default.
+   * @since 2.1
+   */
+  int cursorBatchSize() default 0;


there are many references to cursors, comments etc which are all very mongo-ish but do not apply to couchbase

I removed cursor, comment etc. I left maxExecutionTimeMs just so Meta isn't empty.

daschl · 2020-12-11T06:59:03Z

src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryMethod.java

+		}
+
+		if (meta.cursorBatchSize() != 0) {
+			metaAttributes.setCursorBatchSize(meta.cursorBatchSize());


does not apply? (see other comments)

daschl · 2020-12-17T08:36:04Z

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

+public class Meta {
+
+  private enum MetaKey {
+    MAX_TIME_MS("$maxTimeMS"), MAX_SCAN("$maxScan"), COMMENT("$comment"), SNAPSHOT("$snapshot");


this stuff is mongo related, right? why do we keep this?

I missed this when I cleaned out the Meta annotation. I've removed all the mongo-specific elements, I've left just an EXAMPLE in the enum because it can't be empty. I've also remove the mongo maxTimeMs from the Meta annotation.

This implements pageable for non-reactive queries and realigns reactive queries with other spring-data projects to facilitate the implementaion of pageable (done) and other types of queries such as projection and distinct (not yet done)

* DATACOUCH-588 - Implement pageable and realign repo query This implements pageable for non-reactive queries and realigns reactive queries with other spring-data projects to facilitate the implementaion of pageable (done) and other types of queries such as projection and distinct (not yet done) * DATACOUCH-588 - Implement pageable and realign repo query This implements pageable for non-reactive queries and realigns reactive queries with other spring-data projects to facilitate the implementaion of pageable (done) and other types of queries such as projection and distinct (not yet done) * DATACOUCH-588 - Refactoring part 1 of n. Co-authored-by: mikereiche <[email protected]>

mikereiche requested a review from daschl November 12, 2020 06:50

daschl suggested changes Dec 3, 2020

View reviewed changes

mikereiche force-pushed the datacouch_refactoring_part_1_of_n branch from e6ee57b to 0a72b4c Compare December 8, 2020 02:41

daschl suggested changes Dec 11, 2020

View reviewed changes

mikereiche force-pushed the datacouch_refactoring_part_1_of_n branch from 0a72b4c to dfd7202 Compare December 16, 2020 17:13

daschl suggested changes Dec 17, 2020

View reviewed changes

mikereiche force-pushed the datacouch_refactoring_part_1_of_n branch from dfd7202 to b208391 Compare December 17, 2020 19:00

daschl approved these changes Dec 18, 2020

View reviewed changes

mikereiche added 2 commits December 18, 2020 15:52

mikereiche force-pushed the datacouch_refactoring_part_1_of_n branch 3 times, most recently from e1acbbb to fce318c Compare December 21, 2020 21:45

DATACOUCH-588 - Refactoring part 1 of n.

d9774a3

mikereiche force-pushed the datacouch_refactoring_part_1_of_n branch from fce318c to d9774a3 Compare December 21, 2020 21:47

mikereiche merged commit 3364e0f into master Dec 21, 2020

This was linked to issues Jan 14, 2021

QueryMethod.isCollectionQuery() returns false for Flux<T> findByXXX(..., pageRequest) [DATACOUCH-673] #981

Closed

expiryExpression ignored, but expiry works [DATACOUCH-645] #956

Closed

mikereiche deleted the datacouch_refactoring_part_1_of_n branch March 15, 2021 19:17

DATACOUCH-588 - Refactoring part 1 of n. #278

DATACOUCH-588 - Refactoring part 1 of n. #278

Uh oh!

Conversation

mikereiche commented Nov 12, 2020

Uh oh!

daschl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!