Adds StoreUpgraderTest for enterprise and fixed prop dedup migration …

…issue

community/import-tool/src/main/java/org/neo4j/tooling/ImportTool.java

@@ -47,6 +47,7 @@
 import org.neo4j.kernel.configuration.Config;
 import org.neo4j.kernel.impl.logging.LogService;
 import org.neo4j.kernel.impl.logging.StoreLogService;
+import org.neo4j.kernel.impl.storemigration.ExistingTargetStrategy;
 import org.neo4j.kernel.impl.storemigration.FileOperation;
 import org.neo4j.kernel.impl.storemigration.StoreFile;
 import org.neo4j.kernel.impl.storemigration.StoreFileType;
@@ -415,7 +416,7 @@ public static void main( String[] incomingArguments, boolean defaultSettingsSuit
                 {
                     StoreFile.fileOperation( FileOperation.DELETE, fs, storeDir, null,
                             Iterables.<StoreFile,StoreFile>iterable( StoreFile.values() ),
-                            false, false, StoreFileType.values() );
+                            false, ExistingTargetStrategy.FAIL, StoreFileType.values() );
                 }
                 catch ( IOException e )
                 {

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/ExistingTargetStrategy.java

@@ -0,0 +1,44 @@
+/*
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.kernel.impl.storemigration;
+
+import java.io.IOException;
+
+/**
+ * Different options for what to do in a file operation scenario (like copy or move) where the target
+ * file already exists.
+ */
+public enum ExistingTargetStrategy
+{
+    /**
+     * Fail with an {@link IOException} if the target file already exists.
+     */
+    FAIL,
+
+    /**
+     * Overwrite the target file if it already exists.
+     */
+    OVERWRITE,
+
+    /**
+     * Skip the file operation as a whole if the target file already exists.
+     */
+    SKIP;
+}

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/FileOperation.java

@@ -44,13 +44,17 @@ public enum FileOperation
         @Override
         public void perform( FileSystemAbstraction fs, String fileName,
                 File fromDirectory, boolean skipNonExistentFromFile,
-                File toDirectory, boolean overwrite )
+                File toDirectory, ExistingTargetStrategy existingTargetStrategy )
                 throws IOException
         {
             File fromFile = fromFile( fs, fromDirectory, fileName, skipNonExistentFromFile );
             if ( fromFile != null )
             {
-                fs.copyFile( fromFile, toFile( fs, toDirectory, fileName, overwrite ) );
+                File toFile = toFile( fs, toDirectory, fileName, existingTargetStrategy );
+                if ( toFile != null )
+                {
+                    fs.copyFile( fromFile, toFile );
+                }
             }
         }
     },
@@ -68,14 +72,16 @@ public void perform( FileSystemAbstraction fs, String fileName,
         @Override
         public void perform( FileSystemAbstraction fs, String fileName,
                 File fromDirectory, boolean skipNonExistentFromFile,
-                File toDirectory, boolean overwrite )
+                File toDirectory, ExistingTargetStrategy existingTargetStrategy )
                 throws IOException
         {
             File fromFile = fromFile( fs, fromDirectory, fileName, skipNonExistentFromFile );
             if ( fromFile != null )
             {
-                toFile( fs, toDirectory, fileName, overwrite );
-                fs.moveToDirectory( fromFile, toDirectory );
+                if ( toFile( fs, toDirectory, fileName, existingTargetStrategy ) != null )
+                {
+                    fs.moveToDirectory( fromFile, toDirectory );
+                }
             }
         }
     },
