Moving SchemaMigration changes outside migration.

Handling incomplete states in migrations.

Gemfile

@@ -8,7 +8,7 @@ if branch = ENV['TRAVIS_BRANCH']
   same_branch_exists = `curl --head https://github.com/neo4jrb/neo4j-core/tree/#{branch} | head -1`.match(/200 OK/)
   gem 'neo4j-core', github: 'neo4jrb/neo4j-core', branch: same_branch_exists ? branch : 'master'
 else
-  gem 'neo4j-core', github: 'neo4jrb/neo4j-core'
+  gem 'neo4j-core', path: '../neo4j-core'
 end
 
 # gem 'active_attr', github: 'neo4jrb/active_attr', branch: 'performance'

Guardfile

@@ -7,7 +7,7 @@ guard :rubocop, cli: '--auto-correct --display-cop-names --except Lint/Debugger'
   callback(:start_begin) { puts '👮 🚨 👮 🚨 👮 🚨 👮 🚨 👮 ' }
 end
 
-guard :rspec, cmd: 'bundle exec rspec -fd', failed_mode: :focus do
+guard :rspec, cmd: 'bundle exec rspec', failed_mode: :focus do
   watch(%r{^spec/.+_spec\.rb$})
   watch(%r{^lib/(.+)\.rb}) { |m| "spec/lib/#{m[1]}_spec.rb" }
   watch('spec/spec_helper.rb') { 'spec' }

lib/neo4j/migrations/base.rb

@@ -12,15 +12,7 @@ def initialize(migration_id)
 
       def migrate(method)
         Benchmark.realtime do
-          ActiveBase.run_transaction(transactions?) do
-            if method == :up
-              log_queries { up }
-              SchemaMigration.create!(migration_id: @migration_id)
-            else
-              log_queries { down }
-              SchemaMigration.find_by!(migration_id: @migration_id).destroy
-            end
-          end
+          method == :up ? migrate_up : migrate_down
         end
       end
 
@@ -34,6 +26,43 @@ def down
 
       private
 
+      def migrate_up
+        schema = SchemaMigration.create!(migration_id: @migration_id, incomplete: true)
+        begin
+          run_migration(:up)
+        rescue StandardError => e
+          schema.destroy
+          handle_migration_error!(e)
+        end
+        schema.update!(incomplete: false)
+      end
+
+      def migrate_down
+        schema = SchemaMigration.find_by!(migration_id: @migration_id)
+        schema.update!(incomplete: true)
+        begin
+          run_migration(:down)
+        rescue StandardError => e
+          schema.update!(incomplete: false)
+          handle_migration_error!(e)
+        end
+        schema.destroy
+      end
+
+      def run_migration(direction)
+        migration_transaction { log_queries { public_send(direction) } }
+      end
+
+      def handle_migration_error!(e)
+        fail e unless e.message =~ /Cannot perform data updates in a transaction that has performed schema updates./
+        fail MigrationError,
+             "#{e.message}. Please add `disable_transactions!` in your migration file."
+      end
+
+      def migration_transaction(&block)
+        ActiveBase.run_transaction(transactions?, &block)
+      end
+
       def log_queries
         subscriber = Neo4j::Core::CypherSession::Adaptors::Base.subscribe_to_query(&method(:output))
         yield

lib/neo4j/migrations/runner.rb

@@ -7,34 +7,40 @@ class Runner
       STATUS_TABLE_HEADER = ['Status'.freeze, 'Migration ID'.freeze, 'Migration Name'.freeze].freeze
       UP_MESSAGE = 'up'.freeze
       DOWN_MESSAGE = 'down'.freeze
+      INCOMPLETE_MESSAGE = 'incomplete'.freeze
       MIGRATION_RUNNING = {up: 'running'.freeze, down: 'reverting'.freeze}.freeze
       MIGRATION_DONE = {up: 'migrated'.freeze, down: 'reverted'.freeze}.freeze
 
       def initialize
         SchemaMigration.mapped_label.create_constraint(:migration_id, type: :unique)
-        @up_versions = SortedSet.new(SchemaMigration.all.pluck(:migration_id))
+        @schema_migrations = SchemaMigration.all.to_a
+        @up_versions = SortedSet.new(@schema_migrations.map(&:migration_id))
       end
 
       def all
+        handle_incomplete_states!
         migration_files.each do |migration_file|
           next if up?(migration_file.version)
           migrate(:up, migration_file)
         end
       end
 
       def up(version)
+        handle_incomplete_states!
         migration_file = find_by_version!(version)
         return if up?(version)
         migrate(:up, migration_file)
       end
 
       def down(version)
+        handle_incomplete_states!
         migration_file = find_by_version!(version)
         return unless up?(version)
         migrate(:down, migration_file)
       end
 
       def rollback(steps)
