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

Fix uniqueness matcher when scope is a *_type attr #592

Merged
merged 3 commits into from
Oct 9, 2014
Merged

Conversation

mcmire
Copy link
Collaborator

@mcmire mcmire commented Sep 26, 2014

This is a fix for the long-standing #203, and is taken from #366.


This commit fixes validate_uniqueness_of when used with scoped_to so
that when one of the scope attributes is a polymorphic *_type attribute
and the model has another validation on the same attribute, the matcher
does not fail with an error.

As a part of the matching process, validate_uniqueness_of tries to
create two records that have the same values for each of the attributes
passed to scoped_to, except one of the attributes has a different
value. This way, the second record should be valid because it shouldn't
clash with the first one. It does this one attribute at a time.

Let's say the attribute in question is a polymorphic *_type attribute.
The value of this attribute is intended to be the name of a model as a
string. Let's assume the first record has a meaningful value for this
attribute already and we're trying to find a value for the second
record. In order to produce such a value, validate_uniqueness_of
generally calls #next (a common method in Ruby to generate a succeeding
version of an object sequentially) on the first object's corresponding
value. So in the case of a *_type attribute, since it's a string, it
would call #next on that string. For instance, "User" would become
"Uses".

You might have noticed a problem with this, which is "what if Uses is
not a valid model?" This is okay as long as there's nothing that is
trying to access the polymorphic association. Because as soon as this
happens, Rails will attempt to find a record using the polymorphic type
-- in other words, it will try to find the model that the *_type
attribute corresponds to. One of the ways this can happen is if the
*_type attribute in question has a validation on it itself.

Let's look at an example.

Given these models:

class User < ActiveRecord::Base
end

class Favorite < ActiveRecord::Base
  belongs_to :favoriteable, polymorphic: true
  validates :favoriteable, presence: true
  validates :favoriteable_id, uniqueness: { scope: [:favoriteable_type] }
end

FactoryGirl.define do
  factory :user

  factory :favorite do
    association :favoriteable, factory: :user
  end
end

and the following test:

require 'rails_helper'

describe Favorite do
  context 'validations' do
    before { FactoryGirl.create(:favorite) }
    it do
      should validate_uniqueness_of(:favoriteable_id).
        scoped_to(:favoriteable_type)
    end
  end
end

prior to this commit, the test would have failed with:

Failures:

  1) Favorite validations should require case sensitive unique value for favoriteable_id scoped to favoriteable_type
     Failure/Error: should validate_uniqueness_of(:favoriteable_id).
     NameError:
       uninitialized constant Uses
     # ./spec/models/favorite_spec.rb:6:in `block (3 levels) in <top (required)>'

Here, a Favorite record is created where favoriteable_type is set to
"Uses", and then validations are run on that record. The presence
validation on favoriteable_type is run which tries to access a "Uses"
model. But that model doesn't exist, so the test raises an error.

Now validates_uniqueness_of will set the *_type attribute to a
meaningful value. It still does this by calling #next on the first
record's value, but then it makes a new model that is simply an alias
for the original model. Hence, in our example, Uses would become a model
that is aliased to User.

In order not to clutter the object space, these special models are
created under the Shoulda::Matchers::ActiveModel::Uniqueness::TestModels
namespace, and are automatically removed once validate_uniqueness_of is
finished.

@mcmire mcmire mentioned this pull request Sep 26, 2014
to validate_uniqueness_of(:favoriteable_id).
scoped_to(:favoriteable_type)
ensure
Object.__send__(:remove_const, :Models)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just create some uniquely named classes in the context of the test?

@sgrif
Copy link

sgrif commented Sep 26, 2014

Just one thing that stands out to me here is that this is not at all thread safe. We should probably be locking at the class level while the matcher is working.

@@ -305,6 +309,12 @@ def create_record_without_nil
@existing_record = create_record_in_database
end

def model_class?(model_name)
model_name.constantize.ancestors.include?(::ActiveRecord::Base)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model_name.constantize < ::ActiveRecord::Base

@sgrif
Copy link

sgrif commented Sep 26, 2014

Seems like this could be simplified by just creating a new AR::B subclass with a known name, setting the table_name to same as the class being tested, and using that?

@mcmire mcmire force-pushed the ew-fix-for-203 branch 2 times, most recently from 8353a49 to b8bd447 Compare October 9, 2014 05:07
mcmire and others added 2 commits October 8, 2014 23:22
You can now say `define_class('Models::User')` and it will define a User
class inside of the Models module.

Similarly, you can also say `define_model('Models::User')` and it will
set the table name of the model to `models_users` instead of just
`users`.
Secondary author: Elliot Winkler <elliot.winkler@gmail.com>

This commit fixes `validate_uniqueness_of` when used with `scoped_to` so
that when one of the scope attributes is a polymorphic *_type attribute
and the model has another validation on the same attribute, the matcher
does not fail with an error.

As a part of the matching process, `validate_uniqueness_of` tries to
create two records that have the same values for each of the attributes
passed to `scoped_to`, except one of the attributes has a different
value. This way, the second record should be valid because it shouldn't
clash with the first one. It does this one attribute at a time.

Let's say the attribute in question is a polymorphic *_type attribute.
The value of this attribute is intended to be the name of a model as a
string. Let's assume the first record has a meaningful value for this
attribute already and we're trying to find a value for the second
record. In order to produce such a value, `validate_uniqueness_of`
generally calls #next (a common method in Ruby to generate a succeeding
version of an object sequentially) on the first object's corresponding
value. So in the case of a *_type attribute, since it's a string, it
would call #next on that string. For instance, "User" would become
"Uses".

You might have noticed a problem with this, which is "what if Uses is
not a valid model?" This is okay as long as there's nothing that is
trying to access the polymorphic association. Because as soon as this
happens, Rails will attempt to find a record using the polymorphic type
-- in other words, it will try to find the model that the *_type
attribute corresponds to. One of the ways this can happen is if the
*_type attribute in question has a validation on it itself.

Let's look at an example.

Given these models:

``` ruby
class User < ActiveRecord::Base
end

class Favorite < ActiveRecord::Base
  belongs_to :favoriteable, polymorphic: true
  validates :favoriteable, presence: true
  validates :favoriteable_id, uniqueness: { scope: [:favoriteable_type] }
end

FactoryGirl.define do
  factory :user

  factory :favorite do
    association :favoriteable, factory: :user
  end
end
```

and the following test:

``` ruby
require 'rails_helper'

describe Favorite do
  context 'validations' do
    before { FactoryGirl.create(:favorite) }
    it do
      should validate_uniqueness_of(:favoriteable_id).
        scoped_to(:favoriteable_type)
    end
  end
end
```

prior to this commit, the test would have failed with:

```
Failures:

  1) Favorite validations should require case sensitive unique value for favoriteable_id scoped to favoriteable_type
     Failure/Error: should validate_uniqueness_of(:favoriteable_id).
     NameError:
       uninitialized constant Uses
     # ./spec/models/favorite_spec.rb:6:in `block (3 levels) in <top (required)>'
