From 39aafe60f72c38d158f180e0975c43e4940add30 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 21 Jan 2025 22:06:47 +0100 Subject: [PATCH] More specs --- .github/workflows/migration-tests.yml | 2 +- migrations/config/intermediate_db.yml | 2 +- .../lib/database/intermediate_db/upload.rb | 140 ++++++++++-------- .../lib/database/schema/config_validator.rb | 50 +++++-- migrations/lib/database/schema/loader.rb | 2 - .../database/intermediate_db/upload_spec.rb | 5 + .../spec/lib/database/intermediate_db_spec.rb | 21 +++ migrations/spec/lib/database/migrator_spec.rb | 10 +- .../database/schema/config_validator_spec.rb | 127 ++++++++++++++++ migrations/spec/support/helpers.rb | 4 + 10 files changed, 269 insertions(+), 94 deletions(-) create mode 100644 migrations/spec/lib/database/intermediate_db/upload_spec.rb create mode 100644 migrations/spec/lib/database/schema/config_validator_spec.rb diff --git a/.github/workflows/migration-tests.yml b/.github/workflows/migration-tests.yml index 9ee46997d84..7b5a611675b 100644 --- a/.github/workflows/migration-tests.yml +++ b/.github/workflows/migration-tests.yml @@ -111,7 +111,7 @@ jobs: if: steps.app-cache.outputs.cache-hit != 'true' run: rm -rf tmp/app-cache/uploads && cp -r public/uploads tmp/app-cache/uploads - - name: Check IntermediateDB schema + - name: Validate IntermediateDB schema run: migrations/bin/cli schema validate - name: RSpec diff --git a/migrations/config/intermediate_db.yml b/migrations/config/intermediate_db.yml index 8b2fc63cf5d..a1d51578119 100644 --- a/migrations/config/intermediate_db.yml +++ b/migrations/config/intermediate_db.yml @@ -3,7 +3,7 @@ output: schema_file: "db/intermediate_db_schema/100-base-schema.sql" models_directory: "lib/database/intermediate_db" - models_namespace: Migrations::Database::IntermediateDB + models_namespace: ::Migrations::Database::IntermediateDB schema: tables: diff --git a/migrations/lib/database/intermediate_db/upload.rb b/migrations/lib/database/intermediate_db/upload.rb index 4da5aee8db8..27685ab46cd 100644 --- a/migrations/lib/database/intermediate_db/upload.rb +++ b/migrations/lib/database/intermediate_db/upload.rb @@ -17,76 +17,86 @@ module Migrations::Database::IntermediateDB VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) SQL - class << self - def create_for_file!( + def self.create_for_file!( + path:, + filename: nil, + type: nil, + description: nil, + origin: nil, + user_id: nil + ) + create!( + id: ::Migrations::ID.hash(path), + filename: filename || File.basename(path), path:, - filename: nil, - type: nil, - description: nil, - origin: nil, - user_id: nil + type:, + description:, + origin:, + user_id:, ) - create!( - id: ::Migrations::ID.hash(path), - filename: filename || File.basename(path), - path:, - type:, - description:, - origin:, - user_id:, - ) - end + end - def create_for_url!(url:, filename:, type: nil, description: nil, origin: nil, user_id: nil) - create!( - id: ::Migrations::ID.hash(url), - filename:, - url:, - type:, - description:, - origin:, - user_id:, - ) - end - - def create_for_data!(data:, filename:, type: nil, description: nil, origin: nil, user_id: nil) - create!( - id: ::Migrations::ID.hash(data), - filename:, - data: ::Migrations::Database.to_blob(data), - type:, - description:, - origin:, - user_id:, - ) - end - - private - - def create!( - id:, + def self.create_for_url!( + url:, + filename:, + type: nil, + description: nil, + origin: nil, + user_id: nil + ) + create!( + id: ::Migrations::ID.hash(url), filename:, - path: nil, - data: nil, - url: nil, - type: nil, - description: nil, - origin: nil, - user_id: nil + url:, + type:, + description:, + origin:, + user_id:, + ) + end + + def self.create_for_data!( + data:, + filename:, + type: nil, + description: nil, + origin: nil, + user_id: nil + ) + create!( + id: ::Migrations::ID.hash(data), + filename:, + data: ::Migrations::Database.to_blob(data), + type:, + description:, + origin:, + user_id:, + ) + end + + def self.create!( + id:, + filename:, + path: nil, + data: nil, + url: nil, + type: nil, + description: nil, + origin: nil, + user_id: nil + ) + ::Migrations::Database::IntermediateDB.insert( + SQL, + id, + filename, + path, + data, + url, + type, + description, + origin, + user_id, ) - ::Migrations::Database::IntermediateDB.insert( - SQL, - id, - filename, - path, - data, - url, - type, - description, - origin, - user_id, - ) - end end end end diff --git a/migrations/lib/database/schema/config_validator.rb b/migrations/lib/database/schema/config_validator.rb index 39fae001cfa..fe4c87976ec 100644 --- a/migrations/lib/database/schema/config_validator.rb +++ b/migrations/lib/database/schema/config_validator.rb @@ -15,9 +15,11 @@ module Migrations::Database::Schema @errors.clear validate_with_json_schema(config) - return if has_errors? + return self if has_errors? validate_config(config) + + self end def has_errors? @@ -37,25 +39,28 @@ module Migrations::Database::Schema end def validate_config(config) - validate_plugins(config) + validate_output_config(config) validate_schema_config(config) + validate_plugins(config) end - def validate_plugins(config) - if (plugin_names = config[:plugins]).nil? - @errors << "Plugin configuration not found" - return - end + def validate_output_config(config) + output_config = config[:output] - all_plugin_names = Discourse.plugins.map(&:name) + schema_file_path = File.dirname(output_config[:schema_file]) + schema_file_path = File.expand_path(schema_file_path, ::Migrations.root_path) + @errors << "Directory of `schema_file` does not exist" if !Dir.exist?(schema_file_path) - if (additional_plugins = all_plugin_names.difference(plugin_names)).any? - @errors << "Additional plugins installed. Uninstall them or add to configuration: #{additional_plugins.join(", ")}" - end + models_directory = File.expand_path(output_config[:models_directory], ::Migrations.root_path) + @errors << "`models_directory` does not exist" if !Dir.exist?(models_directory) - if (missing_plugins = plugin_names.difference(all_plugin_names)).any? - @errors << "Configured plugins not installed: #{missing_plugins.join(", ")}" - end + existing_namespace = + begin + Object.const_get(output_config[:models_namespace]).is_a?(Module) + rescue NameError + false + end + @errors << "`models_namespace` is not defined" if !existing_namespace end def validate_schema_config(config) @@ -72,8 +77,8 @@ module Migrations::Database::Schema schema_config .dig(:global, :tables, :exclude) - .sort - .each do |table_name| + &.sort + &.each do |table_name| if !existing_table_names.delete?(table_name) @errors << "Excluded table does not exist: #{table_name}" end @@ -90,5 +95,18 @@ module Migrations::Database::Schema @errors << "Table missing from configuration file: #{table_name}" end end + + def validate_plugins(config) + plugin_names = config[:plugins] + all_plugin_names = Discourse.plugins.map(&:name) + + if (additional_plugins = all_plugin_names.difference(plugin_names)).any? + @errors << "Additional plugins installed. Uninstall them or add to configuration: #{additional_plugins.sort.join(", ")}" + end + + if (missing_plugins = plugin_names.difference(all_plugin_names)).any? + @errors << "Configured plugins not installed: #{missing_plugins.sort.join(", ")}" + end + end end end diff --git a/migrations/lib/database/schema/loader.rb b/migrations/lib/database/schema/loader.rb index 46ac66253b8..f4cf1b92871 100644 --- a/migrations/lib/database/schema/loader.rb +++ b/migrations/lib/database/schema/loader.rb @@ -2,8 +2,6 @@ module Migrations::Database::Schema class Loader - attr_reader :errors - def initialize(schema_config) @schema_config = schema_config @db = ActiveRecord::Base.connection diff --git a/migrations/spec/lib/database/intermediate_db/upload_spec.rb b/migrations/spec/lib/database/intermediate_db/upload_spec.rb new file mode 100644 index 00000000000..518ae204cd7 --- /dev/null +++ b/migrations/spec/lib/database/intermediate_db/upload_spec.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +RSpec.describe ::Migrations::Database::IntermediateDB::Upload do + it_behaves_like "a database entity" +end diff --git a/migrations/spec/lib/database/intermediate_db_spec.rb b/migrations/spec/lib/database/intermediate_db_spec.rb index fd429eef9be..29453218a4b 100644 --- a/migrations/spec/lib/database/intermediate_db_spec.rb +++ b/migrations/spec/lib/database/intermediate_db_spec.rb @@ -94,4 +94,25 @@ RSpec.describe ::Migrations::Database::IntermediateDB do end end end + + it "checks that manually created entities are tested" do + Dir[ + File.join(::Migrations.root_path, "lib", "database", "intermediate_db", "*.rb") + ].each do |path| + next if File.read(path).include?("auto-generated") + + spec_path = + File.join( + ::Migrations.root_path, + "spec", + "lib", + "database", + "intermediate_db", + "#{File.basename(path, ".rb")}_spec.rb", + ) + + expect(File.exist?(spec_path)).to eq(true), "#{spec_path} is missing" + expect(File.read(spec_path)).to include('it_behaves_like "a database entity"') + end + end end diff --git a/migrations/spec/lib/database/migrator_spec.rb b/migrations/spec/lib/database/migrator_spec.rb index 04985f25199..aa5fe97cc46 100644 --- a/migrations/spec/lib/database/migrator_spec.rb +++ b/migrations/spec/lib/database/migrator_spec.rb @@ -9,15 +9,7 @@ RSpec.describe ::Migrations::Database::Migrator do ignore_errors: false ) if migrations_directory - migrations_path = - File.join( - ::Migrations.root_path, - "spec", - "support", - "fixtures", - "schema", - migrations_directory, - ) + migrations_path = File.join(fixture_root, "schema", migrations_directory) end temp_path = storage_path = Dir.mktmpdir if storage_path.nil? diff --git a/migrations/spec/lib/database/schema/config_validator_spec.rb b/migrations/spec/lib/database/schema/config_validator_spec.rb new file mode 100644 index 00000000000..1c54456048e --- /dev/null +++ b/migrations/spec/lib/database/schema/config_validator_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +RSpec.describe ::Migrations::Database::Schema::ConfigValidator do + subject(:validator) { described_class.new } + + def minimal_config + { + output: { + schema_file: "db/intermediate_db_schema/100-base-schema.sql", + models_directory: "lib/database/intermediate_db", + models_namespace: "Migrations::Database::IntermediateDB", + }, + schema: { + tables: { + users: { + columns: { + include: %w[id username], + }, + }, + }, + global: { + columns: { + }, + tables: { + }, + }, + }, + plugins: [], + } + end + + before do + allow(ActiveRecord::Base.connection).to receive(:tables).and_return(["users"]) + allow(ActiveRecord::Base.connection).to receive(:columns).with("users").and_return( + [ + instance_double( + ActiveRecord::ConnectionAdapters::PostgreSQL::Column, + name: "id", + type: :integer, + ), + instance_double( + ActiveRecord::ConnectionAdapters::PostgreSQL::Column, + name: "username", + type: :string, + ), + ], + ) + end + + it "validates the minimal config" do + expect(validator.validate(minimal_config)).to_not have_errors + expect(validator.errors).to be_empty + end + + it "validates the config against JSON schema" do + expect(validator.validate({})).to have_errors + expect(validator.errors).to contain_exactly( + "object at root is missing required properties: output, schema, plugins", + ) + + incomplete_config = minimal_config.except(:plugins) + incomplete_config[:schema].except!(:global) + expect(validator.validate(incomplete_config)).to have_errors + expect(validator.errors).to contain_exactly( + "object at `/schema` is missing required properties: global", + "object at root is missing required properties: plugins", + ) + + invalid_config = minimal_config + invalid_config[:output][:models_namespace] = 123 + expect(validator.validate(invalid_config)).to have_errors + expect(validator.errors).to contain_exactly( + "value at `/output/models_namespace` is not a string", + ) + end + + context "with output config" do + it "checks if directory of `schema_file` exists" do + config = minimal_config + config[:output][:schema_file] = "foo/bar/100-base-schema.sql" + expect(validator.validate(config)).to have_errors + expect(validator.errors).to contain_exactly("Directory of `schema_file` does not exist") + end + + it "checks if `models_directory` exists" do + config = minimal_config + config[:output][:models_directory] = "foo/bar" + expect(validator.validate(config)).to have_errors + expect(validator.errors).to contain_exactly("`models_directory` does not exist") + end + + it "checks if `models_namespace` is an existing namespace" do + config = minimal_config + config[:output][:models_namespace] = "::Foo::Bar::IntermediateDB" + expect(validator.validate(config)).to have_errors + expect(validator.errors).to contain_exactly("`models_namespace` is not defined") + end + end + + context "with plugins config" do + before do + allow(Discourse).to receive(:plugins).and_return( + [ + instance_double(Plugin::Instance, name: "footnote"), + instance_double(Plugin::Instance, name: "chat"), + instance_double(Plugin::Instance, name: "poll"), + ], + ) + end + + it "detects if a configured plugin is missing" do + config = minimal_config + config[:plugins] = %w[foo poll bar chat footnote] + expect(validator.validate(config)).to have_errors + expect(validator.errors).to contain_exactly("Configured plugins not installed: bar, foo") + end + + it "detects if an active plugin isn't configured" do + config = minimal_config + config[:plugins] = %w[poll] + expect(validator.validate(config)).to have_errors + expect(validator.errors).to contain_exactly( + "Additional plugins installed. Uninstall them or add to configuration: chat, footnote", + ) + end + end +end diff --git a/migrations/spec/support/helpers.rb b/migrations/spec/support/helpers.rb index dfb1d6c4d98..12e19f43122 100644 --- a/migrations/spec/support/helpers.rb +++ b/migrations/spec/support/helpers.rb @@ -5,3 +5,7 @@ def reset_memoization(instance, *variables) instance.remove_instance_variable(var) if instance.instance_variable_defined?(var) end end + +def fixture_root + @fixture_root ||= File.join(::Migrations.root_path, "spec", "support", "fixtures") +end