Rails/RedundantPresenceValidationOnBelongsTo does not work if config.active_record.belongs_to_required_by_default is false

[tim-bellinghausen]

Rails/RedundantPresenceValidationOnBelongsTo marks an explicit presence validation as an error, even if config.active_record.belongs_to_required_by_default is set to false. In this case removing the validation alters the behaviour of the model. One might argue that setting required: true on the belongs_to would be better, but there are cases where this is not feasible. One example is a model where a relation is only required in some cases with something like

validates :relation, presence: true, if: -> { some_condition? }

In this particular case, autocorrecting the cop leads to the invalid statement

validates :relation, if: -> { some_condition? }

Expected behavior

If config.active_record.belongs_to_required_by_default is set to false, Rails/RedundantPresenceValidationOnBelongsTo should not mark presence validations for belongs_to relations as an error.

Actual behavior

Rails/RedundantPresenceValidationOnBelongsTo marks presence validations for belongs_to relations as an error without considering the value of config.active_record.belongs_to_required_by_default.

Steps to reproduce the problem

Create a rails application and add config.active_record.belongs_to_required_by_default = false to config/application.rb. Create two models, where the second model has a belongs_to relation to the first. This relation is now not required. Add a presence validation for this relation in the second model. Now run rubocop, the presence validation will now be marked as an error.

RuboCop version

1.24.1 (using Parser 3.0.3.2, rubocop-ast 1.15.1, running on ruby 2.6.5 x86_64-darwin19)
  - rubocop-rails 2.13.1
  - rubocop-rspec 2.7.0

[pirj]

The cop's doc states:

Since Rails 5.0 the default for belongs_to is optional: false unless config.active_record.belongs_to_required_by_default is explicitly set to false. The presence validator is added automatically, and explicit presence validation is redundant.

I can't think of a way for cops to reliably read and parse Rails' configuration files, especially in a multi-app project or a project with engine subdirectories.

Those not adhering to the Rails' default settings, specifically belongs_to_required_by_default should disable the cop in their project's .rubocop.yml:

Rails/RedundantPresenceValidationOnBelongsTo:
  Enabled: false

Personally, I would recommend putting it to .rubocop_todo.yml.

[andyw8]

I agree. As the docs say, this setting is intended to "help you migrate all your models to have their associations required by default."

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-framework-defaults

[koic]

I also agree with @pirj opinion.

NOTE: In the future, it may be possible to develop an implementation that references the Rails standard configuration (e.g.: config/application.rb) created by rails new (which does not include the engine configuration), but it does not currently have that mechanism. And, ideally this Rails/RedundantPresenceValidationOnBelongsTo cop should work in pairs with a new cop who inspects config.active_record.belongs_to_required_by_default = false. Therefore, I will leave it as an enhancement issue, but now it is an expected behavior.

[mockdeep]

FWIW, we disable it for performance reasons. Validating the record can cause extra db queries if the association isn't already loaded. This is especially a problem when validating a number of records. We instead validate on the association _id and trust the foreign key constraint to ensure that the record is actually there.

[pirj]

Good point and a very interesting observation, @mockdeep. On a side note, you may like the approach taken in database_validations.

Wondering why does Rails fetch the whole record SELECT * FROM ... WHERE id = ? when a SELECT COUNT(id) FROM ... WHERE id = ? would suffice to make sure the record is there? 🤔 Supposedly this is due to this line in lib/active_model/validations/presence.rb:

record.errors.add(attr_name, :blank, options) if value.blank?

and blank? that would fetch the entire record. I don't see an obvious way of fixing this so that it doesn't fetch the entire record if it's not loaded yet, and rather just checks if it exists in the DB.

[mockdeep]

Wondering why does Rails fetch the whole record SELECT * FROM ... WHERE id = ? when a SELECT COUNT(id) FROM ... WHERE id = ? would suffice to make sure the record is there?

I think this might be a bit of a philosophical issue. Historically, Rails has put an emphasis on doing many things in application code. They didn't support foreign key constraints in migrations for the longest time. In a way, ActiveRecord is built to help you forget about the underlying database, so validating on an id rather than the record kind of breaks that abstraction. Unfortunately, I think this emphasis means that ActiveRecord lacks some consideration for the actual performance implications of dealing with a database at scale. We do a lot of bulk imports and updates at our company, and we have had to build some of our own tooling to support this and still try to keep the code high level.

I don't want to complain too much, though. AR is a pretty awesome tool, and it's (mostly) getting better. But sometimes I'm surprised when they add a change like this, or when they don't have certain affordances for managing performance.