Skip to content

Commit abb8d64

Browse files
meistermeiermeistermeier
committed
GH-2600 - Fix relproperties and simple relationship hydration.
Misses dynamic relationships right now. Closes #2600
1 parent e3c9e59 commit abb8d64

File tree

2 files changed

+118
-32
lines changed

2 files changed

+118
-32
lines changed

‎src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java‎

Lines changed: 104 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ private <R> MapAccessor determineQueryRoot(MapAccessor mapAccessor, @Nullable Ne
144144
Node node = value.asNode();
145145
if (primaryLabels.stream().anyMatch(node::hasLabel)) { // it has a matching label
146146
// We haven't seen this node yet, so we take it
147-
if (!knownObjects.containsNode(node)) {
147+
if (knownObjects.getObject("N" + IdentitySupport.getInternalId(node)) == null) {
148148
matchingNodes.add(node);
149149
} else {
150150
seenMatchingNodes.add(node);
@@ -264,25 +264,26 @@ private static MapAccessor mergeRootNodeWithRecord(Node node, MapAccessor record
264264
private <ET> ET map(MapAccessor queryResult, MapAccessor allValues, Neo4jPersistentEntity<ET> nodeDescription) {
265265
Collection<Relationship> relationshipsFromResult = extractRelationships(allValues);
266266
Collection<Node> nodesFromResult = extractNodes(allValues);
267-
return map(queryResult, nodeDescription, null, relationshipsFromResult, nodesFromResult);
267+
return map(queryResult, nodeDescription, null, null, relationshipsFromResult, nodesFromResult);
268268
}
269269

270270
private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescription,
271-
@Nullable Object lastMappedEntity, Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult) {
271+
@Nullable Object lastMappedEntity, @Nullable RelationshipDescription relationshipDescription, Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult) {
272272

273273
// prior to SDN 7 local `getInternalId` didn't check relationships, so in that case, they have never been a known
274274
// object. The centralized methods checks those too now. The condition is to recreate the old behaviour without
275275
// losing the central access. The behaviour of knowObjects should take different sources of ids into account,
276276
// as relationships and nodes might have overlapping values
277-
Long internalId = queryResult instanceof Relationship ? null : IdentitySupport.getInternalId(queryResult);
277+
String direction = relationshipDescription != null ? relationshipDescription.getDirection().name() : null;
278+
String internalId = IdentitySupport.getInternalId(queryResult, direction);
278279

279280
Supplier<ET> mappedObjectSupplier = () -> {
280281
if (knownObjects.isInCreation(internalId)) {
281282
throw new MappingException(
282283
String.format(
283284
"The node with id %s has a logical cyclic mapping dependency; " +
284285
"its creation caused the creation of another node that has a reference to this",
285-
internalId)
286+
internalId.substring(1))
286287
);
287288
}
288289
knownObjects.setInCreation(internalId);
@@ -330,7 +331,7 @@ private <ET> ET map(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescr
330331
}
331332

332333

333-
private <ET> void populateProperties(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescription, Long internalId,
334+
private <ET> void populateProperties(MapAccessor queryResult, Neo4jPersistentEntity<ET> nodeDescription, String internalId,
334335
ET mappedObject, @Nullable Object lastMappedEntity,
335336
Collection<Relationship> relationshipsFromResult, Collection<Node> nodesFromResult, boolean objectAlreadyMapped) {
336337

@@ -533,6 +534,23 @@ private AssociationHandler<Neo4jPersistentProperty> populateFrom(MapAccessor que
533534
boolean populatedScalarValue = !persistentProperty.isCollectionLike() && !persistentProperty.isMap()
534535
&& propertyValueNotNull;
535536

537+
if (populatedCollection) {
538+
createInstanceOfRelationships(persistentProperty, queryResult, (RelationshipDescription) association, baseDescription, relationshipsFromResult, nodesFromResult, false)
539+
.ifPresent(value -> {
540+
Collection<?> providedCollection = (Collection<?>) value;
541+
Collection<?> existingValue = (Collection<?>) propertyValue;
542+
Collection<Object> newValue = CollectionFactory.createCollection(existingValue.getClass(), providedCollection.size() + existingValue.size());
543+
544+
RelationshipDescription relationshipDescription = (RelationshipDescription) association;
545+
Map<Object, Object> mergedValues = new HashMap<>();
546+
mergeCollections(relationshipDescription, existingValue, mergedValues);
547+
mergeCollections(relationshipDescription, providedCollection, mergedValues);
548+
549+
newValue.addAll(mergedValues.values());
550+
propertyAccessor.setProperty(persistentProperty, newValue);
551+
});
552+
}
553+
536554
boolean propertyAlreadyPopulated = populatedCollection || populatedMap || populatedScalarValue;
537555

538556
// avoid unnecessary re-assignment of values
@@ -545,9 +563,33 @@ private AssociationHandler<Neo4jPersistentProperty> populateFrom(MapAccessor que
545563
};
546564
}
547565

566+
private void mergeCollections(RelationshipDescription relationshipDescription, Collection<?> values, Map<Object, Object> mergedValues) {
567+
for (Object existingValueInCollection : values) {
568+
if (relationshipDescription.hasRelationshipProperties()) {
569+
Object existingIdPropertyValue = ((Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity())
570+
.getPropertyAccessor(existingValueInCollection)
571+
.getProperty(((Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity()).getIdProperty());
572+
573+
mergedValues.put(existingIdPropertyValue, existingValueInCollection);
574+
} else if (!relationshipDescription.isDynamic()) { // should not happen because this is all inside populatedCollection (but better safe than sorry)
575+
Object existingIdPropertyValue = ((Neo4jPersistentEntity<?>) relationshipDescription.getTarget())
576+
.getPropertyAccessor(existingValueInCollection)
577+
.getProperty(((Neo4jPersistentEntity<?>) relationshipDescription.getTarget()).getIdProperty());
578+
579+
mergedValues.put(existingIdPropertyValue, existingValueInCollection);
580+
}
581+
}
582+
}
583+
584+
private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty persistentProperty, MapAccessor values,
585+
RelationshipDescription relationshipDescription, NodeDescription<?> baseDescription, Collection<Relationship> relationshipsFromResult,
586+
Collection<Node> nodesFromResult) {
587+
return createInstanceOfRelationships(persistentProperty, values, relationshipDescription, baseDescription, relationshipsFromResult, nodesFromResult, true);
588+
}
589+
548590
private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty persistentProperty, MapAccessor values,
549591
RelationshipDescription relationshipDescription, NodeDescription<?> baseDescription, Collection<Relationship> relationshipsFromResult,
550-
Collection<Node> nodesFromResult) {
592+
Collection<Node> nodesFromResult, boolean fetchMore) {
551593

552594
String typeOfRelationship = relationshipDescription.getType();
553595
String targetLabel = relationshipDescription.getTarget().getPrimaryLabel();
@@ -588,6 +630,7 @@ private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty p
588630

589631
if (Values.NULL.equals(list)) {
590632
Long sourceNodeId = IdentitySupport.getInternalId(values);
633+
//Long sourceNodeId = getInternalIdAsLong(values);
591634

592635
Function<Relationship, Long> sourceIdSelector = relationshipDescription.isIncoming() ? IdentitySupport::getEndId : IdentitySupport::getStartId;
593636
Function<Relationship, Long> targetIdSelector = relationshipDescription.isIncoming() ? IdentitySupport::getStartId : IdentitySupport::getEndId;
@@ -614,22 +657,37 @@ private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty p
614657
// If this relationship got processed twice (OUTGOING, INCOMING), it is never needed again
615658
// and therefor should not be in the list.
616659
// Otherwise, for highly linked data it could potentially cause a StackOverflowError.
617-
if (knownObjects.hasProcessedRelationshipCompletely(IdentitySupport.getInternalId(possibleRelationship))) {
660+
String direction = relationshipDescription.getDirection().name();
661+
if (knownObjects.hasProcessedRelationshipCompletely("R" + direction + IdentitySupport.getInternalId(possibleRelationship))) {
618662
relationshipsFromResult.remove(possibleRelationship);
619663
}
620664
// If the target is the same(equal) node, get the related object from the cache.
621665
// Avoiding the call to the map method also breaks an endless cycle of trying to finish
622666
// the property population of _this_ object.
623667
// The initial population will happen at the end of this mapping. This is sufficient because
624668
// it only affects properties not changing the instance of the object.
625-
Object mappedObject = sourceNodeId != null && sourceNodeId.equals(targetNodeId)
626-
? knownObjects.getObject(sourceNodeId)
627-
: map(possibleValueNode, concreteTargetNodeDescription, null, relationshipsFromResult, nodesFromResult);
628-
if (relationshipDescription.hasRelationshipProperties()) {
669+
Object mappedObject;
670+
if (fetchMore) {
671+
mappedObject = sourceNodeId != null && sourceNodeId.equals(targetNodeId)
672+
? knownObjects.getObject("N" + sourceNodeId)
673+
: map(possibleValueNode, concreteTargetNodeDescription, null, null, relationshipsFromResult, nodesFromResult);
674+
} else {
675+
Object objectFromStore = knownObjects.getObject("N" + targetNodeId);
676+
mappedObject = objectFromStore != null
677+
? objectFromStore
678+
: map(possibleValueNode, concreteTargetNodeDescription, null, null, relationshipsFromResult, nodesFromResult);
679+
}
629680

630-
Object relationshipProperties = map(possibleRelationship,
631-
(Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(),
632-
mappedObject, relationshipsFromResult, nodesFromResult);
681+
if (relationshipDescription.hasRelationshipProperties()) {
682+
Object relationshipProperties;
683+
if (fetchMore) {
684+
relationshipProperties = map(possibleRelationship, (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(), mappedObject, relationshipDescription, relationshipsFromResult, nodesFromResult);
685+
} else {
686+
Object objectFromStore = knownObjects.getObject(IdentitySupport.getInternalId(possibleRelationship, relationshipDescription.getDirection().name()));
687+
relationshipProperties = objectFromStore != null
688+
? objectFromStore
689+
: map(possibleRelationship, (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(), mappedObject, relationshipDescription, relationshipsFromResult, nodesFromResult);
690+
}
633691
relationshipsAndProperties.add(relationshipProperties);
634692
mappedObjectHandler.accept(possibleRelationship.type(), relationshipProperties);
635693
} else {
@@ -646,7 +704,15 @@ private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty p
646704
Neo4jPersistentEntity<?> concreteTargetNodeDescription =
647705
getMostConcreteTargetNodeDescription(genericTargetNodeDescription, relatedEntity);
648706

649-
Object valueEntry = map(relatedEntity, concreteTargetNodeDescription, null, relationshipsFromResult, nodesFromResult);
707+
Object valueEntry;
708+
if (fetchMore) {
709+
valueEntry = map(relatedEntity, concreteTargetNodeDescription, null, null, relationshipsFromResult, nodesFromResult);
710+
} else {
711+
Object objectFromStore = knownObjects.getObject(IdentitySupport.getInternalId(relatedEntity, null));
712+
valueEntry = objectFromStore != null
713+
? objectFromStore
714+
: map(relatedEntity, concreteTargetNodeDescription, null, null, relationshipsFromResult, nodesFromResult);
715+
}
650716

651717
if (relationshipDescription.hasRelationshipProperties()) {
652718
String sourceLabel = relationshipDescription.getSource().getMostAbstractParentLabel(baseDescription);
@@ -655,9 +721,16 @@ private Optional<Object> createInstanceOfRelationships(Neo4jPersistentProperty p
655721
Relationship relatedEntityRelationship = relatedEntity.get(relationshipSymbolicName)
656722
.asRelationship();
657723

658-
Object relationshipProperties = map(relatedEntityRelationship,
659-
(Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(),
660-
valueEntry, relationshipsFromResult, nodesFromResult);
724+
Object relationshipProperties;
725+
if (fetchMore) {
726+
relationshipProperties = map(relatedEntityRelationship, (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(), valueEntry, relationshipDescription, relationshipsFromResult, nodesFromResult);
727+
} else {
728+
Object objectFromStore = knownObjects.getObject(IdentitySupport.getInternalId(relatedEntityRelationship, relationshipDescription.getDirection().name()));
729+
relationshipProperties = objectFromStore != null
730+
? objectFromStore
731+
: map(relatedEntityRelationship, (Neo4jPersistentEntity<?>) relationshipDescription.getRelationshipPropertiesEntity(), valueEntry, relationshipDescription, relationshipsFromResult, nodesFromResult);
732+
}
733+
661734
relationshipsAndProperties.add(relationshipProperties);
662735
mappedObjectHandler.accept(relatedEntity.get(RelationshipDescription.NAME_OF_RELATIONSHIP_TYPE).asString(), relationshipProperties);
663736
} else {
@@ -773,14 +846,14 @@ static class KnownObjects {
773846
private final Lock read = lock.readLock();
774847
private final Lock write = lock.writeLock();
775848

776-
private final Map<Long, Object> internalIdStore = new HashMap<>();
777-
private final Map<Long, Boolean> internalCurrentRecord = new HashMap<>();
778-
private final Set<Long> previousRecords = new HashSet<>();
779-
private final Set<Long> idsInCreation = new HashSet<>();
849+
private final Map<String, Object> internalIdStore = new HashMap<>();
850+
private final Map<String, Boolean> internalCurrentRecord = new HashMap<>();
851+
private final Set<String> previousRecords = new HashSet<>();
852+
private final Set<String> idsInCreation = new HashSet<>();
780853

781-
private final Map<Long, Integer> processedRelationships = new HashMap<>();
854+
private final Map<String, Integer> processedRelationships = new HashMap<>();
782855

783-
private void storeObject(@Nullable Long internalId, Object object) {
856+
private void storeObject(@Nullable String internalId, Object object) {
784857
if (internalId == null) {
785858
return;
786859
}
@@ -794,7 +867,7 @@ private void storeObject(@Nullable Long internalId, Object object) {
794867
}
795868
}
796869

797-
private void setInCreation(@Nullable Long internalId) {
870+
private void setInCreation(@Nullable String internalId) {
798871
if (internalId == null) {
799872
return;
800873
}
@@ -806,7 +879,7 @@ private void setInCreation(@Nullable Long internalId) {
806879
}
807880
}
808881

809-
private boolean isInCreation(@Nullable Long internalId) {
882+
private boolean isInCreation(@Nullable String internalId) {
810883
if (internalId == null) {
811884
return false;
812885
}
@@ -829,8 +902,7 @@ private boolean containsNode(Node node) {
829902
}
830903

831904
@Nullable
832-
private Object getObject(@Nullable Long internalId) {
833-
905+
private Object getObject(@Nullable String internalId) {
834906
if (internalId == null) {
835907
return null;
836908
}
@@ -842,7 +914,7 @@ private Object getObject(@Nullable Long internalId) {
842914
}
843915
}
844916

845-
private void removeFromInCreation(@Nullable Long internalId) {
917+
private void removeFromInCreation(@Nullable String internalId) {
846918
if (internalId == null) {
847919
return;
848920
}
@@ -854,7 +926,7 @@ private void removeFromInCreation(@Nullable Long internalId) {
854926
}
855927
}
856928

857-
private boolean alreadyMappedInPreviousRecord(@Nullable Long internalId) {
929+
private boolean alreadyMappedInPreviousRecord(@Nullable String internalId) {
858930
if (internalId == null) {
859931
return false;
860932
}
@@ -874,7 +946,7 @@ private boolean alreadyMappedInPreviousRecord(@Nullable Long internalId) {
874946
* It increases the process count of relationships (mapped by their ids)
875947
* AND checks if it was already processed twice (INCOMING/OUTGOING).
876948
*/
877-
private boolean hasProcessedRelationshipCompletely(Long relationshipId) {
949+
private boolean hasProcessedRelationshipCompletely(String relationshipId) {
878950
try {
879951
write.lock();
880952

‎src/main/java/org/springframework/data/neo4j/core/mapping/IdentitySupport.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.neo4j.driver.Record;
2222
import org.neo4j.driver.types.Entity;
2323
import org.neo4j.driver.types.MapAccessor;
24+
import org.neo4j.driver.types.Node;
2425
import org.neo4j.driver.types.Relationship;
2526
import org.neo4j.driver.types.TypeSystem;
2627
import org.springframework.data.mapping.PersistentPropertyAccessor;
@@ -90,6 +91,19 @@ public static Long getInternalId(@NonNull MapAccessor row) {
9091
return row.get(columnToUse).asLong();
9192
}
9293

94+
@Nullable
95+
public static String getInternalId(@NonNull MapAccessor queryResult, @Nullable String seed) {
96+
if (queryResult instanceof Node) {
97+
return "N" + getInternalId(queryResult);
98+
} else if (queryResult instanceof Relationship) {
99+
return "R" + seed + getInternalId(queryResult);
100+
} else if (!(queryResult.get(Constants.NAME_OF_INTERNAL_ID) == null || queryResult.get(Constants.NAME_OF_INTERNAL_ID).isNull())) {
101+
return "N" + queryResult.get(Constants.NAME_OF_INTERNAL_ID).asLong();
102+
}
103+
104+
return null;
105+
}
106+
93107
/**
94108
* Returns the start id of the relationship
95109
*

0 commit comments

Comments
 (0)