For a model that has secure password, validating uniqueness of a non-password attribute + :allow_nil fails with 'Password digest missing on new record' #371

Closed
qqshfox opened this Issue Oct 18, 2013 · 12 comments

Comments

Projects
None yet
3 participants

qqshfox commented Oct 18, 2013

Here is a spec to reproduce this problem.

require 'spec_helper'

describe Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher do
  context 'a model with a uniqueness validation' do
    context 'has secure password' do
      it 'should not raise error' do
        model = define_model(:example, :attr => :string, :password_digest => :string) do
          validates_uniqueness_of :attr
          has_secure_password
        end
        expect(model.new).to validate_uniqueness_of(:attr)
      end
    end
  end
end
F

Failures:

  1) Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error
     Failure/Error: expect(model.new).to validate_uniqueness_of(:attr)
     RuntimeError:
       Password digest missing on new record
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:118:in `block in create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `tap'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:96:in `first_instance'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:92:in `existing_record'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:204:in `existing_value'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:144:in `validate_everything_except_duplicate_nils?'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:75:in `matches?'
     # ./tt.rb:11:in `block (4 levels) in <top (required)>'

Finished in 0.03043 seconds
1 example, 1 failure

Failed examples:

rspec ./tt.rb:6 # Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error

It fails only on rails 4.0.0.

Collaborator

mcmire commented Oct 18, 2013

validate_uniqueness_of works by making a new record and comparing it with an existing one in the database. If there is not a record already in the database, it will create one. Unfortunately when auto-creating a record, it is not smart enough to set attributes on it that will make it successfully save. Hence, in this case we suggest you pre-create a record before you call validate_uniqueness_of. Like so:

it do
  FactoryGirl.create(:my_model)
  should validate_uniqueness_of(:whatever)
end

However, it is interesting that this fails only under Rails 4. I would expect it to fail also under Rails 3.1 and 3.2.

(EDIT: updated to correct how validate_uniqueness_of actually works)

Collaborator

mcmire commented Oct 18, 2013

It appears that the Rails 4 version of has_secure_password adds a before_create to ensure that password_digest is filled in; however, the Rails 3 version does not have this check. This is why this test is failing in Rails 4 and not Rails 3.

To go back to the point I made in the previous comment, though, the solution is to create a valid record (i.e., one that has password_digest set) before you call validate_uniqueness_of. You can use a factory library to do this, or you can do this yourself.

@mcmire mcmire closed this Oct 18, 2013

qqshfox commented Dec 1, 2013

Hi @mcmire , I managed to make my first spec passed.

I found a new problem when I dig a little deeper with validates_uniqueness_of.

require 'spec_helper'

describe Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher do
  context 'a model with a uniqueness validation' do
    context 'has secure password' do
      it 'should not raise error' do
        model = define_model(:example, :attr => :string, :password_digest => :string) do |m|
          validates_uniqueness_of :attr
          has_secure_password
        end
        inst = model.new
        inst.password = 'test'
        inst.password_confirmation = 'test'
        inst.save!
        expect(model.new).to validate_uniqueness_of(:attr)
      end

      it 'should not raise error 2' do
        model = define_model(:example, :attr => :string, :password_digest => :string) do |m|
          validates_uniqueness_of :attr, allow_nil: true
          has_secure_password
        end
        expect(model.new).to validate_uniqueness_of(:attr).allow_nil
      end

      it 'should not raise error 3' do
        model = define_model(:example, :attr => :string, :password_digest => :string) do |m|
          validates_uniqueness_of :attr, allow_nil: true
          has_secure_password
        end
        inst = model.new
        inst.password = 'test'
        inst.password_confirmation = 'test'
        inst.save!
        expect(model.new).to validate_uniqueness_of(:attr).allow_nil
      end
    end
  end
end
.FF

Failures:

  1) Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 2
     Failure/Error: expect(model.new).to validate_uniqueness_of(:attr).allow_nil
     RuntimeError:
       Password digest missing on new record
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:118:in `block in create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `tap'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:96:in `first_instance'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:92:in `existing_record'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:204:in `existing_value'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:140:in `validate_everything_except_duplicate_nils?'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:75:in `matches?'
     # ./aa.rb:23:in `block (4 levels) in <top (required)>'

  2) Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 3
     Failure/Error: expect(model.new).to validate_uniqueness_of(:attr).allow_nil
     RuntimeError:
       Password digest missing on new record
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:118:in `block in create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `tap'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:148:in `create_record_without_nil'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:141:in `validate_everything_except_duplicate_nils?'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:75:in `matches?'
     # ./aa.rb:35:in `block (4 levels) in <top (required)>'

Finished in 0.10311 seconds
3 examples, 2 failures

Failed examples:

rspec ./aa.rb:18 # Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 2
rspec ./aa.rb:26 # Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 3

