validates_uniqueness_of with :scope ignores duplicates on create, and fails in some valid cases

Here's an capture of my console session

>> a = Product.find(2)
=> #<Product id: 2, name: "olive">
>> pa1 = ProductAttribute.new(:name => "PA1", :position => 1)
=> #<ProductAttribute id: nil, product_id: nil, position: 1, name: "PA1">
>> pa2 = ProductAttribute.new(:name => "PA2", :position => 1)
=> #<ProductAttribute id: nil, product_id: nil, position: 1, name: "PA2">
>> a.product_attributes << pa1
=> [#<ProductAttribute id: 2, product_id: 2, position: 1, name: "PA1">]
>> a.product_attributes << pa2
=> false
>> a.valid?
=> false
>> a.product_attributes
=> [#<ProductAttribute id: 2, product_id: 2, position: 1, name: "PA1">, #<ProductAttribute id: nil, product_id: 2, position: 1, name: "PA2">]
>> a.save!
ActiveRecord::RecordInvalid: Validation failed: Product attributes is invalid

The difference is that I created two separate instances of the Product Attribute. The pa1 instance is saved to the db upon the call to a.product_attributes << pa1, and so the validations will run. When pa2 is added to the product_attributes collection, it too attempts to be saved, so the validations will run but will fail because of the validatates_uniqueness_of. As we can see, validations here work as expected.

In the case you highlighted above, you add the same instance. The first time, it is saved, and all is good. The second time, it is not saved, thus validations are not run. Even if it did, it will still pass, because it still only one database record with the same position and product_id. Remember, the validations are on the ProductAttribute itself, and not on the Product.

Anyway, i'll close the ticket as invalid as the behavior works as expected.

Well, actually the difference was, you were essentially doing an "update". This ticket was for "create" behavior.

This the way ActiveScaffold ends up creating new records. You have subforms with all the associated models. So you're saving all in one shot. Now mabye AS needs to rework how the save new records, but this still seems like fishy Rails behavior to me. Notice the 2nd ProductAttribute remains a "new record" even after save.

See this example:

>> p = Product.new(:name => "Printer")
=> #<Product:0x34f826c @new_record=true, @attributes={"name"=>"Printer"}>
>> pa1 = ProductAttribute.new(:value => "10")
=> #<ProductAttribute:0x34f52ec @new_record=true, @attributes={"product_id"=>nil, "value"=>"10"}>
>> pa2 = ProductAttribute.new(:value => "10")
=> #<ProductAttribute:0x34f231c @new_record=true, @attributes={"product_id"=>nil, "value"=>"10"}>
>> p.product_attributes << pa1
=> [#<ProductAttribute:0x34f52ec @new_record=true, @attributes={"product_id"=>nil, "value"=>"10"}>]
>> p.product_attributes << pa2
=> [#<ProductAttribute:0x34f52ec @new_record=true, @attributes={"product_id"=>nil, "value"=>"10"}>, #<ProductAttribute:0x34f231c @new_record=true, @attributes={"product_id"=>nil, "value"=>"10"}>]
>> p.valid?
=> true
>> p.save
=> true
>> p
=> #<Product:0x34f826c @new_record_before_save=true, @product_attributes=[#<ProductAttribute:0x34f52ec @new_record=false, @attributes={"product_id"=>1, "id"=>1, "value"=>"10"}, @errors=#<ActiveRecord::Errors:0x34ebb98 @errors={}, @base=#<ProductAttribute:0x34f52ec ...>>>, #<ProductAttribute:0x34f231c @new_record=true, @attributes={"product_id"=>1, "value"=>"10"}, @errors=#<ActiveRecord::Errors:0x34ea6e4 @errors={"value"=>["has already been taken"]}, @base=#<ProductAttribute:0x34f231c ...>>>], @new_record=false, @attributes={"name"=>"Printer", "id"=>1}, @errors=#<ActiveRecord::Errors:0x34ebdc8 @errors={}, @base=#<Product:0x34f826c ...>>>
>> p.valid?
=> false

I think you're right - this is a "defect" in ActiveRecord. But I don't think it's a fixable one.

The difference between the 'create' and 'update' scenarios is that when the product is a new record, AR doesn't automatically try and save the product attributes. They get queued up for later.

Then, when you try and save the main product, AR checks that the product is valid. Assuming you have validates_associated set up, AR will even check to make sure that the product attributes are valid. But since the product attributes are valid versus all of the permanent product attributes (the ones in the database), the validates_uniqueness_of check passes. The problem is that the product attributes become invalid during the save. But at that point AR is basically done checking validation, and the main Product saves.

The solution, I think, is to use your database and set up a uniqueness constraint. Oh, and to make sure you're using a transaction around the whole thing. Then when the second ProductAttribute tries to save it'll fail and abort the transaction.

Getting feedback to the user is another matter ...

p.s. I recommend this be closed as wontfix, but I'd rather someone else confirmed my reasoning.

i'm sorry, i slipped earlier. i didn't mean to close the ticket.

I looked at the code and cainlevy's analysis seems to be correct basically activerecord works like this:

If a record that has new has_many associations that are unsaved is saved then:

• The record and the associations are validated
• The record is saved
• The unsaved associations are re-inserted to the now saved record association so they get an id and are then saved (not save!, so while they do run validations they will not notify anyone about the failure)

If anything could be improved then perhaps instead of using "save" to save the associations "save!" could be used, so there would not be silent failures.

btw. it should not be neccessary to run this inside a transaction because save is wrapped in a transaction by default so all before/after callbacks, validations, association saves are ran inside one transaction and if any one of those raises an exception the whole transaction is rolled back (though not the ActiveRecord model state so after such rollback it would be unsafe to do anything with the association because rails is unaware of the fact that although it added the records to database they are not there anymore).

There are also valid models that will fail validation if the scope is a foreign key.

Suppose we have:

class Publisher < ActiveRecord::Base
  has_many :books
end

class Book < ActiveRecord::Base
  belongs_to :publisher
  
  validates_uniqueness_of :title, :scope => :publisher_id
end

Let us represent "unpublished books" by NIL in the publisher_id field. And the validation says that each publisher may only have one book with a given title.

Here's the console demo:

Loading development environment (Rails 2.0.2)

First we create an unpublished book.

>> unpublished_book = Book.new(:title => "Bacon")
=> #<Book id: nil, publisher_id: nil, title: "Bacon", created_at: nil, updated_at: nil>
>> unpublished_book.save!
=> true

Now we create a publisher with a book, no problem.

>> pub = Publisher.new(:name => "Bacon House")
=> #<Publisher id: nil, name: "Bacon House", created_at: nil, updated_at: nil>
>> pub.books.build(:title => "Chunky")
=> #<Book id: nil, publisher_id: nil, title: "Chunky", created_at: nil, updated_at: nil>
>> pub.save!
=> true

Now we create a publisher with a book that has the same title as an unpublished book, and it fails.

>> pub2 = Publisher.new(:name => "Crash Publications")
=> #<Publisher id: nil, name: "Crash Publications", created_at: nil, updated_at: nil>
>> pub2.books.build(:title => "Bacon")
=> #<Book id: nil, publisher_id: nil, title: "Bacon", created_at: nil, updated_at: nil>
>> pub2.save!
ActiveRecord::RecordInvalid: Validation failed: Books is invalid

Why? As tarmo mentions, the validations are run before the record is saved, so we see in the log at validation time:

SELECT * FROM books WHERE (books.title = 'Bacon' AND books.publisher_id IS NULL) LIMIT 1

If it were working right, it would be "2" instead of "NULL".

I don't think we should mark this WONT_FIX just because it's difficult to fix.