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

`validates_uniqueness_of` and `accepts_nested_attributes_for` only validates existing records #4568

Closed
sethvargo opened this Issue Jan 21, 2012 · 27 comments

Comments

Projects
None yet
@sethvargo

sethvargo commented Jan 21, 2012

If I have a nested model, such as:

class Contact < ActiveRecord::Base
  belongs_to :group

  validates_uniqueness_of :phone, :scope => [:group_id]
end
class Group < ActiveRecord::Base
  has_many :contacts

  accepts_nested_attributes_for :contacts
end

The uniqueness validator only applies to existing records in the database... If I submit the same duplicate record as a nested attribute:

{
  "group"  => {
    "name" => "Blah",
    "description" => "Foot",
    "contacts_attributes" => {
      "0" => {
        "id" => "5",
        "name" => "Seth Vargo",
        "phone"=>"1234567890"
      },
        "1" => {
          "id" => "6",
          "name" => "Person B",
          "phone"=>"1234567890"
      },
        "2" => {
          "name" => "Person C",
          "phone" => "1234567890"
        }
      }
   }
}

It will add all those records, even though the phone is supposed to be unique...

@squeedee

This comment has been minimized.

Show comment
Hide comment
@squeedee

squeedee Jan 23, 2012

Looks like a duplicate of #1572 . that may help you know whats happening.

squeedee commented Jan 23, 2012

Looks like a duplicate of #1572 . that may help you know whats happening.

@sethvargo

This comment has been minimized.

Show comment
Hide comment
@sethvargo

sethvargo Jan 23, 2012

Okay, but that doesn't actually specify a solution...

sethvargo commented Jan 23, 2012

Okay, but that doesn't actually specify a solution...

@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Apr 30, 2012

Contributor

This is currently an active issue:

> g = Group.new(name: "Blah", description: "Foot")
=> #<Group id: nil, name: "Blah", description: "Foot", created_at: nil, updated_at: nil>

> g.contacts_attributes = [ { name: "Seth Vargo", phone: "1234567890"}, { name: "Person B", phone: "1234567890" } ]
=> [{:name=>"Seth Vargo", :phone=>"1234567890"}, {:name=>"Person B", :phone=>"1234567890"}]

