Skip to content

Commit

Permalink
Fix MissingNonNullConstraint to work with `ActiveRecord.belongs_to_…
Browse files Browse the repository at this point in the history
…required_validates_foreign_key`
  • Loading branch information
fatkodima committed Aug 27, 2024
1 parent a455a7e commit 7c87723
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 2 deletions.
18 changes: 16 additions & 2 deletions lib/active_record_doctor/detectors/missing_non_null_constraint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,28 @@ def required_presence_validators(model)
model.validators.select do |validator|
validator.is_a?(ActiveRecord::Validations::PresenceValidator) &&
!validator.options[:allow_nil] &&
!validator.options[:if] &&
!validator.options[:unless]
(rails_belongs_to_presence_validator?(validator) || !conditional_validator?(validator))
end
end

def sti_column?(models, column_name)
models.any? { |model| model.inheritance_column == column_name }
end

def rails_belongs_to_presence_validator?(validator)
ActiveRecord.version >= Gem::Version.new("7.1") &&
!ActiveRecord.belongs_to_required_validates_foreign_key &&
validator.options[:message] == :required &&
proc_from_activerecord?(validator.options[:if])
end

def conditional_validator?(validator)
validator.options[:if] || validator.options[:unless]
end

def proc_from_activerecord?(object)
object.is_a?(Proc) && object.binding.receiver.name.start_with?("ActiveRecord")
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,17 @@ def test_presence_true_is_reported_on_boolean_only
validates :email, :active, presence: true
end

# Another model is defined in a different database to ensure that all
# databases are checked.
SecondaryContext.create_table(:users) do |t|
t.boolean :active, null: false
end.define_model do
validates :active, presence: true
end

assert_problems(<<~OUTPUT)
replace the `presence` validator on Context::User.active with `inclusion` - `presence` can't be used on booleans
replace the `presence` validator on SecondaryContext::User.active with `inclusion` - `presence` can't be used on booleans
OUTPUT
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,28 @@ def test_optional_columns_with_required_polymorphic_association_are_disallowed
OUTPUT
end

def test_optional_foreign_keys_with_required_with_if_association_are_disallowed
skip("ActiveRecord < 7.1 doesn't support optimized association presence validation") if ActiveRecord::VERSION::STRING < "7.1"

begin
previous = ActiveRecord.belongs_to_required_validates_foreign_key
ActiveRecord.belongs_to_required_validates_foreign_key = false

Context.create_table(:companies)
Context.create_table(:users) do |t|
t.references :company, null: true
end.define_model do
belongs_to :company, required: true
end

assert_problems(<<~OUTPUT)
add `NOT NULL` to users.company_id - models validates its presence but it's not non-NULL in the database
OUTPUT
ensure
ActiveRecord.belongs_to_required_validates_foreign_key = previous
end
end

def test_required_columns_with_required_polymorphic_association_are_allowed
Context.create_table(:comments) do |t|
t.references :commentable, polymorphic: true, null: false
Expand Down
7 changes: 7 additions & 0 deletions test/setup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ class SecondaryRecord < ApplicationRecord
SecondaryContext = TransientRecord.context_for SecondaryRecord
end

if ActiveRecord.version >= Gem::Version.new("7.1")
# See https://github.com/rails/rails/pull/46522 for details.
# When `false` (default in Rails 7.1) - validate presence only when the foreign key changed.
# When `true` - always validate the association presence.
ActiveRecord.belongs_to_required_validates_foreign_key = true
end

# Prepare the test class.
class Minitest::Test
def setup
Expand Down

0 comments on commit 7c87723

Please sign in to comment.