validate_uniqueness_of does not account for reliance on other records in other validations

[brett-puddick]

So, let's say we have:

# app/models/product.rb
class Product < ApplicationRecord
    has_many :prices

    validates :code, presence: true, uniqueness: true

    validate :custom_validation
    
    def custom_validation
        prices.length > 1
    end
 end

and then we want to test the uniqueness validation:

# spec/app/models/product.rb
...
describe 'validations' do
    it 'ensures that code is unique' do
        product = Product.new(
            code: '123',
            prices: [Price.first, Price.second]
        )

        expect(product).to validate_uniqueness_of(:code)
    end
end

The observed behaviour of what happens in this test is that the matcher takes the first record in the DB (not necessarily the one you provided), builds a new instance of it (without saving) and then goes about its checks manipulating attribute values and asserting if it's valid.

This approach does not take into concern that other validations may fail, and hence the test, as the model has reliances on other data outside of this record. The matcher should either use the record provided (with an expectation that it passes all other required validations) or also build new instances of any associations when grabbing the first record from the DB.

[mcmire]

Hi @brett-puddick,

In general shoulda-matchers does not do anything to fill in attributes on records, so whenever working with validation matchers it works a lot better if you always use a record that satisfies all of the validations. That said, the uniqueness matcher is a weird case, and I agree that it would make more sense to prefer making use of the given record rather than always relying on other data outside of it. I think that particular behavior is a vestige from a long time ago. In fact I remember that someone else raised this issue a while back and I gave some thoughts on it then: https://github.com/thoughtbot/shoulda-matchers/issues/1067. Even though the matcher works in a weird way, I am now not sure whether it now be a good idea to change it fundamentally, as it would technically be a breaking change, and we're more or less feature frozen on the next release. That said, one idea I just thought of would be to change the behavior, but then make it opt-in. That is, add a new configuration option to Shoulda::Matchers.configure, and if it is set, then it will enable the new behavior. This would allow us to deprecate and eventually remove the old behavior at some point in the future in a less surprising manner. What do you think about this?

[myxoh]

The workaround fails if your scope includes the model.

For example this fails for me:

model Account
  belongs_to :organization
  validates :email uniqueness: { case_sensitive: false, scope: :organization_id }
  validates :payment_account, presence: true, on: :create, if: -> { organization.requires_payment? }
end

RSpec.describe Account, type: :model do
  subject(:account) { build(:account) }

  describe 'validations' do
    it do
      account.organization = create(:organization)
      is_expected.to validate_uniqueness_of(:email).case_insensitive.scoped_to(:organization_id)
    end
  end
end

as the matcher is setting (on one of the runs) an invalid organization_id and hence the conditional check on payment_account is failing.

Debugging what the matcher was doing - it run perform_validation 5 times - the last time the record had an organization_id but it did not reference a valid organization hence organization evaluated to nil. I also checked there was only one valid organization model at the time of the spec running (which had a different ID than the organization_id on the Account model).

For now I'm just removing the validation / spec alltogether as I couldn't find a useful workaround using shoulda-matchers