-
Notifications
You must be signed in to change notification settings - Fork 327
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
Support Stream as return type to datastore queries #551
Conversation
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.
Looks good to me. I would just encourage to also update Datastore documentation and one of the samples.
...-datastore/src/test/java/com/google/cloud/spring/data/datastore/it/TestEntityRepository.java
Outdated
Show resolved
Hide resolved
Good idea. I'll look into updating Datastore documentation and sample. |
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 code looks great! I only have minor feedback.
@@ -439,7 +444,7 @@ public Cursor getCursor() { | |||
|
|||
structuredQueryBuilder.setKind(PartTreeDatastoreQuery.this.datastorePersistentEntity.kindName()); | |||
|
|||
singularResult = (!isCountingQuery && collectionType == null) && !PartTreeDatastoreQuery.this.tree.isDelete(); | |||
singularResult = (!isCountingQuery && collectionType == null && !isStreamQuery) && !PartTreeDatastoreQuery.this.tree.isDelete(); |
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 boolean expression is getting too complicated. Since we are touching this line, could you refactor it into an if/else statement, with a view for reducing the negations ("if it's a counting query or a stream query, then singlarResult = false") is easier on the mind than "if it's not a counting query and it's not a stream query, singlarResult = true".
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.
Good point, I'll take the chance to make it easier to read.
Stream<TestEntity> findStreamByColor(String color); | ||
|
||
@Query("select * from test_entities_ci where color = @color") | ||
Stream<TestEntity> findByColorStream(@Param("color") String 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.
The difference between findStreamByColor
and findByColorStream
is very subtle. Could you rename findByColorStream
to something like findGqlStreamByColor
?
And possibly rename findStreamByColor
to findPartTreeStreamByColor
, so the targeted functionality is very clear.
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.
hmm, do you think I should also make a comment here say these method are to test Stream as return type for GQL query and PartTree query respectively? Or is it too much?
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.
no, I think the descriptive naming you have right now is great.
...est/java/com/google/cloud/spring/data/datastore/repository/query/GqlDatastoreQueryTests.java
Outdated
Show resolved
Hide resolved
...est/java/com/google/cloud/spring/data/datastore/repository/query/GqlDatastoreQueryTests.java
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
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.
Nice work!
To fix DatastoreSampleApplicationIntegrationTests broke by #551.
…rm#551) fixes GoogleCloudPlatform#452. Fixed code in GqlDatastoreQuery and PartTreeDatastoreQuery to support for Stream as a return type and added corresponding unit tests and integration tests.
To fix DatastoreSampleApplicationIntegrationTests broke by GoogleCloudPlatform#551.
Bumps [mockito-core](https://github.com/mockito/mockito) from 4.6.0 to 4.6.1. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v4.6.0...v4.6.1) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This is fix for #452.
Fixed code in
GqlDatastoreQuery
andPartTreeDatastoreQuery
to support for Stream as a return type and added corresponding unit tests and integration tests.