From 4fbb40d68fc09a8cc37e0b0ac5a41cc599690dff Mon Sep 17 00:00:00 2001 From: fatkodima Date: Thu, 27 Mar 2025 22:42:19 +0200 Subject: [PATCH] incorrect_length_validation: Consider `inclusion` validations as an alternative --- .../detectors/incorrect_length_validation.rb | 24 +++++-- .../incorrect_length_validation_test.rb | 68 +++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/lib/active_record_doctor/detectors/incorrect_length_validation.rb b/lib/active_record_doctor/detectors/incorrect_length_validation.rb index f19a0767..34e83af2 100644 --- a/lib/active_record_doctor/detectors/incorrect_length_validation.rb +++ b/lib/active_record_doctor/detectors/incorrect_length_validation.rb @@ -33,21 +33,23 @@ def message(model:, attribute:, table:, database_maximum:, model_maximum:) def detect each_model(except: config(:ignore_models), existing_tables_only: true) do |model| each_attribute(model, except: config(:ignore_attributes), type: [:string, :text]) do |column| - model_maximum = maximum_allowed_by_validations(model, column.name.to_sym) - next if model_maximum == column.limit + model_maximum = maximum_allowed_by_length_validation(model, column.name.to_sym) + database_maximum = column.limit + next if model_maximum == database_maximum + next if database_maximum && covered_by_inclusion_validation?(model, column.name.to_sym, database_maximum) problem!( model: model.name, attribute: column.name, table: model.table_name, - database_maximum: column.limit, + database_maximum: database_maximum, model_maximum: model_maximum ) end end end - def maximum_allowed_by_validations(model, column) + def maximum_allowed_by_length_validation(model, column) length_validator = model.validators.find do |validator| validator.kind == :length && validator.options.include?(:maximum) && @@ -55,6 +57,20 @@ def maximum_allowed_by_validations(model, column) end length_validator ? length_validator.options[:maximum] : nil end + + def covered_by_inclusion_validation?(model, column, limit) + inclusion_validator = model.validators.find do |validator| + validator.kind == :inclusion && + validator.attributes.include?(column) && + [:if, :unless].none? { |option| validator.options.key?(option) } && + [:allow_nil, :allow_blank].none? { |option| validator.options[option] == true } + end + + return false if !inclusion_validator + + values = inclusion_validator.options[:in] || inclusion_validator.options[:within] + values.is_a?(Array) && values.all? { |value| value.is_a?(String) && value.size <= limit } + end end end end diff --git a/test/active_record_doctor/detectors/incorrect_length_validation_test.rb b/test/active_record_doctor/detectors/incorrect_length_validation_test.rb index f30b9cc4..b06426b8 100644 --- a/test/active_record_doctor/detectors/incorrect_length_validation_test.rb +++ b/test/active_record_doctor/detectors/incorrect_length_validation_test.rb @@ -61,6 +61,74 @@ def test_no_validation_and_no_limit_is_ok refute_problems end + def test_array_inclusion_validation_with_shorter_values_and_limit_is_ok + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ["new", "vip", "a" * 64] } + end + + refute_problems + end + + def test_array_inclusion_validation_with_longer_values_and_limit_is_error + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ["new", "vip", "a" * 65] } + end + + assert_problems(<<~OUTPUT) + the schema limits users.status to 64 characters but there's no length validator on Context::User.status - remove the database limit or add the validator + OUTPUT + end + + def test_proc_inclusion_validation_and_limit_is_error + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ->(user) {} } + end + + assert_problems(<<~OUTPUT) + the schema limits users.status to 64 characters but there's no length validator on Context::User.status - remove the database limit or add the validator + OUTPUT + end + + def test_array_inclusion_validation_with_shorter_values_and_if_is_error + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ["new", "vip"] }, if: :condition? + end + + assert_problems(<<~OUTPUT) + the schema limits users.status to 64 characters but there's no length validator on Context::User.status - remove the database limit or add the validator + OUTPUT + end + + def test_array_inclusion_validation_with_shorter_values_and_allow_nil_is_error + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ["new", "vip"] }, allow_nil: true + end + + assert_problems(<<~OUTPUT) + the schema limits users.status to 64 characters but there's no length validator on Context::User.status - remove the database limit or add the validator + OUTPUT + end + + def test_array_inclusion_validation_with_shorter_values_and_disallow_nil_is_ok + Context.create_table(:users) do |t| + t.string :status, limit: 64 + end.define_model do + validates :status, inclusion: { in: ["new", "vip"] }, allow_nil: false + end + + refute_problems + end + def test_config_ignore_models Context.create_table(:users) do |t| t.string :email, limit: 64