+        handle_incomplete_states!
         @up_versions.to_a.reverse.first(steps).each do |version|
           down(version)
         end
@@ -51,8 +57,48 @@ def status
         end
       end
 
+      def resolve(version)
+        SchemaMigration.find_by!(migration_id: version).update!(incomplete: false)
+        output "Migration #{version} resolved."
+      end
+
+      def reset(version)
+        SchemaMigration.find_by!(migration_id: version).destroy
+        output "Migration #{version} reset."
+      end
+
       private
 
+      def migration_status(version)
+        return DOWN_MESSAGE unless up?(version)
+        incomplete_states.find { |v| v == version } ? INCOMPLETE_MESSAGE : UP_MESSAGE
+      end
+
+      def handle_incomplete_states!
+        return unless incomplete_states.any?
+        incomplete_versions = incomplete_states.map(&:migration_id)
+        fail MigrationError, <<-MSG
+There are migrations struck in an incomplete states, that could not be fixed automatically:
+#{incomplete_versions.join('\n')}
+This can happen when there's a critical error inside a migration.
+
+If you think they were was completed correctly, run:
+
+#{task_migration_messages('resolve', incomplete_versions)}
+
+If you want to reset and run the migration again, run:
+
+#{task_migration_messages('reset', incomplete_versions)}
+
+MSG
+      end
+
+      def task_migration_messages(type, versions)
+        versions.map do |version|
+          "rake neo4j:migrate:#{type} VERSION=#{version}"
+        end.join("\n")
+      end
+
       def up?(version)
         @up_versions.include?(version)
       end
@@ -101,6 +147,10 @@ def migration_files
         files.map { |file_path| MigrationFile.new(file_path) }
       end
 
+      def incomplete_states
+        @incomplete_states ||= SortedSet.new(@schema_migrations.select(&:incomplete?))
+      end
+
       def files
         Dir[files_path].sort
       end

lib/neo4j/migrations/schema_migration.rb

@@ -4,6 +4,11 @@ class SchemaMigration
       include Neo4j::ActiveNode
       id_property :migration_id
       property :migration_id, type: String
+      property :incomplete, type: Boolean
+
+      def <=>(other)
+        migration_id <=> other.migration_id
+      end
     end
   end
 end

lib/neo4j/tasks/migration.rake

@@ -62,6 +62,20 @@ namespace :neo4j do
       runner = Neo4j::Migrations::Runner.new
       runner.status
     end
+
+    desc 'Resolve an incomplete version state'
+    task resolve: :environment do
+      version = ENV['VERSION'] || fail(ArgumentError, 'VERSION is required')
+      runner = Neo4j::Migrations::Runner.new
+      runner.resolve version
+    end
+
+    desc 'Resolve an incomplete version state'
+    task reset: :environment do
+      version = ENV['VERSION'] || fail(ArgumentError, 'VERSION is required')
+      runner = Neo4j::Migrations::Runner.new
+      runner.reset version
+    end
   end
 
   desc 'Rollbacks migrations given a STEP number'

spec/e2e/migrations_spec.rb

@@ -1,5 +1,7 @@
 module Neo4j
+  # rubocop:disable Metrics/ModuleLength
   module Migrations
+    # rubocop:enable Metrics/ModuleLength
     describe Runner do
       before { delete_schema }
 
@@ -9,13 +11,15 @@ module Migrations
         create_constraint :'Neo4j::Migrations::SchemaMigration', :migration_id, type: :unique
 
         create_constraint :User, :uuid, type: :unique
+        create_constraint :User, :name, type: :unique
         stub_active_node_class('User') do
           property :name
         end
 
         allow_any_instance_of(described_class).to receive(:files_path) do
-          Rails.root.join('spec', 'support', 'migrations', '*.rb')
+          Rails.root.join('spec', 'migration_files', 'migrations', '*.rb')
         end
+        User.delete_all
         SchemaMigration.delete_all
       end
 
@@ -122,42 +126,101 @@ module Migrations
       describe 'schema changes in migrations' do
         before do
           allow_any_instance_of(described_class).to receive(:files_path) do
-            Rails.root.join('spec', 'support', 'transactional_migrations', '*.rb')
+            Rails.root.join('spec', 'migration_files', 'transactional_migrations', '*.rb')
           end
         end
 
         it 'run without raising errors' do
           expect do
