From 804a954a1a359f2d62eb38674d6407ff1608fb41 Mon Sep 17 00:00:00 2001 From: Gerrit Meier Date: Thu, 22 Jun 2023 14:57:16 +0200 Subject: [PATCH] GH-2750 - Avoid toString in relationship creation. Without the string conversion Neo4j will either to an (element)id or label seek instead of a full node scan. Also: In some corner case adding labels to the generic cyclic query can improve the performance of the query. On the other hand it cannot make it worse. Closes #2750 --- .../data/neo4j/core/Neo4jTemplate.java | 11 +++++-- .../neo4j/core/ReactiveNeo4jTemplate.java | 14 ++++---- .../data/neo4j/core/TemplateSupport.java | 32 +++++++++---------- .../neo4j/core/mapping/CypherGenerator.java | 27 +++++++++++++--- ...tedRelationshipProcessingStateMachine.java | 18 +++++------ 5 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java index fe4888cf6..193fa6e25 100644 --- a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java @@ -409,7 +409,12 @@ public final class Neo4jTemplate implements throw new IllegalStateException("Could not retrieve an internal id while saving"); } - String elementId = newOrUpdatedNode.map(IdentitySupport::getElementId).get(); + Object elementId = newOrUpdatedNode.map(node -> { + if (!entityMetaData.isUsingDeprecatedInternalId() && entityMetaData.isUsingInternalIds()) { + return IdentitySupport.getElementId(node); + } + return node.id(); + }).get(); PersistentPropertyAccessor propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved); TemplateSupport.setGeneratedIdIfNecessary(entityMetaData, propertyAccessor, elementId, newOrUpdatedNode); @@ -807,14 +812,14 @@ public final class Neo4jTemplate implements ? stateMachine.getProcessedAs(relatedObjectBeforeCallbacksApplied) : eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied); - String relatedInternalId; + Object relatedInternalId; Entity savedEntity = null; // No need to save values if processed if (stateMachine.hasProcessedValue(relatedValueToStore)) { relatedInternalId = stateMachine.getObjectId(relatedValueToStore); } else { savedEntity = saveRelatedNode(newRelatedObject, targetEntity, includeProperty, currentPropertyPath); - relatedInternalId = TemplateSupport.rendererCanUseElementIdIfPresent(renderer) ? savedEntity.elementId() : Long.toString(savedEntity.id()); + relatedInternalId = TemplateSupport.rendererCanUseElementIdIfPresent(renderer, targetEntity) ? savedEntity.elementId() : savedEntity.id(); stateMachine.markEntityAsProcessed(relatedValueToStore, relatedInternalId); if (relatedValueToStore instanceof MappingSupport.RelationshipPropertiesWithEntityHolder) { Object entity = ((MappingSupport.RelationshipPropertiesWithEntityHolder) relatedValueToStore).getRelatedEntity(); diff --git a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java index db7b48f8a..53820c9ff 100644 --- a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java @@ -447,7 +447,9 @@ public final class ReactiveNeo4jTemplate implements PersistentPropertyAccessor propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved); return idMono.doOnNext(newOrUpdatedNode -> { - var elementId = IdentitySupport.getElementId(newOrUpdatedNode); + var elementId = !entityMetaData.isUsingDeprecatedInternalId() && entityMetaData.isUsingInternalIds() + ? IdentitySupport.getElementId(newOrUpdatedNode) + : newOrUpdatedNode.id(); TemplateSupport.setGeneratedIdIfNecessary(entityMetaData, propertyAccessor, elementId, Optional.of(newOrUpdatedNode)); TemplateSupport.updateVersionPropertyIfPossible(entityMetaData, propertyAccessor, newOrUpdatedNode); finalStateMachine.markEntityAsProcessed(instance, elementId); @@ -942,17 +944,17 @@ public final class ReactiveNeo4jTemplate implements .flatMap(newRelatedObject -> { Neo4jPersistentEntity targetEntity = neo4jMappingContext.getRequiredPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass()); - Mono, AtomicReference>> queryOrSave; + Mono, AtomicReference>> queryOrSave; if (stateMachine.hasProcessedValue(relatedValueToStore)) { - AtomicReference relatedInternalId = new AtomicReference<>(); - String possibleValue = stateMachine.getObjectId(relatedValueToStore); + AtomicReference relatedInternalId = new AtomicReference<>(); + Object possibleValue = stateMachine.getObjectId(relatedValueToStore); if (possibleValue != null) { relatedInternalId.set(possibleValue); } queryOrSave = Mono.just(Tuples.of(relatedInternalId, new AtomicReference<>())); } else { queryOrSave = saveRelatedNode(newRelatedObject, targetEntity, includeProperty, currentPropertyPath) - .map(entity -> Tuples.of(new AtomicReference<>(TemplateSupport.rendererCanUseElementIdIfPresent(renderer) ? entity.elementId() : Long.toString(entity.id())), new AtomicReference<>(entity))) + .map(entity -> Tuples.of(new AtomicReference<>((Object) (TemplateSupport.rendererCanUseElementIdIfPresent(renderer, targetEntity) ? entity.elementId() : entity.id())), new AtomicReference<>(entity))) .doOnNext(t -> { var relatedInternalId = t.getT1().get(); stateMachine.markEntityAsProcessed(relatedValueToStore, relatedInternalId); @@ -963,7 +965,7 @@ public final class ReactiveNeo4jTemplate implements }); } return queryOrSave.flatMap(idAndEntity -> { - String relatedInternalId = idAndEntity.getT1().get(); + Object relatedInternalId = idAndEntity.getT1().get(); Entity savedEntity = idAndEntity.getT2().get(); Neo4jPersistentProperty requiredIdProperty = targetEntity.getRequiredIdProperty(); PersistentPropertyAccessor targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject); diff --git a/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java b/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java index 032093e4e..dbc3550a5 100644 --- a/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java +++ b/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java @@ -204,28 +204,26 @@ public final class TemplateSupport { Statement toStatement(NodeDescription nodeDescription) { - String rootNodeIds = "rootNodeIds"; - String relationshipIds = "relationshipIds"; - String relatedNodeIds = "relatedNodeIds"; - Node rootNodes = Cypher.anyNode(rootNodeIds); - Node relatedNodes = Cypher.anyNode(relatedNodeIds); - Relationship relationships = Cypher.anyNode().relationshipBetween(Cypher.anyNode()).named(relationshipIds); + String primaryLabel = nodeDescription.getPrimaryLabel(); + Node rootNodes = Cypher.node(primaryLabel).named(ROOT_NODE_IDS); + Node relatedNodes = Cypher.anyNode(RELATED_NODE_IDS); + Relationship relationships = Cypher.anyNode().relationshipBetween(Cypher.anyNode()).named(RELATIONSHIP_IDS); return Cypher.match(rootNodes) - .where(Functions.elementId(rootNodes).in(Cypher.parameter(rootNodeIds))) + .where(Functions.elementId(rootNodes).in(Cypher.parameter(ROOT_NODE_IDS))) .with(Functions.collect(rootNodes).as(Constants.NAME_OF_ROOT_NODE)) .optionalMatch(relationships) - .where(Functions.elementId(relationships).in(Cypher.parameter(relationshipIds))) + .where(Functions.elementId(relationships).in(Cypher.parameter(RELATIONSHIP_IDS))) .with(Constants.NAME_OF_ROOT_NODE, Functions.collectDistinct(relationships).as(Constants.NAME_OF_SYNTHESIZED_RELATIONS)) .optionalMatch(relatedNodes) - .where(Functions.elementId(relatedNodes).in(Cypher.parameter(relatedNodeIds))) + .where(Functions.elementId(relatedNodes).in(Cypher.parameter(RELATED_NODE_IDS))) .with( Constants.NAME_OF_ROOT_NODE, Cypher.name(Constants.NAME_OF_SYNTHESIZED_RELATIONS).as(Constants.NAME_OF_SYNTHESIZED_RELATIONS), Functions.collectDistinct(relatedNodes).as(Constants.NAME_OF_SYNTHESIZED_RELATED_NODES) ) - .unwind(Constants.NAME_OF_ROOT_NODE).as(rootNodeIds) + .unwind(Constants.NAME_OF_ROOT_NODE).as(ROOT_NODE_IDS) .with( - Cypher.name(rootNodeIds).as(Constants.NAME_OF_TYPED_ROOT_NODE.apply(nodeDescription).getValue()), + Cypher.name(ROOT_NODE_IDS).as(Constants.NAME_OF_TYPED_ROOT_NODE.apply(nodeDescription).getValue()), Cypher.name(Constants.NAME_OF_SYNTHESIZED_RELATIONS), Cypher.name(Constants.NAME_OF_SYNTHESIZED_RELATED_NODES)) .orderBy(queryFragments.getOrderBy()) @@ -353,7 +351,7 @@ public final class TemplateSupport { static void setGeneratedIdIfNecessary( Neo4jPersistentEntity entityMetaData, PersistentPropertyAccessor propertyAccessor, - String elementId, + Object elementId, Optional databaseEntity ) { if (!entityMetaData.isUsingInternalIds()) { @@ -381,11 +379,11 @@ public final class TemplateSupport { * @param The type of the entity * @return The actual related internal id being used. */ - static String retrieveOrSetRelatedId( + static Object retrieveOrSetRelatedId( Neo4jPersistentEntity entityMetadata, PersistentPropertyAccessor propertyAccessor, Optional databaseEntity, - @Nullable String relatedInternalId + @Nullable Object relatedInternalId ) { if (!entityMetadata.isUsingInternalIds()) { return Objects.requireNonNull(relatedInternalId); @@ -403,7 +401,7 @@ public final class TemplateSupport { } } else { if (relatedInternalId == null && current != null) { - relatedInternalId = (String) current; + relatedInternalId = current; } else if (current == null) { propertyAccessor.setProperty(requiredIdProperty, relatedInternalId); } @@ -415,8 +413,8 @@ public final class TemplateSupport { * Checks if the renderer is configured in such a way that it will use element id or apply toString(id(n)) workaround. * @return {@literal true} if renderer will use elementId */ - static boolean rendererCanUseElementIdIfPresent(Renderer renderer) { - return renderer.render(Cypher.returning(Functions.elementId(Cypher.anyNode("n"))).build()) + static boolean rendererCanUseElementIdIfPresent(Renderer renderer, Neo4jPersistentEntity targetEntity) { + return !targetEntity.isUsingDeprecatedInternalId() && targetEntity.isUsingInternalIds() && renderer.render(Cypher.returning(Functions.elementId(Cypher.anyNode("n"))).build()) .equals("RETURN elementId(n)"); } diff --git a/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java b/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java index 900111edc..9bdb49943 100644 --- a/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java +++ b/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java @@ -430,13 +430,14 @@ public enum CypherGenerator { var startNodeIdFunction = getNodeIdFunction(neo4jPersistentEntity); return match(startNode) .where(startNodeIdFunction.apply(startNode).isEqualTo(idParameter)) - .match(endNode).where(Functions.elementId(endNode).isEqualTo(parameter(Constants.TO_ID_PARAMETER_NAME))) + .match(endNode) + .where(getEndNodeIdFunction((Neo4jPersistentEntity) relationship.getTarget()).apply(endNode).isEqualTo(parameter(Constants.TO_ID_PARAMETER_NAME))) .merge(relationshipFragment) .returning(getReturnedIdExpressionsForRelationship(relationship, relationshipFragment)) .build(); } - private static Function getNodeIdFunction(Neo4jPersistentEntity entity) { + private static Function getNodeIdFunction(@Nullable Neo4jPersistentEntity entity) { Function startNodeIdFunction; var idProperty = entity.getRequiredIdProperty(); @@ -452,6 +453,20 @@ public enum CypherGenerator { return startNodeIdFunction; } + private static Function getEndNodeIdFunction(@Nullable Neo4jPersistentEntity entity) { + + Function startNodeIdFunction; + if (entity == null) { + return Functions::elementId; + } + if (!entity.isUsingDeprecatedInternalId() && entity.isUsingInternalIds()) { + startNodeIdFunction = Functions::elementId; + } else { + startNodeIdFunction = Functions::id; + } + return startNodeIdFunction; + } + private static Function getRelationshipIdFunction(RelationshipDescription relationshipDescription) { Function result = Functions::elementId; @@ -488,7 +503,8 @@ public enum CypherGenerator { .with(row) .match(startNode) .where(getNodeIdFunction(neo4jPersistentEntity).apply(startNode).isEqualTo(idProperty)) - .match(endNode).where(Functions.elementId(endNode).isEqualTo(Cypher.property(row, Constants.TO_ID_PARAMETER_NAME))) + .match(endNode) + .where(getEndNodeIdFunction((Neo4jPersistentEntity) relationship.getTarget()).apply(endNode).isEqualTo(Cypher.property(row, Constants.TO_ID_PARAMETER_NAME))) .merge(relationshipFragment) .returning(getReturnedIdExpressionsForRelationship(relationship, relationshipFragment)) .build(); @@ -521,7 +537,8 @@ public enum CypherGenerator { StatementBuilder.OngoingReadingWithWhere startAndEndNodeMatch = match(startNode) .where(nodeIdFunction.apply(startNode).isEqualTo(idParameter)) - .match(endNode).where(Functions.elementId(endNode).isEqualTo(parameter(Constants.TO_ID_PARAMETER_NAME))); + .match(endNode) + .where(getEndNodeIdFunction((Neo4jPersistentEntity) relationship.getTarget()).apply(endNode).isEqualTo(parameter(Constants.TO_ID_PARAMETER_NAME))); StatementBuilder.ExposesSet createOrMatch = isNew ? startAndEndNodeMatch.create(relationshipFragment) @@ -567,7 +584,7 @@ public enum CypherGenerator { .match(startNode) .where(nodeIdFunction.apply(startNode).isEqualTo(idProperty)) .match(endNode) - .where(endNode.elementId().isEqualTo(Cypher.property(row, Constants.TO_ID_PARAMETER_NAME))) + .where(getEndNodeIdFunction((Neo4jPersistentEntity) relationship.getTarget()).apply(endNode).isEqualTo(Cypher.property(row, Constants.TO_ID_PARAMETER_NAME))) .create(relationshipFragment) .mutate(RELATIONSHIP_NAME, relationshipProperties) .returning(getReturnedIdExpressionsForRelationship(relationship, relationshipFragment)) diff --git a/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java b/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java index 9cefed810..f77c96d15 100644 --- a/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java +++ b/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java @@ -64,7 +64,7 @@ public final class NestedRelationshipProcessingStateMachine { * A map pointing from a processed object to the internal id. * This will be useful during the persistence to avoid another DB network round-trip. */ - private final Map processedObjectsIds = new HashMap<>(); + private final Map processedObjectsIds = new HashMap<>(); public NestedRelationshipProcessingStateMachine(final Neo4jMappingContext mappingContext) { @@ -73,7 +73,7 @@ public final class NestedRelationshipProcessingStateMachine { this.mappingContext = mappingContext; } - public NestedRelationshipProcessingStateMachine(final Neo4jMappingContext mappingContext, Object initialObject, String elementId) { + public NestedRelationshipProcessingStateMachine(final Neo4jMappingContext mappingContext, Object initialObject, Object elementId) { this(mappingContext); Assert.notNull(initialObject, "Initial object must not be null"); @@ -143,7 +143,7 @@ public final class NestedRelationshipProcessingStateMachine { * @param valueToStore If not {@literal null}, all non-null values will be marked as processed * @param elementId The internal id of the value processed */ - public void markEntityAsProcessed(Object valueToStore, String elementId) { + public void markEntityAsProcessed(Object valueToStore, Object elementId) { final long stamp = lock.writeLock(); try { @@ -154,7 +154,7 @@ public final class NestedRelationshipProcessingStateMachine { } } - private void doMarkValueAsProcessed(Object valueToStore, String elementId) { + private void doMarkValueAsProcessed(Object valueToStore, Object elementId) { Object value = extractRelatedValueFromRelationshipProperties(valueToStore); storeHashedVersionInProcessedObjectsIds(valueToStore, elementId); @@ -191,7 +191,7 @@ public final class NestedRelationshipProcessingStateMachine { .findAny(); if (alreadyProcessedObject.isPresent()) { // Skip the show the next time around. processed = true; - String internalId = getObjectId(alreadyProcessedObject.get()); + Object internalId = getObjectId(alreadyProcessedObject.get()); if (internalId != null) { stamp = lock.tryConvertToWriteLock(stamp); doMarkValueAsProcessed(valueToCheck, internalId); @@ -239,11 +239,11 @@ public final class NestedRelationshipProcessingStateMachine { * @return The objects id */ @Nullable - public String getObjectId(Object object) { + public Object getObjectId(Object object) { final long stamp = lock.readLock(); try { Object valueToCheck = extractRelatedValueFromRelationshipProperties(object); - String possibleId = getProcessedObjectIds(valueToCheck); + Object possibleId = getProcessedObjectIds(valueToCheck); return possibleId != null ? possibleId : getProcessedObjectIds(getProcessedAs(valueToCheck)); } finally { lock.unlock(stamp); @@ -261,7 +261,7 @@ public final class NestedRelationshipProcessingStateMachine { } @Nullable - private String getProcessedObjectIds(@Nullable Object entity) { + private Object getProcessedObjectIds(@Nullable Object entity) { if (entity == null) { return null; } @@ -282,7 +282,7 @@ public final class NestedRelationshipProcessingStateMachine { /* * Convenience wrapper functions to avoid exposing the System.identityHashCode "everywhere" in this class. */ - private void storeHashedVersionInProcessedObjectsIds(Object initialObject, String elementId) { + private void storeHashedVersionInProcessedObjectsIds(Object initialObject, Object elementId) { processedObjectsIds.put(System.identityHashCode(initialObject), elementId); }