GH-799 - Prevent OR’ing of filters containing nested paths.

A filter with nested paths will result in several matches chained together. Those matches are nessary to have the nodes holding the properties we want to filter on.

While at first it may seem trivial to think that adding a nested filter with its own match by an OR just doing an OPTIONAL MATCH, this is wrong, as the first part of the OR would need to be marked as OPTIONAL as well.

In the end trying to do so, would create very brittle Cypher statements.

Instead of, this commit improves the current checks into a single place and adds a couple of tests for it.

core/src/main/java/org/neo4j/ogm/session/request/FilteredQueryBuilder.java

@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.StreamSupport;
 
 import org.neo4j.ogm.annotation.Relationship;
 import org.neo4j.ogm.cypher.BooleanOperator;
@@ -37,6 +38,26 @@
  */
 public class FilteredQueryBuilder {
 
+    private static void validateNestedFilters(Iterable<Filter> filters) {
+
+        // Doesn't matter if the one filter has an or or not.
+        long count = StreamSupport.stream(filters.spliterator(), false).count();
+        if (count < 2) {
+            return;
+        }
+
+        boolean orIsPresent = StreamSupport.stream(filters.spliterator(), false)
+            .anyMatch(f -> BooleanOperator.OR == f.getBooleanOperator());
+        boolean nestedOrDeepNestedFilterPresent = StreamSupport.stream(filters.spliterator(), false)
+            .anyMatch(f -> f.isNested() || f.isDeepNested());
+
+        if (orIsPresent && nestedOrDeepNestedFilterPresent) {
+            throw new UnsupportedOperationException(
+                "Filters containing nested paths cannot be combined via the logical OR operator.");
+        }
+
+    }
+
     /**
      * Create a {@link FilteredQuery} which matches nodes filtered by one or more property expressions
      *
@@ -45,6 +66,8 @@ public class FilteredQueryBuilder {
      * @return a {@link FilteredQuery} whose statement() method contains the appropriate Cypher
      */
     public static FilteredQuery buildNodeQuery(String nodeLabel, Iterable<Filter> filterList) {
+
+        validateNestedFilters(filterList);
         return new NodeQueryBuilder(nodeLabel, filterList).build();
     }
 
@@ -56,6 +79,9 @@ public static FilteredQuery buildNodeQuery(String nodeLabel, Iterable<Filter> fi
      * @return a {@link FilteredQuery} whose statement() method contains the appropriate Cypher
      */
     public static FilteredQuery buildRelationshipQuery(String relationshipType, Iterable<Filter> filterList) {
+
+        validateNestedFilters(filterList);
+
         Map<String, Object> properties = new HashMap<>();
         StringBuilder sb = constructRelationshipQuery(relationshipType, filterList, properties);
         return new FilteredQuery(sb, properties);
@@ -73,10 +99,6 @@ private static StringBuilder constructRelationshipQuery(String type, Iterable<Fi
 
         for (Filter filter : filters) {
             if (filter.isNested() || filter.isDeepNested()) {
-                if (filter.getBooleanOperator().equals(BooleanOperator.OR)) {
-                    throw new UnsupportedOperationException(
-                        "OR is not supported for nested properties on a relationship entity");
-                }
                 String startNestedEntityTypeLabel = filter.getNestedEntityTypeLabel();
                 String endNestedEntityTypeLabel = filter.getNestedEntityTypeLabel();
                 String relationshipDirection = filter.getRelationshipDirection();

core/src/main/java/org/neo4j/ogm/session/request/NodeQueryBuilder.java

@@ -29,6 +29,7 @@
 
 /**
  * @author Jasper Blues
+ * @author Michael J. Simons
  */
 public class NodeQueryBuilder {
 
@@ -78,9 +79,6 @@ public FilteredQuery build() {
     }
 
     private void appendNestedFilter(Filter filter) {
-        if (filter.getBooleanOperator().equals(BooleanOperator.OR)) {
-            throw new UnsupportedOperationException("OR is not supported for nested properties on an entity");
-        }
         if (filter.isNestedRelationshipEntity()) {
             MatchClause clause = relationshipPropertyClauseFor(filter.getRelationshipType());
             if (clause == null) {
@@ -101,11 +99,9 @@ private void appendNestedFilter(Filter filter) {
     }
 
     private void appendDeepNestedFilter(Filter filter) {
-        if (filter.getBooleanOperator().equals(BooleanOperator.OR)) {
-            throw new UnsupportedOperationException("OR is not supported for nested properties on an entity");
-        }
         Filter.NestedPathSegment lastPathSegment = filter.getNestedPath().get(filter.getNestedPath().size() - 1);
-        MatchClause clause = new NestedPropertyPathMatchClause(matchClauseId, lastPathSegment.getNestedEntityTypeLabel(), lastPathSegment.isNestedRelationshipEntity());
+        MatchClause clause = new NestedPropertyPathMatchClause(matchClauseId,
+            lastPathSegment.getNestedEntityTypeLabel(), lastPathSegment.isNestedRelationshipEntity());
 
         pathClauses.add(new NestedPathMatchClause(matchClauseId).append(filter));
         nestedClauses.add(clause);

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/persistence/examples/cineasts/annotated/CineastsIntegrationTest.java

@@ -26,14 +26,19 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Map;
+import java.util.function.UnaryOperator;
 
 import org.junit.After;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.neo4j.ogm.cypher.BooleanOperator;
 import org.neo4j.ogm.cypher.ComparisonOperator;
 import org.neo4j.ogm.cypher.Filter;
 import org.neo4j.ogm.cypher.Filters;
+import org.neo4j.ogm.cypher.PropertyValueTransformer;
+import org.neo4j.ogm.cypher.function.FilterFunction;
 import org.neo4j.ogm.domain.cineasts.annotated.Actor;
 import org.neo4j.ogm.domain.cineasts.annotated.Movie;
 import org.neo4j.ogm.domain.cineasts.annotated.Pet;
@@ -44,7 +49,6 @@
 import org.neo4j.ogm.domain.cineasts.annotated.User;
 import org.neo4j.ogm.session.Session;
 import org.neo4j.ogm.session.SessionFactory;
-import org.neo4j.ogm.session.Utils;
 import org.neo4j.ogm.testutil.TestContainersTestBase;
 import org.neo4j.ogm.testutil.TestUtils;
 
@@ -54,6 +58,7 @@
  * @author Michal Bachman
  * @author Adam George
  * @author Mark Angrish
+ * @author Michael J. Simons
  */
 public class CineastsIntegrationTest extends TestContainersTestBase {
 
@@ -214,8 +219,8 @@ public void loadRatingByUserName() {
 
         Collection<Rating> ratings = session.loadAll(Rating.class, userNameFilter.and(ratingFilter));
         assertThat(ratings).hasSize(1);
-
     }
+
     @Test
     public void loadRatingByUserNameAndStars() {
         Filter userNameFilter = new Filter("name", ComparisonOperator.EQUALS, "Michal");
@@ -261,10 +266,7 @@ public void saveAndRetrieveUserWithTitles() {
         assertThat(vince.getTitles().get(0)).isEqualTo(Title.MR);
     }
 
-    /**
-     * @see DATAGRAPH-614
-     */
-    @Test
+    @Test // DATAGRAPH-614
     public void saveAndRetrieveUserWithDifferentCharset() {
         User user = new User();
         user.setLogin("aki");
@@ -350,11 +352,7 @@ public void shouldModifyStringArraysCorrectly() throws MalformedURLException {
         assertThat(user.getNicknames()[1]).isEqualTo("robin");
     }
 
-    /**
-     * @throws MalformedURLException
-     * @see Issue #128
-     */
-    @Test
+    @Test // GH-128
     public void shouldBeAbleToSetNodePropertiesToNull() throws MalformedURLException {
         Movie movie = new Movie("Zootopia", 2016);
         movie.setImdbUrl(new URL("http://www.imdb.com/title/tt2948356/"));
@@ -373,4 +371,99 @@ public void shouldBeAbleToSetNodePropertiesToNull() throws MalformedURLException
         assertThat(movie.getTitle()).isNull();
         assertThat(movie.getImdbUrl()).isNull();
     }
+
+    @Test
+    public void nestedFilteringMustThrowExceptionWithOrInThePipeline() {
+        // rated by the user who doesn't own Catty or by the user who owns Catty
+        Filter filterA = new Filter("name", ComparisonOperator.EQUALS, "Catty");
+        filterA.setNestedPath(
+            new Filter.NestedPathSegment("ratings", Rating.class),
+            new Filter.NestedPathSegment("user", User.class),
+            new Filter.NestedPathSegment("pets", Pet.class)
+        );
+
+        filterA.setNegated(true);
+
+        Filter alwaysTrueFilter = new Filter(new FilterFunction() {
+
+            private Filter theFilter;
+
+            @Override public Filter getFilter() {
+                return theFilter;
+            }
+
+            @Override public void setFilter(Filter filter) {
+                theFilter = filter;
+            }
+
+            @Override public Object getValue() {
+                return null;
+            }
+
+            @Override public String expression(String nodeIdentifier) {
+                return expression(nodeIdentifier, null, null);
+            }
+
+            @Override public Map<String, Object> parameters() {
+                return parameters(null, null);
+            }
+
+            @Override
+            public Map<String, Object> parameters(UnaryOperator createUniqueParameterName,
+                PropertyValueTransformer valueTransformer) {
+                return Collections.emptyMap();
+            }
+
+            @Override
+            public String expression(String nodeIdentifier, String filteredProperty,
+                UnaryOperator createUniqueParameterName) {
+                return "1 = 1 ";
+            }
+
+        });
+        alwaysTrueFilter.setBooleanOperator(BooleanOperator.OR);
+
+        Filter filterB = new Filter("name", ComparisonOperator.EQUALS, "Catty");
+        filterB.setNestedPath(
+            new Filter.NestedPathSegment("ratings", Rating.class),
+            new Filter.NestedPathSegment("user", User.class),
+            new Filter.NestedPathSegment("pets", Pet.class)
+        );
+        filterB.setBooleanOperator(BooleanOperator.AND);
+        Filters filters = new Filters(filterA, alwaysTrueFilter, filterB);
+
+        assertThatExceptionOfType(UnsupportedOperationException.class)
+            .isThrownBy(() -> session.loadAll(Movie.class, filters))
+            .withMessage("Filters containing nested paths cannot be combined via the logical OR operator.");
+    }
+
+    @Test
+    public void nestedFilteringCanBeTheOneAndOnlyOredFilter() {
+        Filter filterB = new Filter("name", ComparisonOperator.EQUALS, "Catty");
+        filterB.setNestedPath(
+            new Filter.NestedPathSegment("ratings", Rating.class),
+            new Filter.NestedPathSegment("user", User.class),
+            new Filter.NestedPathSegment("pets", Pet.class)
+        );
+        filterB.setBooleanOperator(BooleanOperator.OR);
+        Filters filters = new Filters(filterB);
+        Collection<Movie> films = session.loadAll(Movie.class, filters);
+        assertThat(films).hasSize(2);
+    }
+
+    @Test
+    public void nestedFilteringMustThrowExceptionWithOrInThePipelineForRelationshipEntities() {
+        Filter userNameFilter = new Filter("name", ComparisonOperator.EQUALS, "Michal");
+        userNameFilter.setBooleanOperator(BooleanOperator.OR);
+
+        Filter ratingFilter = new Filter("stars", ComparisonOperator.EQUALS, 5);
+
+        userNameFilter.setNestedPath(
+            new Filter.NestedPathSegment("user", User.class)
+        );
+
+        assertThatExceptionOfType(UnsupportedOperationException.class)
+            .isThrownBy(() -> session.loadAll(Rating.class, userNameFilter.and(ratingFilter)))
+            .withMessage("Filters containing nested paths cannot be combined via the logical OR operator.");
+    }
 }

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/session/request/strategy/impl/NodeQueryStatementsTest.java

@@ -537,11 +537,7 @@ public void testFindByMultipleNestedPropertiesInfiniteDepth() {
             "MATCH (n:`Asteroid`) WHERE n.`diameter` > $`diameter_0` MATCH (m0:`Planet`) WHERE m0.`name` = $`collidesWith_name_1` MATCH (n)-[:`COLLIDES`]->(m0) WITH DISTINCT n MATCH p=(n)-[*0..]-(m) RETURN p, ID(n)");
     }
 
-    /**
-     * @see DATAGRAPH-662
-     * //TODO FIXME
-     */
-    @Test(expected = UnsupportedOperationException.class)
+    @Test(expected = UnsupportedOperationException.class) // DATAGRAPH-662
     public void testFindByMultipleNestedPropertiesOred() {
         Filter diameterParam = new Filter("diameter", ComparisonOperator.GREATER_THAN, 60);
 
@@ -551,17 +547,12 @@ public void testFindByMultipleNestedPropertiesOred() {
         planetParam.setNestedEntityTypeLabel("Planet");
         planetParam.setRelationshipType("COLLIDES");
         planetParam.setRelationshipDirection("OUTGOING");
-        assertThat(
-            queryStatements.findByType("Asteroid", new Filters().add(diameterParam).add(planetParam), 1).getStatement())
-            .isEqualTo(
-                "MATCH (n:`Asteroid`) WHERE n.`diameter` > $`diameter` OPTIONAL MATCH (m0:`Planet`) WHERE m0.`name` = $`collidesWith_name` OPTIONAL MATCH (n)-[:`COLLIDES`]->(m0) WITH n MATCH p=(n)-[*0..1]-(m) RETURN p, ID(n)");
+
+        queryStatements.findByType("Asteroid", new Filters().add(diameterParam).add(planetParam), 1)
+            .getStatement();
     }
 
-    /**
-     * @see DATAGRAPH-662
-     * //TODO FIXME
-     */
-    @Test(expected = UnsupportedOperationException.class)
+    @Test(expected = UnsupportedOperationException.class) // DATAGRAPH-662
     public void testFindByMultipleNestedPropertiesOredDepth0() {
         Filter diameterParam = new Filter("diameter", ComparisonOperator.GREATER_THAN, 60);
 
@@ -572,16 +563,11 @@ public void testFindByMultipleNestedPropertiesOredDepth0() {
         planetParam.setRelationshipType("COLLIDES");
         planetParam.setRelationshipDirection("OUTGOING");
 
-        assertThat(
-            queryStatements.findByType("Asteroid", new Filters().add(diameterParam).add(planetParam), 0).getStatement())
-            .isEqualTo(
-                "MATCH (n:`Asteroid`) WHERE n.`diameter` > $`diameter` OPTIONAL MATCH (m0:`Planet`) WHERE m0.`name` = $`collidesWith_name` OPTIONAL MATCH (n)-[:`COLLIDES`]->(m0) RETURN n");
+        String statement = queryStatements.findByType("Asteroid", new Filters().add(diameterParam).add(planetParam), 0)
+            .getStatement();
     }
 
-    /**
-     * @see DATAGRAPH-632
-     */
-    @Test
+    @Test // DATAGRAPH-632
     public void testFindByNestedREProperty() {
         Filter planetParam = new Filter("totalDestructionProbability", ComparisonOperator.EQUALS, "20");
         planetParam.setNestedPropertyName("collision");
@@ -597,10 +583,7 @@ public void testFindByNestedREProperty() {
                 "WITH DISTINCT n MATCH p=(n)-[*0..1]-(m) RETURN p, ID(n)");
     }
 
-    /**
-     * @see OGM-279
-     */
-    @Test
+    @Test // GH-279
     public void testFindByMultipleNestedREProperty() {
         Filter planetParam = new Filter("totalDestructionProbability", ComparisonOperator.EQUALS, "20");
         planetParam.setNestedPropertyName("collision");
@@ -676,11 +659,7 @@ public void testFindByDifferentNestedPropertiesAnded() {
                 "MATCH (n)<-[:`ORBITS`]-(m1) WITH DISTINCT n MATCH p=(n)-[*0..1]-(m) RETURN p, ID(n)");
     }
 
-    /**
-     * @see DATAGRAPH-662
-     * //TODO FIXME
-     */
-    @Test(expected = UnsupportedOperationException.class)
+    @Test(expected = UnsupportedOperationException.class) // DATAGRAPH-662
     public void testFindByDifferentNestedPropertiesOred() {
         Filter planetParam = new Filter("name", ComparisonOperator.EQUALS, "Earth");
 
@@ -695,16 +674,11 @@ public void testFindByDifferentNestedPropertiesOred() {
         moonParam.setRelationshipType("ORBITS");
         moonParam.setRelationshipDirection("INCOMING");
         moonParam.setBooleanOperator(BooleanOperator.OR);
-        assertThat(
-            queryStatements.findByType("Asteroid", new Filters().add(planetParam).add(moonParam), 1).getStatement())
-            .isEqualTo(
-                "MATCH (n:`Asteroid`) MATCH (m0:`Planet`) WHERE m0.`name` = $`collidesWith_name` OPTIONAL MATCH (m1:`Moon`) WHERE m1.`name` = $`moon_name` OPTIONAL MATCH (n)-[:`COLLIDES`]->(m0) OPTIONAL MATCH (n)<-[:`ORBITS`]-(m1) WITH n MATCH p=(n)-[*0..1]-(m) RETURN p, ID(n)");
+
+        queryStatements.findByType("Asteroid", new Filters().add(planetParam).add(moonParam), 1).getStatement();
     }
 
-    /**
-     * @see DATAGRAPH-629
-     */
-    @Test
+    @Test // DATAGRAPH-629
     public void testFindByMultipleNestedPropertiesAnded() {
         Filter planetParam = new Filter("name", ComparisonOperator.EQUALS, "Earth");
         planetParam.setNestedPropertyName("collidesWith");