Shoulda-matchers: Fix uniqueness matcher when scope is a *_type attr in rails5

[garside]

It looks like the fix for #592 isn't included in the rails5 branch of shoulda-matchers. Is this something that could be easily copied over? Not sure how much things have diverged from the rails4 version till now, but the exact issue linked is again an issue.

[mcmire]

@garside The rails5 branch was old (in fact I just deleted it). The new one is rails-5. Can you give that a shot and see if that changes anything?

[garside]

Ah sorry, that's a bit of a red herring title. I'm actually on the rails-5 branch already sadly :(

For what it's worth, I did a quick and dirty comparison of the PR that fixes #592 and the files which it added didn't appear in the rails-5 branch, ex:

https://github.com/thoughtbot/shoulda-matchers/pull/592/files#diff-6c0602ecd220af0c7880e9ccaff0f4c6R2

has no equivalent in:

https://github.com/thoughtbot/shoulda-matchers/blob/rails-5/lib/shoulda/matchers/active_model.rb

I just did a coarse search for the term uniqueness and didn't see it. Though it appears as though this line isn't even in the master branch anymore either:

https://github.com/thoughtbot/shoulda-matchers/blob/master/lib/shoulda/matchers/active_model.rb

Not sure if that's because of how the new code is structured and it was moved under the hood, or if this got lost in the shuffle.

[mcmire]

@garside Ah. So it used to be that the uniqueness matcher lived under the ActiveModel namespace. From a Rails point of view, that's incorrect, so at a certain point after that PR, we moved it under the ActiveRecord namespace.

Here's the change that was made to the matcher:

https://github.com/thoughtbot/shoulda-matchers/blob/rails-5/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb?utf8=%E2%9C%93#L700

and here's the test that was added:

https://github.com/thoughtbot/shoulda-matchers/blob/rails-5/spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb?utf8=%E2%9C%93#L1265

[garside]

Hmm okay. This is the rough sketch of my implementation. Are there any glaring errors you see?

It's an STI table, so this is the base model:

app/models/chronicle.rb:

# == Schema Information
#
# Table name: chronicles
#
#  created_at  :datetime         not null
#  data        :jsonb
#  id          :integer          not null, primary key
#  ip_address  :inet
#  record_id   :integer
#  record_type :string
#  source_id   :integer
#  target_id   :integer
#  type        :string
#  updated_at  :datetime         not null
#
# Indexes
#
#  index_chronicles_on_record_type_and_record_id  (record_type,record_id)
#  index_chronicles_on_source_id                  (source_id)
#  index_chronicles_on_target_id                  (target_id)
#
# Foreign Keys
#
#  fk_rails_...  (source_id => users.id)
#  fk_rails_...  (target_id => users.id)
#

class Chronicle < ApplicationRecord
  include Indestructable

  belongs_to :record, polymorphic: true,  optional: true
  belongs_to :source, class_name: 'User', optional: true
  belongs_to :target, class_name: 'User', optional: true

  validates :ip_address, presence: true
  validates :type,       presence: true
end

And here is the implementation:

app/models/chronicles/record_created.rb:

class Chronicles::RecordCreated < Chronicle
  validates :data,      presence: true
  validates :record,    presence: true
  validates :record_id, uniqueness: { scope: :record_type }
  validates :target,     absence: true
end

spec/models/chronicles/record_created.rb:

require 'rails_helper'

RSpec.describe Chronicles::RecordCreated, type: :model do
  it { expect(described_class <= Chronicle).to eq true }

  describe '#data' do
    it { is_expected.to validate_presence_of(:data) }
  end

  describe '#record' do
    it { is_expected.to validate_presence_of(:record) }
  end

  describe '#target' do
    it { is_expected.to validate_absence_of(:target) }
  end

  describe '#record' do
    it { is_expected.to validate_uniqueness_of(:record_id).scoped_to(:record_type) }
  end
end

Running this gets me one failure:

1) Chronicles::RecordCreated#record should validate that :record_id is case-sensitively unique within the scope of :record_type
     Failure/Error: it { is_expected.to validate_uniqueness_of(:record_id).scoped_to(:record_type) }

     NameError:
       wrong constant name dummy value
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:96:in `perform_validation'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:89:in `validation_result'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:85:in `validation_error_messages'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:64:in `messages'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:25:in `has_messages?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:55:in `messages_match?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validator.rb:21:in `call'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:38:in `matches?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:42:in `does_not_match?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:28:in `each'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:28:in `detect'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:28:in `first_failing'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `public_send'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `run'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/allow_value_matcher.rb:395:in `matches?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validation_matcher.rb:155:in `run_allow_or_disallow_matcher'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_model/validation_matcher.rb:88:in `allows_value_of'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:660:in `block in validate_after_scope_change?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:648:in `each'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:648:in `all?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:648:in `validate_after_scope_change?'
     # /Users/ericgarside/.rvm/gems/ruby-2.4.1/bundler/gems/shoulda-matchers-edaf9cb926ee/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:332:in `matches?'
     # ./spec/models/chronicles/record_created_spec.rb:47:in `block (3 levels) in <top (required)>'

The code seems to work as expected by enforcing this constraint and giving an invalid record when I try to create a duplicate in the console.

[mcmire]

@garside I think your backtrace is incomplete, but even so, I have a guess as to what's happening. By default, the uniqueness matcher creates a new instance of your model and uses it for comparison. When it does this it will set default values for attributes that are involved in the test -- here record_id and record_type. When it is setting record_type, it is not smart enough to know that this needs to be an instance of some class, so it uses "dummy value" as a placeholder. Although this value of course doesn't make any sense, it doesn't cause any issues until ActiveRecord tries to read the association, which happens when validations are run.

To fix this, it's probably best to provide your own instance where you can set your own values:

describe '#record' do
   subject { described_class.new(record_id: 1, record_type: SomeClass) }    # or however you want to make this
    it { is_expected.to validate_uniqueness_of(:record_id).scoped_to(:record_type) }
  end
end

Let me know if this helps.

[garside]

Yep, spot on! I switched to just use the existing factory I had:

RSpec.describe Chronicles::RecordCreated, type: :model do
  describe '#record' do
    subject { build :record_created_chronicle }
    it { is_expected.to validate_presence_of(:record) }
    it { is_expected.to validate_uniqueness_of(:record_id).scoped_to(:record_type) }
  end
end

FactoryGirl.define do
  factory :chronicle do
    factory :record_created_chronicle, class: Chronicles::RecordUpdated do
    end
  end
end

And this started working! Thanks for your help. :)

[mcmire]

Awesome, you're welcome! I'm going to leave this issue open since we probably need to add some kind of helpful error message here to point people in the right direction.

[mcmire]

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!