This project is archived and is in readonly mode.

#5417 open
Javier Fernandez-Ivern

validates_uniqueness_of should not trigger against a record marked for destruction

Reported by Javier Fernandez-Ivern | August 19th, 2010 @ 11:30 PM

When editing multiple records in one form via fields_for, if these records belong to a model with a uniqueness constraint and you happen to delete a record and create a new one with the same value in the unique field, the validation will fail because it's ignoring the fact that one of them is marked for destruction.

I've put together the following test for it. Given the following class definition:

class UniqueParrot < Parrot
  validates_uniqueness_of :name
end

I believe the last assertion in this test should succeed:

  def test_should_skip_uniqueness_validation_if_existing_record_is_marked_for_destruction
    copycat = UniqueParrot.new(:name => 'Roger')
    
    @pirate.parrots << UniqueParrot.new(:name => 'Roger')
    assert_raise(ActiveRecord::RecordInvalid) { assert !(@pirate.parrots << copycat) }
    
    @pirate.parrots.first.mark_for_destruction
    assert @pirate.parrots << copycat
  end

Instead, the validation triggers:

  1) Error:
test_should_skip_uniqueness_validation_if_existing_record_is_marked_for_destruction(TestDestroyAsPartOfAutosaveAssociation):
ActiveRecord::RecordInvalid: Validation failed: Name has already been taken
    /Users/javier/Projects/rails/activerecord/lib/active_record/validations.rb:49:in `save!'
    /Users/javier/Projects/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:30:in `save!'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:242:in `save!'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:289:in `with_transaction_returning_status'
    /Users/javier/Projects/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:139:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:204:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:287:in `with_transaction_returning_status'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:242:in `save!'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb:39:in `insert_record'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:136:in `<<'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:479:in `add_record_to_target_with_callbacks'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:135:in `<<'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:133:in `each'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:133:in `<<'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:158:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:139:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/transactions.rb:204:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:157:in `transaction'
    /Users/javier/Projects/rails/activerecord/lib/active_record/associations/association_collection.rb:132:in `<<'
    ./test/cases/autosave_association_test.rb:642:in `test_should_skip_uniqueness_validation_if_existing_record_is_marked_for_destruction'
    /Users/javier/Projects/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `__send__'
    /Users/javier/Projects/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `run'
    /Users/javier/Projects/rails/activesupport/lib/active_support/callbacks.rb:413:in `_run_setup_callbacks'
    /Users/javier/Projects/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run'

I've been able to work around this in my application by forcing a delete of all destroyed records (and removing them from the params hash) before calling update_attributes on the model, so I'm ok for now. However, this probably needs fixing.

Comments and changes to this ticket

  • Santiago Pastorino

    Santiago Pastorino August 20th, 2010 @ 12:24 AM

    • Importance changed from “” to “Low”

    Javier can you do a test case following this http://rails.lighthouseapp.com/projects/8994/sending-patches Thanks very much.

  • Javier Fernandez-Ivern

    Javier Fernandez-Ivern August 20th, 2010 @ 12:56 AM

    • Tag changed from mark_for_destruction validates_uniqueness_of fields_for update_attributes to fields_for, mark_for_destruction, patch, update_attributes, validates_uniqueness_of

    Done. Sorry, I thought that process was only for people sending solutions not just tests.

  • Javier Fernandez-Ivern

    Javier Fernandez-Ivern August 20th, 2010 @ 12:56 AM

    • Tag changed from fields_for, mark_for_destruction, patch, update_attributes, validates_uniqueness_of to fields_for, mark_for_destruction, patch, test, update_attributes, validates_uniqueness_of
  • Santiago Pastorino

    Santiago Pastorino August 20th, 2010 @ 05:50 AM

    Javier, it's ok a test case help others to make a patch, so when someone post the solution your test and the solution are applied, giving some credit to you too ;).
    Now ... do you want to try a patch? ;).
    Thanks.

  • Javier Fernandez-Ivern

    Javier Fernandez-Ivern August 20th, 2010 @ 06:07 AM

    Yes, but would you mind discussing the problem a little bit first? I no longer like my test case, because there's no guarantee that the record that is marked_for_deletion is actually going to be saved. So in the general case of the target record being marked for deletion, I'm not sure this is a change that needs to be made.

    The case that's more clear-cut is when an association is being saved and it contains items that are marked for deletion and items that conflict with them in uniqueness. In this case, we know both changes are going to happen (barring a transaction blowup or another validation failure). I'd therefore focus on addressing this problem at the association save level, by ordering the child saves so that elements to be destroyed go first.

    Does this seem like the right approach? If so I'll rewrite the test case and take a shot at a patch.

  • Javier Fernandez-Ivern

    Javier Fernandez-Ivern August 20th, 2010 @ 03:02 PM

    Well, I'm looking into resolving this, it's somewhat tricky (to me, at least) since I need to make it work when validating the association, not just when saving. Else it'd be pretty strange.

    Until then, I'm attaching a better test:

      def test_should_skip_uniqueness_validation_if_existing_record_is_marked_for_destruction
        roger = UniqueParrot.create(:name => "Roger")
        wilco = UniqueParrot.create(:name => "Wilco")
        
        @pirate.parrots << roger << wilco
        assert @pirate.valid?
        assert @pirate.save
        
        wilco.name = roger.name
        assert !@pirate.valid?
        assert !@pirate.save
        
        roger.mark_for_destruction
        assert @pirate.valid?
        assert @pirate.save
      end
    
  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 8th, 2010 @ 08:28 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “new” to “open”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “open” to “stale”
  • 2kan

    2kan February 9th, 2011 @ 03:00 AM

    • State changed from “stale” to “open”

    [state:open]

    The problem with this ticket and Javier Fernandez-Ivern's test is not in mark_for_destruction itself. In edge validations do not run for associations which are marked for destruction but the problem in the whole illustrated situation:

    He marks for deletion the roger object. So validation for it doesn't run. But then it validates the wilco object which is not marked for destruction and has the same name with the rojer object which is marked for destruction but still exists in database, so when we try to validate for wilco for uniqueness it fails.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages