Shoulda-matchers: Check for belongs_to asks for explicit "required: true" which is the default in Rails 5+

[tinbka]

For some reason I couldn't check out "rails-5" branch, then I looked it up among the issues here, found words about "rails-5" being merged into "master" and check out master.

After that, virtually all my regression tests was ruined with such message:

  178) Vehicle should belong to user required: true
       Failure/Error: super *args
         Expected Vehicle to have a belongs_to association called user (the association should have been defined with `required: true`, but was not)                                                                                                                                  
       # ./lib/exceptions.rb:4:in `raise'
   >>       super *args                                                                                                                    
       # ./spec/regression/models/vehicle_spec.rb:6:in `block (2 levels) in <top (required)>'
   >>     it { is_expected.to belong_to :user }

There is a mess with optional/required in this gem.

According to http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-belongs_to (and to my experience):

:optional
When set to true, the association will not have its presence validated.

:required
When set to true, the association will also have its presence validated.
This will validate the association itself, not the id.
You can use :inverse_of to avoid an extra query during validation.
NOTE: required is set to true by default and is deprecated.
If you don't want to have association presence validated, use optional: true.

[mcmire]

Hey @tinbka, sorry for the delay on this. Rails 5 made it so that belong_to associations have an implicit presence validation on them (that's what required: true does under the hood). The gem now follows suit, meaning that the belongs_to matcher checks for this validation to be present; if the association is allowed to be empty then the matcher will fail, as you've seen. I notice you didn't show me what your model looks like -- perhaps there is something unique about your association?

[tinbka]

@mcmire hey there.

My model is defined like this

class Vehicle < ApplicationRecord
  belongs_to :user, touch: true
  belongs_to :autopay_partner, class_name: 'Autopay::Partner', optional: true
...

Then, I have a bunch of regression tests which I build/rebuild with regressor gem that produces shoulda-matchers friendly syntax based on the current model definitions:

require 'rails_helper'

RSpec.describe Vehicle, regressor: true do

  # === Relations ===
  it { is_expected.to belong_to :user }
  it { is_expected.to belong_to :autopay_partner }
...

Yeah, I've noticed there is a kind of a flow in the generated code, in that belong_to is always assumed to have the framework default for optional / required flag even though the opposite may be specified in a definition. The generator ignored optional / required flags from definitions, so before the mentioned upgrade, all the tests passed anyway.

However, if we assume that required: true (= optional: false) is the default in the new shoulda-matchers (as in ActiveRecord 5), is_expected.to belong_to :user must pass anyway, because, really, the model definition lacks optional: true, so yes, it is required according to the Vehicle definition.

[mcmire]

@tinbka Hmm, okay. Thanks for the report. I'll have to take a closer look at this.

[Tresky]

Just chiming in to say I have experienced the same issue. +1

EDIT: I looked through the code very minimally at the belong_to matcher. Could it potentially be that required and optional simply need to be swapped because Rails 5 looks for the :optional tag while Rails 4.x will be be looking for the :required tag? This is the first time I've looked at this code base, so this is just a guess.

class AssociationMatcher

  # ...

  def initialize(macro, name)
    @macro = macro
    @name = name
    @options = {}
    @submatchers = []
    @missing = ''

    if macro == :belongs_to
      if RailsShim.active_record_gte_5?
        required
      else
        optional
      end
    end
  end

  # ...

end

[mcmire]

@Tresky Yes, you're right, but in this case it's more about which Rails versions default which options to true or false. In Rails 4.2, belongs_to takes a :required option which is false by default (and allows you to flip it by setting :required to true). In Rails 5, belongs_to takes a :required option that's true by default (and allows you to flip it by setting :optional to true).

So, in order to match this, what our belong_to matcher is doing in the bit of code you found is turning on certain qualifiers by default. In other words, if you're testing a Rails 5 app and you say:

it { should belong_to(:foo) } 

what you're actually saying is:

it { should belong_to(:foo).required }

But if you're testing a Rails 4 app and you say:

it { should belong_to(:foo) } 

what you're actually saying is:

it { should belong_to(:foo).optional }

At least, that's what should be happening 😕

[mcmire]

@tinbka Sorry for getting back to you so late on this. I still don't know what could be causing this. The required qualifier works by testing that there is a presence validation present, and it does this simply by testing that if the attribute in question is set to nil then it should fail validation. I am working on some more messaging which should at least tell you that this test is happening, and it may reveal some more information. In the meantime, though, do you have some extra code in your model that's preventing user from being set to nil?

[mcmire]

@Tresky Same to you. Do you have any custom logic in your model that's preventing your attribute from being set to nil? Alternately, if you could reproduce this making a fresh Rails app (see here for details) then that would be great.

[mcmire]

@tinbka @Tresky Can you check config/initializers/new_framework_defaults? What is Rails.application.config.active_record.belongs_to_required_by_default set to?

[Tresky]

@mcmire My project actually does not have a new_framework_defaults file in it. With this in mind, that setting is also not found in my project anywhere. Perhaps that might be part of my issue?

I do not have any logic in the model that prevents nil. I'll try to find a bit of time to reproduce with a fresh application. Hopefully that will help find and resolve the issue.

[mcmire]

@Tresky Oh, it looks like new_framework_defaults.rb was added in Rails 5.1: https://github.com/rails/rails/commit/fc3fccc351f34c006010a389b6bb9b7f4c6985e9#diff-5566f4f2ea56cf8f20432a8b50aa9350. If you are using Rails 5.0 still, then look for an active_record_belongs_to_required_by_default.rb file in the new_framework_defaults folder. If that's not there... did you upgrade your Rails app from 4 to 5? If so, then you may need to add a new initializer with:

Rails.application.config.active_record.belongs_to_required_by_default = true

[mcmire]

Now that I'm looking at it again, the matcher can look at that option to know whether or not to default to required, can't it? That's not really something you should have to worry about. Yup... that's what I'll do.

[Tresky]

@mcmire I did upgrade from 4 to 5, but I didn't run any special Rails upgrade script. I bet I missed that step because the files you're mentioning sound like autogenerated files that the Rails guys would generate for the upgrade.

With respect to your final comment, I think that makes sense. It would be worth looking into if the setting (Rails.application.config.active_record.belongs_to_required_by_default) is under the same name in all three versions in question: 4.2.x, 5.0.x, and 5.1.x. Best case scenario, the name stays constant, but it might change between version too. Not sure.

[mcmire]

@Tresky @tinbka Please run bundle update shoulda-matchers and let me know if the newest changes solve your issues!

[tinbka]

@mcmire thanks for the attention. With the currently published gem version all the tests pass.

[Tresky]

@mcmire Tests are passing now. Thanks for the work on this issue. I've got some new errors, but I can imagine that they are probably due to the new config setting you had me add that I just haven't updated my models to handle, yet.

Cheers!

[mcmire]

@Tresky. Cool. Just FYI, I made it so that you shouldn't have to add the config setting now, so if it's easier to take it back out for now, then you're free to do that. 🍺

[Tresky]

@mcmire Thanks so much. Just to clarify, are you referring to reference cd96089? Or is there another one that hasn't been pushed yet?

[mcmire]

@Tresky Yup, that's the one.

[Tresky]

Hmmm mine seems to be requiring the config setting to be set. With the setting, I get errors like Validation Failed: User must exist. Without the setting, I get the same sort of error that was mentioned when this issue was opened: Expected Class to have a belongs_to association called user (the association should have been defined with required: true, but was not)

[mcmire]

@Tresky Hmm, interesting. And are you on the latest version of Rails, or which version are you using?

[Tresky]

I'm currently on Rails 5.0.6.

[mcmire]

@Tresky Hmm... so I wasn't able to replicate this. I just tried:

• making a Rails 4.2 app,
• adding an Account model that belongs_to User
• writing a simple test for it (it { should belong_to(:user) }) -- this passed
• upgrading the app to 5.0.6
• re-running the test -- it still passed
• (I did not mess with belongs_to_required_by_default at all)

So I'm a bit stumped here why you're getting an error that it was expecting the association to have required: true; I would have expected it to look for optional: true.

[Tresky]

After investigating a bit more, I think the issue has nothing to do with a bug in shoulda-matchers, but rather to do with the application's implementation of FactoryBot. So, we're all good in terms of this library, I believe.

If I find any issues that I believe are related to shoulda, I'll reopen this issue or start another one.

[mcmire]

Cool! Thanks for checking into this, I appreciate it.

[monfresh]

This seems to have regressed in the master branch, specifically when a model also has a validation for the presence of the parent, such as:

class Address < ActiveRecord::Base
  belongs_to :location

  validates :location, presence: true
end

With the Rubygems version of shoulda-matchers(3.1.2), this test passes in Rails 5.1.4:

describe Address do
  it { is_expected.to belong_to(:location) }
end

However, with the master branch of shoulda-matchers, it fails with this message:

Failure/Error: it { is_expected.to belong_to(:location) }
       Expected Address to have a belongs_to association called location (the association should have been defined with `optional: true`, but was not)

Since Rails 5 will now automatically validate the presence of the parent (because required: true is the default), would it make sense for shoulda-matchers to detect the redundant validation and output a more specific error message?

[mcmire]

@monfresh What's the value of ActiveRecord::Base.belongs_to_required_by_default for you?

[monfresh]

Aha! I totally missed the part where that needs to be set when upgrading from Rails 4 to 5. Never mind me! It works fine when it's set to true.

[mcmire]

Cool :)

