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.

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

Merged
merged 21 commits into from
Jan 7, 2022

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Jan 4, 2022

Implementing new findBy() method in SimpleDatastoreRepository and corresponding FluentQuery.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() and as() 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

Comment on lines 3 to 6
- kind: test_entities_ci
properties:
- name: embeddedEntity.stringField
- name: blobField
- name: size
- name: color
Copy link
Contributor Author

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.

Copy link
Contributor

@elefeint elefeint left a 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);
Copy link
Contributor

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

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.

Copy link
Contributor

@elefeint elefeint left a 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.

@zhumin8 zhumin8 changed the base branch from 3.x to main January 6, 2022 15:34
@zhumin8 zhumin8 changed the base branch from main to 3.x January 6, 2022 15:35
@zhumin8
Copy link
Contributor Author

zhumin8 commented Jan 6, 2022

I'll update base branch once #838 gets merged in.

@zhumin8 zhumin8 changed the base branch from 3.x to main January 6, 2022 16:55
@sonarcloud
Copy link

sonarcloud bot commented Jan 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.3% 90.3% Coverage
0.0% 0.0% Duplication

@zhumin8 zhumin8 merged commit 95538ab into main Jan 7, 2022
@zhumin8 zhumin8 deleted the fluentq branch January 7, 2022 15:30
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants