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

fix null handling for id in query by example #2471

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

dmitry-s
Copy link
Contributor

fixes #2470

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #2471 into master will decrease coverage by 7.13%.
The diff coverage is 57.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2471      +/-   ##
============================================
- Coverage     81.29%   74.16%   -7.14%     
+ Complexity     2343     2150     -193     
============================================
  Files           264      267       +3     
  Lines          7663     7722      +59     
  Branches        790      798       +8     
============================================
- Hits           6230     5727     -503     
- Misses         1097     1625     +528     
- Partials        336      370      +34     
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 74.16% <57.89%> (+0.10%) 2150.00 <7.00> (+31.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...oud/gcp/data/datastore/core/DatastoreTemplate.java 86.73% <57.89%> (-9.77%) 123.00 <7.00> (-11.00)
...gcp/secretmanager/SecretManagerPropertySource.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...a/spanner/repository/query/SpannerQueryMethod.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-6.00%)
...retmanager/SecretManagerPropertySourceLocator.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...figure/config/GcpConfigBootstrapConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...e/spanner/GcpSpannerEmulatorAutoConfiguration.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%)
...ository/config/DatastoreRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...restore/GcpFirestoreEmulatorAutoConfiguration.java 0.00% <0.00%> (-84.85%) 0.00% <0.00%> (-4.00%)
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java 17.64% <0.00%> (-73.53%) 4.00% <0.00%> (-5.00%)
... and 58 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 853a949...639030c. Read the comment docs.

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 love the refactoring and the new comments. So much more intuitive!
Just a couple of minor comments.

filters.add(StructuredQuery.PropertyFilter.eq(fieldName, value));
if (notIgnoredProperty(example, persistentProperty)) {
Value<?> value = getValue(example, probeEntity, persistentEntity, persistentProperty);
NullHandler nullHandler = example.getMatcher().getNullHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

So the check for value instanceof NullValue is not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still required. NullHandler just indicates if we need to include null valued fields or not, and value instanceof NullValue is a check to see if a field holds a null value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's in addFilter now. I missed it.

return value;
}

private <T> boolean notIgnoredProperty(Example<T> example, DatastorePersistentProperty persistentProperty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be easier to read as isIgnoredProperty(), removing the negation here and putting the negation in getDatastoreReadWriter() method instead. Our brains are very used to the isBlah pattern.

@dmitry-s dmitry-s requested a review from elefeint July 21, 2020 19:55
@sonarcloud
Copy link

sonarcloud bot commented Jul 21, 2020

SonarCloud Quality Gate failed.

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

65.5% 65.5% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@dmitry-s dmitry-s merged commit 48a5813 into master Jul 21, 2020
@dmitry-s dmitry-s deleted the datastore-query-by-example-fix branch July 21, 2020 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Query by example API doesn't follow documentation
3 participants