[joroshiba]

I have a similar opposite issue, in the process of upgrading to Rails 5, I have optional set to true on my model, but I am still getting this error.

from build.rb

belongs_to :repo, optional: true

after running build_spec.rb

Failure/Error: it { is_expected.to belong_to(:repo) }
Expected Build to have a belongs_to association called repo (the association should have been defined with `optional: true`, but was not)

[zocoi]

There seems to be a bug with optional: true. I have both Rails.application.config.active_record.belongs_to_required_by_default true and false and could not have the test passed, similar to @bytewalls

belongs_to :submitter, optional: true

     Failure/Error: it { is_expected.to belong_to(:submitter).optional }
       Expected Tip to have a belongs_to association called submitter (the association should have been defined with `optional: true`, but was not)

[mcmire]

@bytewalls @zocoi Hmm. You two are both using the master branch, correct? (Just want to make sure.)

Is there anything non-standard about your model? I'm struggling to think of why you would be getting these issues.

[rubendinho]

Using the master branch (SHA: cd96089a56b97cd11f7502826636895253eca27d) I'm also seeing my association tests fail.

# merchant.rb
class Merchant < ApplicationRecord
  belongs_to :category
end

# merchant_spec.rb
RSpec.describe Merchant, type: :model do
  it { is_expected.to belong_to(:category) }
