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

Bug? in uniqueness verification when pure number was generated #1059

Closed
johnpaulashenfelter opened this issue Oct 10, 2017 · 1 comment
Closed

Comments

@johnpaulashenfelter
Copy link

johnpaulashenfelter commented Oct 10, 2017

Was surprised to suddenly see this on CI for a PR that only updated the rollbar gem...

 Failure/Error: it { is_expected.to validate_uniqueness_of(:activation_token) }
     
     Shoulda::Matchers::ActiveRecord::ValidateUniquenessOfMatcher::NonCaseSwappableValueError:
       Your Office model has a uniqueness validation on :activation_token which
       is declared to be case-sensitive, but the value the uniqueness matcher
       used, "32247654", doesn't contain any alpha characters, so using it to
       test the case-sensitivity part of the validation is ineffective. There
       are two possible solutions for this depending on what you're trying to
       do here:
     
       a) If you meant for the validation to be case-sensitive, then you need
          to give the uniqueness matcher a saved instance of Office with a
          value for :activation_token that contains alpha characters.
     
       b) If you meant for the validation to be case-insensitive, then you need
          to add `case_sensitive: false` to the validation and add
          `case_insensitive` to the matcher.
     
       For more information, please see:
     
       http://matchers.shoulda.io/docs/v3.1.2/file.NonCaseSwappableValueError.html

The relevant code:

class Office < ActiveRecord::Base
...
  validates :access_token, uniqueness: true
  validates :activation_token, uniqueness: true

and the relevant spec

require "rails_helper"

RSpec.describe Office, type: :model do

  describe "Validations" do
    it { is_expected.to validate_uniqueness_of(:access_token) }
    it { is_expected.to validate_uniqueness_of(:activation_token) }

sidenote -- folks from the NYC Thoughtbot office wrote a lot of this code (Mike Wenger, looking at you!)

I really don't understand the details of how the test values are generated even after digging through the source and unit tests for the uniqueness matcher so I'm stymied. Just surprised this would be a surprise and hard-to-replicate failure.

We're setting the value of activation_token using SecureRandom.hex(16) so I believe what happened is we got a "pure" number instead of 312af567 or something, thus the surprise failure. We're on PG9.6 so case sensitivity is the default in the db, but for the hex value it shouldn't matter.

Thoughts?

@mcmire
Copy link
Collaborator

mcmire commented Oct 10, 2017

Hey John,

Sorry you got this error. So what's happening here? By default, the uniqueness matcher tests not only that there is a uniqueness validation present in your model, but that this validation works case-sensitively. It does this by asserting not only that two records that have the same exact value for an attribute fail validation, but that if you swap the case of one of the values, then both records will pass validation. For instance, say you had a Person model with a uniqueness validation on name. If you had this test:

describe Person do
  it { should validate_uniqueness_of(:name) }
end

It would basically translate to this:

describe Person do
  it "fails validation when another Person with the same name exists" do
    # Initially there is a conflict...
    first_person = Person.create!(name: "John")
    second_person = Person.new(name: "John")
    second_person.validate
    expect(second_person.errors[:name]).to include("has already been taken")
    # Now we swap the case
    second_person.name = "john"
    # The conflict is cleared
    expect(second_person.errors[:name]).to be_empty
  end
end

But this assumes that the value of name for whatever subject happens to be contains alpha characters. If it only contains numbers, then you're going to run into a problem:

describe Person do
  it "fails validation when another Person with the same name exists" do
    # Initially there is a conflict...
    first_person = Person.create!(name: "123")
    second_person = Person.new(name: "123")
    second_person.validate
    expect(second_person.errors[:name]).to include("has already been taken")
    # Now we swap the case?
    second_person.name = "123"
    # Oops, this fails!
    expect(second_person.errors[:name]).to be_empty
  end
end

Hence, a while back we chose to make an interception in this case and raise an explicit error instead of allowing the matcher to fail (which would be even more confusing).

In your case, it sounds like since you're using a random value for activation_token, there's a chance that the hex value could contain all numbers, which would cause this error to be raised -- but only intermittently, hence why it's so hard to reproduce.

To fix this, I would modify your Office model so that activation_token is overridable, then build or create an Office with an explicit activation_token in your test:

it do
  office = build(:office, activation_token: "abc123")
  expect(office).to validate_uniqueness_of(:activation_token)
end

Hope this helps.

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

No branches or pull requests

2 participants