Fixes as issue with applying index updates to native unique indexes

Given a uniqueness contraint on Foo:bar. If a transaction make changes
such that in the end, the constraint is not violated but some intermediate
individual step violates the constraint the index would still throw an
exception.

The reason this happens is that native index validates the uniqueness
constraint on every insert (since doing so is for free).

Although actually doing so when applying online updates has never been done
in the lucene indexes because all that is handled in the kernel layer
before committing a transaction anyway. Detecting a conflict while applying
updates to a unique index means that there's a bug inside the kernel layer
and would result in a kernel panic.

Therefore we avoid this problem and do what we do for lucene indexes,
namely by not checking conclicts when applying online updates.

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java

@@ -20,54 +20,72 @@
 package org.neo4j.kernel.impl.index.schema;
 
 import org.neo4j.index.internal.gbptree.ValueMerger;
+import org.neo4j.index.internal.gbptree.ValueMergers;
 import org.neo4j.index.internal.gbptree.Writer;
+import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException;
+import org.neo4j.values.storable.Value;
+import org.neo4j.values.storable.ValueTuple;
 
 /**
  * {@link ValueMerger} which will merely detect conflict, not change any value if conflict, i.e. if the
  * key already exists. After this merge has been used in a call to {@link Writer#merge(Object, Object, ValueMerger)}
- * the {@link #wasConflict()} accessor can be called to check whether or not that call conflicted with
- * an existing key. A call to {@link #wasConflict()} will also clear the conflict flag.
+ * {@link #checkConflict(Value[])} can be called to check whether or not that call conflicted with
+ * an existing key. A call to {@link #checkConflict(Value[])} will also clear the conflict flag.
  *
  * @param <VALUE> type of values being merged.
  */
-class ConflictDetectingValueMerger<KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> implements ValueMerger<KEY,VALUE>
+abstract class ConflictDetectingValueMerger<KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> implements ValueMerger<KEY,VALUE>
 {
-    private boolean conflict;
-    private long existingNodeId;
-    private long addedNodeId;
+    /**
+     * @throw IndexEntryConflictException if merge conflicted with an existing key. This call also clears the conflict flag.
+     */
+    abstract void checkConflict( Value[] values ) throws IndexEntryConflictException;
 
-    @Override
-    public VALUE merge( KEY existingKey, KEY newKey, VALUE existingValue, VALUE newValue )
+    private static ConflictDetectingValueMerger DONT_CHECK = new ConflictDetectingValueMerger()
     {
-        if ( existingKey.entityId != newKey.entityId )
+        @Override
+        public Object merge( Object existingKey, Object newKey, Object existingValue, Object newValue )
         {
-            conflict = true;
-            existingNodeId = existingKey.entityId;
-            addedNodeId = newKey.entityId;
+            return null;
         }
-        return null;
-    }
 
-    /**
-     * @return whether or not merge conflicted with an existing key. This call also clears the conflict flag.
-     */
-    boolean wasConflict()
-    {
-        boolean result = conflict;
-        if ( conflict )
+        @Override
+        void checkConflict( Value[] values )
         {
-            conflict = false;
+            // do nothing
         }
-        return result;
-    }
+    };
 
-    long existingNodeId()
+    static <KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> ConflictDetectingValueMerger<KEY, VALUE> dontCheck()
     {
-        return existingNodeId;
+        return DONT_CHECK;
     }
 
-    long addedNodeId()
+    static class Check<KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> extends ConflictDetectingValueMerger<KEY, VALUE>
     {
-        return addedNodeId;
+        private boolean conflict;
+        private long existingNodeId;
+        private long addedNodeId;
+
+        @Override
+        public VALUE merge( KEY existingKey, KEY newKey, VALUE existingValue, VALUE newValue )
+        {
+            if ( existingKey.entityId != newKey.entityId )
+            {
+                conflict = true;
+                existingNodeId = existingKey.entityId;
+                addedNodeId = newKey.entityId;
+            }
+            return null;
+        }
+
+        void checkConflict( Value[] values ) throws IndexEntryConflictException
+        {
+            if ( conflict )
+            {
+                conflict = false;
+                throw new IndexEntryConflictException( existingNodeId, addedNodeId, ValueTuple.of( values ) );
+            }
+        }
     }
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java

@@ -74,7 +74,7 @@ public abstract class NativeSchemaNumberIndexPopulator<KEY extends SchemaNumberK
         super( pageCache, fs, storeFile, layout, monitor, descriptor, indexId );
         this.treeKey = layout.newKey();
         this.treeValue = layout.newValue();
-        this.conflictDetectingValueMerger = new ConflictDetectingValueMerger<>();
+        this.conflictDetectingValueMerger = new ConflictDetectingValueMerger.Check<>();
     }
 
     @Override

community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java

@@ -27,12 +27,14 @@
 import org.neo4j.kernel.api.index.IndexUpdater;
 import org.neo4j.values.storable.ValueTuple;
 
+import static org.neo4j.kernel.impl.index.schema.ConflictDetectingValueMerger.dontCheck;
+
 class NativeSchemaNumberIndexUpdater<KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue>
         implements IndexUpdater
 {
     private final KEY treeKey;
     private final VALUE treeValue;
-    private final ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger;
+    private final ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger = dontCheck();
     private Writer<KEY,VALUE> writer;
 
     private boolean closed = true;
@@ -42,7 +44,6 @@ class NativeSchemaNumberIndexUpdater<KEY extends SchemaNumberKey, VALUE extends
     {
         this.treeKey = treeKey;
         this.treeValue = treeValue;
-        this.conflictDetectingValueMerger = new ConflictDetectingValueMerger<>();
     }
 
     NativeSchemaNumberIndexUpdater<KEY,VALUE> initialize( Writer<KEY,VALUE> writer, boolean manageClosingOfWriter )
@@ -124,7 +125,7 @@ private static <KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> vo
         treeKey.from( update.getEntityId(), update.values() );
         treeValue.from( update.values() );
         writer.merge( treeKey, treeValue, conflictDetectingValueMerger );
-        assertNoConflict( update, conflictDetectingValueMerger );
+        conflictDetectingValueMerger.checkConflict( update.values() );
     }
 
     static <KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> void processAdd( KEY treeKey, VALUE treeValue,
@@ -135,17 +136,6 @@ static <KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> void proce
         treeKey.from( update.getEntityId(), update.values() );
         treeValue.from( update.values() );
         writer.merge( treeKey, treeValue, conflictDetectingValueMerger );
-        assertNoConflict( update, conflictDetectingValueMerger );
-    }
-
-    private static <KEY extends SchemaNumberKey, VALUE extends SchemaNumberValue> void assertNoConflict( IndexEntryUpdate<?> update,
-            ConflictDetectingValueMerger<KEY,VALUE> conflictDetectingValueMerger ) throws IndexEntryConflictException
-    {
-        if ( conflictDetectingValueMerger.wasConflict() )
-        {
-            long existingNodeId = conflictDetectingValueMerger.existingNodeId();
-            long addedNodeId = conflictDetectingValueMerger.addedNodeId();
-            throw new IndexEntryConflictException( existingNodeId, addedNodeId, ValueTuple.of( update.values() ) );
-        }
+        conflictDetectingValueMerger.checkConflict( update.values() );
     }
 }

community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java

@@ -21,23 +21,28 @@
 
 import org.junit.Test;
 
+import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException;
 import org.neo4j.values.storable.Value;
 import org.neo4j.values.storable.Values;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.neo4j.helpers.ArrayUtil.array;
+import static org.neo4j.values.storable.Values.stringValue;
 
 public class ConflictDetectingValueMergerTest
 {
-    private final ConflictDetectingValueMerger<SchemaNumberKey,SchemaNumberValue> detector = new ConflictDetectingValueMerger<>();
+    private final ConflictDetectingValueMerger<SchemaNumberKey,SchemaNumberValue> detector = new ConflictDetectingValueMerger.Check<>();
 
     @Test
     public void shouldReportConflictOnSameValueAndDifferentEntityIds() throws Exception
     {
         // given
-        Value value = Values.of( 123);
+        Value value = Values.of( 123 );
         long entityId1 = 10;
         long entityId2 = 20;
 
@@ -50,9 +55,17 @@ public void shouldReportConflictOnSameValueAndDifferentEntityIds() throws Except
 
         // then
         assertNull( merged );
-        assertTrue( detector.wasConflict() );
-        assertEquals( entityId1, detector.existingNodeId() );
-        assertEquals( entityId2, detector.addedNodeId() );
+        try
+        {
+            detector.checkConflict( array( value ) );
+            fail( "Should've detected conflict" );
+        }
+        catch ( IndexEntryConflictException e )
+        {
+            assertEquals( entityId1, e.getExistingNodeId() );
+            assertEquals( entityId2, e.getAddedNodeId() );
+            assertEquals( value, e.getSinglePropertyValue() );
+        }
     }
 
     @Test
@@ -71,7 +84,7 @@ public void shouldNotReportConflictOnSameValueSameEntityId() throws Exception
 
         // then
         assertNull( merged );
-        assertFalse( detector.wasConflict() );
+        detector.checkConflict( array() ); // <-- should not throw conflict exception
     }
 
     private static SchemaNumberKey key( long entityId, Value... value )

