#2249 autosaving associations skips validations and persists invalid data in the DB - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2249 ✓resolved

autosaving associations skips validations and persists invalid data in the DB

Reported by Akira Matsuda | March 16th, 2009 @ 08:37 AM | in 2.x

When autosaving associations, the associated models are saved without validations even when their data are invalid.

For example, with these 2 models,


% ./script/generate model pirate catchphrase:string

class Pirate < ActiveRecord::Base
  has_one :ship

  validates_presence_of :catchphrase
end

% ./script/generate model ship pirate:references name:string

class Ship < ActiveRecord::Base
  belongs_to :pirate

  validates_presence_of :name
end

you can save a ship with no name with the following operation.


 p = Pirate.new :catchphrase => "I'm Gonna Be King of the Pirates!"
 #=> #<Pirate id: nil, catchphrase: "I'm Gonna Be King of the Pirates!", created_at: nil, updated_at: nil>
 p.build_ship
 #=> #<Ship id: nil, pirate_id: nil, name: nil, created_at: nil, updated_at: nil>
 p.save
 #=> true
 s = Ship.last
 #=> #<Ship id: 1, pirate_id: 1, name: nil, created_at: "2009-03-16 07:50:14", updated_at: "2009-03-16 07:50:14">
 s.valid?
 #=> false
 s.errors.full_messages
 #=> ["Name can't be blank"]  # note that an invalid data is in the DB!

In my opinion, any model could never be saved without validations except when save_without_validation method are explicitly called by the developer.
I don't know whether a bug or intentional is this, but anyways, this may cause a dozen of serious problems on Rails 2.3, because we may easily create this situation automatically through nested_model_forms.

Very very dangerous, isn't it?

Comments and changes to this ticket

Eloy Duran March 16th, 2009 @ 09:12 AM
- Assigned user set to “Eloy Duran”
You flagged this item as spam.
MOROHASHI Kyosuke March 16th, 2009 @ 09:14 AM
+1

I also think this is very serious problem.
Eloy Duran March 16th, 2009 @ 10:15 AM
- State changed from “new” to “invalid”
I don't know whether a bug or intentional is this, but anyways

It's intentional. If you want to validate a single association, has_one/belongs_to, you will have to define the association with :validate => true. This has always been the case.

this may cause a dozen of serious problems on Rails 2.3, because we may easily create this situation automatically through nested_model_forms.

Nope it won't be a problem, because using NestedAttributes will automatically enable :autosave => true on the association. If :autosave is enabled an association should always be validated and saved.

If you find that not to be the case please file a failing test case.

Thanks
Eloy Duran March 16th, 2009 @ 10:16 AM
Btw: The source for AutosaveAssociation can be viewed here: http://github.com/rails/rails/bl...
You flagged this item as spam.
Akira Matsuda March 16th, 2009 @ 11:20 AM
Nope it won't be a problem, because using NestedAttributes will automatically enable :autosave => true on the association. If :autosave is enabled an association should always be validated and saved.

Ahh, I see. So, this problem is not directly related to NestedAttributes. Sorry for the wrong point of argument.

But I'm still not totally agree with you. Please don't close the ticket.

Let me repeat the most important point.

any model could never be saved without validations except when save_without_validation method are explicitly called by the developer.

I tried the same operation on Rails 2.2.2, and found that on Rails 2.2.2, saving the pirate does not raise an error, and neither saves the invalid Ship model.
On the other hand, ActiveRecord 2.3 changes its behaviour to implicitly inject an invalid record into DB with normal :save method, which should never be happen.

I really love the nested_form feature, but this AR matter is absolutely a worse change. Couldn't it rolled back to AR 2.2.2 behaviour?
Eloy Duran March 16th, 2009 @ 01:08 PM
- State changed from “invalid” to “open”
But I'm still not totally agree with you. Please don't close the ticket.

Sumimasen Matsuda-san.

I did not want to scare you into believing that it would not be fixed, just that the original ticket misled me… Now that you re-explained it some more it seems we are on the same page :)

Let me repeat the most important point. any model could never be saved without validations except when save_without_validation method are explicitly called by the developer. I tried the same operation on Rails 2.2.2, and found that on Rails 2.2.2, saving the pirate does not raise an error, and neither saves the invalid Ship model. On the other hand, ActiveRecord 2.3 changes its behaviour to implicitly inject an invalid record into DB with normal :save method, which should never be happen. I really love the nested_form feature, but this AR matter is absolutely a worse change. Couldn't it rolled back to AR 2.2.2 behaviour?

Ok I have checked the source of 2.2 again and I think I finally figured out why this regression was introduced. As you can see on this line it would indicate that a has_one/belongs_to only validates when :validate => true. However, this did not affect the saving of the record when the parent is a new record, in that case it did #save(true), so it did ran the validations:

http://github.com/rails/rails/bl...

Of course the biggest problem is that there apperantly was no test for this. So when I refactored it I apparently failed to grasp the original intent.

Too bad this only got noticed until now, so I guess we're too late for 2.3. But that's life, there was an RC and it was gone unnoticed, nothing that we can do about it now. I'll fix this on edge asap though.

Thanks
Eloy Duran March 17th, 2009 @ 12:06 AM
- Assigned user changed from “Eloy Duran” to “Pratik”
This should fix it. Thanks for the report.
- validate_new_single_associated_object_on_new_parent_when_saving.diff 3.8 KB
You flagged this item as spam.
Cody Fauser March 31st, 2009 @ 09:55 PM
+1

this fixed the issue we were having with has_one saving invalid child records when we upgraded to Rails 2.3
You flagged this item as spam.
Jordan Brough April 2nd, 2009 @ 07:43 PM
Thanks, this fixed this problem for us also.
José Valim April 11th, 2009 @ 07:48 PM
Just run into this problem. The patch fixed it.

Eloy, can you get it applied?
Tobias Lütke April 27th, 2009 @ 07:24 PM
We are running this as part of Shopify since the patch was posted. +1
Repository April 27th, 2009 @ 07:50 PM
- State changed from “open” to “resolved”
(from [da3c21ead59cb47b8f4c69c6bd95f225a9c8b479]) Ensure the parent record is always saved when the child is invalid. [#2249 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...
You flagged this item as spam.
Joshua Warchol May 20th, 2009 @ 03:20 PM
Just wanted to mention that a side effect of this bug is that before_validations callbacks are not called when doing build_ followed by a save on the parent. The patch fixes this. It's a shame it's not applied to the 2.3-stable branch. Adding :validate => true to the association, using before_save, or applying this patch to frozen rails gets around that problem.

Thank you all for identifying and fixing this issue!
You flagged this item as spam.
Developers May 20th, 2009 @ 07:00 PM
It would be great if this could be applied to 2-3-stable. Like Tobi mentioned, we've been running this patch in production with Shopify since March 31.
You flagged this item as spam.
Cody Fauser May 20th, 2009 @ 07:03 PM
Previous comment was from me. I was logged in with the wrong Lighthouse account.
Repository May 20th, 2009 @ 08:21 PM
(from [a70c78177a564c2f2cd09846a5e7ab6e8669e9f2]) Ensure the parent record is always saved when the child is invalid. [#2249 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/a70c78177a564c2f2cd09846a5e7ab...