Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Jacob Burkhart March 5th, 2009 @ 10:39 PM

  @@@ruby

  def test_respect_validations_on_association_when_autosaving

  pirate = Pirate.new(:catchphrase => "Why don't you validate my parrot?")
  pirate.parrot = Parrot.new
  
  #Parrot is not valid because name is blank
  assert !pirate.parrot.valid?
  #there is an error on name
  assert pirate.parrot.errors["name"]
  
  #parrot is still a new record... can't save it because it's invalid!
  assert pirate.parrot.new_record?
  
  #What no exception raised? but parrot is invalid!
  pirate.save!
  
  #Parrot is STILL not valid right?
  assert !pirate.parrot.valid?
  
  assert pirate.parrot.new_record?, "Parrot should still be a new record! It's invalid, so how and why did it get saved?"
  
  

  end

• 

  Michael Koziarski March 5th, 2009 @ 10:44 PM

  • Assigned user set to “Eloy Duran”
  • Milestone cleared.

• You flagged this item as spam.
  

  Jacob Burkhart March 5th, 2009 @ 11:17 PM

  The documentation (meaning comments in autosave_association.rb) seems to imply that :autosave is a feature you have to turn on by enabling :autosave => true on your association. I don't think "new_record?" should be any sort of exception to this.

  For the case of the Pirate and Parrot I've presented. I think that: * if :autosave were true on Pirate belongs_to :parrot then the pirate save should have failed with a validation error * if :autosave were false on Pirate belongs_to :parrot then the pirate save should have succeeded and skipped saving the parrot

• 

  Eloy Duran March 6th, 2009 @ 09:35 AM

  • if :autosave were true on Pirate belongs_to :parrot then the pirate save should have failed with a validation error

  I agree

  • if :autosave were false on Pirate belongs_to :parrot then the pirate save should have succeeded and skipped saving the parrot

  The documentation [1] is not very clear about what should happen.

  @Michael, do you agree that this is what you would expect if :autosave is not enabled?

  [1] http://github.com/rails/rails/bl...

• 

  José Valim March 6th, 2009 @ 10:02 AM

  Just by curiosity, even with :validate => true this happens? If I'm not mistaken, :validate => false is the default in belongs_to associations.

• 

  Eloy Duran March 7th, 2009 @ 12:19 PM

  The documentation (meaning comments in autosave_association.rb) seems to imply that :autosave is a feature you have to turn on by enabling :autosave => true on your association. I don't think "new_record?" should be any sort of exception to this.

  Well the exceptions are there because I merged all code that already existed in associations.rb into the AutosaveAssociation module. So there are definitely cases where auto-saving should happen even if :autosave isn't enabled.

• 

  Eloy Duran March 7th, 2009 @ 12:23 PM

  Just by curiosity, even with :validate => true this happens? If I'm not mistaken, :validate => false is the default in belongs_to associations.

  That's correct.

• 

  Eloy Duran March 7th, 2009 @ 12:35 PM

  • State changed from “new” to “invalid”

  Indeed, as José spotted correctly, when explicitly setting :validate => true on the belongs_to :parrot association it works as you expected it to:

  
    1) Error:
  test_respect_validations_on_association_when_autosaving(EffeTest):
  ActiveRecord::RecordInvalid: Validation failed: Parrot is invalid
      ./cases/../../lib/active_record/validations.rb:1021:in `save_without_dirty!'
      ./cases/../../lib/active_record/dirty.rb:87:in `save_without_transactions!'
      ./cases/../../lib/active_record/transactions.rb:200:in `save!'
      ./cases/../../lib/active_record/connection_adapters/abstract/database_statements.rb:136:in `transaction'
      ./cases/../../lib/active_record/transactions.rb:182:in `transaction'
      ./cases/../../lib/active_record/transactions.rb:200:in `save!'
      ./cases/../../lib/active_record/transactions.rb:208:in `rollback_active_record_state!'
      ./cases/../../lib/active_record/transactions.rb:200:in `save!'
      cases/autosave_association_test.rb:30:in `test_respect_validations_on_association_when_autosaving'
      ./cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:57:in `__send__'
      ./cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:57:in `run'
  

  Where cases/autosave_association_test.rb:30 is: pirate.save!

• 

  José Valim March 9th, 2009 @ 11:57 AM

  Eloy,

  Looks like that you are the guy looking after nested attributes. So please bear with me. There are several tickets related with nested attributes and how is validating or not validating nested objects.

  After looking to #1930, it says that the associations should be validate even if the object is invalid, but looks like that this only happens when :validate => true (as we noticed in this ticket).

  This is a problem because has_one and belongs_to have :validate => false by default and others have :validate => true.

  I think we could easily solve this by setting :validate => true when :autosave is set to true, except when :validate => false is provided, which I think that everyone expects as default behavior.

  I'd also change the fact that :validate default value changes based on the association, but this would be something very hard to accomplish, even more in a pre-release season. :)

• 

  Eloy Duran March 9th, 2009 @ 03:02 PM

  Looks like that you are the guy looking after nested attributes.

  That's correct. This is however not related to NestedAttributes, but AutosaveAssociation, but it doesn't really matter :)

  After looking to #1930, it says that the associations should be validate even if the object is invalid, but looks like that this only happens when :validate => true (as we noticed in this ticket).

  Well that's not really true, the problem with this ticket is that Parrot belongs_to Pirate. So as you noted in your first comment, the default for a non :autosave enabled association is to not validate, which works as it should.

  This is a problem because has_one and belongs_to have :validate => false by default and others have :validate => true. I think we could easily solve this by setting :validate => true when :autosave is set to true, except when :validate => false is provided, which I think that everyone expects as default behavior.

  It really should already work that way. Or rather, I did my upmost best to not break this when refactoring all old code:

  • Single association: http://github.com/rails/rails/bl...
  • Collection association: http://github.com/rails/rails/bl...

  Of course not everything was covered by extensive test coverage, so it could very well be that something has slipped through… So please do keep bug hunting! :)

  I'd also change the fact that :validate default value changes based on the association, but this would be something very hard to accomplish, even more in a pre-release season. :)

  As a matter of fact, there is even a ticket about making :autosave the default. But that would be something to discuss on the ML I think, so please do that if you'd like to suggest this.

  Thanks

• You flagged this item as spam.
  

  Jacob Burkhart March 9th, 2009 @ 03:07 PM

  Ok, so I previously outlined 2 possible behaviors:

  1. if :autosave were true on Pirate belongs_to :parrot then the pirate save should have failed with a validation error I agree

  2. if :autosave were false on Pirate belongs_to :parrot then the pirate save should have succeeded and skipped saving the parrot

  I understand that validate => true is the correct way to make the validations trigger in scenario 1. However, I still think that active record needs to provide some means of specifying the behavior in scenario 2. autosave, as documented, suggests it is a feature that provides auto-saving of related models when it is enabled on the association. However, it seems that autosave has been automatically enabled for all associations when the other end is a new_record? and there's now way to turn it off. I would really like to see some way of turning off autosave for new_record.

  Why is it that autosave_association.rb, save_belongs_to_association says:

  
            association.save(false) if association.new_record? || reflection.options[:autosave]
  

  Why isn't it just

  
            association.save(false) if reflection.options[:autosave]
  

  What's the reason for wanting autosave to always happen on new_record?

  Perhaps we could do something like

  
     unless reflection.options[:autosave] == false
            association.save(false) if association.new_record? || reflection.options[:autosave]
     end
  

  why not?

• 

  José Valim March 9th, 2009 @ 03:48 PM

  @eloy thanks, now I understand the problem better. :)

  Just more two questions:

  1. Why are you using save(false) when saving the associations?

  2. And as @jacob asked, why are you automatically saving the asssociated object in belongs_to cases if the association is a new record?

• 

  Eloy Duran March 9th, 2009 @ 04:40 PM

  association.save(false) if association.new_record? || reflection.options[:autosave]

  What's the reason for wanting autosave to always happen on new_record?

  Well the reason that AutosaveAssociation always saves when the belongs_to association is a new record is because that was the way it already was before I wrote AutosaveAssociation. See for instance the code from 2.2: http://github.com/rails/rails/bl...

  Now if you'd ask me why this was like this in the first plac,, I'd say you'd have to ask this on the ML. Because at some point somebody (DHH?) decided that that's the appropriate behaviour.

• 

  Eloy Duran March 9th, 2009 @ 04:46 PM

  Why are you using save(false) when saving the associations?

  If the user calls #save(true) on the parent, then the association(s) would already be validated as part of the parents validation callbacks. So there's no need anymore to validate the association.

  If however the user called #save(false) on the parent, then you don't want the associations to be validated.

  And because the associations will be validated already depending on whether or not the parent was validated, we don't need to pass that on to the association save.

  I know it's a bit abstract, please let me know if it was clear to you.

  And as @jacob asked, why are you automatically saving the asssociated object in belongs_to cases if the association is a new record?

  Like my earlier comment; because this has always been the case. Even long before AutosaveAssociation.

• 

  José Valim March 9th, 2009 @ 04:54 PM

  Thanks Eloy for clearing those things up!

• You flagged this item as spam.
  

  Jacob Burkhart March 9th, 2009 @ 06:10 PM

  So how about it then... can we have a new feature where if autosave is excplity set to false on the association... then skip the belongs_to autosave on new_record? Since the "always been the case" autosave was un-configurable. Now that we are providing configuration for autosave... let the configuration matter.

• 

  Eloy Duran March 9th, 2009 @ 06:27 PM

  @José No problemo :)

  So how about it then... can we have a new feature where if autosave is excplity set to false on the association... then skip the belongs_to autosave on new_record? Since the "always been the case" autosave was un-configurable. Now that we are providing configuration for autosave... let the configuration matter.

  That's indeed a possibility and shouldn't break backwards compatibility.

  @Michael: Any thoughts?

• You flagged this item as spam.
  

  Jacob Burkhart March 11th, 2009 @ 02:32 PM

  Apparently the "always been the case" autosave exists for has_one relationships too.... I would really appreciate the ability to explicitly disable autosave for specific relationships

• 

  Eloy Duran March 11th, 2009 @ 02:50 PM

  Apparently the "always been the case" autosave exists for has_one relationships too....

  That's correct.

  I would really appreciate the ability to explicitly disable autosave for specific relationships.

  Ok, then please file a new ticket with the appropriate title and a good description, or even a patch/test case, and assign it to me. Because adding it to this ticket, which is closed, isn't the right place :)

  Thanks.

• You flagged this item as spam.
  

  Jacob Burkhart March 11th, 2009 @ 07:02 PM

  Patch for new_record when autosave => false in #2214

• You flagged this item as spam.
  

  csnk May 18th, 2011 @ 08:22 AM

  We are the professional coats manufacturer, coats supplier, coats factory, custom coats.