Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels #854

Closed
fabn opened this issue Dec 2, 2015 · 7 comments · Fixed by #1610
Closed

Issue with Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels #854

fabn opened this issue Dec 2, 2015 · 7 comments · Fixed by #1610

Comments

@fabn
Copy link

fabn commented Dec 2, 2015

I've just upgraded aasm gem to latest version and now I have a failing shoulda matcher (version 2.8.0) spec. The code is pretty standard, I have a Bookmark model which belongs to a user and a bookmarkable (polymorphic) item. A given user can only bookmark items once thus I have this spec:

it { should validate_uniqueness_of(:user_id).scoped_to(:bookmarkable_id, :bookmarkable_type) }

Running this spec with latest aasm code raises an error. I tracked the error to this line in aasm code. The issue is that the bookmarkable item (passed in matcher by shoulda code) does not retain its original class.

In my spec setup I create a Bookmark using a factory and matcher code uses that factory to compute values for assertion. My factory returns an instance of Place for bookmarkable while in some way the class of bookmarkable used in matcher is

class Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels::Placf < ActiveRecord::Base

That name is likely generated by this code, however I don't know its purpose.

In any case I think that test models generated by your matcher should behave like original one, so they should return original class when #class is called on their instance.

@fabn
Copy link
Author

fabn commented Dec 2, 2015

For the record I temporarily fixed the issue by adding this code in spec setup.

allow_any_instance_of(AASM::Persistence::ActiveRecordPersistence::InstanceMethods).
  to receive(:aasm_ensure_initial_state).and_return(true)

@mcmire
Copy link
Collaborator

mcmire commented Dec 2, 2015

Can you post the exception that was raised along with the backtrace?

@fabn
Copy link
Author

fabn commented Dec 2, 2015

Of course, here it is

Failures:

  1) Bookmark Uniqueness constraint should require case sensitive unique value for user_id scoped to bookmarkable_id, bookmarkable_type
     Failure/Error: it { should validate_uniqueness_of(:user_id).scoped_to(:bookmarkable_id, :bookmarkable_type) }

     NoMethodError:
       undefined method `keys' for nil:NilClass
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/aasm-4.5.0/lib/aasm/persistence/active_record_persistence.rb:138:in `aasm_ensure_initial_state'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/aasm-4.5.0/lib/aasm/persistence/active_record_persistence.rb:36:in `block in included'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:446:in `instance_exec'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:446:in `block in make_lambda'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:228:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:228:in `block in halting_and_conditional'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:506:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:506:in `block in call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:506:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:506:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:92:in `__run_callbacks__'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:778:in `_run_initialize_callbacks'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/core.rb:312:in `init_with'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/persistence.rb:69:in `instantiate'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/querying.rb:50:in `block (2 levels) in find_by_sql'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/result.rb:51:in `block in each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/result.rb:51:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/result.rb:51:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/querying.rb:50:in `map'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/querying.rb:50:in `block in find_by_sql'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/querying.rb:49:in `find_by_sql'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/statement_cache.rb:107:in `execute'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/singular_association.rb:53:in `get_records'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/singular_association.rb:57:in `find_target'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/association.rb:138:in `load_target'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/association.rb:53:in `reload'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/singular_association.rb:9:in `reader'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/associations/builder/association.rb:115:in `bookmarkable'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validator.rb:149:in `block in validate'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validator.rb:148:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validator.rb:148:in `validate'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/validations/presence.rb:5:in `validate'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:455:in `public_send'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:455:in `block in make_lambda'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:192:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:192:in `block in simple'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:504:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:504:in `block in call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:504:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:504:in `call'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:92:in `__run_callbacks__'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:778:in `_run_validate_callbacks'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validations.rb:398:in `run_validations!'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validations/callbacks.rb:113:in `block in run_validations!'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:88:in `__run_callbacks__'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activesupport-4.2.5/lib/active_support/callbacks.rb:778:in `_run_validation_callbacks'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validations/callbacks.rb:113:in `run_validations!'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activemodel-4.2.5/lib/active_model/validations.rb:337:in `valid?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/activerecord-4.2.5/lib/active_record/validations.rb:58:in `valid?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/validator.rb:97:in `validation_errors'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/validator.rb:77:in `collect_messages'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/validator.rb:40:in `messages'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/validator.rb:48:in `has_messages?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:277:in `has_messages?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:273:in `errors_match?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:224:in `block in matches?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:220:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:220:in `none?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:220:in `matches?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_model/validation_matcher.rb:43:in `allows_value_of'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:381:in `block in validate_after_scope_change?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:371:in `each'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:371:in `all?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:371:in `validate_after_scope_change?'
     # /Users/fabio/.rvm/gems/ruby-2.1.5/gems/shoulda-matchers-2.8.0/lib/shoulda/matchers/active_record/validate_uniqueness_of_matcher.rb:247:in `matches?'
     # ./spec/models/bookmark_spec.rb:13:in `block (3 levels) in <top (required)>'

