This project is archived and is in readonly mode.

#2249 ✓resolved
Akira Matsuda

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

    Eloy Duran March 16th, 2009 @ 09:12 AM

    • Assigned user set to “Eloy Duran”
  • MOROHASHI Kyosuke

    MOROHASHI Kyosuke March 16th, 2009 @ 09:14 AM

    +1

    I also think this is very serious problem.

  • Eloy Duran

    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

    Eloy Duran March 16th, 2009 @ 10:16 AM

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

  • Akira Matsuda

    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

    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

    Eloy Duran March 17th, 2009 @ 12:06 AM

    • Assigned user changed from “Eloy Duran” to “Pratik”

    This should fix it. Thanks for the report.

  • Cody Fauser

    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

  • Jordan Brough

    Jordan Brough April 2nd, 2009 @ 07:43 PM

    Thanks, this fixed this problem for us also.

  • José Valim

    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

    Tobias Lütke April 27th, 2009 @ 07:24 PM

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

  • Repository

    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...

  • Joshua Warchol

    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!

  • Developers

    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.

  • Cody Fauser

    Cody Fauser May 20th, 2009 @ 07:03 PM

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

  • Repository

    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...

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>

Attachments

Referenced by

Pages