While documenting stuff related to transactions and callbacks I''ve realized that if a hook raises precisely AR::Rollback save! silently returns nil. I''ve documented that behaviour, but it surprised to me that save! may not raise an exception on failure. Looks like a side effect of the implementation in fact more than something intended. What do you think? --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Aug 14, 3:01 am, "Xavier Noria" <f...@hashref.com> wrote:> While documenting stuff related to transactions and callbacks I''ve > realized that if a hook raises precisely AR::Rollback save! silently > returns nil. I''ve documented that behaviour, but it surprised to me > that save! may not raise an exception on failure. Looks like a side > effect of the implementation in fact more than something intended. > > What do you think?I think this behavior is correct: AR::Rollback is never supposed to be re-raised. That said, this behavior should be explicitly documented. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Thu, Aug 14, 2008 at 11:47 AM, Hongli Lai <honglilai@gmail.com> wrote:> I think this behavior is correct: AR::Rollback is never supposed to be > re-raised. That said, this behavior should be explicitly documented.What puzzles me is the current usage: begin if model.save! # success else # faillure due to AR::Rollback end rescue AR::RecordNotSaved # failure due to something else end Isn''t that a strange pattern for save!? --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Thu, Aug 14, 2008 at 12:03 PM, Xavier Noria <fxn@hashref.com> wrote:> > On Thu, Aug 14, 2008 at 11:47 AM, Hongli Lai <honglilai@gmail.com> wrote: > >> I think this behavior is correct: AR::Rollback is never supposed to be >> re-raised. That said, this behavior should be explicitly documented. > > What puzzles me is the current usage: > > begin > if model.save! > # success > else > # faillure due to AR::Rollback > end > rescue AR::RecordNotSaved > # failure due to something else > end > > Isn''t that a strange pattern for save!?Yes imho it is. Since AR::Rollback represents in essence a reason because record is not saved I can''t see what would be the reason to be treated a different way. -- Kind Regards, Aitor Garcia Cofounder - Linking Paths http://www.linkingpaths.com --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
I am not enterily sold on this option but we could manually raise AR::RecordNotFound on AR::Rollback inside save! and of course document it. Not sure about the implementation because silencing AR::Rollback is done deep in database_statements.rb but I am just thinking about the usage by now. That would give an idiomatic way to cancel + rollback that in turn results in expected client code. Another option would be to document the current behaviour (this is already done) and warn people about the resulting if/else. You can still deal with the rollback in a rescue clause raising something different from AR::Rollback begin model.save! # success rescue AR::RecordNotSaved, MyApp::SomeError => e # failure end --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Thu, Aug 14, 2008 at 12:34 PM, Xavier Noria <fxn@hashref.com> wrote:> I am not enterily sold on this option but we could manually raise > AR::RecordNotFoundAR::RecordNotSaved --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> Isn''t that a strange pattern for save!?Yeah, it is, but I''m not sure that people should be raising AR::Rollback themselves anyway, it''s intended as a way to abort the transaction and continue processing, and if you''re not using it for that, then you should be raising some other exception :) -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, Aug 22, 2008 at 8:27 AM, Michael Koziarski <michael@koziarski.com> wrote:>> Isn''t that a strange pattern for save!? > > Yeah, it is, but I''m not sure that people should be raising > AR::Rollback themselves anyway, it''s intended as a way to abort the > transaction and continue processing, and if you''re not using it for > that, then you should be raising some other exception :)Good. If people raise their own exception to rollback, then I think the strange pattern is found in non-bang save: begin if save # success else # validation error end rescue MyRollBack # failure due to rollback end Perhaps the message could be that if you may rollback in the callback chain you normally would only use the save! method? --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
In fact, I wonder why halting the chain in before_* callbacks commits the transaction. Wouldn''t you expect a rollback? If AR executed a rollback automatically, in addition, save and save! would preserve their usage idioms. That would still leave room for best practices for rollbacks in after_* hooks, but I think that would be fine. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, Aug 22, 2008 at 1:47 PM, Xavier Noria <fxn@hashref.com> wrote:> > In fact, I wonder why halting the chain in before_* callbacks commits > the transaction. Wouldn''t you expect a rollback? If AR executed a > rollback automatically, in addition, save and save! would preserve > their usage idioms.That does sound more ... sensible. Why don''t you give it a try in a branch and see what breaks?> That would still leave room for best practices for rollbacks in > after_* hooks, but I think that would be fine. > > > >-- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, Aug 22, 2008 at 2:22 PM, Michael Koziarski <michael@koziarski.com> wrote:>> In fact, I wonder why halting the chain in before_* callbacks commits >> the transaction. Wouldn''t you expect a rollback? If AR executed a >> rollback automatically, in addition, save and save! would preserve >> their usage idioms. > > That does sound more ... sensible. Why don''t you give it a try in a > branch and see what breaks?Absolutely! I''ll write back with something. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, Aug 22, 2008 at 2:49 PM, Xavier Noria <fxn@hashref.com> wrote:> On Fri, Aug 22, 2008 at 2:22 PM, Michael Koziarski > <michael@koziarski.com> wrote: > >>> In fact, I wonder why halting the chain in before_* callbacks commits >>> the transaction. Wouldn''t you expect a rollback? If AR executed a >>> rollback automatically, in addition, save and save! would preserve >>> their usage idioms. >> >> That does sound more ... sensible. Why don''t you give it a try in a >> branch and see what breaks? > > Absolutely! I''ll write back with something.Here we go! http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/891-let-cancels-from-before-filters-issue-a-rollback --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---