If the model has a attribute which must be unique but allow to be nil

According to

        def matches?(subject)
          @subject = subject.class.new
          @expected_message ||= :taken
          set_scoped_attributes &&
            validate_everything_except_duplicate_nils? &&
            validate_after_scope_change? &&
            allows_nil?
        end

First, validate_everything_except_duplicate_nils? will be called,

        def validate_everything_except_duplicate_nils?
          if @options[:allow_nil] && existing_value.nil?
            create_record_without_nil
          end

          disallows_value_of(existing_value, @expected_message)
        end

Then try to get the existing_value,

        def existing_value
          value = existing_record.send(@attribute)
          if @options[:case_insensitive] && value.respond_to?(:swapcase!)
            value.swapcase!
          end
          value
        end
  • Case 1: If there is no record in db yet, it will create a new record.
  • Case 2: If there already exists a record, the existing_value may be nil or not.
    • Case 2a: if the existing_value is nil
    • Case 2b: if the existing_value is not nil
  • Case 1 will fail my spec
  • Case 2a: create_record_without_nil called, spec failed
  • Case 2b: allows_nil? called
        def allows_nil?
          if @options[:allow_nil]
            ensure_nil_record_in_database
            allows_value_of(nil, @expected_message)
          else
            true
          end
        end

        def ensure_nil_record_in_database
          unless existing_record_is_nil?
            create_record_in_database(nil_value: true)
          end
        end

        def existing_record_is_nil?
          @existing_record.present? && existing_value.nil?
        end

@existing_record.present? is true and existing_value.nil? is false, create_record_in_database will still be called. The spec fails.

The spec failed in all 3 paths. Any suggestion?

Thanks,
Hanfei

Collaborator

mcmire commented Dec 1, 2013

I would actually expect your second spec to fail since there is no pre-existing record.

The third spec is failing because it expects the record you're creating to have a attr attribute whose value is non-nil. Since you don't, it tries to create a record, which fails for the same reason it has failed before.

qqshfox commented Dec 2, 2013

Please have a look at the Case 3, thanks.

qqshfox commented Dec 2, 2013

Typo, Case 2b.

Case 2b: if the existing_value is not nil

Collaborator

mcmire commented Dec 2, 2013

The third test corresponds to your case 2b, yes? If so, you've set it up incorrectly.

qqshfox commented Dec 2, 2013

In all the 3 paths, we arrives create_record.

qqshfox commented Dec 2, 2013

require 'spec_helper'

describe Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher do
  context 'a model with a uniqueness validation' do
    context 'has secure password' do
      it 'should not raise error 4' do
        model = define_model(:example, :attr => :string, :password_digest => :string) do |m|
          validates_uniqueness_of :attr, allow_nil: true
          has_secure_password
        end
        inst = model.new
        inst.attr = 'non nil'
        inst.password = 'test'
        inst.password_confirmation = 'test'
        inst.save!
        expect(model.new).to validate_uniqueness_of(:attr).allow_nil
      end
    end
  end
end
F

Failures:

  1) Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 4
     Failure/Error: expect(model.new).to validate_uniqueness_of(:attr).allow_nil
     RuntimeError:
       Password digest missing on new record
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:118:in `block in create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `tap'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:116:in `create_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:101:in `ensure_nil_record_in_database'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:84:in `allows_nil?'
     # ./lib/shoulda/matchers/active_model/validate_uniqueness_of_matcher.rb:77:in `matches?'
     # ./bb.rb:16:in `block (4 levels) in <top (required)>'

Finished in 0.05854 seconds
1 example, 1 failure

Failed examples:

rspec ./bb.rb:6 # Shoulda::Matchers::ActiveModel::ValidateUniquenessOfMatcher a model with a uniqueness validation has secure password should not raise error 4
Collaborator

mcmire commented Dec 2, 2013

I see. So it's failing now because of the allows_nil? check which calls ensure_nil_record_in_database. So when using :allow_nil, it will always attempt to create a record. That's what you were saying.

Thanks for the report. I've reopened this and updated the description of the issue to match.

Just curious, any ideas on what we could do to fix this?

@mcmire mcmire reopened this Dec 2, 2013

anthonynavarre pushed a commit that referenced this issue Jan 20, 2014

Contributor

anthonynavarre commented Jan 20, 2014

@qqshfox can you check out #426 to see if it sufficiently addresses the issue from your perspective?

anthonynavarre pushed a commit that referenced this issue Jan 20, 2014

anthonynavarre pushed a commit that referenced this issue Jan 21, 2014

anthonynavarre pushed a commit that referenced this issue Jan 21, 2014

anthonynavarre pushed a commit that referenced this issue Jan 22, 2014

Collaborator

mcmire commented Jan 22, 2014

Fixed in f5f8e0a.

@mcmire mcmire closed this Jan 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment