-
-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rails/RedundantPresenceValidationOnBelongsTo does not work if config.active_record.belongs_to_required_by_default is false #630
Comments
The cop's doc states:
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 Rails/RedundantPresenceValidationOnBelongsTo:
Enabled: false Personally, I would recommend putting it to |
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 |
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.: |
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 |
Good point and a very interesting observation, @mockdeep. Wondering why does Rails fetch the whole record record.errors.add(attr_name, :blank, options) if value.blank? and |
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. |
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 settingrequired: true
on thebelongs_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 likeIn this particular case, autocorrecting the cop leads to the invalid statement
Expected behavior
If
config.active_record.belongs_to_required_by_default
is set to false, Rails/RedundantPresenceValidationOnBelongsTo should not mark presence validations forbelongs_to
relations as an error.Actual behavior
Rails/RedundantPresenceValidationOnBelongsTo marks presence validations for
belongs_to
relations as an error without considering the value ofconfig.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
toconfig/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
The text was updated successfully, but these errors were encountered: