Improved testing and error handling on constraint index creation on o…

…rphaned indexes

community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java

@@ -532,8 +532,8 @@ public UniquenessConstraint uniquePropertyConstraintCreate( KernelStatement stat
             }
             return constraint;
         }
-        catch ( ConstraintVerificationFailedKernelException | DropIndexFailureException | TransactionFailureException
-                e )
+        catch ( ConstraintVerificationFailedKernelException | DropIndexFailureException | TransactionFailureException |
+                AlreadyConstrainedException e )
         {
             throw new CreateConstraintFailureException( constraint, e );
         }

community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ConstraintIndexCreator.java

@@ -24,18 +24,20 @@
 
 import org.neo4j.kernel.api.KernelAPI;
 import org.neo4j.kernel.api.KernelTransaction;
-import org.neo4j.kernel.api.ReadOperations;
 import org.neo4j.kernel.api.Statement;
+import org.neo4j.kernel.api.StatementTokenNameLookup;
 import org.neo4j.kernel.api.constraints.UniquenessConstraint;
 import org.neo4j.kernel.api.exceptions.TransactionFailureException;
 import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException;
 import org.neo4j.kernel.api.exceptions.index.IndexNotFoundKernelException;
 import org.neo4j.kernel.api.exceptions.index.IndexPopulationFailedKernelException;
+import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException;
 import org.neo4j.kernel.api.exceptions.schema.ConstraintVerificationFailedKernelException;
 import org.neo4j.kernel.api.exceptions.schema.CreateConstraintFailureException;
 import org.neo4j.kernel.api.exceptions.schema.DropIndexFailureException;
 import org.neo4j.kernel.api.exceptions.schema.SchemaRuleNotFoundException;
 import org.neo4j.kernel.api.exceptions.schema.UniquenessConstraintVerificationFailedKernelException;
+import org.neo4j.kernel.api.exceptions.schema.SchemaKernelException.OperationContext;
 import org.neo4j.kernel.api.index.IndexDescriptor;
 import org.neo4j.kernel.api.index.PropertyAccessor;
 import org.neo4j.kernel.impl.api.KernelStatement;