Is caused by this line triggered in an after_initialize callback because it uses object class as key.

And here's a minimal AR class to reproduce this error

class Bookmark < ActiveRecord::Base
  belongs_to :user, required: true, touch: true
  belongs_to :bookmarkable, polymorphic: true, required: true
  # Prevent users to add same bookmark twice
  validates_uniqueness_of :user_id, scope: [:bookmarkable_id, :bookmarkable_type]
end

However notice that AASM is not used in Bookmark class but is used in another class whose instance is assigned to bookmarkable property.

@mcmire
Copy link
Collaborator

mcmire commented Dec 2, 2015

Okay, thanks for that.

What the uniqueness matcher is doing is this:

  1. Ensures that a Bookmark exists in the database based on a record you provide
  2. Build a new record, copies over all of the attributes involved in the test (including the attributes you're passing to scoped_to) and then asserts that this new record is not valid (because it fails the uniqueness check)
  3. Goes through the scope attributes and asserts that if each is changed, then the record is valid

So in your case, it would follow these steps:

  1. existing_record = create(:bookmark, user_id: 1, bookmarkable_id: 1, bookmarkable_type: SomeClass)

  2. new_record = Bookmark.new(user_id: 1, bookmarkable_id: 1, bookmarkable_type: SomeClass); assert that new_record is not valid

  3. Do something like this:

    new_record_2 = new_record.clone
    new_record_2.bookmarkable_id = 2
    # (assert that new_record_2 is valid)
    new_record_3 = new_record.clone
    new_record_3 = bookmarkable_type = ???
    # (assert that new_record_3 is valid)
    

Notice how the matcher has to choose a new value for bookmarkable_type. The value has to be different than whatever the existing record is using, and it also has to be a real class, otherwise ActiveRecord will bork.

So the way that we handle this situation is to make a new ActiveRecord model at runtime which is namespaced under Shoulda::Matchers::ActiveRecord::Uniqueness::TestModels, and then plug that into bookmarkable_type. Historically, we took the class name that the existing record was using, converted it to a string, and then called succ on it. We probably don't need to do this anymore now that the generated model is namespaced, but we still do this. So that's why you're seeing Placf.

That's the rationale behind what you're seeing. As for the cause of this issue, I'm not really sure -- it seems odd to me that your Bookmark model, as well as AASM, even cares what bookmarkable_type is set to.

We may be able to fix this by having our generated model inherit from Place. But I'd have to dig into this to know exactly what's going on here.

I think you may have to test this one yourself. I can give you some guidance on this if you like.

@fabn
Copy link
Author

fabn commented Dec 3, 2015

By looking at your explanation and at the code I think there's nothing more that you can do in matcher code. The class has to be different to correctly specs uniqueness constraint.

Maybe it's an issue in aasm itself (I have to try if it works with STI, I don't use it), however I think that shoulda-matchers code is fine as is, thus this issue can be closed if you agree.