community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java

@@ -29,7 +29,6 @@
 
 import org.neo4j.io.fs.FileSystemAbstraction;
 import org.neo4j.io.pagecache.PageCache;
-import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException;
 import org.neo4j.kernel.api.index.IndexAccessor;
 import org.neo4j.kernel.api.index.IndexEntryUpdate;
 import org.neo4j.kernel.api.index.IndexPopulator;
@@ -124,7 +123,7 @@ public void getPopulatorMustCreateNonUniquePopulatorForTypeGeneral() throws Exce
     /* getOnlineAccessor */
 
     @Test
-    public void getOnlineAccessorMustCreateUniqueAccessorForTypeUnique() throws Exception
+    public void shouldNotCheckConflictsWhenApplyingUpdatesInOnlineAccessor() throws Exception
     {
         // given
         provider = newProvider();
@@ -137,33 +136,6 @@ public void getOnlineAccessorMustCreateUniqueAccessorForTypeUnique() throws Exce
             Value value = Values.intValue( 1 );
             indexUpdater.process( IndexEntryUpdate.add( 1, descriptor.schema(), value ) );
 
-            // then
-            try
-            {
-                indexUpdater.process( IndexEntryUpdate.add( 2, descriptor.schema(), value ) );
-                fail( "Should have failed" );
-            }
-            catch ( IndexEntryConflictException e )
-            {
-                // good
-            }
-        }
-    }
-
-    @Test
-    public void getOnlineAccessorMustCreateNonUniqueAccessorForTypeGeneral() throws Exception
-    {
-        // given
-        provider = newProvider();
-
-        // when
-        IndexDescriptor descriptor = descriptor();
-        try ( IndexAccessor accessor = provider.getOnlineAccessor( indexId, descriptor, samplingConfig() );
-              IndexUpdater indexUpdater = accessor.newUpdater( IndexUpdateMode.ONLINE ) )
-        {
-            Value value = Values.intValue( 1 );
-            indexUpdater.process( IndexEntryUpdate.add( 1, descriptor.schema(), value ) );
-
             // then
             // ... expect no failure on duplicate value
             indexUpdater.process( IndexEntryUpdate.add( 2, descriptor.schema(), value ) );

community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java

@@ -0,0 +1,90 @@
+/*
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.kernel.api.impl.index;
+
+import org.junit.Rule;
+import org.junit.Test;
+
+import java.util.Map;
+
+import org.neo4j.graphdb.Label;
+import org.neo4j.graphdb.Node;
+import org.neo4j.graphdb.NotFoundException;
+import org.neo4j.graphdb.Transaction;
+import org.neo4j.test.rule.DatabaseRule;
+import org.neo4j.test.rule.ImpermanentDatabaseRule;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class AccidentalUniquenessConstraintViolationIT
+{
+    private static final Label Foo = Label.label( "Foo" );
+    private static final String BAR = "bar";
+
+    @Rule
+    public final DatabaseRule db = new ImpermanentDatabaseRule();
+
+    @Test
+    public void shouldApplyChangesWithIntermediateConstraintViolations() throws Exception
+    {
+        // given
+        try ( Transaction tx = db.beginTx() )
+        {
+            db.schema().constraintFor( Foo ).assertPropertyIsUnique( BAR ).create();
+            tx.success();
+        }
+        Node fourtyTwo;
+        Node fourtyOne;
+        try ( Transaction tx = db.beginTx() )
+        {
+            fourtyTwo = db.createNode( Foo );
+            fourtyTwo.setProperty( BAR, 42 );
+            fourtyOne = db.createNode( Foo );
+            fourtyOne.setProperty( BAR, 41 );
+            tx.success();
+        }
+
+        // when
+        try ( Transaction tx = db.beginTx() )
+        {
+            Map<String,Object> props = fourtyOne.getAllProperties();
+            fourtyOne.delete();
+            fourtyTwo.setProperty( BAR, props.get( BAR ) );
+            tx.success();
+        }
+
+        // then
+        try ( Transaction tx = db.beginTx() )
+        {
+            assertEquals( 41, fourtyTwo.getProperty( BAR ) );
+            try
+            {
+                fourtyOne.getProperty( BAR );
+                fail( "Should be deleted" );
+            }
+            catch ( NotFoundException e )
+            {
+                // good
+            }
+            tx.success();
+        }
+    }
+}