end

I am on Rails 5.1 and Rails.application.config.active_record.belongs_to_required_by_default is set to true. Therefore, since required: true is the global default behaviour, I expect not to have to declare required: true on the matcher to get the test to pass.

This is the output I'm getting instead:

 Failure/Error: it { should belong_to(:category) }
        Expected Merchant to have a belongs_to association called category (the association should have been defined with `required: true`, but was not)

[joroshiba]

I am on the master branch, same sha as @rubendinho.

I have set config.active_record.belongs_to_required_by_default to true in order to try and get test working. However, I have also gone through and updated my optionals manually on all belongs_to so I should be able to reset. Even with both optional: true being manually set and belongs_to_required_by_default set to true I get this error.

[AlexVPopov]

So I had the following problem:

• rails 5.1.5

class Expense < ApplicationRecord
  belongs_to :user
  validates :user, presence: true
end

class User < ApplicationRecord
  has_many :expenses, -> { order date: :desc }, dependent: :destroy
end

I know the validation for user is redundant after Rails 5, but I had forgotten to remove it. With revision 4d17faee7d6ba4cd577481d1910c1c6512e937b5 I had no failing tests. With revision e35f083b88c5fbccd319cd4fcfff57ceae6076f2 one test started failing:

  1) Expense associations should belong to user optional: true
     Failure/Error: it { is_expected.to belong_to(:user) }
       Expected Expense to have a belongs_to association called user (the association should have been defined with `optional: true`, but was not)
     # ./spec/models/expense_spec.rb:14:in `block (3 levels) in <top (required)>'

