This project is archived and is in readonly mode.

#2646 open
Jarl Friis

validations not called when model updating using nested attributes

Reported by Jarl Friis | May 14th, 2009 @ 11:03 AM | in 3.x

When using attributes= on a nested attribute for which :allow_destroy => true, and deleting an entry, no validation is taking place.

I supply a test case that demonstrates the problem.

Comments and changes to this ticket

  • Matt Jones

    Matt Jones May 14th, 2009 @ 08:03 PM

    There's an issue here, but your test does not trigger it. You'll need to save the Pirate record before bird.id will even have a value to use in the attribute call.

    I've attached a diff with a correct test; the fix is going to be more complicated.

    The issue is that, until the after_save callbacks from the nested_attributes code run, pirate.birds.size is 1. The bird instance has marked_for_destruction? set, but has not yet been destroyed.

    @jarl - in the interim, you might want to try a custom validation that only counts records with marked_for_destruction? false.

    Example:

    assert_nothing_raised do
      Pirate.validates_each(:birds) do |record, attr, value|
        record.errors.add attr, 'is too short.' if value.reject { |v| v.marked_for_destruction? }.size < 1
      end
    end
    

    or even define a custom attribute (a bit of a hack):

      Pirate.class_eval do
        def birds_not_destroyed
          birds.reject { |v| v.marked_for_destruction? }
        end
      end
    

    and run validates_size_of on that.

  • Michael Koziarski

    Michael Koziarski May 15th, 2009 @ 04:24 AM

    • Assigned user set to “Eloy Duran”

    Assigning to eloy as he's the dude here.

  • Jarl Friis

    Jarl Friis May 15th, 2009 @ 09:16 AM

    • Tag changed from accepts_nested_attributes_for, associations, validation, validations to accepts_nested_attributes_for, active_record, associations, update_attributes, validation, validations

    Thanks for taken my ticket serious.

    Actually the line in my test reading

    p.attributes = {:birds_attributes => [ {:_delete => 1, :id => bird.id}]}
    

    should have been

    p.update_attributes :birds_attributes => [ {:_delete => 1, :id => bird.id}]
    

    This was more like the code generated by scaffold that lead me to discover this problem, and as you have mentioned update_attributes do call save after the attributes=. Sorry for the confusion.

    @Matt: Thanks a lot for the temporary workaround you have suggested.

  • Jarl Friis

    Jarl Friis May 15th, 2009 @ 10:01 AM

    What ever the fix is going to be I think it is worth considering to make a fix so that p.birds.size should result in 0 after

    p.attributes = {:birds_attributes => [ {:_delete => 1, :id => bird.id}]}
    

    Basically that probably means that size has to be implemented to not count birds for which marked_for_destruction? is true. Remember that birds that is marked_for_destruction only still exists because the ids are need when p is saved to the database (so that the birds can be deleted). Alternatively is to store all birds that is marked_for_destruction into a different list which is then cleared when p is saved to the database.

  • Jarl Friis

    Jarl Friis May 15th, 2009 @ 02:51 PM

    @Matt: I see your point now. The test have to call save before the call to attributes=.

    Please ignore my comment @ 09:16 AM

  • Eloy Duran

    Eloy Duran May 16th, 2009 @ 09:42 AM

    The dude has reserved some time at the end of the coming week to review problems and patches. :)

  • Eloy Duran

    Eloy Duran July 12th, 2009 @ 01:04 PM

    I stil haven't been able to make up my mind on how to fix this cleanly. I'm currently leaning to overriding AssociationCollection #size & #length to ignore records that have been marked for destruction. Thoughts?

  • Michael Koziarski

    Michael Koziarski July 13th, 2009 @ 12:56 AM

    Wouldn't it be cleaner just to remove the records from the in memory array once they're marked for destruction. Then size and length will function as intended?

  • José Valim

    José Valim July 13th, 2009 @ 08:20 AM

    Good suggestion Koz, it would solve another problems as having destroyed records being validated when they shouldn't.

  • Matt Jones

    Matt Jones July 13th, 2009 @ 08:34 AM

    The only issue with removing them is that if some other validation fails, they'll need to be someplace so they can be returned back to the user.

    The most specific instance of this would be a validation that ensures that exactly N associated records exist (bad design, but it could happen). In that case, a user will have to delete some records if more are added, but the whole array will need to be around to display with the validation error.

  • Eloy Duran

    Eloy Duran July 13th, 2009 @ 08:38 AM

    While I was typing an elaborate comment, Matt posted his, which says exactly what I wanted to say, but better :)

    So the problem is how to put them back after a failed validation, which might occur elsewhere then the current association, or an exception is raised etc. This might not be such a problem at all of course, I'd just have to look into it, but it seemed a way more complicated solution then having the collection return the size of what's actually going to be in the database.

  • Adam Ingram-Goble

    Adam Ingram-Goble January 2nd, 2010 @ 10:22 PM

    would it be possible to either move the marked_for_destruction records to another in memory array, or modify the association method so that it doesn't return the marked records by default? Either procedure allow for "rolling back" the failure. I think this is important for supporting the principle of least surprise.

  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 08:38 AM

    • Assigned user changed from “Eloy Duran” to “José Valim”
    • Importance changed from “” to “”
  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 11:38 AM

    • Assigned user changed from “José Valim” to “Neeraj Singh”

    Did not know there were so many tickets related to nested_attributes. I will take a look at this one too.

  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 12:57 PM

    There aren't that many different ones though :)

    This is basically the same problem that comes up once in a while; how to make validates_size_of etc work with records marked for destruction. To use a separate array or not, that is the question…

  • Neeraj Singh

    Neeraj Singh July 29th, 2010 @ 03:28 AM

    • State changed from “new” to “open”
  • wout

    wout August 19th, 2010 @ 09:41 AM

    I found out why associations sometimes aren't saved. If one of the database fields is changed validations are started and associations are saved, but if an attr_accessor value is changed not. Currently I add a hidden field to my nested forms setting :updated_at to Time.now and all is working fine again.

  • Ryan Bigg

    Ryan Bigg November 8th, 2010 @ 01:48 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • bingbing

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>

Referenced by

Pages