Missing Presence Validation with Polymorphic Type Column

[Numie]

I have a polymorphic association like:

belongs_to :documentable, polymorphic: true

There are not_null db constraints on the related documentable_id and documentable_type columns. When I run active_record_doctor, I get the following error:

add a `presence` validator to Document.documentable_type - it's NOT NULL but lacks a validator

I don't think an error should be raised since ActiveRecord already requires both the type and id columns from the single belongs_to association.

[gregnavis]

@Numie, I think what's wrong here is that the error message should be different: it should tell you to add optional: false to the association. I'm working on that enhancement.

Please double-check this, but I think if you do the following:

model = YourModel.new(documentable: nil)
model.validate

Then you won't find an error on documentable.

[fatkodima]

belongs_to associations are required by default, so do not get why you should add optional: false to them?
Since OP has not null constraints on both columns, there should not be any error messages from this gem.

[gregnavis]

@fatkodima, you're right. I was confused by the fact that belongs_to seem not to be required by default in active_record_doctor test suite. For example, when I took your PR and made the following change:

  def test_non_null_column_is_not_reported_if_polymorphic_association_validation_present
    Context.create_table(:images) do |t|
      t.references :imageable, null: false, polymorphic: true
    end.define_model do
      # I removed required from the association.
      belongs_to :imageable, polymorphic: true
    end

    refute_problems
  end

Then I got an error:

% DATABASE_ADAPTER=postgresql ruby -Ilib -Itest -rsetup test/active_record_doctor/detectors/missing_presence_validation_test.rb -n test_non_null_column_is_not_reported_if_polymorphic_association_validation_present
Using postgresql
Run options: -n test_non_null_column_is_not_reported_if_polymorphic_association_validation_present --seed 40829

# Running:

F

Failure:
ActiveRecordDoctor::Detectors::MissingPresenceValidationTest#test_non_null_column_is_not_reported_if_polymorphic_association_validation_present [test/active_record_doctor/detectors/missing_presence_validation_test.rb:52]:
--- expected
+++ actual
@@ -1 +1 @@
-[]
+["add a `presence` validator to Context::Image.imageable_id - it's NOT NULL but lacks a validator", "add a `presence` validator to Context::Image.imageable_type - it's NOT NULL but lacks a validator"]



bin/rails test test/active_record_doctor/detectors/missing_presence_validation_test.rb:45

Are you able to reproduce it on your end?

[fatkodima]

Yes, I get the same error with your changes.

belongs_to_required_by_default is set to nil in ActiveRecord by default and is set to true in a rails app via
https://github.com/rails/rails/blob/f0d66d6c47b9def492a6b5f70a7a45907e38fadc/railties/lib/rails/application/configuration.rb#L120-L122

So we need to add to test/setup.rb

ActiveRecord::Base.belongs_to_required_by_default = true

or leave a required: true as in the original test case.

[gregnavis]

Thanks for the link. Sadly, it seems the default is not set within the active record gem.

I prefer not to add an explicit dependency on the rails. however, given required is deprecated I suggest we pass optional explicitly. I'll leave some more feedback on the PR.

[gregnavis]

@Numie, I pushed #230 to fix the issue properly. Could you test that branch locally and tell me whether it's working as expected?

[Numie]

@gregnavis It does resolve the original issue, however I'm getting a new output that was not present before:

add `optional: false` to User.account - the foreign key account_id is NOT NULL

The above output does not go away even if I add optional: false, which should be unnecessary anyway since belongs_to is required by default.

belongs_to :account

[gregnavis]

I could reproduce that in the test suite only when the association was NOT required.

Could you run bin/rails console in your app and post the results of the following three commands:

ActiveRecord::Base.belongs_to_required_by_default

User.validators.select { _1.attributes.include?(:account) }

User.new.tap(&:validate).errors.details[:account]

Example output from one of my projects:

ActiveRecord::Base.belongs_to_required_by_default
# => true

Order.validators.select { _1.attributes.include?(:shipping_address) }
# => [#<ActiveRecord::Validations::PresenceValidator:0x000000011e110a48
#  @attributes=[:shipping_address],
#  @options=
#   {message: :required,
#    if: #<Proc:0x000000011e110c50 /Users/gregnavis/.gem/ruby/3.4.4/gems/activerecord-8.0.2/lib/active_record/associations/builder/belongs_to.rb:130 (lambda)>}>]

Order.new.tap(&:validate).errors.details[:shipping_address]
# => [{error: :blank}]

[Numie]

ActiveRecord::Base.belongs_to_required_by_default
=> true

User.validators.select  { _1.attributes.include?(:account) }
=> 
[#<ActiveRecord::Validations::PresenceValidator:0x00000001276527f0
  @attributes=[:account],
  @options={:message=>:required}>]

User.new.tap(&:validate).errors.details[:account]
=> [{:error=>:blank}]

[gregnavis]

Thank you, @Numie. I was able to reproduce this problem on my own project. I should have an update later today.

[gregnavis]

@Numie, I pushed a small update that fixed the problem on my end. Could you pull the branch and test again?

[Numie]

@gregnavis It's fixed for me too, thanks!

[gregnavis]

Perfect! Thank you for reporting and collaborating on fixing it.