> g.save!
> g.contacts
=> [#<Contact id: 3, name: "Seth Vargo", phone: "1234567890", group_id: 2>, #<Contact id: 4, name: "Person B", phone: "1234567890", group_id: 2>]
Contributor

frodsan commented Apr 30, 2012

This is currently an active issue:

> g = Group.new(name: "Blah", description: "Foot")
=> #<Group id: nil, name: "Blah", description: "Foot", created_at: nil, updated_at: nil>

> g.contacts_attributes = [ { name: "Seth Vargo", phone: "1234567890"}, { name: "Person B", phone: "1234567890" } ]
=> [{:name=>"Seth Vargo", :phone=>"1234567890"}, {:name=>"Person B", :phone=>"1234567890"}]

> g.save!
> g.contacts
=> [#<Contact id: 3, name: "Seth Vargo", phone: "1234567890", group_id: 2>, #<Contact id: 4, name: "Person B", phone: "1234567890", group_id: 2>]
@danieltamiosso

This comment has been minimized.

Show comment
Hide comment
@danieltamiosso

danieltamiosso commented Aug 13, 2012

+1

@ischuster

This comment has been minimized.

Show comment
Hide comment
@ischuster

ischuster commented Aug 13, 2012

+1

@McRipper

This comment has been minimized.

Show comment
Hide comment
@McRipper

McRipper commented Aug 29, 2012

+1

@jamezilla

This comment has been minimized.

Show comment
Hide comment
@jamezilla

jamezilla commented Sep 25, 2012

+1

@Backoo

This comment has been minimized.

Show comment
Hide comment
@Backoo

Backoo Nov 12, 2012

+1 ! ! !

Backoo commented Nov 12, 2012

+1 ! ! !

@mrwade

This comment has been minimized.

Show comment
Hide comment
@mrwade

mrwade commented Nov 12, 2012

+1

@macfanatic

This comment has been minimized.

Show comment
Hide comment
@macfanatic

macfanatic Nov 13, 2012

Still present in Rails 3.2.8, +1

macfanatic commented Nov 13, 2012

Still present in Rails 3.2.8, +1

@macfanatic

This comment has been minimized.

Show comment
Hide comment
@macfanatic

macfanatic Nov 13, 2012

Here's the hacked solution I came up with, that allowed me to put the error on the failed attribute itself, and not on the base of the parent object as the solution floating around here suggests: http://stackoverflow.com/questions/2772236/validates-uniqueness-of-in-nested-model-rails#answers-header

class Event < ActiveRecord::Base
  has_many :vendors, dependent: :destroy
  before_validation do
    vendors.map do |e| 
      stream_ids = vendors.collect(&:audio_stream_id).compact
      stream_ids_counted = stream_ids.inject({}) { |hash,stream_id| hash[stream_id] = (hash[stream_id] || 0) + 1; hash }
      duplicate_stream_ids = stream_ids_counted.select { |k,v| v > 1 }.keys
      e.instance_variable_set "@duplicate_stream_ids", duplicate_stream_ids
    end
  end
end
class Vendor < ActiveRecord::Base
  belongs_to :event
  validate :ensure_unique_audio_stream_per_event

   def ensure_unique_audio_stream_per_event
      errors.add(:audio_stream_id, "taken") if @duplicate_stream_ids.present? and @duplicate_stream_ids.include?(audio_stream_id)
    end
end

So, in usage:

@event = Event.first
@event.vendors << Vendor.new(audio_stream_id: 1)
@event.vendors << Vendor.new(audio_stream_id: 1)
@event.valid? #false
@event.full_messages # the :audio_stream_id attribute in the corresponding Vendor record is marked, so Formtastic & others displays the message nicely

macfanatic commented Nov 13, 2012

Here's the hacked solution I came up with, that allowed me to put the error on the failed attribute itself, and not on the base of the parent object as the solution floating around here suggests: http://stackoverflow.com/questions/2772236/validates-uniqueness-of-in-nested-model-rails#answers-header

class Event < ActiveRecord::Base
  has_many :vendors, dependent: :destroy
  before_validation do
    vendors.map do |e| 
      stream_ids = vendors.collect(&:audio_stream_id).compact
      stream_ids_counted = stream_ids.inject({}) { |hash,stream_id| hash[stream_id] = (hash[stream_id] || 0) + 1; hash }
      duplicate_stream_ids = stream_ids_counted.select { |k,v| v > 1 }.keys
      e.instance_variable_set "@duplicate_stream_ids", duplicate_stream_ids
    end
  end
end
class Vendor < ActiveRecord::Base
  belongs_to :event
  validate :ensure_unique_audio_stream_per_event

   def ensure_unique_audio_stream_per_event
      errors.add(:audio_stream_id, "taken") if @duplicate_stream_ids.present? and @duplicate_stream_ids.include?(audio_stream_id)
    end
end

So, in usage:

@event = Event.first
@event.vendors << Vendor.new(audio_stream_id: 1)
@event.vendors << Vendor.new(audio_stream_id: 1)
@event.valid? #false
@event.full_messages # the :audio_stream_id attribute in the corresponding Vendor record is marked, so Formtastic & others displays the message nicely
@jeyb

This comment has been minimized.

Show comment
Hide comment
@jeyb

jeyb Nov 26, 2012

Contributor

I opened a pull request with a fix for this bug, take a look. Let me know if you have suggestions or comments.

Contributor

jeyb commented Nov 26, 2012

I opened a pull request with a fix for this bug, take a look. Let me know if you have suggestions or comments.

@mshappe

This comment has been minimized.

Show comment
Hide comment
@mshappe

mshappe Feb 25, 2013

👍

Wangjohn's pull request #9364 looks like it goes a long way toward fixing this issue, but nobody has even commented on it yet.

mshappe commented Feb 25, 2013

👍

Wangjohn's pull request #9364 looks like it goes a long way toward fixing this issue, but nobody has even commented on it yet.

@sharp

This comment has been minimized.

Show comment
Hide comment
@sharp

sharp Jul 25, 2013

Same problem :)

sharp commented Jul 25, 2013

Same problem :)

@kugaevsky

This comment has been minimized.

Show comment
Hide comment
@kugaevsky

kugaevsky commented Aug 8, 2013

+1

@evserykh

This comment has been minimized.

Show comment
Hide comment
@evserykh

evserykh commented Aug 16, 2013

👍

@xamut

This comment has been minimized.

Show comment
Hide comment
@xamut

xamut commented Aug 16, 2013

+1

Empact added a commit to Empact/rails that referenced this issue Nov 21, 2013

Simplify and improve jeyb and wangjohn's implementation of associatio…
…n uniqueness validation. Fixes #4568.

Remove the validation context-related changes because they were
removed from mainline in 75135a9
@KendallPark

This comment has been minimized.

Show comment
Hide comment
@KendallPark

KendallPark commented Jan 15, 2014

+1

@laurocaetano

This comment has been minimized.

Show comment
Hide comment
@laurocaetano

laurocaetano Jan 16, 2014

Contributor

Hi guys!

I think it's not worth adding support for this case. It's going to change a lot of code just for a 'partial' fix. I mean partial, because it may not have database constraints.

Some information about this can be found here.

wdty?

Contributor

laurocaetano commented Jan 16, 2014

Hi guys!

I think it's not worth adding support for this case. It's going to change a lot of code just for a 'partial' fix. I mean partial, because it may not have database constraints.

Some information about this can be found here.

wdty?

@lichtamberg

This comment has been minimized.

Show comment
Hide comment
@lichtamberg

lichtamberg commented Feb 6, 2014

+111111

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 6, 2014

Member

I agree with @laurocaetano. This is a partial fix that demands a lot of code.

The proper fix, as documented is to use database constraints.

Member

rafaelfranca commented Feb 6, 2014

I agree with @laurocaetano. This is a partial fix that demands a lot of code.

The proper fix, as documented is to use database constraints.

@timurkhafizov

This comment has been minimized.

Show comment
Hide comment
@timurkhafizov

timurkhafizov commented Feb 7, 2014

+1

@adamwong246

This comment has been minimized.

Show comment
Hide comment
@adamwong246

adamwong246 commented Apr 7, 2014

+infinity

@DazDotOne

This comment has been minimized.

Show comment
Hide comment
@DazDotOne

DazDotOne Apr 12, 2014

+1 seems to be an issue still in rails 4 (unless there's some easy solution that I have overlooked)

DazDotOne commented Apr 12, 2014

+1 seems to be an issue still in rails 4 (unless there's some easy solution that I have overlooked)

@alagu

This comment has been minimized.

Show comment
Hide comment
@alagu

alagu May 17, 2014

+1 still seems to be happening to me.

alagu commented May 17, 2014

+1 still seems to be happening to me.

@rderoldan1

This comment has been minimized.

Show comment
Hide comment
@rderoldan1

rderoldan1 commented May 22, 2014

+1

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 May 24, 2014

Member

@alagu @rderoldan1 : Please read Rafael's answer, this is not easily fixable so this issue has been marked as closed but the problem remains.

Member

robin850 commented May 24, 2014

@alagu @rderoldan1 : Please read Rafael's answer, this is not easily fixable so this issue has been marked as closed but the problem remains.

@rails rails locked and limited conversation to collaborators Oct 1, 2014

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