-            described_class.new.up '8888888888'
-          end.not_to raise_error
+            expect do
+              described_class.new.up '8888888888'
+            end.not_to raise_error
+          end.to change { Neo4j::Core::Label.new(:Book, current_session).constraint?(:isbn) }.to(true)
+        end
+      end
+
+      describe 'incomplete states' do
+        it 'leaves incomplete states on up' do
+          allow_any_instance_of(SchemaMigration).to receive(:update!)
+          described_class.new.up '9500000000'
+          expect do
+            described_class.new.up '9500000000'
+          end.to raise_error(/incomplete states/)
+        end
+
+        it 'leaves incomplete states on down' do
+          described_class.new.up '9500000000'
+          allow_any_instance_of(SchemaMigration).to receive(:destroy)
+          described_class.new.down '9500000000'
+          expect do
+            described_class.new.down '9500000000'
+          end.to raise_error(/incomplete states/)
+        end
+
+        describe '#resolve' do
+          it 'fixes incomplete states' do
+            migration = SchemaMigration.create! migration_id: 'some', incomplete: true
+            SchemaMigration.find_by!(migration_id: migration.migration_id)
+
+            expect do
+              described_class.new.resolve 'some'
+            end.to change { migration.reload.incomplete }.to(false)
+          end
+        end
+
+        describe '#reset' do
+          it 'rollbacks incomplete states' do
+            SchemaMigration.create! migration_id: 'some', incomplete: true
+            expect do
+              described_class.new.reset 'some'
+            end.to change { SchemaMigration.count }.by(-1)
+          end
+        end
+      end
+
+      describe 'schema and data changes in migrations' do
+        before do
+          allow_any_instance_of(described_class).to receive(:files_path) do
+            Rails.root.join('spec', 'migration_files', 'transactional_migrations', '*.rb')
+          end
+        end
+
+        it 'run correctly when transactions are disabled' do
+          described_class.new.up '9999999999'
+        end
+
+        it 'fails with a custom error message when transactions are enabled' do
+          expect do
+            described_class.new.up '0000000000'
+          end.to raise_error(/Please add `disable_transactions!`/)
         end
       end
 
       describe 'transactional behavior in migrations' do
         before do
-          create_constraint :Contact, :uuid, type: :unique
-          create_constraint :Contact, :phone, type: :unique
           stub_active_node_class('Contact') do
             property :phone
           end
 
           Contact.delete_all
+          create_constraint :Contact, :uuid, type: :unique
+          create_constraint :Contact, :phone, type: :unique
           Contact.create! phone: '123123'
 
           allow_any_instance_of(described_class).to receive(:files_path) do
-            Rails.root.join('spec', 'support', 'transactional_migrations', '*.rb')
+            Rails.root.join('spec', 'migration_files', 'transactional_migrations', '*.rb')
           end
         end
 
+        let!(:joe) { User.create! name: 'Joe' }
+
         it 'rollbacks any change when one of the queries fails' do
-          joe = User.create! name: 'Joe'
           expect do
             expect { described_class.new.up '1231231231' }.to raise_error(/already exists/)
           end.not_to change { joe.reload.name }
         end
 
         it 'rollbacks nothing when transactions are disabled' do
-          joe = User.create! name: 'Joe'
           expect do
             expect { described_class.new.up '1234567890' }.to raise_error(/already exists/)
           end.to change { joe.reload.name }.to('Jack')

spec/e2e/query_spec.rb

@@ -212,13 +212,6 @@ def self.ordered_by_subject
         before { allow(DateTime).to receive(:now).and_return(*timestamps) }
         after { expect(Teacher.count).to eq 1 }
 
-        # The ActiveNode stubbing is doing some odd things with the `name` method on the defined classes,
-        # so please excuse this kludge.
-        after(:all) do
-          Object.send(:remove_const, :TeacherFoo)
-          Object.send(:remove_const, :Substitute)
-        end
-
         before(:each) do
           stub_active_node_class('TeacherFoo')
           stub_named_class('Substitute', TeacherFoo)

spec/migration_files/transactional_migrations/0000000000_schema_and_data_update.rb

@@ -0,0 +1,10 @@
+class SchemaAndDataUpdate < Neo4j::Migrations::Base
+  def up
+    add_constraint :Book, :isbn
+    execute 'CREATE (n:`Contact` {phone: "123123"})'
+  end
+
+  def down
+    fail Neo4j::IrreversibleMigration
+  end
+end

spec/support/transactional_migrations/1234567890_migration_without_transactions.rb → spec/migration_files/transactional_migrations/1234567890_migration_without_transactions.rb

@@ -5,6 +5,7 @@ def up
     execute 'MATCH (u:`User`) WHERE u.name = {name} SET u.name = {new_name}',
             name: 'Joe', new_name: 'Jack'
     execute 'CREATE (n:`Contact` {phone: "123123"})'
+    fail
   end
 
   def down

spec/migration_files/transactional_migrations/9999999999_schema_and_data_update_without_transactions.rb

@@ -0,0 +1,12 @@
+class SchemaAndDataUpdateWithoutTransactions < Neo4j::Migrations::Base
+  disable_transactions!
+
+  def up
+    add_constraint :Book, :isbn
+    execute 'CREATE (n:`Contact` {phone: "123123"})'
+  end
+
+  def down
+    fail Neo4j::IrreversibleMigration
+  end
+end