Handle missing properties when populating index

If we create an index on some properties and some nodes are missing one or more of the indexed properties we should not fail with a silent null pointer exception in the populating, thread rendering the index unusable.
neo4j · Apr 8, 2017 · 0e8c2dc · 0e8c2dc
1 parent 2bab615
commit 0e8c2dc
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 12 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/index/NodeUpdates.java
@@ -24,11 +24,11 @@
 import java.util.List;
 
 import org.neo4j.collection.primitive.Primitive;
+import org.neo4j.collection.primitive.PrimitiveArrays;
 import org.neo4j.collection.primitive.PrimitiveIntCollection;
 import org.neo4j.collection.primitive.PrimitiveIntIterator;
 import org.neo4j.collection.primitive.PrimitiveIntObjectMap;
 import org.neo4j.collection.primitive.PrimitiveIntSet;
-import org.neo4j.collection.primitive.PrimitiveArrays;
 import org.neo4j.helpers.collection.Iterables;
 import org.neo4j.kernel.api.index.IndexEntryUpdate;
 import org.neo4j.kernel.api.properties.DefinedProperty;
@@ -131,17 +131,17 @@ public final long getNodeId()
         return nodeId;
     }
 
-    public long[] labelsChanged()
+    long[] labelsChanged()
     {
         return PrimitiveArrays.symmetricDifference( labelsBefore, labelsAfter );
     }
 
-    public long[] labelsUnchanged()
+    long[] labelsUnchanged()
     {
         return PrimitiveArrays.intersect( labelsBefore, labelsAfter );
     }
 
-    public PrimitiveIntCollection propertiesChanged()
+    PrimitiveIntCollection propertiesChanged()
     {
         assert !hasLoadedAdditionalProperties : "Calling propertiesChanged() is not valid after non-changed " +
                                                 "properties have already been loaded.";
@@ -231,7 +231,7 @@ else if ( !relevantBefore && relevantAfter )
                         nodeId, indexKey, valuesAfter( propertyIds )
                 ) );
             }
-            else if ( relevantBefore && relevantAfter )
+            else if ( relevantBefore )
             {
                 if ( valuesChanged( propertyIds ) )
                 {
@@ -308,7 +308,8 @@ private boolean hasPropsBefore( int[] propertyIds )
     {
         for ( int propertyId : propertyIds )
         {
-            if ( !knownProperties.get( propertyId ).hasBefore() )
+            PropertyValue propertyValue = knownProperties.get( propertyId );
+            if ( propertyValue == null || !propertyValue.hasBefore() )
             {
                 return false;
             }
@@ -320,7 +321,8 @@ private boolean hasPropsAfter( int[] propertyIds )
     {
         for ( int propertyId : propertyIds )
         {
-            if ( !knownProperties.get( propertyId ).hasAfter() )
+            PropertyValue propertyValue = knownProperties.get( propertyId );
+            if ( propertyValue == null || !propertyValue.hasAfter() )
             {
                 return false;
             }
@@ -343,7 +345,8 @@ private Object[] valuesAfter( int[] propertyIds )
         Object[] values = new Object[propertyIds.length];
         for ( int i = 0; i < propertyIds.length; i++ )
         {
-            values[i] = knownProperties.get( propertyIds[i] ).after;
+            PropertyValue propertyValue = knownProperties.get( propertyIds[i] );
+            values[i] = propertyValue == null ? null : propertyValue.after;
         }
         return values;
     }
@@ -444,7 +447,7 @@ enum PropertyValueType
         Before,
         After,
         UnChanged,
-        Changed;
+        Changed
     }
 
     private static class PropertyValue

diff --git a/...te/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala b/...te/src/test/scala/org/neo4j/internal/cypher/acceptance/CompositeIndexAcceptanceTest.scala
@@ -19,8 +19,8 @@
  */
 package org.neo4j.internal.cypher.acceptance
 
-import org.neo4j.cypher.{CypherExecutionException, ExecutionEngineFunSuite, NewPlannerTestSupport, SyntaxException}
-import org.neo4j.graphdb.{ConstraintViolationException, Node}
+import org.neo4j.cypher.{ExecutionEngineFunSuite, NewPlannerTestSupport}
+import org.neo4j.graphdb.Node
 import org.neo4j.kernel.GraphDatabaseQueryService
 import org.scalatest.matchers.{MatchResult, Matcher}
 
@@ -162,7 +162,8 @@ class CompositeIndexAcceptanceTest extends ExecutionEngineFunSuite with NewPlann
 
   test("should be able to update composite index when only one property has changed") {
     graph.createIndex("Person", "firstname", "lastname")
-    val n = executeWithCostPlannerAndInterpretedRuntimeOnly("CREATE (n:Person {firstname:'Joe', lastname:'Soap'}) RETURN n").columnAs("n").toList(0).asInstanceOf[Node]
+    val n = executeWithCostPlannerAndInterpretedRuntimeOnly(
+      "CREATE (n:Person {firstname:'Joe', lastname:'Soap'}) RETURN n").columnAs("n").toList.head.asInstanceOf[Node]
     executeWithCostPlannerAndInterpretedRuntimeOnly("MATCH (n:Person) SET n.lastname = 'Bloggs'")
     val result = executeWithCostPlannerAndInterpretedRuntimeOnly("MATCH (n:Person) where n.firstname = 'Joe' and n.lastname = 'Bloggs' RETURN n")
     result should use("NodeIndexSeek")
@@ -243,6 +244,36 @@ class CompositeIndexAcceptanceTest extends ExecutionEngineFunSuite with NewPlann
     result should evaluateTo(List(Map("x" -> 1), Map("x" -> 3), Map("x" -> 5), Map("x" -> 7), Map("x" -> 9)))
   }
 
+  test("should handle missing properties when populating index") {
+    // Given
+    createLabeledNode(Map("foo" -> 42), "PROFILES")
+    createLabeledNode(Map("foo" -> 42, "bar" -> 1337, "baz" -> 1980), "L")
+
+    // When
+    graph.createIndex("L", "foo", "bar", "baz")
+
+    // Then
+    graph should haveIndexes(":L(foo,bar,baz)")
+    val result = executeWithAllPlanners("MATCH (n:L {foo: 42, bar: 1337, baz: 1980}) RETURN count(n)")
+    result should evaluateTo(Seq(Map("count(n)" -> 1)))
+    result should useIndex(":L(foo,bar,baz")
+  }
+
+  test("should handle missing properties when adding to index after creation") {
+    // Given
+    createLabeledNode(Map("foo" -> 42), "PROFILES")
+
+    // When
+    graph.createIndex("L", "foo", "bar", "baz")
+    createLabeledNode(Map("foo" -> 42, "bar" -> 1337, "baz" -> 1980), "L")
+
+    // Then
+    graph should haveIndexes(":L(foo,bar,baz)")
+    val result = executeWithAllPlanners("MATCH (n:L {foo: 42, bar: 1337, baz: 1980}) RETURN count(n)")
+    result should evaluateTo(Seq(Map("count(n)" -> 1)))
+    result should useIndex(":L(foo,bar,baz")
+  }
+
   case class haveIndexes(expectedIndexes: String*) extends Matcher[GraphDatabaseQueryService] {
     def apply(graph: GraphDatabaseQueryService): MatchResult = {
       graph.inTx {