About `Rails/RedundantPresenceValidationOnBelongsTo`

[stephannv]

Hi, people. I'm upgrading my project dependencies (including rubocop, rubocop-rails, etc.), and noted a new offense warning: Rails/RedundantPresenceValidationOnBelongsTo. My code:

class User < ApplicationRecord
end

class Post < ApplicationRecord
  belongs_to :user

  validates :user_id, presence: true
end

But I don't think this presence validation is redundant. With: validates :user_id, presence: true

post = Post.new(user: User.new) => #<Post:...>
post.valid? # => false 
post.save # => false

Without: validates :user_id, presence: true

post = Post.new(user: User.new) => #<Post:...>
post.valid? # => true
post.save # => PG::NotNullViolation: ERROR:  null value in column "user_id" of relation "posts" violates not-null constraint (ActiveRecord::NotNullViolation)

In other words. This isn't redundant behavior, because the application behaves in different ways with validation and without validation. The new defaults introduced on Rails 5.0.0 (https://github.com/rubocop/rubocop-rails/issues/578) cares only about the presence of the object, it doesn't care about if the object has an ID, this is why I add a presence validation to relation id.

I know that I can disable this cop on my .rubocop.yml, but should this cop be enabled by default? Or, is this cop misleading the devs?

---

Expected behavior

Rails/RedundantPresenceValidationOnBelongsTo shouldn't detect offense.

Actual behavior

Rails/RedundantPresenceValidationOnBelongsTo is detecting offense.

Steps to reproduce the problem

# app/models/user.rb
class User < ApplicationRecord
end

# app/models/post.rb
class Post < ApplicationRecord
  belongs_to :user

  validates :user_id, presence: true
end

RuboCop version

1.25.0 (using Parser 3.1.0.0, rubocop-ast 1.15.1, running on ruby 3.0.2 x86_64-darwin21)
  - rubocop-performance 1.13.2
  - rubocop-rails 2.13.2
  - rubocop-rspec 2.7.0

[pirj]

It feels that the default autosave behaviour is broken in your example for some reason.

If the :autosave option is not present, then new associated objects will be saved

I'll take a stab at creating an executable example to make sure of what is the default behaviour using Rails template. If you want to take care of this earlier - please don't hesitate.

[stephannv]

Thanks @pirj. I've noticed that the error will only occur if the parent object has some wrong validation. I created a reproducible snippet:

require 'bundler/inline'

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'activerecord', '7.0.1'
  gem 'sqlite3'
  gem 'debug'
end

require 'active_record'
require 'minitest/autorun'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :users do |t|
    t.string :name, null: false
  end

  create_table :posts do |t|
    t.references :user, null: false, foreign_key: true
  end
end

class User < ActiveRecord::Base
  validates :name, presence: true
end

class Post < ActiveRecord::Base
  belongs_to :user
end

class Test < Minitest::Test
  def setup
    @post = Post.new(user: User.new)
  end

  def teardown
    Post.clear_validators!
  end

  def test_without_validation
    assert @post.valid? == true
    assert @post.save == false # => RAISES ERROR HERE
  end

  def test_with_validation
    Post.validates :user_id, presence: true

    assert @post.valid? == false
    assert @post.save == false
  end
end

I think that removing the validation of object_id leads to object to behave differently when the parent object isn't valid. I like to think:

• I can waive validation of parent id when I creating two objects at same time, eg. post and its tags.
• If I want to guarantee that parent object is persisted, I have to validate parent id. eg. user and post.

[pirj]

parent object has some wrong validation

You mean that it behaves differently (unexpectedly) when the associated User model instance validation fails?

I'll dare saying that this is a Rails issue, and post.save should say something like user is invalid and not attempt to persist it to the database. Swallowing User's validation error and proceeding with persisting post doesn't seem justified. What do you think?

[stephannv]

Yeah, @pirj. I think this is an old Rails issue that I assumed as normal behavior and I always had to work around to avoid problems. But what you think about rubocop recommendation to remove validation? Due to this strange Rails behavior, removing validation can break some applications out there. If you think this isn't Rubocop's concern, I can close this issue. At least, I think we can add a note to safety paragraph:

## Safety
This cop’s autocorrection is unsafe because it changes the default error message from "can’t be blank" to "must exist". 

Due to Rails behavior, without association_id presence validation, if associated object can't be saved, the object will try to persist anyway and it may raise exception due to database restrictions.

But I think this cop should be changed to detect redundancy on: validates :user, presence: true and not validates :user_id, presence: true.

[pirj]

It surely makes sense to add a note to the safety section. It would be perfect if the note referred to a Rails ticket, especially if the Rails team acknowledges this ticket as a Rails bug. Are you up to submit a ticket to Rails bug tracker?

[stephannv]

@pirj but I'm not sure if this is a bug, it can a design choice. The belongs_to implicit presence validation is about presence of object, isn't related if object is persisted or not. In this case post = Post.new(user: User.new), the user is present, so valid? is true and Rails will try to persist the object and can raises a database error. The validates :user_id, presence: true isn't bullet proof too, if I do post = Post.new(user: User.new(id: 5)), it can raises an error too if I have a foreing key on user_id and user with id = 5 doesn't exist. My whole point is: Doing validates :user, presence: true (this is the implict presence validation added by belongs_to ) isn't the same as doing validates :user_id, presence: true, the behaviors are different, so I think rubocop by default should not to recommend to remove validates :user_id, presence, the recommendation should be to remove only validates :user, presence: true.

[pirj]

I agree with your point, with a small note.

the user is present, so valid? is true

It makes sense according to the Rails guides:

If you set the :validate option to true, then new associated objects will be validated whenever you save this object. By default, this is false: new associated objects will not be validated when this object is saved.

So setting

class Post < ...
  belongs_to :user, validate: true

is a better approach. Would you agree?

rubocop by default should not to recommend to remove validates :user_id, presence, the recommendation should be to remove only validates :user, presence: true.

then becomes:

instead of recommending to just remove validates :user_id, presence: true, RuboCop should also recommend adding a validate: true on an association

Does this sound reasonable to you?

[stephannv]

Yeah, it makes sense @pirj. Maybe this can became two cops: Rails/RedundantPresenceValidationOnBelongsTo, one for validates :user, presence: true redundancy and other for recommending associated object validation, using validates: true or validates_associated. I don't have full knowledge about validates: true or validates_associated, so I don't know how it exactly works and how the cop would work.

[pirj]

Would you like to share the effort, @stephannv ?

[stephannv]

Hi, @pirj. Sorry for the delay. I'm involved in a game development in my free time and I won't be able to look at this problem.

[tenpaiyomi]

I ran into this today and wanted to make a note, I don't believe the expected replacement of validates :user, presence: true is belongs_to :user, validate: true. The Rails guide makes a note about 2 specific and applicable options here, :validate and :optional. :validate causes the associated object to be validated upon save of the parent object, and defaults to false, whereas :optional determines if the presence of the object is required and defaults to false.

If the user simply has validates :user, presence: true, then just removing it would result in the (explicitly) correct behavior. Advocating for setting validate: true is completely different and separate behavior. I (personally) believe that validate: true should be set, but I don't believe it is relevant here.

Perhaps just adding a note for the user to review the difference between the belongs_to options for :validate and :optional and letting them select the correct options is the more appropriate path?

[pirj]

validate: true should be set, but I don't believe it is relevant here

Indeed. This is why we're thinking of:

this can became two cops

adding a note for the user

Makes sense, too. Do you have some wording in mind, would you like to contribute such a note?

[halo]

Often I'll send params[:user_id] from a <form> and show the errors on the page, if no user was selected.

I'm not sure whether this is a feature or a bug, but only if I add a presence validation manually it will properly generate an error on the Post model, indicating that the user is missing.

So this has nothing to do with validating associated models. It's on the immediate record and I need to frequently violate the cop to make it work.

I'm a bit surprised nobody else seems to have this problem, so I wish I knew whether the cop is wrong, or Rails or me 😅

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  gem 'activerecord', '7.0.1'
  gem 'sqlite3'
  gem 'debug'
end

require 'active_record'
require 'minitest/autorun'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :users do |t|
    t.string :name, null: false
  end

  create_table :posts do |t|
    t.references :user, null: false, foreign_key: true
  end
end

class User < ActiveRecord::Base
end

class Post < ActiveRecord::Base
  belongs_to :user
end

class Test < Minitest::Test
  def setup
    @post = Post.new
  end

  def teardown
    Post.clear_validators!
  end

  def test_with_validation
    Post.validates :user, presence: true # Cop violation
    @post.validate

    assert @post.errors.any? == true # This is expected and correct
  end

  def test_with_validation_on_id
    Post.validates :user_id, presence: true # Cop violation
    @post.validate

    assert @post.errors.any? == true # This is expected and correct
  end

  def test_without_validation
    # Cop is happy
    @post.validate

    assert @post.errors.any? == true # Fails! But I wanted a validation error.
  end
end

[pirj]

What's strange, it this test Post.belongs_to_required_by_default is nil. While in the real code it is supposed to be true.

Add

ActiveRecord::Base.belongs_to_required_by_default = true

to your snippet.

You also have to remove Post.clear_validators! from teardown, and call it from test_with_validation and test_with_validation_on_id. Otherwise, it also clears the presence validator defined by belongs_to, @halo .

[halo]

Ah, thank you very much for verifying my results. I improved the test and the error message is properly populated.

It's up to me now, to look up (and display) errors on toolbox even though the <input> is on toolbox_id.

For posterity, here the improved test setup:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  gem 'activerecord', '7.0.1'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new($stdout)
ActiveRecord::Base.belongs_to_required_by_default = true

ActiveRecord::Schema.define do
  create_table :toolboxes do |t|
  end

  create_table :nails do |t|
    t.references :toolbox, null: false, foreign_key: true
  end

  create_table :screws do |t|
    t.references :toolbox, null: false, foreign_key: true
  end
end

class Toolbox < ActiveRecord::Base
end

class Nail < ActiveRecord::Base
  belongs_to :toolbox
end

class Screw < ActiveRecord::Base
  belongs_to :toolbox
  validates :toolbox_id, presence: true # Cop violation
end

class Test < Minitest::Test
  def test_correct_setup
    assert ActiveRecord::Base.belongs_to_required_by_default == true
  end

  def test_with_validation_on_id
    nail = Nail.new
    nail.validate

    assert nail.errors.attribute_names == [:toolbox] # PASS
  end

  def test_without_validation
    screw = Screw.new
    screw.validate

    assert screw.errors.attribute_names == %i[toolbox toolbox_id] # PASS
  end
end

[pirj]

As a side note, I'd say that it might make sense to ask the Rails team a question regarding executable test cases specifically for ActiveRecord.

Without loading the defaults those test cases are rather inconsistent with the normal behaviour like we've just seen. And ::Rails is not even defined, just like it's load_defaults, so I'm personally puzzled how to make it consistent, and if this inconsistency is only for belongs_to_required_by_default, or other options could differ from their defaults, too.