Restructure of IndexProvider compatiblity tests

- Fix incorrect testing of single value entries in composite indexes

community/kernel/src/main/java/org/neo4j/kernel/api/index/IndexEntryUpdate.java

@@ -42,15 +42,18 @@ public class IndexEntryUpdate
 
     private IndexEntryUpdate( long entityId, LabelSchemaDescriptor descriptor, UpdateMode updateMode, Object... values )
     {
-        this.entityId = entityId;
-        this.descriptor = descriptor;
-        this.before = null;
-        this.values = values;
-        this.updateMode = updateMode;
+        this( entityId, descriptor, updateMode, null, values );
     }
+
     private IndexEntryUpdate( long entityId, LabelSchemaDescriptor descriptor, UpdateMode updateMode, Object[] before,
             Object[] values )
     {
+        // we do not support partial index entries
+        assert descriptor.getPropertyIds().length == values.length :
+                format( "IndexEntryUpdate values must be of same length as index compositness. " +
+                        "Index on %s, but got values %s", descriptor.toString(), Arrays.toString( values ) );
+        assert before == null || before.length == values.length;
+
         this.entityId = entityId;
         this.descriptor = descriptor;
         this.before = before;

community/kernel/src/test/java/org/neo4j/kernel/api/index/CompositeIndexAccessorCompatibility.java

@@ -19,52 +19,118 @@
  */
 package org.neo4j.kernel.api.index;
 
-import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 
-import org.neo4j.kernel.api.schema_new.LabelSchemaDescriptor;
+import org.neo4j.kernel.api.schema_new.index.NewIndexDescriptor;
 import org.neo4j.kernel.api.schema_new.index.NewIndexDescriptorFactory;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.neo4j.kernel.api.schema_new.IndexQuery.exact;
 import static org.neo4j.kernel.api.schema_new.IndexQuery.exists;
 
-public class CompositeIndexAccessorCompatibility extends IndexAccessorCompatibility
-{
-    protected IndexAccessor accessor;
-    private LabelSchemaDescriptor schemaDescriptor;
 
-    public CompositeIndexAccessorCompatibility( IndexProviderCompatibilityTestSuite testSuite )
+@Ignore( "Not a test. This is a compatibility suite that provides test cases for verifying" +
+        " SchemaIndexProvider implementations. Each index provider that is to be tested by this suite" +
+        " must create their own test class extending IndexProviderCompatibilityTestSuite." +
+        " The @Ignore annotation doesn't prevent these tests to run, it rather removes some annoying" +
+        " errors or warnings in some IDEs about test classes needing a public zero-arg constructor." )
+public abstract class CompositeIndexAccessorCompatibility extends IndexAccessorCompatibility
+{
+    public CompositeIndexAccessorCompatibility(
+            IndexProviderCompatibilityTestSuite testSuite, NewIndexDescriptor descriptor )
     {
-        super( testSuite, NewIndexDescriptorFactory.forLabel( 1000, 100, 200, 300 ), false );
+        super( testSuite, descriptor );
     }
 
-    @Before
-    public void setUp() throws Exception
+    @Test
+    public void testIndexSeekAndScanByString() throws Exception
     {
-        schemaDescriptor = descriptor.schema();
+        updateAndCommit( asList(
+                IndexEntryUpdate.add( 1L, descriptor.schema(), "a", "a" ),
+                IndexEntryUpdate.add( 2L, descriptor.schema(), "b", "b" ),
+                IndexEntryUpdate.add( 3L, descriptor.schema(), "a", "b" ) ) );
+
+        assertThat( query( exact( 0, "a" ), exact( 1, "a" ) ), equalTo( singletonList( 1L ) ) );
+        assertThat( query( exact( 0, "b" ), exact( 1, "b" ) ), equalTo( singletonList( 2L ) ) );
+        assertThat( query( exact( 0, "a" ), exact( 1, "b" ) ), equalTo( singletonList( 3L ) ) );
+        assertThat( query( exists( 1 ) ), equalTo( asList( 1L, 2L, 3L ) ) );
     }
 
     @Test
-    public void testIndexSeekAndScan() throws Exception
+    public void testIndexSeekAndScanByNumber() throws Exception
     {
         updateAndCommit( asList(
-                IndexEntryUpdate.add( 1L, schemaDescriptor, "a", "a" ),
-                IndexEntryUpdate.add( 2L, schemaDescriptor, "a", "a" ),
-                IndexEntryUpdate.add( 3L, schemaDescriptor, "b", "b" ),
-                IndexEntryUpdate.add( 4L, schemaDescriptor, "a", "b" )
-        ) );
-
-        assertThat( query( exact( 0, "a" ), exact( 1, "a" ) ), equalTo( asList( 1L, 2L ) ) );
-        assertThat( query( exact( 0, "b" ), exact( 1, "b" ) ), equalTo( asList( 3L ) ) );
-        assertThat( query( exact( 0, "a" ), exact( 1, "b" ) ), equalTo( asList( 4L ) ) );
-        assertThat( query( exists( 1 ) ), equalTo( asList( 1L, 2L, 3L, 4L ) ) );
+                IndexEntryUpdate.add( 1L, descriptor.schema(), 333, 333 ),
+                IndexEntryUpdate.add( 2L, descriptor.schema(), 101, 101 ),
+                IndexEntryUpdate.add( 3L, descriptor.schema(), 333, 101 ) ) );
+
+        assertThat( query( exact( 0, 333 ), exact( 1, 333 ) ), equalTo( singletonList( 1L ) ) );
+        assertThat( query( exact( 0, 101 ), exact( 1, 101 ) ), equalTo( singletonList( 2L ) ) );
+        assertThat( query( exact( 0, 333 ), exact( 1, 101 ) ), equalTo( singletonList( 3L ) ) );
+        assertThat( query( exists( 1 ) ), equalTo( asList( 1L, 2L, 3L ) ) );
+    }
+
+    // This behaviour is expected by General indexes
+
+    public static class General extends CompositeIndexAccessorCompatibility
+    {
+        public General( IndexProviderCompatibilityTestSuite testSuite )
+        {
+            super( testSuite, NewIndexDescriptorFactory.forLabel( 1000, 100, 200 ) );
+        }
+
+        @Test
+        public void testDuplicatesInIndexSeekByString() throws Exception
+        {
+            updateAndCommit( asList(
+                    IndexEntryUpdate.add( 1L, descriptor.schema(), "a", "a" ),
+                    IndexEntryUpdate.add( 2L, descriptor.schema(), "a", "a" ) ) );
+
+            assertThat( query( exact( 0, "a" ), exact( 1, "a" ) ), equalTo( asList( 1L, 2L ) ) );
+        }
+
+        @Test
+        public void testDuplicatesInIndexSeekByNumber() throws Exception
+        {
+            updateAndCommit( asList(
+                    IndexEntryUpdate.add( 1L, descriptor.schema(), 333, 333 ),
+                    IndexEntryUpdate.add( 2L, descriptor.schema(), 333, 333 ) ) );
+
+            assertThat( query( exact( 0, 333 ), exact( 1, 333 ) ), equalTo( asList( 1L, 2L ) ) );
+        }
+    }
+
+    // This behaviour is expected by Unique indexes
+
+    public static class Unique extends CompositeIndexAccessorCompatibility
+    {
+        public Unique( IndexProviderCompatibilityTestSuite testSuite )
+        {
+            super( testSuite, NewIndexDescriptorFactory.uniqueForLabel( 1000, 100, 200 ) );
+        }
+
+        @Test
+        public void closingAnOnlineIndexUpdaterMustNotThrowEvenIfItHasBeenFedConflictingData() throws Exception
+        {
+            // The reason is that we use and close IndexUpdaters in commit - not in prepare - and therefor
+            // we cannot have them go around and throw exceptions, because that could potentially break
+            // recovery.
+            // Conflicting data can happen because of faulty data coercion. These faults are resolved by
+            // the exact-match filtering we do on index seeks in StateHandlingStatementOperations.
+
+            updateAndCommit( asList(
+                    IndexEntryUpdate.add( 1L, descriptor.schema(), "a", "a" ),
+                    IndexEntryUpdate.add( 2L, descriptor.schema(), "a", "a" ) ) );
+
+            assertThat( query( exact( 0, "a" ), exact( 1, "a" ) ), equalTo( asList( 1L, 2L ) ) );
+        }
     }
 
 //TODO: add when supported:
-    //testIndexSeekByNumber
     //testIndexSeekByString
     //testIndexSeekByPrefix
     //testIndexSeekByPrefixOnNonStrings

community/kernel/src/test/java/org/neo4j/kernel/api/index/CompositeIndexPopulatorCompatibility.java

@@ -0,0 +1,149 @@
+/*
+ * Copyright (c) 2002-2017 "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.index;
+
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.util.Arrays;
+
+import org.neo4j.collection.primitive.PrimitiveLongCollections;
+import org.neo4j.collection.primitive.PrimitiveLongIterator;
+import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException;
+import org.neo4j.kernel.api.schema_new.IndexQuery;
+import org.neo4j.kernel.api.schema_new.OrderedPropertyValues;
+import org.neo4j.kernel.api.schema_new.index.NewIndexDescriptor;
+import org.neo4j.kernel.api.schema_new.index.NewIndexDescriptorFactory;
+import org.neo4j.kernel.configuration.Config;
+import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig;
+import org.neo4j.storageengine.api.schema.IndexReader;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+import static org.neo4j.helpers.collection.Iterators.asSet;
+
+@Ignore( "Not a test. This is a compatibility suite that provides test cases for verifying" +
+        " SchemaIndexProvider implementations. Each index provider that is to be tested by this suite" +
+        " must create their own test class extending IndexProviderCompatibilityTestSuite." +
+        " The @Ignore annotation doesn't prevent these tests to run, it rather removes some annoying" +
+        " errors or warnings in some IDEs about test classes needing a public zero-arg constructor." )
+public class CompositeIndexPopulatorCompatibility extends IndexProviderCompatibilityTestSuite.Compatibility
+{
+    public CompositeIndexPopulatorCompatibility(
+            IndexProviderCompatibilityTestSuite testSuite, NewIndexDescriptor descriptor )
+    {
+        super( testSuite, descriptor );
+    }
+
+    public static class General extends CompositeIndexPopulatorCompatibility
+    {
+        public General( IndexProviderCompatibilityTestSuite testSuite )
+        {
+            super( testSuite, NewIndexDescriptorFactory.forLabel( 1000, 100, 200 ) );
+        }
+
+        @Test
+        public void shouldProvidePopulatorThatAcceptsDuplicateEntries() throws Exception
+        {
+            // when
+            IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.empty() );
+            IndexPopulator populator = indexProvider.getPopulator( 17, descriptor, indexSamplingConfig );
+            populator.create();
+            populator.add( Arrays.asList(
+                    IndexEntryUpdate.add( 1, descriptor.schema(), "v1", "v2" ),
+                    IndexEntryUpdate.add( 2, descriptor.schema(), "v1", "v2" ) ) );
+            populator.close( true );
+
+            // then
+            IndexAccessor accessor = indexProvider.getOnlineAccessor( 17, descriptor, indexSamplingConfig );
+            try ( IndexReader reader = accessor.newReader() )
+            {
+                PrimitiveLongIterator nodes = reader.query( IndexQuery.exact( 1, "v1" ), IndexQuery.exact( 1, "v2" ) );
+                assertEquals( asSet( 1L, 2L ), PrimitiveLongCollections.toSet( nodes ) );
+            }
+            accessor.close();
+        }
+    }
+
+    public static class Unique extends CompositeIndexPopulatorCompatibility
+    {
+        String value1 = "value1";
+        String value2 = "value2";
+        String value3 = "value3";
+        int nodeId1 = 3;
+        int nodeId2 = 4;
+
+        public Unique( IndexProviderCompatibilityTestSuite testSuite )
+        {
+            super( testSuite, NewIndexDescriptorFactory.uniqueForLabel( 1000, 100, 200 ) );
+        }
+
+        @Test
+        public void shouldEnforceUniqueConstraintsDirectly() throws Exception
+        {
+            // when
+            IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.empty() );
+            IndexPopulator populator = indexProvider.getPopulator( 17, descriptor, indexSamplingConfig );
+
+            populator.create();
+            populator.add( Arrays.asList(
+                    IndexEntryUpdate.add( nodeId1, descriptor.schema(), value1, value2 ),
+                    IndexEntryUpdate.add( nodeId2, descriptor.schema(), value1, value2 ) ) );
+            try
+            {
+                NodePropertyAccessor propertyAccessor =
+                        new NodePropertyAccessor( nodeId1, descriptor.schema(), value1, value2 );
+                propertyAccessor.addNode( nodeId2, descriptor.schema(), value1, value2 );
+                populator.verifyDeferredConstraints( propertyAccessor );
+
+                fail( "expected exception" );
+            }
+            // then
+            catch ( IndexEntryConflictException conflict )
+            {
+                assertEquals( nodeId1, conflict.getExistingNodeId() );
+                assertEquals( OrderedPropertyValues.ofUndefined( value1, value2 ), conflict.getPropertyValues() );
+                assertEquals( nodeId2, conflict.getAddedNodeId() );
+            }
+        }
+
+        @Test
+        public void shouldNotRestrictUpdatesDifferingOnSecondProperty() throws Exception
+        {
+            // given
+            IndexSamplingConfig indexSamplingConfig = new IndexSamplingConfig( Config.empty() );
+            IndexPopulator populator = indexProvider.getPopulator( 17, descriptor, indexSamplingConfig );
+
+            populator.create();
+
+            // when
+            populator.add( Arrays.asList(
+                    IndexEntryUpdate.add( nodeId1, descriptor.schema(), value1, value2 ),
+                    IndexEntryUpdate.add( nodeId2, descriptor.schema(), value1, value3 ) ) );
+
+            NodePropertyAccessor propertyAccessor =
+                    new NodePropertyAccessor( nodeId1, descriptor.schema(), value1, value2 );
+            propertyAccessor.addNode( nodeId2, descriptor.schema(), value1, value3 );
+
+            // then this should pass fine
+            populator.verifyDeferredConstraints( propertyAccessor );
+        }
+    }
+}