However, after I removed the validation for user inside Expense, the test started passing. So I guess, the moral of the story is - check to see if you still have any redundant presence validations for belongs_to associations.

[mcmire]

@rubendinho @bytewalls That's really strange. Is it possible for you to recreate this in a fresh app? You can use https://github.com/mcmire/shoulda-matchers-test-apps to create one.

[Gmarn]

Hi,
We recently face to the same issue with polymorphic dependence, with @mtejedorwolox:

Model classes

class Price < ApplicationRecord
  belongs_to :menuable, polymorphic: true, inverse_of: :prices # , required: true
end

class Complement < ApplicationRecord
  has_many :prices, as: :menuable, dependent: :destroy, inverse_of: :menuable
end

The test:

describe Price do
  subject(:price) { build(:price) }

  it { is_expected.to belong_to(:menuable) }
end

If I let the required: true uncomment I have the error:

Failure/Error: it { is_expected.to belong_to(:menuable) }
       Expected Price to have a belongs_to association called menuable (the association should have been defined with `required: `, but was not)

If I comment/remove it, I do not have the error and the default behaviour is required: true

So I resolve this removing required: true from Price model.

The result is this is confusing and should be good we can let the required: true even if it's the default behaviour.

[mcmire]

Hi @Gmarn, sorry to hear you're seeing this. What is the value of ActiveRecord::Base.belongs_to_required_by_default in your app?

[Gmarn]

Hi @mcmire
I have in config/initializers/new_framework_defaults.rb this configuration: Rails.application.config.active_record.belongs_to_required_by_default = true

[mcmire]

@Gmarn Okay. So just so I understand — if you leave off required: true from your association, you get the error, and adding it back makes the test pass? Or is the other way around?

[Gmarn]

@mcmire, If I have required: true in my association if have the error mentioned during the test only.
So I removed it from my belongs_to association. Like that:

class Price < ApplicationRecord
  belongs_to :menuable, polymorphic: true, inverse_of: :prices
end

The consequences are:

• test passed;
• behaviour is the one expected;
• but I can not mentioned required: true clearly in my code.

[darrenterhune]

@Gmarn if you have belongs_to_required_by_default = true set in your initializer you shouldn't be defining it in your model association. This is the behaviour in Rails 5 and defining it is deprecated. So I think your ok here and this isn't a shoulda-matchers issue at all 😄

[mcmire]

@darrenterhune That is true, although it shouldn't make a difference whether you add required: true to your association or not in this case — the matcher should work either way. I'll take a closer look at this.

[Gmarn]

@darrenterhune I agree with you, at functionality layer this work. But here my issue is at reading of my code. Also @mcmire recently mentioned it well, even if I can work with this bug, it stay a bug that the gem find an error when there is no error.

[andrey-skat]

I have the same issue, but with belongs_to_required_by_default set to false. And with model

class Post
  belongs_to :user
  validates :user, presence: true
end

I get error Expected Post to have a belongs_to association called user (the association should have been defined withrequired: false, but was not)

shoulda-matcher 4.0.0.rc1
rails 5.2.2

PS: Actually I updated to rc version just to get rid from ::Fixnum deprecation warning. Could you make version 3.1.3 with that fix?

[mcmire]

@andrey-skat Hey, can you make a new issue for removing the Fixnum deprecation warning? I can release a v3.1.4 to address that.

[mcmire]