@@ -89,9 +91,9 @@ public ConstraintIndexCreator( Supplier<KernelAPI> kernelSupplier, IndexingServi
     public long createUniquenessConstraintIndex( KernelStatement state, SchemaReadOperations schema,
             int labelId, int propertyKeyId )
             throws ConstraintVerificationFailedKernelException, TransactionFailureException,
-            CreateConstraintFailureException, DropIndexFailureException
+            CreateConstraintFailureException, DropIndexFailureException, AlreadyConstrainedException
     {
-        IndexDescriptor descriptor = getOrCreateConstraintIndex( state, labelId, propertyKeyId );
+        IndexDescriptor descriptor = getOrCreateConstraintIndex( state, schema, labelId, propertyKeyId );
         UniquenessConstraint constraint = new UniquenessConstraint( labelId, propertyKeyId );
 
         boolean success = false;
@@ -211,11 +213,10 @@ private void awaitIndexPopulation( UniquenessConstraint constraint, long indexId
         }
     }
 
-    public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state,
-            int labelId, int propertyKeyId )
+    public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state, SchemaReadOperations schema,
+            int labelId, int propertyKeyId ) throws AlreadyConstrainedException
     {
-        ReadOperations readOperations = state.readOperations();
-        for ( IndexDescriptor descriptor : loop( readOperations.uniqueIndexesGetForLabel( labelId ) ) )
+        for ( IndexDescriptor descriptor : loop( schema.uniqueIndexesGetForLabel( state, labelId ) ) )
         {
             if ( descriptor.getPropertyKeyId() == propertyKeyId )
             {
@@ -224,10 +225,14 @@ public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state,
                 // constraint creation, due to crash or similar, hence the missing owner.
                 try
                 {
-                    if ( readOperations.indexGetOwningUniquenessConstraintId( descriptor ) == null )
+                    if ( schema.indexGetOwningUniquenessConstraintId( state, descriptor ) == null )
                     {
                         return descriptor;
                     }
+                    throw new AlreadyConstrainedException(
+                            new UniquenessConstraint( descriptor.getLabelId(), descriptor.getPropertyKeyId() ),
+                            OperationContext.CONSTRAINT_CREATION,
+                            new StatementTokenNameLookup( state.readOperations() ) );
                 }
                 catch ( SchemaRuleNotFoundException e )
                 {

community/kernel/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintIndexCreatorTest.java

@@ -24,6 +24,7 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import org.neo4j.helpers.collection.Iterators;
 import org.neo4j.kernel.api.KernelAPI;
 import org.neo4j.kernel.api.KernelTransaction;
 import org.neo4j.kernel.api.Statement;
@@ -32,6 +33,7 @@
 import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.kernel.api.exceptions.TransactionFailureException;
 import org.neo4j.kernel.api.exceptions.index.IndexPopulationFailedKernelException;
+import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException;
 import org.neo4j.kernel.api.exceptions.schema.ConstraintVerificationFailedKernelException;
 import org.neo4j.kernel.api.index.IndexDescriptor;
 import org.neo4j.kernel.api.index.PreexistingIndexEntryConflictException;
@@ -56,6 +58,8 @@
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.verifyZeroInteractions;
 import static org.mockito.Mockito.when;
+
+import static org.neo4j.helpers.collection.Iterators.iterator;
 import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedParts;
 import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedState;
 import static org.neo4j.kernel.impl.store.SchemaStorage.IndexRuleKind.CONSTRAINT;
@@ -69,7 +73,8 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception
         StatementOperationParts constraintCreationContext = mockedParts();
         StatementOperationParts indexCreationContext = mockedParts();
 
-        IndexDescriptor descriptor = new IndexDescriptor( 123, 456 );
+        int labelId = 123;
+        IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 );
         KernelStatement state = mockedState();
 
         IndexingService indexingService = mock( IndexingService.class );
@@ -80,7 +85,8 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception
         IndexProxy indexProxy = mock( IndexProxy.class );
         when( indexingService.getIndexProxy( 2468L ) ).thenReturn( indexProxy );
         PropertyAccessor propertyAccessor = mock( PropertyAccessor.class );
-
+        when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) )
+                .thenReturn( Iterators.emptyIterator() );
         ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false );
 
         // when
@@ -92,6 +98,7 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception
         verify( kernel.statements.get( 0 ).txState() ).constraintIndexRuleDoAdd( descriptor );
         verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() );
         verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT );
+        verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId );
         verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() );
         verify( indexProxy ).awaitStoreScanCompleted();
     }
@@ -103,7 +110,8 @@ public void shouldDropIndexIfPopulationFails() throws Exception
         StatementOperationParts constraintCreationContext = mockedParts();
         KernelStatement state = mockedState();
 
-        IndexDescriptor descriptor = new IndexDescriptor( 123, 456 );
+        int labelId = 123;
+        IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 );
 
         IndexingService indexingService = mock( IndexingService.class );
         StubKernel kernel = new StubKernel();
@@ -116,13 +124,14 @@ public void shouldDropIndexIfPopulationFails() throws Exception
         doThrow( new IndexPopulationFailedKernelException( descriptor, "some index", cause) )
                 .when(indexProxy).awaitStoreScanCompleted();
         PropertyAccessor propertyAccessor = mock( PropertyAccessor.class );
-
+        when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) )
+                .thenReturn( Iterators.emptyIterator() );
         ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false );
 
         // when
         try
         {
-            creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, 456 );
+            creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), labelId, 456 );
 
             fail( "expected exception" );
         }
@@ -134,12 +143,13 @@ public void shouldDropIndexIfPopulationFails() throws Exception
         }
         assertEquals( 2, kernel.statements.size() );
         TransactionState tx1 = kernel.statements.get( 0 ).txState();
-        verify( tx1 ).constraintIndexRuleDoAdd( new IndexDescriptor( 123, 456 ) );
+        verify( tx1 ).constraintIndexRuleDoAdd( new IndexDescriptor( labelId, 456 ) );
         verifyNoMoreInteractions( tx1 );
         verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT );
+        verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId );
         verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() );
         TransactionState tx2 = kernel.statements.get( 1 ).txState();
-        verify( tx2 ).constraintIndexDoDrop( new IndexDescriptor( 123, 456 ) );
+        verify( tx2 ).constraintIndexDoDrop( new IndexDescriptor( labelId, 456 ) );
         verifyNoMoreInteractions( tx2 );
     }
 
@@ -171,7 +181,8 @@ public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Excepti
         StubKernel kernel = new StubKernel();
         IndexingService indexingService = mock( IndexingService.class );
         StatementOperationParts constraintCreationContext = mockedParts();
-        IndexDescriptor descriptor = new IndexDescriptor( 123, 456 );
+        int labelId = 123;
+        IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 );
         PropertyAccessor propertyAccessor = mock( PropertyAccessor.class );
 
         KernelStatement state = mockedState();
@@ -180,17 +191,114 @@ public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Excepti
                 .thenReturn( 2468L );
         IndexProxy indexProxy = mock( IndexProxy.class );
         when( indexingService.getIndexProxy( anyLong() ) ).thenReturn( indexProxy );
+        when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) )
+                .thenReturn( Iterators.emptyIterator() );
 
         ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, true );
 
         // when
-        creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, 456 );
+        creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), labelId, 456 );
 
         // then
         verify( state.locks().pessimistic() ).releaseExclusive( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() );
         verify( state.locks().pessimistic() ).acquireExclusive( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() );
     }
 