@@ -84,7 +90,7 @@ public void perform( FileSystemAbstraction fs, String fileName,
         @Override
         public void perform( FileSystemAbstraction fs, String fileName,
                 File directory, boolean skipNonExistentFromFile,
-                File unusedFile, boolean unusedBoolean )
+                File unusedFile, ExistingTargetStrategy unused )
                 throws IOException
         {
             File file = fromFile( fs, directory, fileName, skipNonExistentFromFile );
@@ -97,7 +103,7 @@ public void perform( FileSystemAbstraction fs, String fileName,
 
     public abstract void perform( FileSystemAbstraction fs, String fileName,
             File fromDirectory, boolean skipNonExistentFromFile,
-            File toDirectory, boolean overwrite ) throws IOException;
+            File toDirectory, ExistingTargetStrategy existingTargetStrategy ) throws IOException;
 
     protected File fromFile( FileSystemAbstraction fs, File directory, String name, boolean skipNonExistent )
     {
@@ -111,12 +117,24 @@ protected File fromFile( FileSystemAbstraction fs, File directory, String name,
         return fromFile;
     }
 
-    protected File toFile( FileSystemAbstraction fs, File directory, String name, boolean overwrite )
+    protected File toFile( FileSystemAbstraction fs, File directory, String name,
+            ExistingTargetStrategy existingTargetStrategy )
     {
         File file = new File( directory, name );
-        if ( overwrite )
+        if ( fs.fileExists( file ) )
         {
-            fs.deleteFile( file );
+            switch ( existingTargetStrategy )
+            {
+            case FAIL:
+                // Let the copy operation fail. Is this a good idea? This is how we did before this switch case
+            case OVERWRITE:
+                fs.deleteFile( file );
+                return file;
+            case SKIP:
+                return null;
+            default:
+                throw new IllegalStateException( existingTargetStrategy.name() );
+            }
         }
         return file;
     }

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/LogFiles.java

@@ -64,7 +64,8 @@ public static void move( FileSystemAbstraction fs, File fromDirectory, File toDi
         File[] logFiles = fs.listFiles( fromDirectory, FILENAME_FILTER );
         for ( File logFile : logFiles )
         {
-            FileOperation.MOVE.perform( fs, logFile.getName(), fromDirectory, false, toDirectory, false );
+            FileOperation.MOVE.perform( fs, logFile.getName(), fromDirectory, false, toDirectory,
+                    ExistingTargetStrategy.FAIL );
         }
     }
 

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/StoreFile.java

@@ -222,15 +222,16 @@ public static Iterable<StoreFile> currentStoreFiles()
     public static void fileOperation( FileOperation operation, FileSystemAbstraction fs, File fromDirectory,
             File toDirectory, StoreFile... files ) throws IOException
     {
-        fileOperation( operation, fs, fromDirectory, toDirectory, storeFiles( files ), false, false );
+        fileOperation( operation, fs, fromDirectory, toDirectory, storeFiles( files ), false,
+                ExistingTargetStrategy.FAIL );
     }
 
     public static void fileOperation( FileOperation operation, FileSystemAbstraction fs, File fromDirectory,
             File toDirectory, Iterable<StoreFile> files,
-            boolean allowSkipNonExistentFiles, boolean allowOverwriteTarget ) throws IOException
+            boolean allowSkipNonExistentFiles, ExistingTargetStrategy existingTargetStrategy ) throws IOException
     {
         fileOperation( operation, fs, fromDirectory, toDirectory, files, allowSkipNonExistentFiles,
-                allowOverwriteTarget, StoreFileType.values() );
+                existingTargetStrategy, StoreFileType.values() );
     }
 
     /**
@@ -244,7 +245,7 @@ public static void fileOperation( FileOperation operation, FileSystemAbstraction
      */
     public static void fileOperation( FileOperation operation, FileSystemAbstraction fs, File fromDirectory,
             File toDirectory, Iterable<StoreFile> files,
-            boolean allowSkipNonExistentFiles, boolean allowOverwriteTarget,
+            boolean allowSkipNonExistentFiles, ExistingTargetStrategy existingTargetStrategy,
             StoreFileType... types ) throws IOException
     {
         // TODO: change the order of files to handle failure conditions properly
@@ -254,7 +255,7 @@ public static void fileOperation( FileOperation operation, FileSystemAbstraction
             {
                 String fileName = storeFile.fileName( type );
                 operation.perform( fs, fileName,
-                        fromDirectory, allowSkipNonExistentFiles, toDirectory, allowOverwriteTarget );
+                        fromDirectory, allowSkipNonExistentFiles, toDirectory, existingTargetStrategy );
             }
         }
     }

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/legacylogs/LegacyLogs.java

@@ -29,6 +29,7 @@
 import org.neo4j.cursor.IOCursor;
 import org.neo4j.helpers.collection.Pair;
 import org.neo4j.io.fs.FileSystemAbstraction;
+import org.neo4j.kernel.impl.storemigration.ExistingTargetStrategy;
 import org.neo4j.kernel.impl.storemigration.FileOperation;
 import org.neo4j.kernel.impl.transaction.log.LogVersionedStoreChannel;
 import org.neo4j.kernel.impl.transaction.log.NoSuchTransactionException;
@@ -160,7 +161,7 @@ public void operate( FileOperation op, File from, File to ) throws IOException
         File[] logFiles = fs.listFiles( from, versionedLegacyLogFilesFilter );
         for ( File file : logFiles )
         {
-            op.perform( fs, file.getName(), from, false, to, true );
+            op.perform( fs, file.getName(), from, false, to, ExistingTargetStrategy.OVERWRITE );
         }
     }
 

community/kernel/src/main/java/org/neo4j/kernel/impl/storemigration/participant/StoreMigrator.java

@@ -63,6 +63,7 @@
 import org.neo4j.kernel.impl.store.record.PrimitiveRecord;
 import org.neo4j.kernel.impl.store.record.RelationshipRecord;
 import org.neo4j.kernel.impl.storemigration.DirectRecordStoreMigrator;
+import org.neo4j.kernel.impl.storemigration.ExistingTargetStrategy;
 import org.neo4j.kernel.impl.storemigration.StoreFile;
 import org.neo4j.kernel.impl.storemigration.StoreFileType;
 import org.neo4j.kernel.impl.storemigration.StoreMigratorCheckPointer;
@@ -169,16 +170,10 @@ public void migrate( File storeDir, File migrationDir, MigrationProgressMonitor.
                         progressMonitor, oldFormat, newFormat );
             }
         }
-        else
+
+        if ( versionToMigrateFrom.equals( LowLimitV2_1.STORE_VERSION ) )
         {
-            // This means that the formats are the same, not that the store versions are the same.
-            // Store version may change between versions to signal other things, so we may still have
-            // to do some amount of migration here.
-            if ( versionToMigrateFrom.equals( LowLimitV2_1.STORE_VERSION ) )
-            {
-                removeDuplicateEntityProperties(
-                        storeDir, migrationDir, pageCache, schemaIndexProvider, oldFormat );
-            }
+            removeDuplicateEntityProperties( storeDir, migrationDir, pageCache, schemaIndexProvider, oldFormat );
         }
 
         // DO NOT migrate logs. LegacyLogs is able to migrate logs, but only changes its format, not any
@@ -310,14 +305,14 @@ private void removeDuplicateEntityProperties( File storeDir, File migrationDir,
                 StoreFile.PROPERTY_STRING_STORE,
                 StoreFile.NODE_STORE,
                 StoreFile.NODE_LABEL_STORE,
-                StoreFile.SCHEMA_STORE ), false, false, StoreFileType.STORE );
+                StoreFile.SCHEMA_STORE ), false, ExistingTargetStrategy.SKIP, StoreFileType.STORE );
 
         // copy ids only if present
         StoreFile.fileOperation( COPY, fileSystem, storeDir, migrationDir, Iterables.<StoreFile,StoreFile>iterable(
                 StoreFile.PROPERTY_STORE,
                 StoreFile.PROPERTY_KEY_TOKEN_NAMES_STORE,
                 StoreFile.PROPERTY_KEY_TOKEN_STORE,
-                StoreFile.NODE_STORE ), true, false, StoreFileType.ID );
+                StoreFile.NODE_STORE ), true, ExistingTargetStrategy.SKIP, StoreFileType.ID );
 
         // let's remove trailers here on the copied files since the new code doesn't remove them since in 2.3
         // there are no store trailers
@@ -379,23 +374,12 @@ private void migrateWithBatchImporter( File storeDir, File migrationDir, long la
                     Inputs.input( nodes, relationships, IdMappers.actual(), IdGenerators.fromInput(), true,
                             Collectors.badCollector( badOutput, 0 ) ) );
 
-            // During migration the batch importer only writes node, relationship, relationship group and counts stores.
-            // Delete the property store files from the batch import migration so that even if we won't
-            // migrate property stores as part of deduplicating property key tokens or properties then
-            // we won't move these empty property store files to the store directory, overwriting the old ones.
+            // During migration the batch importer doesn't necessarily writes all entities, depending on
+            // which stores needs migration. Node, relationship, relationship group stores are always written
+            // anyways and cannot be avoided with the importer, but delete the store files that weren't written
+            // (left empty) so that we don't overwrite those in the real store directory later.
             Collection<StoreFile> storesToDeleteFromMigratedDirectory = new ArrayList<>();
-            if ( oldFormat.node().equals( newFormat.node() ) )
-            {
-                storesToDeleteFromMigratedDirectory.add( StoreFile.NODE_STORE );
-            }
-            if ( oldFormat.relationship().equals( newFormat.relationship() ) )
-            {
-                storesToDeleteFromMigratedDirectory.add( StoreFile.RELATIONSHIP_STORE );
-            }
-            if ( oldFormat.relationshipGroup().equals( newFormat.relationshipGroup() ) )
-            {
-                storesToDeleteFromMigratedDirectory.add( StoreFile.RELATIONSHIP_GROUP_STORE );
-            }
+            storesToDeleteFromMigratedDirectory.add( StoreFile.NEO_STORE );
             if ( !requiresPropertyMigration )
             {
                 // We didn't migrate properties, so the property stores in the migrated store are just empty/bogus
@@ -406,7 +390,7 @@ private void migrateWithBatchImporter( File storeDir, File migrationDir, long la
             }
             if ( !requiresDynamicStoreMigration )
             {
-                // We didn't migrate labels (dynamic node labels)
+                // We didn't migrate labels (dynamic node labels) or any other dynamic store
                 storesToDeleteFromMigratedDirectory.addAll( asList(
                         StoreFile.NODE_LABEL_STORE,
                         StoreFile.LABEL_TOKEN_STORE,
@@ -418,15 +402,15 @@ private void migrateWithBatchImporter( File storeDir, File migrationDir, long la
                         StoreFile.SCHEMA_STORE ) );
             }
             StoreFile.fileOperation( DELETE, fileSystem, migrationDir, null, storesToDeleteFromMigratedDirectory,
-                    true, false, StoreFileType.values() );
+                    true, null, StoreFileType.values() );
         }
 
         // The batch importer will create a whole store. so
         // disregard the new and empty ".id" files, i.e. reuse the existing id files
         StoreFile.fileOperation( DELETE, fileSystem, migrationDir, null,
                 StoreFile.currentStoreFiles(),
                 true, // allow to skip non existent source files
-                false, // not allow to overwrite target files
+                null,
                 StoreFileType.ID );
     }
 
@@ -454,7 +438,7 @@ private void prepareBatchImportMigration( File storeDir, File migrationDir, Reco
             Iterable<StoreFile> storeFiles = iterable( StoreFile.NODE_LABEL_STORE );
             StoreFile.fileOperation( COPY, fileSystem, storeDir, migrationDir, storeFiles,
                     true, // OK if it's not there (1.9)
-                    false, StoreFileType.values() );
+                    ExistingTargetStrategy.FAIL, StoreFileType.values() );
         }
         else
         {
@@ -623,7 +607,7 @@ public void moveMigratedFiles( File migrationDir, File storeDir, String versionT
         // Move the migrated ones into the store directory
         StoreFile.fileOperation( MOVE, fileSystem, migrationDir, storeDir, StoreFile.currentStoreFiles(),
                 true, // allow to skip non existent source files
-                true, // allow to overwrite target files
+                ExistingTargetStrategy.OVERWRITE, // allow to overwrite target files
                 StoreFileType.values() );
 
         RecordFormats oldFormat = InternalRecordFormatSelector.fromVersion( versionToUpgradeFrom );
@@ -664,7 +648,7 @@ public void rebuildCounts( File storeDir, String versionToMigrateFrom ) throws I
             Iterable<StoreFile> countsStoreFiles =
                     Iterables.iterable( StoreFile.COUNTS_STORE_LEFT, StoreFile.COUNTS_STORE_RIGHT );
             StoreFile.fileOperation( DELETE, fileSystem, storeDir, storeDir,
-                    countsStoreFiles, true, false, StoreFileType.STORE );
+                    countsStoreFiles, true, null, StoreFileType.STORE );
             File neoStore = new File( storeDir, DEFAULT_NAME );
             long lastTxId = MetaDataStore.getRecord( pageCache, neoStore, Position.LAST_TRANSACTION_ID );
             rebuildCountsFromScratch( storeDir, lastTxId, pageCache );

enterprise/backup/src/main/java/org/neo4j/backup/BackupTool.java

@@ -41,6 +41,7 @@
 import org.neo4j.kernel.configuration.Config;
 import org.neo4j.kernel.impl.logging.SimpleLogService;
 import org.neo4j.kernel.impl.store.MismatchingStoreIdException;
+import org.neo4j.kernel.impl.storemigration.ExistingTargetStrategy;
 import org.neo4j.kernel.impl.storemigration.LogFiles;
 import org.neo4j.kernel.impl.storemigration.StoreFile;
 import org.neo4j.kernel.impl.storemigration.StoreFileType;
@@ -364,7 +365,8 @@ private static void moveExistingDatabase( FileSystemAbstraction fs, File toDir )
         {
             throw new IOException( "Trouble making target backup directory " + backupDir.getAbsolutePath() );
         }
-        StoreFile.fileOperation( MOVE, fs, toDir, backupDir, StoreFile.currentStoreFiles(), false, false,
+        StoreFile.fileOperation( MOVE, fs, toDir, backupDir, StoreFile.currentStoreFiles(), false,
+                ExistingTargetStrategy.FAIL,
                 StoreFileType.values() );
         LogFiles.move( fs, toDir, backupDir );
     }

enterprise/neo4j-enterprise/pom.xml

@@ -89,6 +89,11 @@
         <artifactId>hamcrest-library</artifactId>
         <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <scope>test</scope>
+    </dependency>
 
     <dependency>
       <groupId>org.neo4j</groupId>