Presence validations which have an if condition on related models seem broken

[maknz]

After upgrading from 3.1.3 to 4.0.1, I've found that two of our tests that use shoulda-matchers have started to fail. Both of these tests are asserting that a model validates presence of an attribute, as well as including an if condition for whether to run the validation, that accesses related models.

Suppose we have two models, User and Group, whereby a user belongs to a group and a group has many users. Additionally, a Group belongs to a Provider, which is used to determine some behaviour of Groups and Users that use that provider. Given that, I have a presence validation for an identifier on User that is only required if the group is using a particular provider:

validates_presence_of :thing_id, if: -> (u) { u.group.provider.does_thing? }

Which is tested with:

# Use factory bot to make a user that belongs to a group which has
# a provider that does the thing.
subject { create(:user, :does_thing_group) }

it { should validate_presence_of(:thing_id) }

In 3.1.3, this test passed. In 4.0.1, I now get:

  1) User should belong to group required: true
     Failure/Error: validates_presence_of :thing_id, if: -> (u) { u.group.provider.does_thing? }

     NoMethodError:
       undefined method `provider' for nil:NilClass
     # ./app/models/user.rb:42:in `block in <class:User>'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:96:in `perform_validation'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:89:in `validation_result'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:85:in `validation_error_messages'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:64:in `messages'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:25:in `has_messages?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:55:in `messages_match?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/validator.rb:21:in `call'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:38:in `matches?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `each'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `detect'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `first_passing'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `public_send'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `run'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/allow_value_matcher.rb:401:in `does_not_match?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_model/disallow_value_matcher.rb:32:in `matches?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb:61:in `submatcher_passes?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb:23:in `matches?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matcher.rb:1211:in `block in failing_submatchers'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matcher.rb:1210:in `select'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matcher.rb:1210:in `failing_submatchers'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matcher.rb:1407:in `submatchers_match?'
     # /usr/local/bundle/gems/shoulda-matchers-4.0.1/lib/shoulda/matchers/active_record/association_matcher.rb:1157:in `matches?'

Initially I thought this was a factory_bot change or bug, as it seems like the associations of the User object created by the factory haven't been created. However, by downgrading solely the shoulda-matchers gem, the tests pass again, which leads me to believe this is likely a change in the 4.x version of shoulda-matchers.

Ruby version: 2.6.1
Rails version: 5.2.3
shoulda-matchers gem version: 4.0.1
factory_bot gem version: 5.0.2
factory_bot_rails gem version: 5.0.1

[mcmire]

Hey, so I took a closer look at the backtrace and it looks like the failure is coming from AssociationMatcher, i.e. belong_to or some such. What other tests do you have in your model spec?

[ZeroPointEnergy]

I think I hit the same or a similar problem here: sodabrew/puppet-dashboard#388

There are two tests which are now failing but if I downgrade shoula-matcher to ~> 3 they pass again.

[ZeroPointEnergy]

In my case I bisected the issue to commit 3af3d9f . It also seems like a should belongs_to now triggers valiadatiors which wasn't the case previously.

[mcmire]

@ZeroPointEnergy Yup, you got it — it will now check to make sure that the record fails (or doesn't fail) validation if the association is set to nil.

Thanks for a link to your repo, I'll take a closer look at it.

[mcmire]

Closing this issue because of missing context for the original problem posted.

[maknz]

We've fixed the issue in our project, it was technically an issue with our code even though it was exposed by shoulda-matchers 4.x. I wanted to explain how we encountered this for anyone else who might stumble upon it.

Consider a model such as:

class Wheel
  belongs_to :car 
  validates_presence_of :colour, if: :car_requires_coloured_wheels?

  def car_requires_coloured_wheels?
    car.requires_coloured_wheels?
  end
end

On the face of it, the condition on the presence validation seems fine because car should always be present because the association is required, not optional.

However, that's misguided. While the model will refuse to save unless the wheel belongs to a car, the model can still be validated independently of that, which is what shoulda-matchers is doing per @ZeroPointEnergy's finding above.

So when shoulda-matchers is running our should belong_to(:car) and purposefully setting our car association to nil, it validates the model and hits our car_requires_coloured_wheels?, at which point car is of course nil, which causes our exception.

The fix is for our validation condition to not assume the association exists; something like

  def car_requires_coloured_wheels?
    return false unless car

    car.requires_coloured_wheels?
  end

Thanks @mcmire and @ZeroPointEnergy for your work and help with this.