@andrey-skat Also, do you have any other things in your model that around :user? We have an explicit test for your exact case to ensure that still works, so I'm a bit perplexed as to how you could be getting that error.

[mcmire]

I just tried to reproduce the last few bugs mentioned here in a fresh Rails 5.2.2 app using 4.0.0.rc1 and I wasn't able to do that. With that in mind I'm going to close this issue as I feel like for most people this should be working (and it's getting hard to track all of the bugs here). If you are experiencing any more bugs then please file a new issue. Even better, if you can replicate your bug using one of the test apps here then that would be appreciated!

[contentfree]

Just coming by to say that my it { should belong_to(:my_association) } fails with Rails 5.1.6.1 and shoulda-matchers 4.0.1 unless I include required: true when defining the association. (belongs_to_required_by_default = true BTW). So basically the opposite of @Gmarn ?

[mcmire]

Sorry to hear this @contentfree. Again, if you can replicate this in a test app then that would help me a lot!

[dgilperez]

I'm having a similar unexpected behaviour here, that may help nail down these issues. With belongs_to_required_by_default set to true, and the following code:

class User
  belongs_to :account, required: true

  before_validation :set_account 

  def set_account 
    self.account ||= Account.current # set as a global
  end
end

I'm seeing:

  it { is_expected.to belong_to(:account).required } #=> this fails with the abovementioned error 
  it { is_expected.to belong_to(:account) }          #=> this fails with the abovementioned error 
  it { is_expected.to belong_to(:account).optional } #=> this passes

Ofc, I was expecting the required matcher to pass and the optional to fail.

In the meanwhile I'm setting the association to optional.

[ngouy]

@mcmire demo app : https://github.com/ngouy/shoulda-matchers-test-apps (go for the rails-5-2rspec). I updated shoulda matchers to 4.x (it was 3.x)

Though not sure it's the same problem. But I don't understand why it fails. Even in my real app when subject (in rspec) is build with factory, so have it's related relations all set, shouda-matcher create an empty one and it always fails when checking belongs_to associations

[mcmire]

@ngouy I just tried out the test app you made and ran the tests for user_spec.rb. Aside from the :yolo association, the other two tests seemed to pass. So I'm not sure what issue you're running into here. Could you elaborate?

[mcmire]

@dgilperez Sorry, I didn't see this before until now! When you use belongs_to :association, required: true (or simply belongs_to :association), this will add a presence validation on your association to validate that it is set. Similarly, because this is the default behavior in Rails, by default the belong_to matcher will confirm that this presence validation is in place by asserting that when the association is nil, the record has errors. For your model, setting the association to nil before validations are run will do nothing, because you override this behavior. This means that there's no way that the validation can fail, and also, this causes the matcher to fail.

You observed that tacking .optional onto the end of the matcher made the test pass. This technically works, but it's only because this will tell the matcher to check that setting the association to nil does _not_ cause the record to fail. As we just established, your code converts nil to an Account instance, and since an Account is something and not nothing, the presence validation on your association passes, and so does the presence check inside of the belong_to matcher.

That said, a more semantic way to achieve this without using .optional is to skip the presence validation altogether. You can achieve this by using .without_validating_presence. Hope that helps.

[dgilperez]

@mcmire thanks for the explanation. .without_validating_presence looks exactly what we need :)

[ngouy]

@mcmire (just pushed a new version without the byebug, my bad)

But I think the answer you gave just above seems to solve this issue

[mcmire]

@ngouy Ah ok. I gotcha. Well, in your case, I don't think it would be appropriate to use without_validating_presence. Unlike @dgilperez's case, you're not changing prof in any way. So the question to ask yourself here is, when validations get run, is there a chance that prof could be nil? If so, does this cause the record to fail validation? This is exactly the question that belong_to is seeking to answer. In your case, however, you don't account for prof being nil, so when that happens, your is_ready method fails. If you don't want that to happen, you should check that prof isn't nil first before trying to call id on it. (Same goes for classroom.)