-
Notifications
You must be signed in to change notification settings - Fork 310
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.
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
Implement new findBy() method with FluentQuery for SimpleDatastoreRepository #836
Conversation
- kind: test_entities_ci | ||
properties: | ||
- name: embeddedEntity.stringField | ||
- name: blobField | ||
- name: size | ||
- name: color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is copied from main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks overall very good. Let's talk about support for return type conversion a bit more today.
|
||
@Override | ||
public R firstValue() { | ||
return (R) SimpleDatastoreRepository.this.findFirstSorted(this.example, this.sort); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this assert that the current sort is not Sort.unsorted()
before trying to find first sorted? Or at least log a warning. So users don't have to troubleshoot non-repeatable behavior...
@NonNull | ||
@Override | ||
public Page<R> page(@NonNull Pageable pageable) { | ||
return (Page<R>) SimpleDatastoreRepository.this.findAll(this.example, pageable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast from page<source>
to page<returnType>
gave me pause, so I looked into it. Not supporting type conversions for now sounds fine, but then we should probably remove the R
parameter completely. The class signature would then be DatastoreFluentQueryByExample<S extends T> implements FluentQuery.FetchableFluentQuery<S>
, and the only place R
would show up is in the signature of findBy
. This should be reasonably safe, since DatastoreFluentQueryByExample
is a package-private class that's an implementation detail, and we can change it if we add support for as
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this point we'll end up merging this PR straight into main, not into 3.x.
...oud-gcp-samples/spring-cloud-gcp-data-datastore-sample/src/main/java/com/example/Singer.java
Outdated
Show resolved
Hide resolved
I'll update base branch once #838 gets merged in. |
update index.yaml for existing test (already changed in main branch)."
…on't support type conversions for now.
Kudos, SonarCloud Quality Gate passed! |
…ository (GoogleCloudPlatform#836) Implementing new findBy() method in SimpleDatastoreRepository and corresponding FluentQuery.FetchableFluentQuery interface introduced by spring-data-commons-2.6.0 (GoogleCloudPlatform#2421). This is basically implementing already supported query methods to fit functional programming model. Note: In this PR, leaving project() and as() methods as unsupported for now. As Datastore query by example does not support projection overall yet. fix for GoogleCloudPlatform#692
Implementing new
findBy()
method inSimpleDatastoreRepository
and correspondingFluentQuery.FetchableFluentQuery
interface introduced by spring-data-commons-2.6.0 (#2421).This is basically implementing already supported query methods to fit functional programming model.
Note: In this PR, leaving
project()
andas()
methods as unsupported for now. As Datastore query by example does not support projection overall yet(doc). Let's have a separate discussion about it.fix for #692