Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

support for nested properties in PartTree methods #2307

Merged
merged 3 commits into from
Apr 9, 2020
Merged

support for nested properties in PartTree methods #2307

merged 3 commits into from
Apr 9, 2020

Conversation

dmitry-s
Copy link
Contributor

@dmitry-s dmitry-s commented Apr 8, 2020

fixes #2305

@dmitry-s dmitry-s requested review from meltsufin and elefeint April 8, 2020 00:07
@dmitry-s dmitry-s added the datastore GCP Datastore label Apr 8, 2020
elefeint
elefeint previously requested changes Apr 8, 2020
View reviewed changes
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.

Looks good, couple of questions -- and I really like the tests.

...va/org/springframework/cloud/gcp/data/datastore/repository/query/PartTreeDatastoreQuery.java Outdated Show resolved Hide resolved
...va/org/springframework/cloud/gcp/data/datastore/repository/query/PartTreeDatastoreQuery.java Outdated Show resolved Hide resolved
...va/org/springframework/cloud/gcp/data/datastore/repository/query/PartTreeDatastoreQuery.java Outdated Show resolved Hide resolved
meltsufin
meltsufin suggested changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, but also notice that the build is red.
Also, please make sure the integration tests and the sample work.
Thanks!

...va/org/springframework/cloud/gcp/data/datastore/repository/query/PartTreeDatastoreQuery.java Outdated Show resolved Hide resolved
...va/org/springframework/cloud/gcp/data/datastore/repository/query/PartTreeDatastoreQuery.java Outdated
DatastorePersistentProperty persistentProperty = (DatastorePersistentProperty) this.datastorePersistentEntity
.getPersistentProperty(part.getProperty().getSegment());
Iterable<PropertyPath> iterable = () -> part.getProperty().iterator();
List<DatastorePersistentProperty> properties =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this block into a separate helper private method buildName like you did for Firestore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we need not only the name, but also the persistent property

@dmitry-s dmitry-s requested review from meltsufin and elefeint April 8, 2020 23:03
@codecov
Copy link

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

Merging #2307 into master will decrease coverage by 8.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2307      +/-   ##
============================================
- Coverage     80.97%   72.72%   -8.26%     
+ Complexity     2313     2065     -248     
============================================
  Files           258      258              
  Lines          7464     7471       +7     
  Branches        763      763              
============================================
- Hits           6044     5433     -611     
- Misses         1100     1689     +589     
- Partials        320      349      +29
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 72.72% <100.00%> (+0.01%) 2065.00 <6.00> (+2.00)
Impacted Files Coverage Δ Complexity Δ
...store/mapping/FirestorePersistentPropertyImpl.java 83.33% <ø> (ø) 5.00 <0.00> (ø)
...store/repository/query/PartTreeDatastoreQuery.java 91.51% <100.00%> (-2.16%) 61.00 <6.00> (-2.00)
...store/repository/query/PartTreeFirestoreQuery.java 90.36% <100.00%> (-7.23%) 21.00 <0.00> (-2.00)
...a/spanner/repository/query/SpannerQueryMethod.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-6.00%)
...figure/config/GcpConfigBootstrapConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...restore/repository/query/FirestoreQueryMethod.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...epository/config/SpannerRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...retmanager/SecretManagerPropertySourceLocator.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...pository/support/FirestoreQueryLookupStrategy.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...ository/config/DatastoreRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a754e27...c31cd20. Read the comment docs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

meltsufin
meltsufin approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge if all of the integration tests are passing.

@dmitry-s dmitry-s merged commit 3c4d6fc into master Apr 9, 2020
@dmitry-s dmitry-s deleted the datastore-nested-properties branch April 9, 2020 13:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@meltsufin meltsufin meltsufin approved these changes

@elefeint elefeint Awaiting requested review from elefeint

Assignees
No one assigned
Labels
datastore GCP Datastore
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Datastore - add nested properties support for PartTree methods
3 participants
@dmitry-s @meltsufin @elefeint