diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java index 7754f7f48290..466c5a2dbb95 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java +++ b/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 type of values being merged. */ -class ConflictDetectingValueMerger implements ValueMerger +abstract class ConflictDetectingValueMerger implements ValueMerger { - 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 ConflictDetectingValueMerger dontCheck() { - return existingNodeId; + return DONT_CHECK; } - long addedNodeId() + static class Check extends ConflictDetectingValueMerger { - 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 ) ); + } + } } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java index a901609ad361..11c8a6173de8 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java @@ -74,7 +74,7 @@ public abstract class NativeSchemaNumberIndexPopulator(); + this.conflictDetectingValueMerger = new ConflictDetectingValueMerger.Check<>(); } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java index 15eb2e1d1121..047fff56d318 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java +++ b/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 implements IndexUpdater { private final KEY treeKey; private final VALUE treeValue; - private final ConflictDetectingValueMerger conflictDetectingValueMerger; + private final ConflictDetectingValueMerger conflictDetectingValueMerger = dontCheck(); private Writer writer; private boolean closed = true; @@ -42,7 +44,6 @@ class NativeSchemaNumberIndexUpdater(); } NativeSchemaNumberIndexUpdater initialize( Writer writer, boolean manageClosingOfWriter ) @@ -124,7 +125,7 @@ private static vo treeKey.from( update.getEntityId(), update.values() ); treeValue.from( update.values() ); writer.merge( treeKey, treeValue, conflictDetectingValueMerger ); - assertNoConflict( update, conflictDetectingValueMerger ); + conflictDetectingValueMerger.checkConflict( update.values() ); } static void processAdd( KEY treeKey, VALUE treeValue, @@ -135,17 +136,6 @@ static void proce treeKey.from( update.getEntityId(), update.values() ); treeValue.from( update.values() ); writer.merge( treeKey, treeValue, conflictDetectingValueMerger ); - assertNoConflict( update, conflictDetectingValueMerger ); - } - - private static void assertNoConflict( IndexEntryUpdate update, - ConflictDetectingValueMerger 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() ); } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java index fad3089a2805..9d3da5ef1a48 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java +++ b/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 detector = new ConflictDetectingValueMerger<>(); + private final ConflictDetectingValueMerger 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 ) diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java index ea5101175e0a..a1aa6c165845 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java +++ b/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 ) ); diff --git a/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java b/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java new file mode 100644 index 000000000000..d6953a6f2189 --- /dev/null +++ b/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 . + */ +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 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(); + } + } +}