Validation was scoped by type

[tillsc]

Consider a class Base and a class Child < Base with a uniqueness check in Base.some_property

The uniqueness check did a resource.model.first(opts) where resource.model would resolve to Child. But nobody said that the uniqueness is scoped by the type (that should be done with :scope => :type).

This commit adds an (previously failing) spec and a fix

[rykov]

If I understand this correctly, it will always check uniqueness via the base model of any of the inherited resources. So in the example below, all the uniqueness checks will be against the Account type?

class Account
  include DataMapper::Resource
  property :username, :unique => true
end

class FBUser < Account
  property :email, :unique => true
end

class EmailUser < Account
  property :email, :unique => true
end

class CorporateUser < User
end

If so, this presents a problem if I'd like to keep uniqueness for CorporateUser and EmailUser, while letting FBUser have its own uniqueness space.

[tillsc]

@rykov: You're right. Your approach from #64 now seems to be the better solution to me. I'll fix that. Or do you want to port your patch to the current master?

[rykov]

👍 👍 👍

[tillsc]

@tpitale Do you have an opinion about this?

[tpitale]

I think I understand the change that's being made. Conceptually, I could see it going either way.

Personally, If I have a property on a base class (which would be present on all of the child classes) and I want that property to be uniquely validated, I would expect the validation to be on the base class.

If I put that validation on one of the child classes, but not others … I'm not sure what I would really expect to be happening.

So, in code, this makes sense to me:

class Account
  include DataMapper::Resource

  property :id, Serial
  property :email, Text, unique: true
end

class Admin < Account
end

class User < Account
end

Admin.create(email: '[email protected]') # => works
User.create(email: '[email protected]') # => fails validation

But this code … I'm not sure how it should be handled:

class Account
  include DataMapper::Resource

  property :id, Serial
end

class Admin < Account
  property :email, Text, unique: true
end

class User < Account
  property :email, Text, unique: true
end

Admin.create(email: '[email protected]') # => works
User.create(email: '[email protected]') # => ???

I could go either way … maybe this should be scoped to the type if I intentionally put the property on the child classes. I could also see this being a shot-myself-in-the-foot mistake.

[rykov]

Just like in any other STI context, whatever property is specified in a particular model, applies to resources of that class and any child class. So I think uniqueness scoping should apply to the model where it's specified and all the children within the parent's scope.

[tillsc]

Why should you want to define a uniqueness constraint in a subclass when you want it to affect the base class too? I really think this is a bug.

Using a :scope => :typedoesn't work neither because it would add uniqueness constraints to all parent, sibling and child classes. Even worse: the :type scoped uniqueness constraint would not respect further inheritance. Example:

class Account
  include DataMapper::Resource
  property :id, Serial
  property :type, Discriminator
end

class Admin < Account
  property :email, Text, unique: :type
end 

class SuperHeroAdmin < Admin
end

Admin.create(email: '[email protected]') # => works
SuperHeroAdmin.create(email: '[email protected]') # => works too... wtf??

The major argument against this approach is from my POV that STI based uniqueness checks are not applicable to database constraints (UNIQUE indexes). Following this contra argumentation we should prevent users from defining uniqueness checks in STI subclasses by raising an exception.

[tpitale]

I think the example I was trying to make was what to do in a case where the child classes have fields that the base class does not. And what happens if those fields happen to have the same name. I don't think I know enough about DM's STI to properly answer that. Also, that may not be what this issue is really about.

[tillsc]

That is why I apply the following patch in my projects:

https://gist.github.com/tillsc/b2b3b3af67740f6013be

[tpitale]

@tillsc I think go ahead with this PR.