```

Here, a Favorite record is created where `favoriteable_type` is set to
"Uses", and then validations are run on that record. The presence
validation on `favoriteable_type` is run which tries to access a "Uses"
model. But that model doesn't exist, so the test raises an error.

Now `validates_uniqueness_of` will set the *_type attribute to a
meaningful value. It still does this by calling #next on the first
record's value, but then it makes a new model that is simply an alias
for the original model. Hence, in our example, Uses would become a model
that is aliased to User.
[ci skip]
@mcmire mcmire merged commit 585f651 into master Oct 9, 2014
@mcmire mcmire deleted the ew-fix-for-203 branch October 9, 2014 06:31
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jun 12, 2015
pkgsrc change: allow build on Ruby 2.2.

# 2.8.0

### Deprecations

* `ensure_length_of` has been renamed to `validate_length_of`.
  `ensure_length_of` is deprecated and will be removed in 3.0.0.

* `set_the_flash` has been renamed to `set_flash`. `set_the_flash` is
  deprecated and will be removed in 3.0.0.

* `set_session(:foo)` is deprecated in favor of `set_session[:foo]`.
  `set_session(:foo)` will be invalid syntax in 3.0.0.

* Using `should set_session[:key].to(nil)` to assert that that a value has not
  been set is deprecated. Please use `should_not set_session[:key]` instead.
  In 3.0.0, `should set_session[:key].to(nil)` will only pass if the value is
  truly nil.

### Bug fixes

* Fix `delegate_method` so that it works again with shoulda-context. ([#591])

* Fix `validate_uniqueness_of` when used with `scoped_to` so that when one of
  the scope attributes is a polymorphic `*_type` attribute and the model has
  another validation on the same attribute, the matcher does not fail with an
  error. ([#592])

* Fix `has_many` used with `through` so that when the association does not
  exist, and the matcher fails, it does not raise an error when producing the
  failure message. ([#588])

* Fix `have_and_belong_to_many` used with `join_table` so that it does not fail
  when `foreign_key` and/or `association_foreign_key` was specified on the
  association as a symbol instead of a string. ([#584])

* Fix `allow_value` when an i18n translation key is passed to `with_message` and
  the `:against` option is used to specify an alternate attribute. A bug here
  also happened to affect `validate_confirmation_of` when an i18n translation
  key is passed to `with_message`. ([#593])

* Fix `class_name` qualifier for association matchers so that if the model being
  referenced is namespaced, the matcher will correctly resolve the class before
  checking it against the association's `class_name`. ([#537])

* Fix `validate_inclusion_of` used with `with_message` so that it fails if given
  a message that does not match the message on the validation. ([#598])

* Fix `route` matcher so that when controller and action are specified in hash
  notation (e.g. `posts#show`), route parameters such as `id` do not need to be
  specified as a string but may be specified as a number as well. ([#602])

* Fix `allow_value`, `validate_numericality_of` and `validate_inclusion_of` so
  that they handle RangeErrors emitted from ActiveRecord 4.2. These exceptions
  arise whenever we attempt to set an attribute using a value that lies outside
  the range of the column (assuming the column is an integer). RangeError is now
  treated specially, failing the test instead of bubbling up as an error.
  ([#634], [#637], [#642])

### Features

* Add ability to test `:primary_key` option on associations. ([#597])

* Add `allow_blank` qualifier to `validate_uniqueness_of` to complement
  the `allow_blank` option. ([#543])

* Change `set_session` so that #[] and #to qualifiers are optional, similar to
  `set_flash`. That is, you can now say `should set_session` to assert that any
  flash value has been set, or `should set_session.to('value')` to assert that
  any value in the session is 'value'.

* Change `set_session` so that its #to qualifier supports regexps, similar to
  `set_flash`.

* Add `with_prefix` qualifier to `delegate_method` to correspond to the `prefix`
  option for Rails's `delegate` macro. ([#622])

* Add support for Rails 4.2, especially fixing `serialize` matcher to remove
  warning about `serialized_attributes` being deprecated. ([#627])

* Update `dependent` qualifier on association matchers to support `:destroy`,
  `:delete`, `:nullify`, `:restrict`, `:restrict_with_exception`, and
  `:restrict_with_error`. You can also pass `true` or `false` to assert that
  the association has (or has not) been declared with *any* dependent option.
  ([#631])

### Improvements

* Tweak `allow_value` failure message so that it reads a bit nicer when listing
  existing errors.

[#591]: thoughtbot/shoulda-matchers#591
[#592]: thoughtbot/shoulda-matchers#592
[#588]: thoughtbot/shoulda-matchers#588
[#584]: thoughtbot/shoulda-matchers#584
[#593]: thoughtbot/shoulda-matchers#593
[#597]: thoughtbot/shoulda-matchers#597
[#537]: thoughtbot/shoulda-matchers#537
[#598]: thoughtbot/shoulda-matchers#598
[#602]: thoughtbot/shoulda-matchers#602
[#543]: thoughtbot/shoulda-matchers#543
[#622]: thoughtbot/shoulda-matchers#622
[#627]: thoughtbot/shoulda-matchers#627
[#631]: thoughtbot/shoulda-matchers#631
[#634]: thoughtbot/shoulda-matchers#634
[#637]: thoughtbot/shoulda-matchers#637
[#642]: thoughtbot/shoulda-matchers#642

# 2.7.0

### Deprecations

* `ensure_inclusion_of` has been renamed to `validate_inclusion_of`.
  `ensure_inclusion_of` is deprecated and will be removed in 3.0.0.

* `ensure_exclusion_of` has been renamed to `validate_exclusion_of`.
  `ensure_exclusion_of` is deprecated and will be removed in 3.0.0.

### Bug fixes

* Fix `delegate_method` so that it does not raise an error if the method that
  returns the delegate object is private.

* Warn when `ensure_inclusion_of` is chained with `.in_array([false, true])`
  as well as with `.in_array([true, false])`.

* Fix `set_session` so that the `to` qualifier if given nil checks that the
  session variable in question was set to nil (previously this actually did
  nothing).

* Fix `filter_param` so that it works when `config.filter_parameters` contains
  regexes.

* Fix `delegate_method` so that it can be required independent of Active
  Support.

* Fix `validate_uniqueness_of`. When used against an unpersisted record whose
  model contained a non-nullable column other than the one being validated, the
  matcher would break. Even if the test set that column to a value beforehand,
  the record had to be persisted in order for the matcher to work. Now this is
  no longer the case and the record can remain unpersisted.

* Fix `validate_absence_of`: it required that a string be passed as the
  attribute name rather than a symbol (which is the usual and documented usage).

### Improvements

* `have_and_belongs_to_many` now checks to make sure that the join table
  contains the correct columns for the left- and right-hand side of the
  association.

* Reword failure message for `delegate_method` so that it's a little more
  helpful.

### Features

* Add new matcher `define_enum_for` to test usage of the `enum` macro introduced
  in Rails 4.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants