DATACOUCH-588 - Implement pageable and realign repo query #249
Conversation
src/main/java/org/springframework/data/couchbase/core/ReactiveFindByQueryOperationSupport.java
Show resolved
Hide resolved
| @@ -127,6 +137,27 @@ public Query with(final Pageable pageable) { | |||
| return with(pageable.getSort()); | |||
| } | |||
|
|
|||
| /** | |||
| * limit | |||
| * @return is this a 'delete'? | ||
| */ | ||
| public boolean isDeleteQuery() { | ||
| return getName().toLowerCase().startsWith("delete"); |
There was a problem hiding this comment.
Is spring-core guaranteed to be on the class path? Maybe you can use StringUtils.startsWithIgnoreCase(getName(), "delete");
src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryMethod.java
Outdated
Show resolved
Hide resolved
| query.limit(tree.getMaxResults()); | ||
| } | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug(String.format("Created query %s for * fields.", query.export() /*, query.getFieldsObject()*/)); |
There was a problem hiding this comment.
Using String.format to build a log message is often a code smell. String.format is a very expensive to begin with, and we even might end up throwing away the result depending on the log level.
As an alternative, it's possible to take advantage of SLF4J placeholders:
LOG.debug("Created query {} for * fields.", query.export());
[Mike] Looks like I can only Edit your comment, not reply. I will fix this. It is guarded with isDebugEnabled() specifically so the query.export() won't be executed.
mikereiche
force-pushed
the
datacouch_588_pageable_and_realign
branch
from
4b637d6 to
df21147
Compare
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)
mikereiche
force-pushed
the
datacouch_588_pageable_and_realign
branch
from
df21147 to
3aff13c
Compare
|
I'd like to get @mp911de on the overall approach and design, once that is "spring best practice" approved let's focus on reviewing all the bits (since this is a bigger change set) |
There was a problem hiding this comment.
This pull request is pretty cluttered with several types of changes which makes it a bit difficult to understand what's happening here. In general, it makes a lot of sense to align the Couchbase module with MongoDB/Cassandra as this simplifies the implementation approach.
The original authors aren't required to be retained on the copy since similar code patterns are available from the other store modules.
Please note that reactive pagination is limited to Flux<T> as there's no such thing like PageFlux. We're discussing right now, whether Mono<Page<T>> would make sense as return type. We did a first experiment in Cassandra that turned out to work well.
Let me know whether there are specific topics you need more feedback on.
| * | ||
| * @return is this a 'delete'? | ||
| */ | ||
| public boolean isDeleteQuery() { |
There was a problem hiding this comment.
Those methods should go into the actual CouchbaseQuery (or AbstractCouchbaseQuery). Assuming the method name starts with a particular prefix is pretty risky especially when the query method uses an annotated query. MongoDB's @Query defines attributes such as delete or exists. PartTree (the source for query derivation) exposes similar information.
| } | ||
|
|
||
| /* | ||
| final class ReactivePagedExecution<FindWithQuery> implements CouchbaseQueryExecution { // TODO ?? |
There was a problem hiding this comment.
There's pretty much commented-out code blocks in here.
Reactive pagination returns typically a Flux<T>. An alternative approach can be Mono<Page<T>>.
|
|
||
| return PageableExecutionUtils.getPage(matching.all().collectList().block(), pageable, () -> { | ||
|
|
||
| long count = operation.matching(query.skip(-1).limit(-1)).count().block(); |
There was a problem hiding this comment.
While the code isn't active, please note that calling .block() methods is almost a guarantee to break the entire processing flow. Please refrain from calling .block().
| * @see org.springframework.data.mongodb.repository.query.AbstractMongoQuery#isCountQuery() | ||
| */ | ||
| @Override | ||
| protected boolean isCountQuery() { |
| @@ -124,6 +133,15 @@ private String assembleEntityQuery(final boolean count) { | |||
| return query.toN1qlString(template, this.domainType, count); | |||
| } | |||
|
|
|||
| @Override | |||
| public FindWithProjection<T> inCollection(String collection) { | |||
There was a problem hiding this comment.
inCollection/distinct methods are MongoDB-specific features. It makes only sense to re-apply the same pattern if Couchbase can provide such functionality.
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)
…base into datacouch_588_pageable_and_realign
…g-projects/spring-data-couchbase into datacouch_588_pageable_and_realign
This is intended to be a preview for Michael Nitschinger.
The javadoc (especially author) is yet to be updated.
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)