Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

autosaving associations skips validations and persists invalid data in the DB

#2249

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?

Reported by Akira Matsuda · March 16th, 2009 @ 08:37 AM

State: resolved
Milestone: 2.x
Assigned to: Pratik Pratik
Importance: none

Activity

  1. Eloy Duran
    Eloy Duran
    • Assigned user set to Eloy Duran

    March 16th, 2009 @ 09:12 AM

  2. MOROHASHI Kyosuke
    MOROHASHI Kyosuke

    +1

    I also think this is very serious problem.

    March 16th, 2009 @ 09:14 AM

  3. Eloy Duran
    Eloy Duran
    • 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

    March 16th, 2009 @ 10:15 AM

  4. Eloy Duran
    Eloy Duran

    Btw: The source for AutosaveAssociation can be viewed here: http://github.com/rails/rails/bl...

    March 16th, 2009 @ 10:16 AM

  5. Akira Matsuda
    Akira Matsuda

    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?

    March 16th, 2009 @ 11:20 AM

  6. Eloy Duran
    Eloy Duran
    • 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

    March 16th, 2009 @ 01:08 PM

  7. Eloy Duran
    Eloy Duran
    • Assigned user changed from Eloy Duran to Pratik

    This should fix it. Thanks for the report.

    March 17th, 2009 @ 12:06 AM

  8. Cody Fauser
    Cody Fauser

    +1

    this fixed the issue we were having with has_one saving invalid child records when we upgraded to Rails 2.3

    March 31st, 2009 @ 09:55 PM

  9. Jordan Brough
    Jordan Brough

    Thanks, this fixed this problem for us also.

    April 2nd, 2009 @ 07:43 PM

  10. José Valim
    José Valim

    Just run into this problem. The patch fixed it.

    Eloy, can you get it applied?

    April 11th, 2009 @ 07:48 PM

  11. Tobias Lütke
    Tobias Lütke

    We are running this as part of Shopify since the patch was posted. +1

    April 27th, 2009 @ 07:24 PM

  12. Repository
    Repository
    • State changed from open to resolved

    (from [da3c21ead59cb47b8f4c69c6bd95f225a9c8b479]) Ensure the parent record is always saved when the child is invalid. [#2249 autosaving associations skips validations and persists invalid data in the DB state:resolved]

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

    April 27th, 2009 @ 07:50 PM

  13. Joshua Warchol
    Joshua Warchol

    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!

    May 20th, 2009 @ 03:20 PM

  14. Developers
    Developers

    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.

    May 20th, 2009 @ 07:00 PM

  15. Cody Fauser
    Cody Fauser

    Previous comment was from me. I was logged in with the wrong Lighthouse account.

    May 20th, 2009 @ 07:03 PM

  16. Repository
    Repository

    May 20th, 2009 @ 08:21 PM