Fixed leaking space filling curve settings

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

@@ -66,7 +66,7 @@ <T> void loadExistingIndexes( SpatialIndexCache<T> indexCache ) throws IOExcepti
 
     SpatialFileLayout forCrs( CoordinateReferenceSystem crs )
     {
-        return new SpatialFileLayout( crs, settingsFactory.settingsFor( crs ), indexDirectory );
+        return new SpatialFileLayout( crs, settingsFactory.settingsFor( crs ).clone(), indexDirectory );
     }
 
     private void addExistingFiles( List<SpatialFileLayout> existing )

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

@@ -69,12 +69,28 @@ public class SpaceFillingCurveSettings
     private String failureMessage;
 
     public SpaceFillingCurveSettings( int dimensions, int maxBits, Envelope extents )
+    {
+        this( dimensions, extents, calcMaxLevels( dimensions, maxBits ) );
+    }
+
+    private SpaceFillingCurveSettings( int dimensions, Envelope extents, int maxLevels )
     {
         this.dimensions = dimensions;
         this.extents = extents;
+        this.maxLevels = maxLevels;
+    }
+
+    @Override
+    public SpaceFillingCurveSettings clone()
+    {
+        return new SpaceFillingCurveSettings( this.dimensions, this.extents, this.maxLevels );
+    }
+
+    private static int calcMaxLevels( int dimensions, int maxBits )
+    {
         int maxConfigured = maxBits / dimensions;
         int maxSupported = (dimensions == 2) ? HilbertSpaceFillingCurve2D.MAX_LEVEL : HilbertSpaceFillingCurve3D.MAX_LEVEL;
-        this.maxLevels = Math.min( maxConfigured, maxSupported );
+        return Math.min( maxConfigured, maxSupported );
     }
 
     /**
@@ -159,6 +175,24 @@ else if ( dimensions == 3 )
         }
     }
 
+    public boolean equals( SpaceFillingCurveSettings other )
+    {
+        return this.dimensions == other.dimensions && this.maxLevels == other.maxLevels && this.extents.equals( other.extents );
+    }
+
+    @Override
+    public boolean equals( Object obj )
+    {
+        if ( obj instanceof SpaceFillingCurveSettings )
+        {
+            return equals( (SpaceFillingCurveSettings) obj );
+        }
+        else
+        {
+            return false;
+        }
+    }
+
     @Override
     public String toString()
     {

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

@@ -42,8 +42,6 @@ abstract class SpatialIndexAccessorTest extends NativeIndexAccessorTest<SpatialI
     NativeIndexAccessor<SpatialIndexKey,NativeIndexValue> makeAccessorWithSamplingConfig( IndexSamplingConfig samplingConfig ) throws IOException
     {
         fileLayout = new SpatialIndexFiles.SpatialFileLayout( CoordinateReferenceSystem.WGS84, settings, super.getIndexFile() );
-        SpatialIndexFiles.SpatialFileLayout fileLayout =
-                new SpatialIndexFiles.SpatialFileLayout( CoordinateReferenceSystem.WGS84, settings, super.getIndexFile() );
         return new SpatialIndexAccessor.PartAccessor( pageCache, fs, fileLayout, IMMEDIATE, monitor, indexDescriptor, samplingConfig,
                 new StandardConfiguration() );
     }

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

@@ -0,0 +1,257 @@
+/*
+ * Copyright (c) 2002-2018 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.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.impl.index.schema;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.RuleChain;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.neo4j.gis.spatial.index.curves.StandardConfiguration;
+import org.neo4j.graphdb.config.Setting;
+import org.neo4j.index.internal.gbptree.GBPTree;
+import org.neo4j.io.pagecache.IOLimiter;
+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;
+import org.neo4j.kernel.api.index.IndexProvider;
+import org.neo4j.kernel.api.index.IndexUpdater;
+import org.neo4j.kernel.api.schema.index.SchemaIndexDescriptor;
+import org.neo4j.kernel.api.schema.index.SchemaIndexDescriptorFactory;
+import org.neo4j.kernel.configuration.Config;
+import org.neo4j.kernel.impl.api.index.sampling.IndexSamplingConfig;
+import org.neo4j.kernel.impl.index.schema.config.SpaceFillingCurveSettings;
+import org.neo4j.kernel.impl.index.schema.config.SpaceFillingCurveSettingsFactory;
+import org.neo4j.kernel.impl.index.schema.config.SpatialIndexSettings;
+import org.neo4j.test.rule.PageCacheRule;
+import org.neo4j.test.rule.TestDirectory;
+import org.neo4j.test.rule.fs.DefaultFileSystemRule;
+import org.neo4j.values.storable.CoordinateReferenceSystem;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+import static org.junit.rules.RuleChain.outerRule;
+import static org.neo4j.index.internal.gbptree.RecoveryCleanupWorkCollector.IMMEDIATE;
+import static org.neo4j.kernel.api.index.IndexDirectoryStructure.directoriesByProvider;
+import static org.neo4j.kernel.impl.api.index.IndexUpdateMode.ONLINE;
+import static org.neo4j.test.rule.PageCacheRule.config;
+
+public class SpatialIndexSettingsTest
+{
+    private static final CoordinateReferenceSystem crs = CoordinateReferenceSystem.WGS84;
+    private static final Config config1 = Config.defaults();
+    private static final Config config2 = configWithRange( 0, -90, 180, 90 );
+    private static final SpaceFillingCurveSettingsFactory settings1 = new SpaceFillingCurveSettingsFactory( config1 );
+    private static final SpaceFillingCurveSettingsFactory settings2 = new SpaceFillingCurveSettingsFactory( config2 );
+
+    private SchemaIndexDescriptor schemaIndexDescriptor1;
+    private SchemaIndexDescriptor schemaIndexDescriptor2;
+    private LayoutTestUtil<SpatialSchemaKey,NativeSchemaValue> layoutUtil1;
+    private LayoutTestUtil<SpatialSchemaKey,NativeSchemaValue> layoutUtil2;
+    private long indexId1 = 1;
+    private long indexId2 = 2;
+
+    final DefaultFileSystemRule fs = new DefaultFileSystemRule();
+    private final TestDirectory directory = TestDirectory.testDirectory( getClass(), fs.get() );
+    private final PageCacheRule pageCacheRule = new PageCacheRule( config().withAccessChecks( true ) );
+
+    @Rule
+    public final RuleChain rules = outerRule( fs ).around( directory ).around( pageCacheRule );
+
+    private PageCache pageCache;
+    private IndexProvider.Monitor monitor = IndexProvider.Monitor.EMPTY;
+
+    @Before
+    public void setupTwoIndexes() throws IOException
+    {
+        pageCache = pageCacheRule.getPageCache( fs );
+
+        // Define two indexes based on different labels and different settings
+        layoutUtil1 = createLayoutTestUtil( 42, settings1 );
+        layoutUtil2 = createLayoutTestUtil( 43, settings2 );
+        schemaIndexDescriptor1 = layoutUtil1.indexDescriptor();
+        schemaIndexDescriptor2 = layoutUtil2.indexDescriptor();
+
+        // Create the two indexes as empty, based on differently configured settings above
+        createEmptyIndex( indexId1, schemaIndexDescriptor1, settings1 );
+        createEmptyIndex( indexId2, schemaIndexDescriptor2, settings2 );
+    }
+
+    @Test
+    public void shouldAddToSpatialIndexWithDefaults() throws Exception
+    {
+        // given
+        SpatialIndexProvider provider = newSpatialIndexProvider( config1 );
+        addUpdates( provider, indexId1, schemaIndexDescriptor1, layoutUtil1 );
+
+        // then
+        verifySpatialSettings( indexFile( indexId1 ), settings1.settingsFor( crs ) );
+    }
+
+    @Test
+    public void shouldAddToSpatialIndexWithModifiedSettings() throws Exception
+    {
+        // given
+        SpatialIndexProvider provider = newSpatialIndexProvider( config2 );
+        addUpdates( provider, indexId2, schemaIndexDescriptor2, layoutUtil2 );
+
+        // then
+        verifySpatialSettings( indexFile( indexId2 ), settings2.settingsFor( crs ) );
+    }
+
+    @Test
+    public void shouldAddToTwoDifferentIndexesOneDefaultAndOneModified() throws Exception
+    {
+        // given
+        SpatialIndexProvider provider = newSpatialIndexProvider( config2 );
+        addUpdates( provider, indexId1, schemaIndexDescriptor1, layoutUtil1 );
+        addUpdates( provider, indexId2, schemaIndexDescriptor2, layoutUtil2 );
+
+        // then even though the provider was created with modified settings, only the second index should have them
+        verifySpatialSettings( indexFile( indexId1 ), settings1.settingsFor( crs ) );
+        verifySpatialSettings( indexFile( indexId2 ), settings2.settingsFor( crs ) );
+    }
+
+    @Test
+    public void shouldNotLeakSpaceFillingCurveSettingsBetweenExistingAndNewIndexes() throws Exception
+    {
+        // given two indexes previously created with different settings
+        Config config = configWithRange( -10, -10, 10, 10 );
+        SpatialIndexProvider provider = newSpatialIndexProvider( config );
+        addUpdates( provider, indexId1, schemaIndexDescriptor1, layoutUtil1 );
+        addUpdates( provider, indexId2, schemaIndexDescriptor2, layoutUtil2 );
+
+        // and when creating and populating a third index with a third set of settings
+        long indexId3 = 3;
+        SpaceFillingCurveSettingsFactory settings3 = new SpaceFillingCurveSettingsFactory( config );
+        SpatialLayoutTestUtil layoutUtil3 = createLayoutTestUtil( 44, settings3 );
+        SchemaIndexDescriptor schemaIndexDescriptor3 = layoutUtil3.indexDescriptor();
+        createEmptyIndex( indexId3, schemaIndexDescriptor3, provider );
+        addUpdates( provider, indexId3, schemaIndexDescriptor3, layoutUtil3 );
+
+        // Then all indexes should still have their own correct and different settings
+        verifySpatialSettings( indexFile( indexId1 ), settings1.settingsFor( crs ) );
+        verifySpatialSettings( indexFile( indexId2 ), settings2.settingsFor( crs ) );
+        verifySpatialSettings( indexFile( indexId3 ), settings3.settingsFor( crs ) );
+    }
+
+    private IndexSamplingConfig samplingConfig()
+    {
+        return new IndexSamplingConfig( Config.defaults() );
+    }
+
+    private SpatialLayoutTestUtil createLayoutTestUtil( int labelId, SpaceFillingCurveSettingsFactory settings )
+    {
+        return new SpatialLayoutTestUtil( SchemaIndexDescriptorFactory.forLabel( labelId, 666 ), settings.settingsFor( crs ), crs );
+    }
+
+    private SpatialIndexProvider newSpatialIndexProvider( Config config )
+    {
+        return new SpatialIndexProvider( pageCache, fs, directoriesByProvider( directory.graphDbDir() ), monitor, IMMEDIATE, false, config );
+    }
+
+    private void addUpdates( SpatialIndexProvider provider, long indexId, SchemaIndexDescriptor schemaIndexDescriptor,
+            LayoutTestUtil<SpatialSchemaKey,NativeSchemaValue> layoutUtil ) throws IOException, IndexEntryConflictException
+    {
+        IndexAccessor accessor = provider.getOnlineAccessor( indexId, schemaIndexDescriptor, samplingConfig() );
+        try ( IndexUpdater updater = accessor.newUpdater( ONLINE ) )
+        {
+            // when
+            for ( IndexEntryUpdate<SchemaIndexDescriptor> update : layoutUtil.someUpdates() )
+            {
+                updater.process( update );
+            }
+        }
+        accessor.force( IOLimiter.unlimited() );
+        accessor.close();
+    }
+
+    private SpatialIndexFiles.SpatialFileLayout makeFileLayout( long indexId, SpaceFillingCurveSettingsFactory settings )
+    {
+        return new SpatialIndexFiles.SpatialFileLayout( CoordinateReferenceSystem.WGS84, settings.settingsFor( crs ), indexDir( indexId ) );
+    }
+
+    private File indexDir( long indexId )
+    {
+        return new File( indexRoot(), Long.toString( indexId ) );
+    }
+
+    private File indexFile( long indexId )
+    {
+        // The indexFile location is independent of the settings, so we just use the defaults
+        return makeFileLayout( indexId, new SpaceFillingCurveSettingsFactory( Config.defaults() ) ).indexFile;
+    }
+
+    private File indexRoot()
+    {
+        return new File( new File( new File( directory.graphDbDir(), "schema" ), "index" ), "spatial-1.0" );
+    }
+
+    private void createEmptyIndex( long indexId, SchemaIndexDescriptor schemaIndexDescriptor, SpaceFillingCurveSettingsFactory settings ) throws IOException
+    {
+        SpatialIndexFiles.SpatialFileLayout fileLayout = makeFileLayout( indexId, settings );
+        SpatialIndexPopulator.PartPopulator populator =
+                new SpatialIndexPopulator.PartPopulator( pageCache, fs, fileLayout, monitor, schemaIndexDescriptor, indexId, samplingConfig(),
+                        new StandardConfiguration() );
+        populator.create();
+        populator.close( true );
+    }
+
+    private void createEmptyIndex( long indexId, SchemaIndexDescriptor schemaIndexDescriptor, SpatialIndexProvider provider ) throws IOException
+    {
+        IndexPopulator populator = provider.getPopulator( indexId, schemaIndexDescriptor, samplingConfig() );
+        populator.create();
+        populator.close( true );
+    }
+
+    private void verifySpatialSettings( File indexFile, SpaceFillingCurveSettings expectedSettings )
+    {
+        try
+        {
+            SpaceFillingCurveSettings settings = new SpaceFillingCurveSettings( 2, 2, null );
+            GBPTree.readHeader( pageCache, indexFile, settings.headerReader( NativeSchemaIndexHeaderReader::readFailureMessage ) );
+            assertThat( "Should get correct results from header", settings, equalTo( expectedSettings ) );
+        }
+        catch ( IOException e )
+        {
+            fail( "Failed to read GBPTree header: " + e.getMessage() );
+        }
+    }
+
+    private static Config configWithRange( double minX, double minY, double maxX, double maxY )
+    {
+        Setting<Double> wgs84_x_min = SpatialIndexSettings.makeCRSRangeSetting( CoordinateReferenceSystem.WGS84, 0, "min" );
+        Setting<Double> wgs84_y_min = SpatialIndexSettings.makeCRSRangeSetting( CoordinateReferenceSystem.WGS84, 1, "min" );
+        Setting<Double> wgs84_x_max = SpatialIndexSettings.makeCRSRangeSetting( CoordinateReferenceSystem.WGS84, 0, "max" );
+        Setting<Double> wgs84_y_max = SpatialIndexSettings.makeCRSRangeSetting( CoordinateReferenceSystem.WGS84, 1, "max" );
+        Config config = Config.defaults();
+        config.augment( wgs84_x_min, Double.toString( minX ) );
+        config.augment( wgs84_y_min, Double.toString( minY ) );
+        config.augment( wgs84_x_max, Double.toString( maxX ) );
+        config.augment( wgs84_y_max, Double.toString( maxY ) );
+        return config;
+    }
+}