Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

validations not called when model updating using nested attributes #618

Closed
lighthouse-import opened this Issue · 19 comments

2 participants

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/2646
Created by Jarl Friis - 2011-03-04 01:59:41 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Matt Jones - 2009-05-14 19:03:35 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-05-15 03:24:30 UTC

Assigning to eloy as he's the dude here.

@lighthouse-import

Imported from Lighthouse.
Comment by Jarl Friis - 2009-05-15 08:16:03 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Jarl Friis - 2009-05-15 09:01:00 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Jarl Friis - 2009-05-15 13:51:38 UTC

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

Please ignore my comment @ 09:16 AM

@lighthouse-import

Imported from Lighthouse.
Comment by Eloy Duran - 2009-05-16 08:42:53 UTC

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

@lighthouse-import

Imported from Lighthouse.
Comment by Eloy Duran - 2009-07-12 12:04:20 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-07-12 23:56:26 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by José Valim - 2009-07-13 07:20:36 UTC

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

@lighthouse-import

Imported from Lighthouse.
Comment by Matt Jones - 2009-07-13 07:34:09 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Eloy Duran - 2009-07-13 07:38:55 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Adam Ingram-Goble - 2010-01-02 22:22:11 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-05-04 17:48:49 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Neeraj Singh - 2010-07-08 10:38:26 UTC

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

@lighthouse-import

Imported from Lighthouse.
Comment by Eloy Duran - 2010-07-08 11:57:17 UTC

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…

@lighthouse-import

Imported from Lighthouse.
Comment by wout - 2010-08-19 08:41:43 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Ryan Bigg - 2010-11-08 01:48:05 UTC

Automatic cleanup of spam.

@jonlhouse

Thanks for the tip on the hidden field. You're right it seems that updated "virtual attributes" don't trigger the callback hooks. Although oddly enough the nested fields are validated.

So validations are run but before_validation is not on nested models when only virtual attribute are changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.