validations not called when model updating using nested attributes

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.

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.

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

Please ignore my comment @ 09:16 AM

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

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?

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?

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

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.

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.

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.

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…

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.

omega watch