+    @Test
+    public void shouldReuseExistingOrphanedConstraintIndex() throws Exception
+    {
+        // given
+        StatementOperationParts constraintCreationContext = mockedParts();
+        StatementOperationParts indexCreationContext = mockedParts();
+
+        int labelId = 123;
+        int propertyKeyId = 456;
+        IndexDescriptor descriptor = new IndexDescriptor( labelId, propertyKeyId );
+        KernelStatement state = mockedState();
+
+        IndexingService indexingService = mock( IndexingService.class );
+        StubKernel kernel = new StubKernel();
+
+        long orphanedConstraintIndexId = 111;
+        when( constraintCreationContext.schemaReadOperations().indexGetCommittedId( state, descriptor, CONSTRAINT ) )
+                .thenReturn( orphanedConstraintIndexId );
+        IndexProxy indexProxy = mock( IndexProxy.class );
+        when( indexingService.getIndexProxy( orphanedConstraintIndexId ) ).thenReturn( indexProxy );
+        PropertyAccessor propertyAccessor = mock( PropertyAccessor.class );
+        when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) )
+                .thenReturn( iterator( descriptor ) );
+        when( constraintCreationContext.schemaReadOperations().indexGetOwningUniquenessConstraintId(
+                state, descriptor ) ).thenReturn( null ); // which means it has no owner
+        ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false );
+
+        // when
+        long indexId = creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, propertyKeyId );
+
+        // then
+        assertEquals( orphanedConstraintIndexId, indexId );
+        assertEquals( "There should have been no need to acquire a statement to create the constraint index", 0,
+                kernel.statements.size() );
+        verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() );
+        verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT );
+        verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId );
+        verify( constraintCreationContext.schemaReadOperations() )
+                .indexGetOwningUniquenessConstraintId( state, descriptor );
+        verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() );
+        verify( indexProxy ).awaitStoreScanCompleted();
+    }
+
+    @Test
+    public void shouldFailOnExistingOwnedConstraintIndex() throws Exception
+    {
+        // given
+        StatementOperationParts constraintCreationContext = mockedParts();
+        StatementOperationParts indexCreationContext = mockedParts();
+
+        int labelId = 123;
+        int propertyKeyId = 456;
+        IndexDescriptor descriptor = new IndexDescriptor( labelId, propertyKeyId );
+        KernelStatement state = mockedState();
+
+        IndexingService indexingService = mock( IndexingService.class );
+        StubKernel kernel = new StubKernel();
+
+        long constraintIndexId = 111;
+        long constraintIndexOwnerId = 222;
+        when( constraintCreationContext.schemaReadOperations().indexGetCommittedId( state, descriptor, CONSTRAINT ) )
+                .thenReturn( constraintIndexId );
+        IndexProxy indexProxy = mock( IndexProxy.class );
+        when( indexingService.getIndexProxy( constraintIndexId ) ).thenReturn( indexProxy );
+        PropertyAccessor propertyAccessor = mock( PropertyAccessor.class );
+        when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) )
+                .thenReturn( iterator( descriptor ) );
+        when( constraintCreationContext.schemaReadOperations().indexGetOwningUniquenessConstraintId(
+                state, descriptor ) ).thenReturn( constraintIndexOwnerId ); // which means there's an owner
+        when( state.readOperations().labelGetName( labelId ) ).thenReturn( "MyLabel" );
+        when( state.readOperations().propertyKeyGetName( propertyKeyId ) ).thenReturn( "MyKey" );
+        ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false );
+
+        // when
+        try
+        {
+            creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123,
+                    propertyKeyId );
+            fail( "Should've failed" );
+        }
+        catch ( AlreadyConstrainedException e )
+        {
+            // THEN good
+        }
+
+        // then
+        assertEquals( "There should have been no need to acquire a statement to create the constraint index", 0,
+                kernel.statements.size() );
+        verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() );
+        verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId );
+        verify( constraintCreationContext.schemaReadOperations() )
+                .indexGetOwningUniquenessConstraintId( state, descriptor );
+        verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() );
+    }
+
     private class StubKernel implements KernelAPI
     {
         private final List<KernelStatement> statements = new ArrayList<>();

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

@@ -32,6 +32,7 @@
 import org.neo4j.graphdb.Node;
 import org.neo4j.graphdb.Transaction;
 import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction;
+import org.neo4j.graphdb.schema.IndexDefinition;
 import org.neo4j.helpers.collection.Iterables;
 import org.neo4j.kernel.impl.api.index.IndexingService;
 import org.neo4j.kernel.impl.storageengine.impl.recordstorage.RecordStorageEngine;
@@ -44,16 +45,20 @@
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import static org.neo4j.helpers.collection.Iterables.single;
+
 public class ConstraintRecoveryIT
 {
+    private static final String KEY = "prop";
+    private static final Label LABEL = Label.label( "label1" );
+
     @Rule
     public EphemeralFileSystemRule fileSystemRule = new EphemeralFileSystemRule();
 
-    private static final Label LABEL = Label.label( "label1" );
     private GraphDatabaseAPI db;
 
     @Test
-    public void shouldNotHaveAnIndexIfUniqueConstraintCreationOnRecoveryFails() throws IOException
+    public void shouldHaveAvailableOrphanedConstraintIndexIfUniqueConstraintCreationFails() throws IOException
     {
         // given
         final EphemeralFileSystemAbstraction fs = fileSystemRule.get();
@@ -87,15 +92,15 @@ public void indexPopulationScanComplete()
             for ( int i = 0; i < 2; i++ )
             {
                 Node node1 = db.createNode( LABEL );
-                node1.setProperty( "prop", true );
+                node1.setProperty( KEY, true );
             }
 
             tx.success();
         }
 
         try ( Transaction tx = db.beginTx() )
         {
-            db.schema().constraintFor( LABEL ).assertPropertyIsUnique( "prop" ).create();
+            db.schema().constraintFor( LABEL ).assertPropertyIsUnique( KEY ).create();
             fail("Should have failed with ConstraintViolationException");
             tx.success();
         }
@@ -128,7 +133,9 @@ public void indexPopulationScanComplete()
 
         try(Transaction tx = db.beginTx())
         {
-            assertEquals(0, Iterables.count(Iterables.asList( db.schema().getIndexes() )));
+            IndexDefinition orphanedConstraintIndex = single( db.schema().getIndexes() );
+            assertEquals( LABEL.name(), orphanedConstraintIndex.getLabel().name() );
+            assertEquals( KEY, single( orphanedConstraintIndex.getPropertyKeys() ) );
         }
 
         db.shutdown();