From 4aa58754e318aa264f1d8effcc761df7ad243e97 Mon Sep 17 00:00:00 2001 From: Corey Beres Date: Mon, 30 May 2022 04:06:57 -0400 Subject: [PATCH] GH-2544 - Check for empty collections in `saveAllAs` methods. When an empty collection is passed to imperative and reactive `saveAllAs` variants a common element type can't be computed. We did not check for this scenario correctly and failed with a NPE. The introduction of a marker class fixes #2544 Co-authored-by: Michael Simons --- .../data/neo4j/core/Neo4jTemplate.java | 9 ++++++++ .../neo4j/core/ReactiveNeo4jTemplate.java | 19 +++++++++++---- .../data/neo4j/core/TemplateSupport.java | 12 ++++++++++ .../data/neo4j/core/TemplateSupportTest.java | 2 +- .../imperative/Neo4jTemplateIT.java | 21 +++++++++++++++++ .../reactive/ReactiveNeo4jTemplateIT.java | 23 ++++++++++++++++++- 6 files changed, 79 insertions(+), 7 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 636c6f0cc..e030b4cf0 100644 --- a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java @@ -99,6 +99,7 @@ import org.springframework.util.Assert; * @author Michael J. Simons * @author Philipp Tölle * @author Gerrit Meier + * @author Corey Beres * @soundtrack Motörhead - We Are Motörhead * @since 6.0 */ @@ -522,6 +523,14 @@ public final class Neo4jTemplate implements Class commonElementType = TemplateSupport.findCommonElementType(instances); + if (commonElementType == null) { + throw new IllegalArgumentException("Could not determine a common element of an heterogeneous collection."); + } + + if (commonElementType == TemplateSupport.EmptyIterable.class) { + return Collections.emptyList(); + } + if (resultType.isAssignableFrom(commonElementType)) { @SuppressWarnings("unchecked") // Nicer to live with this than streaming, mapping and collecting to avoid the cast. It's easier on the reactive side. List saveElements = (List) saveAll(instances); 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 b803cc0c8..4af1018d3 100644 --- a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java @@ -19,11 +19,6 @@ import static org.neo4j.cypherdsl.core.Cypher.anyNode; import static org.neo4j.cypherdsl.core.Cypher.asterisk; import static org.neo4j.cypherdsl.core.Cypher.parameter; -import org.neo4j.cypherdsl.core.renderer.Configuration; -import org.springframework.data.mapping.Association; -import org.springframework.data.neo4j.core.TemplateSupport.FilteredBinderFunction; -import org.springframework.data.neo4j.core.mapping.AssociationHandlerSupport; -import org.springframework.data.neo4j.core.schema.TargetNode; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; @@ -54,6 +49,7 @@ import org.neo4j.cypherdsl.core.Cypher; import org.neo4j.cypherdsl.core.Functions; import org.neo4j.cypherdsl.core.Node; import org.neo4j.cypherdsl.core.Statement; +import org.neo4j.cypherdsl.core.renderer.Configuration; import org.neo4j.cypherdsl.core.renderer.Renderer; import org.neo4j.driver.Value; import org.neo4j.driver.types.Entity; @@ -67,10 +63,13 @@ import org.springframework.beans.factory.BeanFactoryAware; import org.springframework.core.log.LogAccessor; import org.springframework.dao.IncorrectResultSizeDataAccessException; import org.springframework.dao.OptimisticLockingFailureException; +import org.springframework.data.mapping.Association; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.PropertyPath; import org.springframework.data.mapping.callback.ReactiveEntityCallbacks; +import org.springframework.data.neo4j.core.TemplateSupport.FilteredBinderFunction; import org.springframework.data.neo4j.core.TemplateSupport.NodesAndRelationshipsByIdStatementProvider; +import org.springframework.data.neo4j.core.mapping.AssociationHandlerSupport; import org.springframework.data.neo4j.core.mapping.Constants; import org.springframework.data.neo4j.core.mapping.CreateRelationshipStatementHolder; import org.springframework.data.neo4j.core.mapping.CypherGenerator; @@ -88,6 +87,7 @@ import org.springframework.data.neo4j.core.mapping.NodeDescription; import org.springframework.data.neo4j.core.mapping.PropertyFilter; import org.springframework.data.neo4j.core.mapping.RelationshipDescription; import org.springframework.data.neo4j.core.mapping.callback.ReactiveEventSupport; +import org.springframework.data.neo4j.core.schema.TargetNode; import org.springframework.data.neo4j.repository.query.QueryFragments; import org.springframework.data.neo4j.repository.query.QueryFragmentsAndParameters; import org.springframework.data.projection.ProjectionFactory; @@ -478,6 +478,15 @@ public final class ReactiveNeo4jTemplate implements Class commonElementType = TemplateSupport.findCommonElementType(instances); + if (commonElementType == null) { + return Flux.error(() -> new IllegalArgumentException( + "Could not determine a common element of an heterogeneous collection.")); + } + + if (commonElementType == TemplateSupport.EmptyIterable.class) { + return Flux.empty(); + } + if (resultType.isAssignableFrom(commonElementType)) { return saveAll(instances).map(resultType::cast); } 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 56b152711..b05d6b1aa 100644 --- a/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java +++ b/src/main/java/org/springframework/data/neo4j/core/TemplateSupport.java @@ -64,6 +64,14 @@ import org.springframework.util.Assert; @API(status = API.Status.INTERNAL, since = "6.0.9") public final class TemplateSupport { + /** + * Indicator for an empty collection + */ + public static final class EmptyIterable { + private EmptyIterable() { + } + } + enum FetchType { ONE, @@ -81,6 +89,10 @@ public final class TemplateSupport { .filter(o -> o != null) .map(Object::getClass).collect(Collectors.toSet()); + if (allClasses.isEmpty()) { + return EmptyIterable.class; + } + Class candidate = null; for (Class type : allClasses) { if (candidate == null) { diff --git a/src/test/java/org/springframework/data/neo4j/core/TemplateSupportTest.java b/src/test/java/org/springframework/data/neo4j/core/TemplateSupportTest.java index e9ad3466e..b41ecf964 100644 --- a/src/test/java/org/springframework/data/neo4j/core/TemplateSupportTest.java +++ b/src/test/java/org/springframework/data/neo4j/core/TemplateSupportTest.java @@ -88,7 +88,7 @@ class TemplateSupportTest { void shouldNotFailWithEmptyInput() { Class type = TemplateSupport.findCommonElementType(Collections.emptyList()); - assertThat(type).isNull(); + assertThat(type).isEqualTo(TemplateSupport.EmptyIterable.class); } @Test diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/Neo4jTemplateIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/Neo4jTemplateIT.java index 2980dc368..48fa04130 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/imperative/Neo4jTemplateIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/imperative/Neo4jTemplateIT.java @@ -17,9 +17,11 @@ package org.springframework.data.neo4j.integration.imperative; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import lombok.Data; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -76,6 +78,7 @@ import org.springframework.transaction.annotation.EnableTransactionManagement; * @author Gerrit Meier * @author Michael J. Simons * @author Rosetta Roberts + * @author Corey Beres */ @Neo4jIntegrationTest class Neo4jTemplateIT { @@ -795,6 +798,24 @@ class Neo4jTemplateIT { assertThat(people).allMatch(p -> p.getAddress() != null); } + @Test // GH-2544 + void saveAllAsWithEmptyList() { + List projections = neo4jTemplate.saveAllAs(Collections.emptyList(), ClosedProjection.class); + + assertThat(projections).isEmpty(); + } + + @Test // GH-2544 + void saveWeirdHierarchy() { + + List things = new ArrayList<>(); + things.add(1); + things.add("eins"); + + assertThatIllegalArgumentException().isThrownBy(() -> neo4jTemplate.saveAllAs(things, ClosedProjection.class)) + .withMessage("Could not determine a common element of an heterogeneous collection."); + } + @Test void updatingFindShouldWork() { Map params = new HashMap<>(); diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveNeo4jTemplateIT.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveNeo4jTemplateIT.java index 26c138dbd..7a7a456de 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveNeo4jTemplateIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveNeo4jTemplateIT.java @@ -19,10 +19,10 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.neo4j.cypherdsl.core.Cypher.parameter; import lombok.Data; -import org.springframework.data.neo4j.test.Neo4jReactiveTestConfiguration; import reactor.core.publisher.Flux; import reactor.test.StepVerifier; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -71,6 +71,7 @@ import org.springframework.data.neo4j.test.BookmarkCapture; import org.springframework.data.neo4j.test.Neo4jExtension; import org.springframework.data.neo4j.test.Neo4jExtension.Neo4jConnectionSupport; import org.springframework.data.neo4j.test.Neo4jIntegrationTest; +import org.springframework.data.neo4j.test.Neo4jReactiveTestConfiguration; import org.springframework.transaction.ReactiveTransactionManager; import org.springframework.transaction.annotation.EnableTransactionManagement; @@ -688,6 +689,26 @@ class ReactiveNeo4jTemplateIT { .verifyComplete(); } + @Test // GH-2544 + void saveAllAsWithEmptyList(@Autowired ReactiveNeo4jTemplate template) { + + template.saveAllAs(Collections.emptyList(), ClosedProjection.class) + .as(StepVerifier::create) + .verifyComplete(); + } + + @Test // GH-2544 + void saveWeirdHierarchy(@Autowired ReactiveNeo4jTemplate template) { + + List things = new ArrayList<>(); + things.add(1); + things.add("eins"); + + template.saveAllAs(things, ClosedProjection.class) + .as(StepVerifier::create) + .verifyErrorMatches(t -> t instanceof IllegalArgumentException && t.getMessage().equals("Could not determine a common element of an heterogeneous collection.")); + } + @Test void saveAllAsWithClosedProjectionShouldWork(@Autowired ReactiveNeo4jTemplate template) {