has_many :uniq should filter duplicates on insertion (<<) #8573

Closed
turadg opened this Issue Dec 20, 2012 · 5 comments

Comments

Projects
None yet
4 participants

turadg commented Dec 20, 2012

Use case and stop-gap solution are described in this StackOverflow QA.

Use case

Given,

class User < ActiveRecord::Base
  has_many :user_roles
  has_many :roles, :through => :user_roles
end

Dev wants to append a role without having to check:

user.roles << role  # automatically checks roles.include?

Novice Rails devs might assume that adding :uniq would work:

  has_many :roles, :through => :user_roles, :uniq => true

Of course it doesn't because that presently affects only queries, ignoring duplicates. What would be nice if it it also ignored attempts to add a duplicate, running up against any DB constraints.

A work-around described on the QA is:

  has_many :roles, :through => :user_roles do
    def <<(new_item)
      super( Array(new_item) - proxy_association.owner.roles )
    end
  end

Could this contextual redefinition of << be made automatically when marking a has_many as uniq?

Owner

rafaelfranca commented Dec 20, 2012

I don't think so. This is your domain logic so it your responsibility to check it. Also we don't get feature requests in the issues tracker.

Thanks.

turadg commented Dec 20, 2012

Sorry, I read "only bug reports and pull requests" in CONTRIBUTING and thought of it as a bug.

has_many dedupes. Why shouldn't has_many :through ?

saki7 commented May 2, 2013

+1 for this. I really consider this as a bug.

Why do you think that people using :uniq does not require de-duplication?
<< is actually an only way to append an element to the association; it really should take care about duplications by itself. Otherwise collection.where() becomes the only way to do that.

Contributor

mbhnyc commented May 2, 2013

This keeps coming up!

see: #10047 (comment)

Will re-iterate my 👍 (and agree that it doesn't seem like the expected behavior and thus bug-esque) but apparently there's a race condition issue here — the (non-optimal) fix is to add an unique constraint on the database layer.

saki7 commented May 2, 2013

@mbhnyc Thanks for referencing! I'll check it out.

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