For my specific issue, I can solve it using the stub above or by changing the bookmarkable item using a class that doesn't use aasm.

@mcmire
Copy link
Collaborator

mcmire commented Dec 3, 2015

Alright, sounds good. If you play around with it some more and figure out a fix then let me know and I can revisit this.

@mcmire mcmire closed this as completed Dec 3, 2015
@fabn
Copy link
Author

fabn commented Dec 3, 2015

For the record I found out that aasm works with STI so maybe a little improvement you can do in your code is to create the test model as subclass of original class, but as I said before it's an edge case that you can probably ignore.

StefSchenkelaars added a commit to StefSchenkelaars/shoulda-matchers that referenced this issue Jul 2, 2021
The uniqueness validation matcher generates a fake class when the
scope name ends on _type. This fake class is a duplicate of the
original class. In most cases this is not a problem, however when
using single table inheritance (STI), this new fake class is not
a subclass of the original class and therefore ActiveRecord will
throw an error. See thoughtbot#1245

A similar thing happens when the original class is using an AASM
state machine. The state machine settings are stored in a global
registry. So when the fake class is initialized, AASM cannot find
the settings of the set state machines (due to duplication) in the
registry. See thoughtbot#854

These problems are fixed by making the fake class inherrit from the
original class. Therefore it closes thoughtbot#1245 and closes thoughtbot#854
StefSchenkelaars added a commit to StefSchenkelaars/shoulda-matchers that referenced this issue Jul 2, 2021
The uniqueness validation matcher generates a fake class when the
scope name ends on _type. This fake class is a duplicate of the
original class. In most cases this is not a problem, however when
using single table inheritance (STI), this new fake class is not
a subclass of the original class and therefore ActiveRecord will
throw an error. See thoughtbot#1245

A similar thing happens when the original class is using an AASM
state machine. The state machine settings are stored in a global
registry. So when the fake class is initialized, AASM cannot find
the settings of the set state machines (due to duplication) in the
registry. See thoughtbot#854

These problems are fixed by making the fake class inherrit from the
original class. Therefore it closes thoughtbot#1245 and closes thoughtbot#854
StefSchenkelaars added a commit to StefSchenkelaars/shoulda-matchers that referenced this issue Jul 2, 2021
The uniqueness validation matcher generates a fake class when the
scope name ends on _type. This fake class is a duplicate of the
original class. In most cases this is not a problem, however when
using single table inheritance (STI), this new fake class is not
a subclass of the original class and therefore ActiveRecord will
throw an error. See thoughtbot#1245

A similar thing happens when the original class is using an AASM
state machine. The state machine settings are stored in a global
registry. So when the fake class is initialized, AASM cannot find
the settings of the set state machines (due to duplication) in the
registry. See thoughtbot#854

These problems are fixed by making the fake class inherrit from the
original class. Therefore it closes thoughtbot#1245 and closes thoughtbot#854
matsales28 pushed a commit that referenced this issue Feb 2, 2024
The uniqueness validation matcher generates a fake class when the
scope name ends on _type. This fake class is a duplicate of the
original class. In most cases this is not a problem, however when
using single table inheritance (STI), this new fake class is not
a subclass of the original class and therefore ActiveRecord will
throw an error. See #1245

These problems are fixed by making the fake class inherrit from the
original class. Therefore it closes #1245 and closes #854
matsales28 added a commit that referenced this issue Feb 9, 2024
The uniqueness validation matcher generates a fake class when the
scope name ends on _type. This fake class is a duplicate of the
original class. In most cases this is not a problem, however when
using single table inheritance (STI), this new fake class is not
a subclass of the original class and therefore ActiveRecord will
throw an error. See #1245

These problems are fixed by making the fake class inherrit from the
original class. Therefore it closes #1245 and closes #854

Co-authored-by: Stef Schenkelaars <stef.schenkelaars@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants