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

Treat composite keys correct during load and save operations. #790

Merged
merged 13 commits into from
Apr 22, 2020

Conversation

michael-simons
Copy link
Collaborator

Please have a look at the message of the first commit for reasoning.

Primary keys having a `CompositeAttributeConverter` have not been taking into consideration when saving and loading: The map would be used as "the" value for the id. That would might work when the map would be converted by the drivers, but the queries would not match anything due to the fact that no keys in the map are compared.

This commit changes this by creating an inline cypher map from the properties that are matched on during load and written during save as well.

This fixes #789.
All joining and split operations of composite keys have been moved into
`NodeQueryStatements`. This is not an optimal place, but much better
than distributing it all over the interfaces.

We cannot find an optimal place due to the way things are packaged.
@codecov-io
Copy link

codecov-io commented Apr 21, 2020

Codecov Report

Merging #790 into master will decrease coverage by 0.07%.
The diff coverage is 74.07%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #790      +/-   ##
============================================
- Coverage     80.39%   80.32%   -0.08%     
- Complexity     3003     3015      +12     
============================================
  Files           275      275              
  Lines          9181     9218      +37     
  Branches       1363     1377      +14     
============================================
+ Hits           7381     7404      +23     
- Misses         1299     1310      +11     
- Partials        501      504       +3     
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/org/neo4j/ogm/metadata/ClassInfo.java 87.91% <25.00%> (-0.82%) 218.00 <2.00> (-1.00)
...java/org/neo4j/ogm/autoindex/AutoIndexManager.java 81.39% <34.78%> (-6.58%) 45.00 <1.00> (ø)
...c/main/java/org/neo4j/ogm/autoindex/AutoIndex.java 61.98% <50.00%> (-0.30%) 41.00 <0.00> (ø)
...ion/request/strategy/impl/NodeQueryStatements.java 97.56% <94.44%> (-2.44%) 19.00 <12.00> (+6.00) ⬇️
.../java/org/neo4j/ogm/context/EntityGraphMapper.java 93.38% <100.00%> (+0.04%) 130.00 <0.00> (+1.00)
...er/builders/statement/NewNodeStatementBuilder.java 96.87% <100.00%> (ø) 11.00 <1.00> (+2.00)
...g/neo4j/ogm/session/delegates/SessionDelegate.java 94.50% <100.00%> (+0.31%) 33.00 <10.00> (+3.00)
.../strategy/impl/IdCollectionMatchClauseBuilder.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...on/request/strategy/impl/IdMatchClauseBuilder.java 87.50% <100.00%> (+1.78%) 6.00 <2.00> (ø)
... and 2 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 15af68d...a1607ea. Read the comment docs.

This allows properties in @CompositeIndex to be prefixed with `fieldName.` so that the auto index manager can recognize indexes belonging to fields that are decomposed by a map composite converter.

We also log a big warning in case a field is to be decomposed but doesn’t carry a composite index but a normal one.
@michael-simons michael-simons merged commit cb49c6c into master Apr 22, 2020
@michael-simons michael-simons deleted the issue/789 branch April 22, 2020 09:28
michael-simons added a commit that referenced this pull request Apr 22, 2020
This change has a couple of aspects:

= Unwrap composite keys during load and save.

Primary keys having a `CompositeAttributeConverter` have not been taking into consideration when saving and loading: The map would be used as "the" value for the id. That would might work when the map would be converted by the drivers, but the queries would not match anything due to the fact that no keys in the map are compared.

This commit changes this by creating an inline cypher map from the properties that are matched on during load and written during save as well.

This fixes #789.

= Make the AutoIndexManager aware of decomposed fields.

This allows properties in @CompositeIndex to be prefixed with `fieldName.` so that the auto index manager can recognize indexes belonging to fields that are decomposed by a map composite converter.

We also log a big warning in case a field is to be decomposed but doesn’t carry a composite index but a normal one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SemanticError: Cannot merge node using null property value for key.
3 participants