diff --git a/CHANGELOG.md b/CHANGELOG.md index a52aaf2..ac3c036 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ - Consolidate database configuration into `config/database.yml` - Add local development database setup with Docker Compose - Fix RuboCop integration. Thank to [Evgeny Matveyev](https://github.com/evgeny-matveev) for fixing it. +- Add SQLite support for `db_belongs_to` validation +- Add SQLite support for `validates_db_uniqueness_of` options: `index_name`, `where`, and complex indexes ## [1.1.1] - 14-03-2022 ### Improvements diff --git a/README.md b/README.md index fdf00d9..cb07fea 100644 --- a/README.md +++ b/README.md @@ -64,16 +64,9 @@ you will get the following performance improvement: ![](benchmarks/composed.png) -## Caveats - -- `db_belongs_to` doesn't work with SQLite due to a poor error message. -- In Rails 4, the gem validations work differently than the ActiveRecord ones when `validate: false` option is passed to `save`/`save!`. They incorrectly return a validation message instead of raising a proper constraint violation exception. In Rails >= 5 they correctly raise the exceptions they supposed to. - ## db_belongs_to -Supported databases are `PostgreSQL` and `MySQL`. -**Note**: Unfortunately, `SQLite` raises a poor error message -by which we can not determine exact foreign key which raised an error. +Supported databases are `PostgreSQL`, `MySQL`, and `SQLite`. ### Usage diff --git a/benchmarks/composed_benchmarks.rb b/benchmarks/composed_benchmarks.rb index ee377e4..51ef9f1 100644 --- a/benchmarks/composed_benchmarks.rb +++ b/benchmarks/composed_benchmarks.rb @@ -4,7 +4,7 @@ require_relative 'gc_suite' require_relative 'database_cleaner' -[postgresql_configuration, mysql_configuration].each do |database_configuration| +[sqlite_configuration, postgresql_configuration, mysql_configuration].each do |database_configuration| ActiveRecord::Base.establish_connection(database_configuration) clear_database!(database_configuration) diff --git a/benchmarks/configurations.rb b/benchmarks/configurations.rb index 895bb48..463e706 100644 --- a/benchmarks/configurations.rb +++ b/benchmarks/configurations.rb @@ -1,22 +1,15 @@ +require_relative '../config/database_config' + +DATABASE_CONFIGURATIONS = DatabaseConfig.load(symbolize_keys: true) + def postgresql_configuration - { - adapter: 'postgresql', - database: 'database_validations_test', - host: ENV['PGHOST'] || '127.0.0.1' - } + DATABASE_CONFIGURATIONS['postgresql'] end def mysql_configuration - { - adapter: 'mysql2', - database: 'database_validations_test', - host: ENV['MYSQLHOST'] || '127.0.0.1' - } + DATABASE_CONFIGURATIONS['mysql'] end def sqlite_configuration - { - adapter: 'sqlite3', - database: ':memory:' - } + DATABASE_CONFIGURATIONS['sqlite'] end diff --git a/benchmarks/db_belongs_to_benchmark.rb b/benchmarks/db_belongs_to_benchmark.rb index 452c537..3d7e936 100644 --- a/benchmarks/db_belongs_to_benchmark.rb +++ b/benchmarks/db_belongs_to_benchmark.rb @@ -4,7 +4,7 @@ require_relative 'gc_suite' require_relative 'database_cleaner' -[postgresql_configuration, mysql_configuration].each do |database_configuration| +[sqlite_configuration, postgresql_configuration, mysql_configuration].each do |database_configuration| ActiveRecord::Base.establish_connection(database_configuration) clear_database!(database_configuration) ActiveRecord::Schema.define(version: 1) do diff --git a/lib/database_validations/lib/adapters/mysql_adapter.rb b/lib/database_validations/lib/adapters/mysql_adapter.rb index fceff93..5cfcbb0 100644 --- a/lib/database_validations/lib/adapters/mysql_adapter.rb +++ b/lib/database_validations/lib/adapters/mysql_adapter.rb @@ -4,14 +4,15 @@ class MysqlAdapter < BaseAdapter ADAPTER = :mysql2 class << self - def unique_index_name(error_message) - error_message[/key '([^']+)'/, 1]&.split('.')&.last + def unique_index_name(error) + error.message[/key '([^']+)'/, 1]&.split('.')&.last end - def unique_error_columns(_error_message); end + def unique_error_columns(_error); end - def foreign_key_error_column(error_message) - error_message[/FOREIGN KEY \(`([^`]+)`\)/, 1] + def foreign_key_error_column(error) + column = error.message[/FOREIGN KEY \(`([^`]+)`\)/, 1] + column ? [column] : [] end end end diff --git a/lib/database_validations/lib/adapters/postgresql_adapter.rb b/lib/database_validations/lib/adapters/postgresql_adapter.rb index 0b559a5..f1fe802 100644 --- a/lib/database_validations/lib/adapters/postgresql_adapter.rb +++ b/lib/database_validations/lib/adapters/postgresql_adapter.rb @@ -4,16 +4,17 @@ class PostgresqlAdapter < BaseAdapter ADAPTER = :postgresql class << self - def unique_index_name(error_message) - error_message[/unique constraint "([^"]+)"/, 1] + def unique_index_name(error) + error.message[/unique constraint "([^"]+)"/, 1] end - def unique_error_columns(error_message) - error_message[/Key \((.+)\)=/, 1].split(', ') + def unique_error_columns(error) + error.message[/Key \((.+)\)=/, 1].split(', ') end - def foreign_key_error_column(error_message) - error_message[/Key \(([^)]+)\)/, 1] + def foreign_key_error_column(error) + column = error.message[/Key \(([^)]+)\)/, 1] + column ? [column] : [] end end end diff --git a/lib/database_validations/lib/adapters/sqlite_adapter.rb b/lib/database_validations/lib/adapters/sqlite_adapter.rb index 6a3eb36..dfefc4b 100644 --- a/lib/database_validations/lib/adapters/sqlite_adapter.rb +++ b/lib/database_validations/lib/adapters/sqlite_adapter.rb @@ -4,14 +4,21 @@ class SqliteAdapter < BaseAdapter ADAPTER = :sqlite3 class << self - def unique_index_name(_error_message); end + def unique_index_name(error) + error.message[/UNIQUE constraint failed: index '([^']+)'/, 1] + end - def unique_error_columns(error_message) - error_message.scan(/\w+\.([^,:]+)/).flatten + def unique_error_columns(error) + error.message.scan(/\w+\.([^,:]+)/).flatten end - def foreign_key_error_column(error_message) - error_message[/\("([^"]+)"\) VALUES/, 1] + def foreign_key_error_column(error) + return [] unless error.respond_to?(:sql) && error.sql + + columns_clause = error.sql[/\(([^)]+)\)\s*VALUES/i, 1] + return [] unless columns_clause + + columns_clause.scan(/"([^"]+)"/).flatten end end end diff --git a/lib/database_validations/lib/rescuer.rb b/lib/database_validations/lib/rescuer.rb index 0b45c32..7bde96e 100644 --- a/lib/database_validations/lib/rescuer.rb +++ b/lib/database_validations/lib/rescuer.rb @@ -15,21 +15,58 @@ def handled?(instance, error, validate) end def process(validate, instance, error, key_types) + keys = resolve_keys(instance, error, key_types) + attribute_validator = find_matching_validator(instance, keys) + return false unless attribute_validator + + process_validator(validate, instance, attribute_validator) + end + + def resolve_keys(instance, error, key_types) adapter = Adapters.factory(instance.class) - keys = key_types.map do |key_generator, error_processor| - KeyGenerator.public_send(key_generator, adapter.public_send(error_processor, error.message)) + key_types.flat_map do |key_generator, error_processor| + result = adapter.public_send(error_processor, error) + + # FK adapters return an array of candidate columns, each generating a + # separate key. Uniqueness adapters return columns that form a single + # composite key, passed together to the key generator. + if key_generator == :for_db_presence + Array(result).map { |column| KeyGenerator.public_send(key_generator, column) } + else + [KeyGenerator.public_send(key_generator, result)] + end end + end + + def find_matching_validator(instance, keys) + first_match = nil keys.each do |key| attribute_validator = instance._db_validators[key] - next unless attribute_validator - return process_validator(validate, instance, attribute_validator) + first_match ||= attribute_validator + + next if (keys.size > 1) && !foreign_key_invalid?(instance, attribute_validator) + + return attribute_validator end - false + # TOCTOU fallback: if disambiguate queries all passed (concurrent insert), + # use the first matching validator rather than leaving the error unhandled. + first_match + end + + def foreign_key_invalid?(instance, attribute_validator) + attribute = attribute_validator.attribute + reflection = instance.class._reflect_on_association(attribute) + return true unless reflection + + fk_value = instance.read_attribute(reflection.foreign_key) + return true if fk_value.blank? + + !reflection.klass.exists?(reflection.association_primary_key => fk_value) end def process_validator(validate, instance, attribute_validator) diff --git a/spec/adapters/foreign_key_error_column_spec.rb b/spec/adapters/foreign_key_error_column_spec.rb new file mode 100644 index 0000000..6e11c5c --- /dev/null +++ b/spec/adapters/foreign_key_error_column_spec.rb @@ -0,0 +1,46 @@ +RSpec.describe 'Adapter.foreign_key_error_column' do + def build_error(message:, sql: nil) + error = ActiveRecord::InvalidForeignKey.new(message) + error.set_query(sql, []) if sql + error + end + + describe DatabaseValidations::Adapters::PostgresqlAdapter do + it 'extracts column from error message' do + error = build_error( + message: 'PG::ForeignKeyViolation: ERROR: insert or update on table "db_belongs_users" violates ' \ + 'foreign key constraint "fk_rails_abc123" DETAIL: Key (company_id)=(-1) is not present in table "companies".' + ) + expect(described_class.foreign_key_error_column(error)).to eq(['company_id']) + end + end + + describe DatabaseValidations::Adapters::MysqlAdapter do + it 'extracts column from error message' do + error = build_error( + message: 'Mysql2::Error: Cannot add or update a child row: a foreign key constraint fails ' \ + '(`test`.`db_belongs_users`, CONSTRAINT `fk_rails_abc123` FOREIGN KEY (`company_id`) ' \ + 'REFERENCES `companies` (`id`))' + ) + expect(described_class.foreign_key_error_column(error)).to eq(['company_id']) + end + end + + describe DatabaseValidations::Adapters::SqliteAdapter do + it 'extracts column from single-column INSERT SQL' do + error = build_error( + message: 'SQLite3::ConstraintException: FOREIGN KEY constraint failed', + sql: 'INSERT INTO "db_belongs_users" ("company_id") VALUES (?) RETURNING "id"' + ) + expect(described_class.foreign_key_error_column(error)).to eq(['company_id']) + end + + it 'extracts all columns from multi-column INSERT SQL' do + error = build_error( + message: 'SQLite3::ConstraintException: FOREIGN KEY constraint failed', + sql: 'INSERT INTO "db_belongs_users" ("company_id", "department_id") VALUES (?, ?) RETURNING "id"' + ) + expect(described_class.foreign_key_error_column(error)).to eq(%w[company_id department_id]) + end + end +end diff --git a/spec/validations/db_belongs_to_spec.rb b/spec/validations/db_belongs_to_spec.rb index 1b28ff5..9efaeef 100644 --- a/spec/validations/db_belongs_to_spec.rb +++ b/spec/validations/db_belongs_to_spec.rb @@ -4,6 +4,8 @@ class Company < ActiveRecord::Base; end class BelongsUser < ActiveRecord::Base; end class DbBelongsUser < ActiveRecord::Base; end + class Department < ActiveRecord::Base; end + class MultiFkUser < ActiveRecord::Base; end # rubocop:enable RSpec/LeakyConstantDeclaration # rubocop:enable Lint/ConstantDefinitionInBlock @@ -138,6 +140,53 @@ def define_tables end end end + + describe 'multiple db_belongs_to associations' do # rubocop:disable RSpec/MultipleMemoizedHelpers: + let(:department_klass) { define_class(Department, :departments) } + + let(:multi_fk_klass) do + define_class(MultiFkUser, :multi_fk_users) do + db_belongs_to :company + db_belongs_to :department + end + end + + before do + ActiveRecord::Schema.define(version: 2) do + create_table :departments + create_table :multi_fk_users do |t| + t.belongs_to :company, foreign_key: true + t.belongs_to :department, foreign_key: true + end + end + end + + it 'handles invalid company_id with valid department_id' do + company_klass + department = department_klass.create! + record = multi_fk_klass.new(company_id: -1, department_id: department.id) + expect(record.save).to be false + expect(record.errors[:company]).to be_present + expect(record.errors[:department]).to be_empty + end + + it 'handles valid company_id with invalid department_id' do + company = company_klass.create! + department_klass + record = multi_fk_klass.new(company_id: company.id, department_id: -1) + expect(record.save).to be false + expect(record.errors[:department]).to be_present + expect(record.errors[:company]).to be_empty + end + + it 'handles both invalid' do + company_klass + department_klass + record = multi_fk_klass.new(company_id: -1, department_id: -1) + expect(record.save).to be false + expect(record.errors.messages.keys).to include(:company).or include(:department) + end + end end describe 'postgresql' do @@ -149,17 +198,14 @@ def define_tables include_examples 'works as belongs_to' end - # TODO: validate options - # describe 'sqlite3' do - # before do - # define_database(sqlite_configuration) - # define_tables - # end - # - # specify do - # expect { db_belongs_to_user_klass }.to raise_error DatabaseValidations::Errors::UnsupportedDatabase - # end - # end + describe 'sqlite3' do + before do + define_database(sqlite_configuration) + define_tables + end + + include_examples 'works as belongs_to' + end describe 'mysql' do before do diff --git a/spec/validations/validates_db_uniqueness_of_spec.rb b/spec/validations/validates_db_uniqueness_of_spec.rb index 430f768..72321b2 100644 --- a/spec/validations/validates_db_uniqueness_of_spec.rb +++ b/spec/validations/validates_db_uniqueness_of_spec.rb @@ -889,6 +889,11 @@ def catch_error_message before { define_database(sqlite_configuration) } include_examples 'works as expected' + include_examples 'supports condition option' + include_examples 'supports index_name option' + include_examples 'supports complex indexes' + include_examples 'supports index_name with where option' + include_examples 'supports index_name with scope option' include_examples 'when index_name is passed only one attribute can be provided' end