Breaks shoulda's validate_uniqueness_of matcher

[jmuheim]

I'm testing uniqueness validations using shoulda like this:

  describe 'uniqueness validations' do
    subject { build :project, creator: @user }

    it { should validate_uniqueness_of(:name).scoped_to(:customer) }
  end

Since I introduced mobility, this doesn't work for translated attributes anymore:

     Failure/Error: should validate_uniqueness_of(:name)
     
       Product did not properly validate that :name is case-sensitively unique.
         After taking the given Product, whose :name is ‹"Product test name"›,
         and saving it as the existing record, then making a new Product and
         setting its :name to a different value, ‹"pRODUCT TEST NAME"›, the
         matcher expected the new Product to be valid, but it was invalid
         instead, producing these validation errors:
     
         * name: ["has already been taken"]

When trying manually, the validation works as always. Only the shoulda matcher seems to have a problem.

[jmuheim]

See https://github.com/jmuheim/base/blob/master/spec/models/page_spec.rb#L13

[shioyama]

Yes, the uniqueness validation in Mobility does not handle case sensitivity, so this is an expected result. I should maybe document that somewhere.

The problem is that Mobility has to do validation in a backend-independent way, and doing a LOWER comparison requires that you know something about the backend strategy. e.g. it would be different for a jsonb/hstore column.

[jmuheim]

Thanks for your response. So this means that Mobility replaces the default mechanism or what? And that replacement doesn't watch out for LOWER comparison?

IMHO, I think there should be at least an option to force LOWER comparison somewhere?

[shioyama]

There is no mechanism for translated attributes, since they are not "real" attributes. Mobility adds a roughly-compatible uniqueness validation. I added it with some hesitation because I knew that it would not be complete.

I won't add any backend-specific conditions like LOWER because it makes things much more complciated. If it's important to have a case-sensitive comparison, then you can add a custom validator, or validate on the actual columns.

e.g. I believe you are using the column backend. In this case, you can validate uniqueness of the column, with the locale suffix. So you could validate :title_en, uniqueness: true, and it would validate the title in English.

I'm closing this since it's not fixable.

[shioyama]

p.s. If you want to ask more about what you could do to handle this validation, please add a question on SO with the "mobility" tag.

[shioyama]

Hmm, ok re-opening this, I misread the error message. It's saying validation is failing for case-sensitive uniqueness, which should work.

[shioyama]

So I checked this in a small application, and the matchers work for me.

What you posted looks suspicious. You're testing that the model validates uniqueness scoped to customer, but the error doesn't mention the scope, which it should.

Ah, ok now I see, this one will fail since it's testing with case insensitivy. But the one you posted above (scoped to customer) should work.

I'm closing this again, but I will add a warning message if the case sensitive option is passed in so that people know that the Mobility validator does not support this.

Thanks for posting.

[shioyama]

To anyone here: case-insensitive uniqueness validation now works as of 0.7.0.

[jmuheim]

I'd like to bring this one up again, as I still can't seem to find a solution that's working for me.

If I have this:

validates :title, uniqueness: {scope: :parent_id}

And this:

it { should validate_uniqueness_of(:title).scoped_to :parent_id }

I get this error:

   Expected Page to validate that :title is case-sensitively unique within
   the scope of :parent_id, but this could not be proved.
     Given an existing Page whose :title is ‹"Page test title"›, after
     making a new Page and setting its :title to a different value, ‹"pAGE
     TEST TITLE"› and its :parent_id to a different value, ‹nil›, the
     matcher expected the new Page to be valid, but it was invalid instead,
     producing these validation errors:
 
     * title: ["has already been taken"]

I tried validating the languages separately:

validates :title_de, uniqueness: {scope: :parent_id}
validates :title_en, uniqueness: {scope: :parent_id}

With the original validation:

it { should validate_uniqueness_of(:title).scoped_to :parent_id }

I get this error:

   Expected Page to validate that :title is case-sensitively unique within
   the scope of :parent_id, but this could not be proved.
     Expected the validation to be scoped to :parent_id, but it was not
     scoped to anything.

With testing on each language separately...

it { should validate_uniqueness_of(:title_de).scoped_to :parent_id }
it { should validate_uniqueness_of(:title_en).scoped_to :parent_id }

It seems to work:

2 examples, 0 failures

But this feels wrong to me: the model should be language agnostic. And apparently, in my feature specs, a lot of tests now fail with the following error:

 ActiveRecord::RecordInvalid:
   Validation failed: Title (de) has already been taken

When writing my own spec, everything seems to work as expected:

    # In User.rb
    # validates uniqueness: {scope: :parent_id}

    it 'validates language-specific uniqueness of title (scoped to parent_id)' do
      page = create :page, title:    'English title',
                           title_de: 'Deutscher Titel',
                           creator:  @user

      new_page = build :page, creator: @user

      # Try to set same title (should be rejected)
      new_page.title = page.title
      expect(new_page.valid?).to be_falsy
      expect(new_page.errors[:title]).to include 'has already been taken'

      # Try to set different title (should be accepted)
      new_page.title = 'New english title'
      expect(new_page.valid?).to be_truthy

      # Try to set same title as in German (should be accepted)
      new_page.title = page.title_de
      expect(new_page.valid?).to be_truthy

      # Try to set same title, but with different parent_id (should be accepted)
      new_page.title = page.title
      new_page.parent = page
      expect(new_page.valid?).to be_truthy
    end

But it would be much easier to rely on the generic matchers.

[shioyama]

I'll repeat what I wrote before:

There is no mechanism for translated attributes, since they are not "real" attributes. Mobility adds a roughly-compatible uniqueness validation. I added it with some hesitation because I knew that it would not be complete.

And of course, people expect too much from this. Translated attributes are not "attributes" in the AR sense, they do the dance as well as they can but when you take things too far they break.

Uniqueness validation is a basic feature meant to help people a bit, but it is not close to 100% compatible with standard AR uniqueness validation (it simplifies some things to avoid huge complexity). I was happy to add case-insensitivity since it's something that makes the feature much more useful, but it's not meant to be entirely compatible with the original version.

FWIW though, I suspect the reason your specs are passing and the shoulda matcher one is failing has to do with the scope condition (and its :parent_id to a different value, ‹nil›). If you add a test which changes the parent_id to nil, and checks validation, it may fail validation (although it should succeed, since parent_id is different).

If you have an actual test which fails (one that you wrote yourself), I would be happy to add it and try to fix it if it's reasonable. Maybe the scope condition doesn't handle nil entirely like AR, that wouldn't be surprising.

[jmuheim]

Thank you for taking the time for another statement, @shioyama.

I think it finally reached my brain that not everything can be expected as usual when adding complex functionality, and I will have to add a work-around every now and then.

Thanks a lot for your great gem, it really serves me well.

[shioyama]

Thanks for being understanding. I don't mean to be blunt about these things, but I believe it's very important for open source projects to define and defend their boundaries in order to remain maintainable. So I try to be very clear about what the gem should and should not do (although I'm always more receptive if someone else is going to do the coding part 😉).

[timkrins]

I just came across a similar situation, but my issue was due to shoulda querying the column type of a column that doesn't exist (because it is a mobility translated field)
(gives the error Minitest::UnexpectedError: NoMethodError: undefined method 'type' for nil:NilClass).