From b3e163160e00c0768278385f5ded9bf561e4bfa2 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Tue, 21 Jan 2025 14:09:06 +0100 Subject: [PATCH] fix: Proper conjunct (nested) filters on relationship queries. Fixes #1218. Signed-off-by: Michael Simons --- .../session/request/FilteredQueryBuilder.java | 45 ++++++---- .../ogm/cypher/FilterIntegrationTest.java | 23 +++++ .../java/org/neo4j/ogm/domain/music/Node.java | 55 ++++++++++++ .../ogm/domain/music/RelBetweenNodes.java | 69 +++++++++++++++ .../impl/RelationshipQueryStatementsTest.java | 87 ++++++++++--------- 5 files changed, 217 insertions(+), 62 deletions(-) create mode 100644 neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/Node.java create mode 100644 neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/RelBetweenNodes.java diff --git a/core/src/main/java/org/neo4j/ogm/session/request/FilteredQueryBuilder.java b/core/src/main/java/org/neo4j/ogm/session/request/FilteredQueryBuilder.java index 346ba7017..2a4c71bef 100644 --- a/core/src/main/java/org/neo4j/ogm/session/request/FilteredQueryBuilder.java +++ b/core/src/main/java/org/neo4j/ogm/session/request/FilteredQueryBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2002-2022 "Neo4j," + * Copyright (c) 2002-2025 "Neo4j," * Neo4j Sweden AB [http://neo4j.com] * * This file is part of Neo4j. @@ -185,7 +185,7 @@ private static StringBuilder constructRelationshipQuery(String type, Iterable properties, - List relationshipFilters, StringBuilder query, String initialDirection) { + FiltersAtStartNode filtersAtStartNode, FiltersAtStartNode filtersAtEndNode, List relationshipFilters, + StringBuilder query, String initialDirection) { + String startLabel = filtersAtStartNode.startNodeLabel == null ? "" : String.format(":`%s`", filtersAtStartNode.startNodeLabel); + String endLabel = filtersAtEndNode.startNodeLabel == null ? "" : String.format(":`%s`", filtersAtEndNode.startNodeLabel); if (initialDirection == null || initialDirection.equals(Relationship.OUTGOING)) { - query.append(String.format("MATCH (n)-[r0:`%s`]->(m) ", type)); + query.append(String.format("MATCH (n%s)-[r0:`%s`]->(m%s) ", startLabel, type, endLabel)); } else { - query.append(String.format("MATCH (n)<-[r0:`%s`]-(m) ", type)); + query.append(String.format("MATCH (n%s)<-[r0:`%s`]-(m%s) ", startLabel, type, endLabel)); } - if (relationshipFilters.size() > 0) { + + boolean hasFiltersAtStart = !filtersAtStartNode.content.isEmpty(); + boolean hasFiltersAtEnd = !filtersAtEndNode.content.isEmpty(); + if (!relationshipFilters.isEmpty() || hasFiltersAtStart || hasFiltersAtEnd) { query.append("WHERE "); + if (!filtersAtStartNode.content.isEmpty() || !filtersAtEndNode.content.isEmpty()) { + query.append("( "); + appendFilters(filtersAtStartNode.content, "n", query, properties); + if (hasFiltersAtStart && hasFiltersAtEnd && filtersAtEndNode.content.get(0).getBooleanOperator() == BooleanOperator.NONE) { + query.append("AND "); + } + appendFilters(filtersAtEndNode.content, "m", query, properties); + query.append(")"); + if (!relationshipFilters.isEmpty()) { + query.append(" AND "); + } + } appendFilters(relationshipFilters, "r0", query, properties); } } - private static void createNodeMatchSubquery(Map properties, FiltersAtStartNode filtersAtStartNode, - StringBuilder query, String nodeIdentifier) { - String nodeLabel = filtersAtStartNode.startNodeLabel; - List nodeFilters = filtersAtStartNode.content; - if (!(nodeLabel == null || nodeFilters.isEmpty())) { - query.append(String.format("MATCH (%s:`%s`) WHERE ", nodeIdentifier, nodeLabel)); - appendFilters(nodeFilters, nodeIdentifier, query, properties); - } - } - private static void appendFilters(List filters, String nodeIdentifier, StringBuilder query, Map properties) { for (Filter filter : filters) { diff --git a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/cypher/FilterIntegrationTest.java b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/cypher/FilterIntegrationTest.java index e63ec7cb0..d2782a88c 100644 --- a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/cypher/FilterIntegrationTest.java +++ b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/cypher/FilterIntegrationTest.java @@ -21,11 +21,15 @@ import static org.assertj.core.api.Assertions.*; import java.io.IOException; +import java.util.Collections; +import org.assertj.core.api.Assertions; import org.junit.After; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; +import org.neo4j.ogm.domain.music.Node; +import org.neo4j.ogm.domain.music.RelBetweenNodes; import org.neo4j.ogm.domain.music.Studio; import org.neo4j.ogm.session.Session; import org.neo4j.ogm.session.SessionFactory; @@ -100,4 +104,23 @@ public void ignoreCaseShouldBeApplicableToEndingWith() { .extracting(Studio::getName) .containsExactly(emi); } + + @Test // GH-1218 + public void nestedFiltersOnRelationshipMustBeProperlyConjucted() { + + Filters filters = new Filters(Collections.emptyList()); + Filter filter1 = new Filter("id", ComparisonOperator.EQUALS, "my_id"); + filter1.setNestedPropertyName("start"); + filter1.setNestedPropertyType(Node.class); + Filter filter2 = new Filter("id", ComparisonOperator.EQUALS, "my_id"); + filter2.setNestedPropertyName("end"); + filter2.setNestedPropertyType(Node.class); + filters.add(filter1); + filters.or(filter2); + try { + session.loadAll(RelBetweenNodes.class, filters); + } catch (Exception e) { + Assertions.fail("Filtered query did not work."); + } + } } diff --git a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/Node.java b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/Node.java new file mode 100644 index 000000000..7c6e2f173 --- /dev/null +++ b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/Node.java @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2002-2025 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.ogm.domain.music; + +import java.util.HashSet; +import java.util.Set; + +import org.neo4j.ogm.annotation.Id; +import org.neo4j.ogm.annotation.NodeEntity; +import org.neo4j.ogm.annotation.Relationship; + +/** + * GH-1218 + */ +@NodeEntity +public class Node { + + @Id + String id; + + @Relationship(type = "relationship") + public Set relationships = new HashSet<>(); + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } + + public Set getRelationships() { + return relationships; + } + + public void setRelationships(Set relationships) { + this.relationships = relationships; + } +} diff --git a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/RelBetweenNodes.java b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/RelBetweenNodes.java new file mode 100644 index 000000000..372b41325 --- /dev/null +++ b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/music/RelBetweenNodes.java @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2002-2025 "Neo4j," + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.neo4j.ogm.domain.music; + +import org.neo4j.ogm.annotation.EndNode; +import org.neo4j.ogm.annotation.GeneratedValue; +import org.neo4j.ogm.annotation.Id; +import org.neo4j.ogm.annotation.NodeEntity; +import org.neo4j.ogm.annotation.Relationship; +import org.neo4j.ogm.annotation.RelationshipEntity; +import org.neo4j.ogm.annotation.StartNode; +import org.neo4j.ogm.domain.drink.Manufacturer; +import org.neo4j.ogm.id.UuidStrategy; + +/** + * GH-1218 + */ +@RelationshipEntity(type = "relationship") +public class RelBetweenNodes { + + @Id @GeneratedValue + private Long id; + + @StartNode + private Node start; + + @EndNode + private Node end; + + public Node getEnd() { + return end; + } + + public void setEnd(Node end) { + this.end = end; + } + + public Long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public Node getStart() { + return start; + } + + public void setStart(Node start) { + this.start = start; + } +} diff --git a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/session/request/strategy/impl/RelationshipQueryStatementsTest.java b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/session/request/strategy/impl/RelationshipQueryStatementsTest.java index 3c31aa3fe..e83c345d8 100644 --- a/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/session/request/strategy/impl/RelationshipQueryStatementsTest.java +++ b/neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/session/request/strategy/impl/RelationshipQueryStatementsTest.java @@ -23,6 +23,7 @@ import static org.neo4j.ogm.cypher.ComparisonOperator.*; import java.util.Arrays; +import java.util.List; import org.junit.Test; import org.neo4j.ogm.cypher.BooleanOperator; @@ -30,6 +31,8 @@ import org.neo4j.ogm.cypher.Filter; import org.neo4j.ogm.cypher.Filters; import org.neo4j.ogm.cypher.query.PagingAndSortingQuery; +import org.neo4j.ogm.domain.music.Node; +import org.neo4j.ogm.domain.music.RelBetweenNodes; import org.neo4j.ogm.exception.core.InvalidDepthException; import org.neo4j.ogm.exception.core.MissingOperatorException; import org.neo4j.ogm.session.request.strategy.QueryStatements; @@ -173,12 +176,12 @@ public void testFindByNestedPropertyOutgoing() { planetFilter.setRelationshipType("ORBITS"); planetFilter.setRelationshipDirection("OUTGOING"); assertThat(query.findByType("ORBITS", new Filters().add(planetFilter), 4).getStatement()) - .isEqualTo("MATCH (n:`Planet`) WHERE n.`name` = $`world_name_0` " + - "MATCH (n)-[r0:`ORBITS`]->(m) WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + - "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + - "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths " + - "UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + .isEqualTo( + "MATCH (n:`Planet`)-[r0:`ORBITS`]->(m) WHERE ( n.`name` = $`world_name_0` ) " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + + "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -189,11 +192,12 @@ public void testFindByNestedPropertyIncoming() { planetFilter.setRelationshipType("ORBITS"); planetFilter.setRelationshipDirection("INCOMING"); assertThat(query.findByType("ORBITS", new Filters().add(planetFilter), 4).getStatement()) - .isEqualTo("MATCH (m:`Planet`) WHERE m.`name` = $`world_name_0` MATCH (n)<-[r0:`ORBITS`]-(m) " + - "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + - "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + - "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + .isEqualTo( + "MATCH (n)<-[r0:`ORBITS`]-(m:`Planet`) WHERE ( m.`name` = $`world_name_0` ) " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + + "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -212,12 +216,12 @@ public void testFindByMultipleNestedProperties() { planetMoonsFilter.setBooleanOperator(BooleanOperator.AND); assertThat(query.findByType("ORBITS", new Filters().add(planetNameFilter, planetMoonsFilter), 4).getStatement()) - .isEqualTo("MATCH (n:`Planet`) WHERE n.`name` = $`world_name_0` AND n.`moons` = $`moons_moons_1` " + - "MATCH (n)-[r0:`ORBITS`]->(m) WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + - "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + - "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths " + - "UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + .isEqualTo( + "MATCH (n:`Planet`)-[r0:`ORBITS`]->(m) WHERE ( n.`name` = $`world_name_0` AND n.`moons` = $`moons_moons_1` ) " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + + "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -236,12 +240,11 @@ public void testFindByMultipleNestedPropertiesOnBothEnds() { planetFilter.setRelationshipDirection("INCOMING"); assertThat(query.findByType("ORBITS", new Filters().add(moonFilter, planetFilter), 4).getStatement()).isEqualTo( - "MATCH (n:`Moon`) WHERE n.`name` = $`world_name_0` MATCH (m:`Planet`) WHERE m.`colour` = $`colour_colour_1` " - + - "MATCH (n)-[r0:`ORBITS`]->(m) WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + - "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + - "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + "MATCH (n:`Moon`)-[r0:`ORBITS`]->(m:`Planet`) WHERE ( n.`name` = $`world_name_0` AND m.`colour` = $`colour_colour_1` ) " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + + "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -306,12 +309,12 @@ public void testFindByBaseAndNestedPropertyOutgoing() { Filter time = new Filter("time", ComparisonOperator.EQUALS, 3600); time.setBooleanOperator(BooleanOperator.AND); assertThat(query.findByType("ORBITS", new Filters().add(planetFilter, time), 4).getStatement()) - .isEqualTo("MATCH (n:`Planet`) WHERE n.`name` = $`world_name_0` " + - "MATCH (n)-[r0:`ORBITS`]->(m) WHERE r0.`time` = $`time_1` " + - "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + - "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m MATCH p2 = (m)-[*0..4]-() " + - "WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + .isEqualTo( + "MATCH (n:`Planet`)-[r0:`ORBITS`]->(m) WHERE ( n.`name` = $`world_name_0` ) AND r0.`time` = $`time_1` " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + + "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -323,12 +326,13 @@ public void testFindByBaseAndNestedPropertyIncoming() { planetFilter.setRelationshipDirection("INCOMING"); Filter time = new Filter("time", ComparisonOperator.EQUALS, 3600); assertThat(query.findByType("ORBITS", new Filters().add(planetFilter, time), 4).getStatement()) - .isEqualTo("MATCH (m:`Planet`) WHERE m.`name` = $`world_name_0` " + - "MATCH (n)<-[r0:`ORBITS`]-(m) WHERE r0.`time` = $`time_1` " + - "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + - "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m MATCH p2 = (m)-[*0..4]-() " + - "WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + - "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + .isEqualTo( + "MATCH (n)<-[r0:`ORBITS`]-(m:`Planet`) WHERE ( m.`name` = $`world_name_0` ) AND r0.`time` = $`time_1` " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + + "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, " + + "COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test // DATAGRAPH-632 @@ -350,14 +354,11 @@ public void testFindByBaseAndMultipleNestedPropertiesOnBothEnds() { assertThat(query.findByType("ORBITS", new Filters().add(moonFilter, planetFilter, time), 4).getStatement()) .isEqualTo( - "MATCH (n:`Moon`) WHERE n.`name` = $`world_name_0` MATCH (m:`Planet`) WHERE m.`colour` = $`colour_colour_1` " - + - "MATCH (n)-[r0:`ORBITS`]->(m) WHERE r0.`time` = $`time_2` " + - "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m MATCH p1 = (n)-[*0..4]-() " + - "WITH r0, COLLECT(DISTINCT p1) AS startPaths, m MATCH p2 = (m)-[*0..4]-() " + - "WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths WITH r0,startPaths + endPaths AS paths " - + - "UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); + "MATCH (n:`Moon`)-[r0:`ORBITS`]->(m:`Planet`) WHERE ( n.`name` = $`world_name_0` AND m.`colour` = $`colour_colour_1` ) AND r0.`time` = $`time_2` " + + "WITH DISTINCT(r0) as r0,startnode(r0) AS n, endnode(r0) AS m " + + "MATCH p1 = (n)-[*0..4]-() WITH r0, COLLECT(DISTINCT p1) AS startPaths, m " + + "MATCH p2 = (m)-[*0..4]-() WITH r0, startPaths, COLLECT(DISTINCT p2) AS endPaths " + + "WITH r0,startPaths + endPaths AS paths UNWIND paths AS p RETURN DISTINCT p, ID(r0)"); } @Test(expected = MissingOperatorException.class) // GH-73