From 11507bd16a1ad593dfdc8a6840023301ecbc7666 Mon Sep 17 00:00:00 2001 From: Lars Vonk Date: Thu, 16 Nov 2023 18:41:11 +0100 Subject: [PATCH 1/2] Move errors to own file --- lib/sequent/migrations/errors.rb | 11 +++++++++++ lib/sequent/migrations/planner.rb | 1 + lib/sequent/migrations/view_schema.rb | 5 +---- 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 lib/sequent/migrations/errors.rb diff --git a/lib/sequent/migrations/errors.rb b/lib/sequent/migrations/errors.rb new file mode 100644 index 00000000..e990bc8f --- /dev/null +++ b/lib/sequent/migrations/errors.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Sequent + module Migrations + class MigrationError < RuntimeError; end + class MigrationNotStarted < MigrationError; end + class MigrationDone < MigrationError; end + class ConcurrentMigration < MigrationError; end + + end +end diff --git a/lib/sequent/migrations/planner.rb b/lib/sequent/migrations/planner.rb index 0a916d5a..446aa51a 100644 --- a/lib/sequent/migrations/planner.rb +++ b/lib/sequent/migrations/planner.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true + module Sequent module Migrations class Planner diff --git a/lib/sequent/migrations/view_schema.rb b/lib/sequent/migrations/view_schema.rb index a1627552..024401c2 100644 --- a/lib/sequent/migrations/view_schema.rb +++ b/lib/sequent/migrations/view_schema.rb @@ -3,6 +3,7 @@ require 'parallel' require 'postgresql_cursor' +require_relative 'errors' require_relative '../support/database' require_relative '../sequent' require_relative '../util/timer' @@ -16,10 +17,6 @@ module Sequent module Migrations - class MigrationError < RuntimeError; end - class MigrationNotStarted < MigrationError; end - class MigrationDone < MigrationError; end - class ConcurrentMigration < MigrationError; end ## # ViewSchema is used for migration of you view_schema. For instance From b7339cf6569e0fe8439ebb1293d304dbf737f098 Mon Sep 17 00:00:00 2001 From: Lars Vonk Date: Thu, 16 Nov 2023 18:41:41 +0100 Subject: [PATCH 2/2] Validate plan before starting migration to avoid lingering state --- lib/sequent/migrations/errors.rb | 1 + lib/sequent/migrations/planner.rb | 9 ++++++--- lib/sequent/migrations/view_schema.rb | 8 ++++++++ spec/lib/sequent/migrations/view_schema_spec.rb | 12 ++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/sequent/migrations/errors.rb b/lib/sequent/migrations/errors.rb index e990bc8f..e868b68b 100644 --- a/lib/sequent/migrations/errors.rb +++ b/lib/sequent/migrations/errors.rb @@ -7,5 +7,6 @@ class MigrationNotStarted < MigrationError; end class MigrationDone < MigrationError; end class ConcurrentMigration < MigrationError; end + class InvalidMigrationDefinition < MigrationError; end end end diff --git a/lib/sequent/migrations/planner.rb b/lib/sequent/migrations/planner.rb index 446aa51a..f0f6aa71 100644 --- a/lib/sequent/migrations/planner.rb +++ b/lib/sequent/migrations/planner.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require_relative 'errors' module Sequent module Migrations @@ -101,7 +102,8 @@ def create_migrations(migrations) def map_to_migrations(migrations) migrations.reduce({}) do |memo, (version, ms)| unless ms.is_a?(Array) - fail "Declared migrations for version #{version} must be an Array. For example: {'3' => [FooProjector]}" + fail InvalidMigrationDefinition, + "Declared migrations for version #{version} must be an Array. For example: {'3' => [FooProjector]}" end memo[version] = ms.flat_map do |migration| @@ -110,14 +112,15 @@ def map_to_migrations(migrations) #{Sequent.configuration.migration_sql_files_directory}/#{migration.table_name}_#{version}.sql EOS unless File.exist?(alter_table_sql_file_name) - fail "Missing file #{alter_table_sql_file_name} to apply for version #{version}" + fail InvalidMigrationDefinition, + "Missing file #{alter_table_sql_file_name} to apply for version #{version}" end migration.copy(version) elsif migration < Sequent::Projector migration.managed_tables.map { |table| ReplayTable.create(table, version) } else - fail "Unknown Migration #{migration}" + fail InvalidMigrationDefinition, "Unknown Migration #{migration}" end end diff --git a/lib/sequent/migrations/view_schema.rb b/lib/sequent/migrations/view_schema.rb index 024401c2..297078bb 100644 --- a/lib/sequent/migrations/view_schema.rb +++ b/lib/sequent/migrations/view_schema.rb @@ -188,6 +188,7 @@ def executor # # @raise ConcurrentMigrationError if migration is already running def migrate_online + ensure_valid_plan! migrate_metadata_tables return if Sequent.new_version == current_version @@ -216,6 +217,9 @@ def migrate_online rescue ConcurrentMigration # Do not rollback the migration when this is a concurrent migration as the other one is running raise + rescue InvalidMigrationDefinition + # Do not rollback the migration when since there is nothing to rollback + raise rescue Exception => e # rubocop:disable Lint/RescueException rollback_migration raise e @@ -281,6 +285,10 @@ def migrate_offline private + def ensure_valid_plan! + plan + end + def migrate_metadata_tables Sequent::ApplicationRecord.transaction do in_view_schema do diff --git a/spec/lib/sequent/migrations/view_schema_spec.rb b/spec/lib/sequent/migrations/view_schema_spec.rb index 9200c816..668d4a01 100644 --- a/spec/lib/sequent/migrations/view_schema_spec.rb +++ b/spec/lib/sequent/migrations/view_schema_spec.rb @@ -593,6 +593,18 @@ expect(AccountRecord).to have_column('foobar') end end + + it 'missing the correct alter table file' do + expect(MessageRecord).to_not have_column('foobar') + Sequent.configuration.migration_sql_files_directory = 'spec/fixtures/db/2' + SpecMigrations.copy_and_add('3', [Sequent::Migrations.alter_table(MessageRecord)]) + SpecMigrations.version = 3 + expect(next_migration).to_not receive(:replay!) + + expect { next_migration.migrate_online }.to raise_error(Sequent::Migrations::InvalidMigrationDefinition) + + expect(Sequent::Migrations::Versions.where(version: 3).first).to be_nil + end end end end