From 9f7babce2fa9c99d2bd7799f2e1198c87f553b48 Mon Sep 17 00:00:00 2001 From: Ben Butler-Cole Date: Tue, 13 Sep 2016 16:41:09 +0100 Subject: [PATCH] Modify builtin index-related procedures to take a single 'specifier' rather than separate label and property --- .../builtinprocs/BuiltInProcedures.java | 14 ++- .../kernel/builtinprocs/IndexProcedures.java | 43 ++++----- .../kernel/builtinprocs/IndexSpecifier.java | 96 +++++++++++++++++++ .../builtinprocs/AwaitIndexProcedureTest.java | 18 ++-- .../builtinprocs/BuiltInProceduresTest.java | 4 +- .../builtinprocs/IndexSpecifierTest.java | 73 ++++++++++++++ .../ResampleIndexProcedureTest.java | 14 +-- .../integrationtest/BuiltInProceduresIT.java | 8 +- 8 files changed, 217 insertions(+), 53 deletions(-) create mode 100644 community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexSpecifier.java create mode 100644 community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/IndexSpecifierTest.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/BuiltInProcedures.java b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/BuiltInProcedures.java index 34056bf6cd809..68d13073b9066 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/BuiltInProcedures.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/BuiltInProcedures.java @@ -125,27 +125,25 @@ public Stream listIndexes() throws ProcedureException } } - @Description( "Wait for an index to come online." ) + @Description( "Wait for an index to come online (for example: CALL db.awaitIndex(\":Person(name)\"))." ) @Procedure( name = "db.awaitIndex", mode = READ ) - public void awaitIndex( @Name( "label" ) String labelName, - @Name( "property" ) String propertyKeyName, + public void awaitIndex( @Name( "index" ) String index, @Name( value = "timeOutSeconds", defaultValue = "300" ) long timeout ) throws ProcedureException { try ( IndexProcedures indexProcedures = indexProcedures() ) { - indexProcedures.awaitIndex( labelName, propertyKeyName, timeout, TimeUnit.SECONDS ); + indexProcedures.awaitIndex( index, timeout, TimeUnit.SECONDS ); } } - @Description( "Schedule resampling of an index." ) + @Description( "Schedule resampling of an index (for example: CALL db.resampleIndex(\":Person(name)\"))." ) @Procedure( name = "db.resampleIndex", mode = READ ) - public void resampleIndex( @Name( "label" ) String labelName, - @Name( "property" ) String propertyKeyName ) throws ProcedureException + public void resampleIndex( @Name( "index" ) String index ) throws ProcedureException { try ( IndexProcedures indexProcedures = indexProcedures() ) { - indexProcedures.resampleIndex( labelName, propertyKeyName ); + indexProcedures.resampleIndex( index ); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexProcedures.java b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexProcedures.java index dc0a74f9b73e2..aaf232f591aee 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexProcedures.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexProcedures.java @@ -35,8 +35,6 @@ import org.neo4j.kernel.impl.api.index.IndexingService; import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingMode; -import static java.lang.String.format; - public class IndexProcedures implements AutoCloseable { private final Statement statement; @@ -50,22 +48,19 @@ public IndexProcedures( KernelTransaction tx, IndexingService indexingService ) this.indexingService = indexingService; } - public void awaitIndex( String labelName, String propertyKeyName, long timeout, TimeUnit timeoutUnits ) + public void awaitIndex( String indexSpecification, long timeout, TimeUnit timeoutUnits ) throws ProcedureException { - int labelId = getLabelId( labelName ); - int propertyKeyId = getPropertyKeyId( propertyKeyName ); - String indexDescription = formatIndex( labelName, propertyKeyName ); - IndexDescriptor index = getIndex( labelId, propertyKeyId, indexDescription ); - waitUntilOnline( index, indexDescription, timeout, timeoutUnits ); + IndexSpecifier index = parse( indexSpecification ); + int labelId = getLabelId( index.label() ); + int propertyKeyId = getPropertyKeyId( index.property() ); + waitUntilOnline( getIndex( labelId, propertyKeyId, index ), index, timeout, timeoutUnits ); } - public void resampleIndex( String labelName, String propertyKeyName ) throws ProcedureException + public void resampleIndex( String indexSpecification ) throws ProcedureException { - triggerSampling( getIndex( - getLabelId( labelName ), - getPropertyKeyId( propertyKeyName ), - formatIndex( labelName, propertyKeyName ) ) ); + IndexSpecifier index = parse( indexSpecification ); + triggerSampling( getIndex( getLabelId( index.label() ), getPropertyKeyId( index.property() ), index ) ); } public void resampleOutdatedIndexes() @@ -73,6 +68,11 @@ public void resampleOutdatedIndexes() indexingService.triggerIndexSampling( IndexSamplingMode.TRIGGER_REBUILD_UPDATED ); } + private IndexSpecifier parse( String specification ) + { + return new IndexSpecifier( specification ); + } + private int getLabelId( String labelName ) throws ProcedureException { int labelId = operations.labelGetForName( labelName ); @@ -94,7 +94,7 @@ private int getPropertyKeyId( String propertyKeyName ) throws ProcedureException return propertyKeyId; } - private IndexDescriptor getIndex( int labelId, int propertyKeyId, String indexDescription ) throws + private IndexDescriptor getIndex( int labelId, int propertyKeyId, IndexSpecifier index ) throws ProcedureException { try @@ -103,11 +103,12 @@ private IndexDescriptor getIndex( int labelId, int propertyKeyId, String indexDe } catch ( SchemaRuleNotFoundException e ) { - throw new ProcedureException( Status.Schema.IndexNotFound, e, "No index on %s", indexDescription ); + throw new ProcedureException( Status.Schema.IndexNotFound, e, "No index on %s", index ); } } - private void waitUntilOnline( IndexDescriptor index, String indexDescription, long timeout, TimeUnit timeoutUnits ) + private void waitUntilOnline( IndexDescriptor index, IndexSpecifier indexDescription, long timeout, TimeUnit + timeoutUnits ) throws ProcedureException { try @@ -126,7 +127,7 @@ private void waitUntilOnline( IndexDescriptor index, String indexDescription, lo } } - private boolean isOnline( String indexDescription, IndexDescriptor index ) throws ProcedureException + private boolean isOnline( IndexSpecifier indexDescription, IndexDescriptor index ) throws ProcedureException { InternalIndexState state = getState( indexDescription, index ); switch ( state ) @@ -143,7 +144,8 @@ private boolean isOnline( String indexDescription, IndexDescriptor index ) throw } } - private InternalIndexState getState( String indexDescription, IndexDescriptor index ) throws ProcedureException + private InternalIndexState getState( IndexSpecifier indexDescription, IndexDescriptor index ) + throws ProcedureException { try { @@ -160,11 +162,6 @@ private void triggerSampling( IndexDescriptor index ) indexingService.triggerIndexSampling( index, IndexSamplingMode.TRIGGER_REBUILD_ALL ); } - private String formatIndex( String labelName, String propertyKeyName ) - { - return format( ":%s(%s)", labelName, propertyKeyName ); - } - @Override public void close() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexSpecifier.java b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexSpecifier.java new file mode 100644 index 0000000000000..558aa11d11ec0 --- /dev/null +++ b/community/kernel/src/main/java/org/neo4j/kernel/builtinprocs/IndexSpecifier.java @@ -0,0 +1,96 @@ +/* + * Copyright (c) 2002-2016 "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.builtinprocs; + +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.neo4j.helpers.collection.Pair; + +import static java.lang.String.format; + +public class IndexSpecifier +{ + + private final String specification; + private final String label; + private final String property; + + public IndexSpecifier( String specification ) + { + this.specification = specification; + Pair components = parse(); + label = components.first(); + property = components.other(); + } + + public String label() + { + return label; + } + + public String property() + { + return property; + } + + @Override + public String toString() + { + return specification; + } + + private Pair parse() + { + Pattern pattern = Pattern.compile( "" + + ":" + or( simpleIdentifier( "simpleLabel" ), complexIdentifier( "complexLabel" ) ) + + "\\(" + or( simpleIdentifier( "simpleProperty" ), complexIdentifier( "complexProperty" ) ) + "\\)" + ); + Matcher matcher = pattern.matcher( specification ); + if ( !matcher.find() ) + { + throw new IllegalArgumentException( "Cannot parse index specification " + specification ); + } + return Pair.of( + either( matcher.group( "simpleLabel" ), matcher.group( "complexLabel" ) ), + either( matcher.group( "simpleProperty" ), matcher.group( "complexProperty" ) ) + ); + } + + private String either( String first, String second ) + { + return first != null ? first : second; + } + + private static String or( String first, String second ) + { + return "(:?" + first + "|" + second + ")"; + } + + private static String simpleIdentifier( String name ) + { + return format( "(?<%s>[A-Za-z0-9_]+)", name ); + } + + private static String complexIdentifier( String name ) + { + return format( "(?:`(?<%s>[^`]+)`)", name ); + } +} diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/AwaitIndexProcedureTest.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/AwaitIndexProcedureTest.java index 2600ef9825485..4839e69e7949e 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/AwaitIndexProcedureTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/AwaitIndexProcedureTest.java @@ -63,11 +63,11 @@ public class AwaitIndexProcedureTest @Test public void shouldThrowAnExceptionIfTheLabelDoesntExist() throws ProcedureException { - when( operations.labelGetForName( "non-existent-label" ) ).thenReturn( -1 ); + when( operations.labelGetForName( "NonExistentLabel" ) ).thenReturn( -1 ); try { - procedure.awaitIndex( "non-existent-label", null, timeout, timeoutUnits ); + procedure.awaitIndex( ":NonExistentLabel(prop)", timeout, timeoutUnits ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -79,11 +79,11 @@ public void shouldThrowAnExceptionIfTheLabelDoesntExist() throws ProcedureExcept @Test public void shouldThrowAnExceptionIfThePropertyKeyDoesntExist() throws ProcedureException { - when( operations.propertyKeyGetForName( "non-existent-property-key" ) ).thenReturn( -1 ); + when( operations.propertyKeyGetForName( "nonExistentProperty" ) ).thenReturn( -1 ); try { - procedure.awaitIndex( null, "non-existent-property-key", timeout, timeoutUnits ); + procedure.awaitIndex( ":Label(nonExistentProperty)", timeout, timeoutUnits ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -102,7 +102,7 @@ public void shouldLookUpTheIndexByLabelIdAndPropertyKeyId() .thenReturn( new IndexDescriptor( 0, 0 ) ); when( operations.indexGetState( any( IndexDescriptor.class ) ) ).thenReturn( ONLINE ); - procedure.awaitIndex( null, null, timeout, timeoutUnits ); + procedure.awaitIndex( ":Person(name)", timeout, timeoutUnits ); verify( operations ).indexGetForLabelAndPropertyKey( 123, 456 ); } @@ -120,7 +120,7 @@ public void shouldThrowAnExceptionIfTheIndexHasFailed() try { - procedure.awaitIndex( null, null, timeout, timeoutUnits ); + procedure.awaitIndex( ":Person(name)", timeout, timeoutUnits ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -141,7 +141,7 @@ public void shouldThrowAnExceptionIfTheIndexDoesNotExist() try { - procedure.awaitIndex( null, null, timeout, timeoutUnits ); + procedure.awaitIndex( ":Person(name)", timeout, timeoutUnits ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -174,7 +174,7 @@ public InternalIndexState answer( InvocationOnMock invocationOnMock ) throws Thr { try { - procedure.awaitIndex( null, null, timeout, timeoutUnits ); + procedure.awaitIndex( ":Person(name)", timeout, timeoutUnits ); } catch ( ProcedureException e ) { @@ -205,7 +205,7 @@ public void shouldTimeoutIfTheIndexTakesTooLongToComeOnline() { try { - procedure.awaitIndex( null, null, timeout, timeoutUnits ); + procedure.awaitIndex( ":Person(name)", timeout, timeoutUnits ); } catch ( ProcedureException e ) { diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/BuiltInProceduresTest.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/BuiltInProceduresTest.java index 484526806677f..57799376a23bf 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/BuiltInProceduresTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/BuiltInProceduresTest.java @@ -173,13 +173,13 @@ public void shouldListCorrectBuiltinProcedures() throws Throwable { // When/Then assertThat( call( "dbms.procedures" ), containsInAnyOrder( - record( "db.awaitIndex", "db.awaitIndex(label :: STRING?, property :: STRING?, timeOutSeconds = 300 :: INTEGER?) :: VOID", "Wait for an index to come online." ), + record( "db.awaitIndex", "db.awaitIndex(index :: STRING?, timeOutSeconds = 300 :: INTEGER?) :: VOID", "Wait for an index to come online (for example: CALL db.awaitIndex(\":Person(name)\"))." ), record( "db.constraints", "db.constraints() :: (description :: STRING?)", "List all constraints in the database." ), record( "db.indexes", "db.indexes() :: (description :: STRING?, state :: STRING?, type :: STRING?)", "List all indexes in the database." ), record( "db.labels", "db.labels() :: (label :: STRING?)", "List all labels in the database." ), record( "db.propertyKeys", "db.propertyKeys() :: (propertyKey :: STRING?)", "List all property keys in the database." ), record( "db.relationshipTypes", "db.relationshipTypes() :: (relationshipType :: STRING?)", "List all relationship types in the database." ), - record( "db.resampleIndex", "db.resampleIndex(label :: STRING?, property :: STRING?) :: VOID", "Schedule resampling of an index." ), + record( "db.resampleIndex", "db.resampleIndex(index :: STRING?) :: VOID", "Schedule resampling of an index (for example: CALL db.resampleIndex(\":Person(name)\"))." ), record( "db.resampleOutdatedIndexes", "db.resampleOutdatedIndexes() :: VOID", "Schedule resampling of all outdated indexes." ), record( "dbms.components", "dbms.components() :: (name :: STRING?, versions :: LIST? OF STRING?, edition :: STRING?)", "List DBMS components and their versions." ), record( "dbms.procedures", "dbms.procedures() :: (name :: STRING?, signature :: STRING?, description :: STRING?)", "List all procedures in the DBMS." ), diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/IndexSpecifierTest.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/IndexSpecifierTest.java new file mode 100644 index 0000000000000..880e4d20d9f1a --- /dev/null +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/IndexSpecifierTest.java @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2002-2016 "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.builtinprocs; + +import org.junit.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.fail; + +public class IndexSpecifierTest +{ + @Test + public void shouldFormatAsCanonicalRepresentation() + { + assertThat( new IndexSpecifier( ":Person(name)" ).toString(), is( ":Person(name)" ) ); + } + + @Test + public void shouldParseASimpleLabel() + { + assertThat( new IndexSpecifier( ":Person_23(name)" ).label(), is( "Person_23" ) ); + } + + @Test + public void shouldParseASimpleProperty() + { + assertThat( new IndexSpecifier( ":Person(a_Name_123)" ).property(), is( "a_Name_123" ) ); + } + + @Test + public void shouldParseANastyLabel() + { + assertThat( new IndexSpecifier( ":`:(!\"£$%^&*( )`(name)" ).label(), is( ":(!\"£$%^&*( )" ) ); + } + + @Test + public void shouldParseANastyProperty() + { + assertThat( new IndexSpecifier( ":Person(`(:!\"£$%^&*( )`)" ).property(), is( "(:!\"£$%^&*( )" ) ); + } + + @Test + public void shouldProduceAReasonableErrorIfTheSpecificationCantBeParsed() + { + try + { + new IndexSpecifier( "rubbish" ); + fail( "expected exception" ); + } + catch ( IllegalArgumentException e ) + { + //expected + } + } +} diff --git a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/ResampleIndexProcedureTest.java b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/ResampleIndexProcedureTest.java index 8bd9eb1b6c923..8ce810731b57c 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/ResampleIndexProcedureTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/builtinprocs/ResampleIndexProcedureTest.java @@ -50,11 +50,11 @@ public class ResampleIndexProcedureTest @Test public void shouldThrowAnExceptionIfTheLabelDoesntExist() throws ProcedureException { - when( operations.labelGetForName( "non-existent-label" ) ).thenReturn( -1 ); + when( operations.labelGetForName( "NonExistentLabel" ) ).thenReturn( -1 ); try { - procedure.resampleIndex( "non-existent-label", null ); + procedure.resampleIndex( ":NonExistentLabel(prop)" ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -66,11 +66,11 @@ public void shouldThrowAnExceptionIfTheLabelDoesntExist() throws ProcedureExcept @Test public void shouldThrowAnExceptionIfThePropertyKeyDoesntExist() throws ProcedureException { - when( operations.propertyKeyGetForName( "non-existent-property-key" ) ).thenReturn( -1 ); + when( operations.propertyKeyGetForName( "nonExistentProperty" ) ).thenReturn( -1 ); try { - procedure.resampleIndex( null, "non-existent-property-key" ); + procedure.resampleIndex( ":Label(nonExistentProperty)" ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -88,7 +88,7 @@ public void shouldLookUpTheIndexByLabelIdAndPropertyKeyId() when( operations.indexGetForLabelAndPropertyKey( anyInt(), anyInt() ) ) .thenReturn( new IndexDescriptor( 0, 0 ) ); - procedure.resampleIndex( null, null ); + procedure.resampleIndex( ":Person(name)" ); verify( operations ).indexGetForLabelAndPropertyKey( 123, 456 ); } @@ -105,7 +105,7 @@ public void shouldThrowAnExceptionIfTheIndexDoesNotExist() try { - procedure.resampleIndex( null, null ); + procedure.resampleIndex( ":Person(name)" ); fail( "Expected an exception" ); } catch ( ProcedureException e ) @@ -120,7 +120,7 @@ public void shouldTriggerResampling() throws SchemaRuleNotFoundException, Proced IndexDescriptor index = new IndexDescriptor( 123, 456 ); when( operations.indexGetForLabelAndPropertyKey( anyInt(), anyInt() ) ).thenReturn( index ); - procedure.resampleIndex( null, null ); + procedure.resampleIndex( ":Person(name)" ); verify( indexingService ).triggerIndexSampling( index, IndexSamplingMode.TRIGGER_REBUILD_ALL ); } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java index 136bad004a54e..1ee9b1c20e472 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/integrationtest/BuiltInProceduresIT.java @@ -107,10 +107,10 @@ public void listProcedures() throws Throwable assertThat( asList( stream ), containsInAnyOrder( equalTo( new Object[]{"db.constraints", "db.constraints() :: (description :: STRING?)", "List all constraints in the database."} ), equalTo( new Object[]{"db.indexes", "db.indexes() :: (description :: STRING?, state :: STRING?, type :: STRING?)", "List all indexes in the database."} ), - equalTo( new Object[]{"db.awaitIndex", "db.awaitIndex(label :: STRING?, property :: STRING?, " + - "timeOutSeconds = 300 :: INTEGER?) :: VOID", "Wait for an index to come online."} ), - equalTo( new Object[]{"db.resampleIndex", "db.resampleIndex(label :: STRING?, property :: STRING?) " + - ":: VOID", "Schedule resampling of an index."} ), + equalTo( new Object[]{"db.awaitIndex", "db.awaitIndex(index :: STRING?, timeOutSeconds = 300 :: INTEGER?) :: VOID", + "Wait for an index to come online (for example: CALL db.awaitIndex(\":Person(name)\"))."} ), + equalTo( new Object[]{"db.resampleIndex", "db.resampleIndex(index :: STRING?) :: VOID", + "Schedule resampling of an index (for example: CALL db.resampleIndex(\":Person(name)\"))."} ), equalTo( new Object[]{"db.resampleOutdatedIndexes", "db.resampleOutdatedIndexes() :: VOID", "Schedule resampling of all outdated indexes."} ), equalTo( new Object[]{"db.propertyKeys", "db.propertyKeys() :: (propertyKey :: STRING?)", "List all property keys in the database."} ),