Hi all, There was a question recently on the Rails ML that asked for basically a database restrict. If an object has_many somethings and there are children somethings, don''t allow that object to be destroyed. I have a couple of questions about this. 1. Is having this as an option for :dependent a good idea? 2. Assuming that it is a reasonable thing to have, how do I go about a. writing a test for the patch b. submitting a patch, (I''m on windows) I have found the code in active record to make the change, downloaded the source and got the standard tests working, but I have no idea of what test to write to confirm it works. Do I write a seperate class with the option included? I really have no idea, very noob at this. Any pointers would be appreciated. Cheers Daniel _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
> 2. Assuming that it is a reasonable thing to have, how do I go about > a. writing a test for the patchYou mean that as "where do I put my tests?" or "what do I test for?" If the former, look around the existing tests, see if it fits within any, otherwise create a new test file. If the latter, create a record with associated records and one without them. Try to destroy each. Test that one raised an error (or returned false, depends on how you implement it), and that the other was destroyed.> b. submitting a patch, (I''m on windows)Can you redirect output in the windows shell? I think so, not sure. Anyway, get the svn command line tools, open cmd, go to the rails working copy where you made the changes, and type: svn diff > myfile.diff Then attach the generated file to a ticket in trac (which probably is not working yet, so just post it here) You should make the change on a subversion working copy of the rails source, not on an unversioned copy. Also, if you add any new files, be sure to `svn add` them.> Do I write a seperate class with the option included? I really > have no idea, very noob at this.You mean for the actual change or the tests? change: You should probably alter the code that handles the dependent attribute in place. tests: see above. If you''re not sure about the quality of the final patch, post it here and we''ll go over it.
Hi Caio thanx for the response. Pls see below. On 7/23/06, Caio Chassot <lists@v2studio.com> wrote:> > > 2. Assuming that it is a reasonable thing to have, how do I go about > > a. writing a test for the patch > > You mean that as "where do I put my tests?" or "what do I test for?" > > If the former, look around the existing tests, see if it fits within > any, otherwise create a new test file. If the latter, create a record > with associated records and one without them. Try to destroy each. > Test that one raised an error (or returned false, depends on how you > implement it), and that the other was destroyed.If I create a new record with associated records, to me that means that I need a new two new class definitions and some fixtures. Is this correct?> b. submitting a patch, (I''m on windows) > > Can you redirect output in the windows shell? I think so, not sure. > Anyway, get the svn command line tools, open cmd, go to the rails > working copy where you made the changes, and type: > > svn diff > myfile.diffThanx that''s what I was looking for for generating a diff on windows. Then attach the generated file to a ticket in trac (which probably is> not working yet, so just post it here) > > You should make the change on a subversion working copy of the rails > source, not on an unversioned copy. Also, if you add any new files, > be sure to `svn add` them. > > > Do I write a seperate class with the option included? I really > > have no idea, very noob at this. > > You mean for the actual change or the tests? > change: You should probably alter the code that handles the dependent > attribute in place. > tests: see above.For the tests. The actual change is one line. (see below) If you''re not sure about the quality of the final patch, post it here> and we''ll go over it. > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >The actual code to include for the patch is in http://dev.rubyonrails.org/svn/rails/trunk/activerecord/lib/active_record/associations.rb and is def configure_dependency_for_has_many(reflection) if reflection.options[:dependent] && reflection.options[:exclusively_dependent] raise ArgumentError, '':dependent and :exclusively_dependent are mutually exclusive options. You may specify one or the other.'' end if reflection.options[:exclusively_dependent] reflection.options[:dependent] = :delete_all #warn "The :exclusively_dependent option is deprecated. Please use :dependent => :delete_all instead.") end # See HasManyAssociation#delete_records. Dependent associations # delete children, otherwise foreign key is set to NULL. # Add polymorphic type if the :as option is present dependent_conditions = %(#{reflection.primary_key_name} \#{record.quoted_id}) if reflection.options[:as] dependent_conditions += " AND #{reflection.options[:as]}_type = ''#{base_class.name}''" end case reflection.options[:dependent] when :destroy, true module_eval "before_destroy ''#{reflection.name}.each { |o| o.destroy }''" when :delete_all module_eval "before_destroy { |record| #{reflection.class_name}.delete_all(%(#{dependent_conditions})) }" when :nullify module_eval "before_destroy { |record| #{reflection.class_name}.update_all(%(#{reflection.primary_key_name} NULL), %(#{dependent_conditions})) }" when :restrict module_eval "before_destroy { |record| #{reflection.name}.size == 0 }" when nil, false # pass else raise ArgumentError, ''The :dependent option expects either :destroy, :delete_all, or :nullify'' end end Basically I''m just halting the destroy option if there are no children. There will need to be a similar line in the has_one method. def configure_dependency_for_has_one(reflection) case reflection.options[:dependent] when :destroy, true module_eval "before_destroy ''#{reflection.name}.destroy unless #{reflection.name}.nil?''" when :nullify module_eval "before_destroy ''#{reflection.name}.update_attribute(\"#{reflection.primary_key_name}\", nil)''" when :restrict module_eval "before_destroy { |record| #{reflection.name}.size == 0 }" when nil, false # pass else raise ArgumentError, "The :dependent option expects either :destroy or :nullify." end So then do I just write a couple of classes, matching fixtures and a test file for this? Thanx for your help _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 2006-07-23, at 01:01 , Daniel N wrote:> > If I create a new record with associated records, to me that means > that I need a new two new class definitions and some fixtures. Is > this correct?If you can work with the existing fixtures and models, better. Otherwise create your own.> raise ArgumentError, "The :dependent option expects either :destroy > or : nullify."You''ll want to change this error message too.> So then do I just write a couple of classes, matching fixtures and > a test file for this? >Yep. And do post a patch, not just altered code.
On 7/23/06, Caio Chassot <lists@v2studio.com> wrote:> > > On 2006-07-23, at 01:01 , Daniel N wrote: > > > > If I create a new record with associated records, to me that means > > that I need a new two new class definitions and some fixtures. Is > > this correct? > > If you can work with the existing fixtures and models, better. > Otherwise create your own. > > > raise ArgumentError, "The :dependent option expects either :destroy > > or : nullify." > > You''ll want to change this error message too. > > > So then do I just write a couple of classes, matching fixtures and > > a test file for this? > > > > Yep. And do post a patch, not just altered code. > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-coreCaio, Thanx so much for your help. This has helped a lot. I wrote the change and posted a patch but the mailman decided that it was too big and needed admin approval. I went down the road of writing new classes and fixtures, adding into the db schemas etc. Worked a treat, all tests passed. Since I posted that patch tho I have been thinking. What would happen in this situation. class MyClass < ActiveRecord has_many :things, :dependent => :destroy_all has_many :important_things, :dependent => :restrict end In this case if I have a var @my_class and call @my_class.destroy, during it''s before_destroy: 1. it will look at the things and see that it has :destroy_all and go ahead and destroy them. 2. time goes on and it gets to the before_destroy for important_things and halts the destroy action because it has some imporant_things. Does this mean that things are destroyed or is the whole thing wrapped up in a transaction that gets rolled back if the destroy as a whole fails? If it partially deletes some things, then halts when it reaches an association with the restrict option I don''t think this is robust. Again, Thanx for your time on this Caio, I really appreciate it. Cheers Dan _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
> Since I posted that patch tho I have been thinking. What would > happen in this situation. > > class MyClass < ActiveRecord > has_many :things, :dependent => :destroy_all > has_many :important_things, :dependent => :restrict > end > > Does this mean that things are destroyed or is the whole thing > wrapped up in a transaction that gets rolled back if the destroy as > a whole fails? > > If it partially deletes some things, then halts when it reaches an > association with the restrict option I don''t think this is robust.The behavior is really up to you, isn''t it? I''d say write a test case for mixed dependent types, and make sure you get the rollback you desire.
On 7/25/06, Caio Chassot <lists@v2studio.com> wrote:> > > Since I posted that patch tho I have been thinking. What would > > happen in this situation. > > > > class MyClass < ActiveRecord > > has_many :things, :dependent => :destroy_all > > has_many :important_things, :dependent => :restrict > > end > > > > Does this mean that things are destroyed or is the whole thing > > wrapped up in a transaction that gets rolled back if the destroy as > > a whole fails? > > > > If it partially deletes some things, then halts when it reaches an > > association with the restrict option I don''t think this is robust. > > The behavior is really up to you, isn''t it? > > I''d say write a test case for mixed dependent types, and make sure > you get the rollback you desire. > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-coreI''ve had a crack at this one and it is as I feared. If I define the restrict association first, then the destroy chain is halted and all is well. But if I have an association where a :dependent => :destroy is declared before the association with :restrict then the first association is destroyed before the destroy chain is halted. I have taken my first real plunge into the rails source and found a number of places where I thought a transaction might go to prevent this but to no avail. Actually my head is spinning a bit trying to put all the pieces of active_record together. For this to work I think I need to put a transaction around the entire destroy chain, but I have no idea how to do this. Any pointers anyone could give would be great. Thanx Dan _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On Jul 26, 2006, at 6:06 AM, Daniel N wrote:> I''ve had a crack at this one and it is as I feared. > > If I define the restrict association first, then the destroy chain > is halted and all is well. But if I have an association where > a :dependent => :destroy is declared before the association > with :restrict then the first association is destroyed before the > destroy chain is halted.You can wrap the destroy method to leapfrog the callback chain. associations.rb, configure_dependency_* case reflection.options[:dependent] when :restrict class_eval <<-end_eval def destroy_with_has_many_#{reflection.name} unless #{reflection.name}.blank? raise DestroyRestricted.new(self, # {reflection.name.inspect}) end end alias_method_chain :destroy, "has_many_#{reflection.name}" end_eval # ... end base.rb module ActiveRecord class DestroyRestricted < ActiveRecordError def initialize(model, by) super "#{model.class.name} #{model.id} destroy restricted by #{by}" end end end> I have taken my first real plunge into the rails source and found a > number of places where I thought a transaction might go to prevent > this but to no avail. Actually my head is spinning a bit trying to > put all the pieces of active_record together. > > For this to work I think I need to put a transaction around the > entire destroy chain, but I have no idea how to do this. > > Any pointers anyone could give would be great.destroy and its callbacks are wrapped in a single transaction (see transactions.rb) so you can raise an exception to rollback. example: class ParentController < ApplicationController def destroy @parent.destroy redirect_to parent_url(@parent) rescue ActiveRecord::DestroyRestricted => restricted flash[:error] = restricted.message redirect_to :back end end Best, jeremy
On 7/27/06, Jeremy Kemper <jeremy@bitsweat.net> wrote:> > On Jul 26, 2006, at 6:06 AM, Daniel N wrote: > > I''ve had a crack at this one and it is as I feared. > > > > If I define the restrict association first, then the destroy chain > > is halted and all is well. But if I have an association where > > a :dependent => :destroy is declared before the association > > with :restrict then the first association is destroyed before the > > destroy chain is halted. > > You can wrap the destroy method to leapfrog the callback chain. > > associations.rb, configure_dependency_* > > case reflection.options[:dependent] > when :restrict > class_eval <<-end_eval > def destroy_with_has_many_#{reflection.name} > unless #{reflection.name}.blank? > raise DestroyRestricted.new(self, # > {reflection.name.inspect}) > end > end > > alias_method_chain :destroy, "has_many_#{reflection.name}" > end_eval > # ... > end > > > base.rb > > module ActiveRecord > class DestroyRestricted < ActiveRecordError > def initialize(model, by) > super "#{model.class.name} #{model.id} destroy restricted by > #{by}" > end > end > end > > > > I have taken my first real plunge into the rails source and found a > > number of places where I thought a transaction might go to prevent > > this but to no avail. Actually my head is spinning a bit trying to > > put all the pieces of active_record together. > > > > For this to work I think I need to put a transaction around the > > entire destroy chain, but I have no idea how to do this. > > > > Any pointers anyone could give would be great. > > destroy and its callbacks are wrapped in a single transaction (see > transactions.rb) so you can raise an exception to rollback. > > > example: > > class ParentController < ApplicationController > def destroy > @parent.destroy > redirect_to parent_url(@parent) > rescue ActiveRecord::DestroyRestricted => restricted > flash[:error] = restricted.message > redirect_to :back > end > end > > > Best, > jeremy > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >Thanx Jeremy. This looks very different to what I had in mind. Lots better :) I''ll get into trying to get this to work. Just a thought tho. By raising an exception, this would put destroy into a different category, this should perhaps be destroy! to fit with the other methods that raise errors. Would including an exception into the destory method mean that it would break exisitng code? I guess you would only need to use a rescue if you have declared a relationship as rectricted. I''ll have a go anyway and see what ppl think. Thanx for you help Cheers _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On Jul 28, 2006, at 8:29 PM, Daniel N wrote:> Just a thought tho. By raising an exception, this would put > destroy into a different category, this should perhaps be destroy! > to fit with the other methods that raise errors.Good call. Really, destroy_with_callbacks should rollback the transaction when a before_destroy callback is false rather than returning false and committing. Then the implementation simplifies back to case reflection.options[:dependent] when :restrict class_eval "before_destroy { |model| model.# {reflection.name}.blank? }" jeremy
On Saturday, July 29, 2006, at 1:29 PM, Daniel N wrote:>On 7/27/06, Jeremy Kemper <jeremy@bitsweat.net> wrote: >> >> On Jul 26, 2006, at 6:06 AM, Daniel N wrote: >> > I''ve had a crack at this one and it is as I feared. >> > >> > If I define the restrict association first, then the destroy chain >> > is halted and all is well. But if I have an association where >> > a :dependent => :destroy is declared before the association >> > with :restrict then the first association is destroyed before the >> > destroy chain is halted. >> >> You can wrap the destroy method to leapfrog the callback chain. >> >> associations.rb, configure_dependency_* >> >> case reflection.options[:dependent] >> when :restrict >> class_eval <<-end_eval >> def destroy_with_has_many_#{reflection.name} >> unless #{reflection.name}.blank? >> raise DestroyRestricted.new(self, # >> {reflection.name.inspect}) >> end >> end >> >> alias_method_chain :destroy, "has_many_#{reflection.name}" >> end_eval >> # ... >> end >> >> >> base.rb >> >> module ActiveRecord >> class DestroyRestricted < ActiveRecordError >> def initialize(model, by) >> super "#{model.class.name} #{model.id} destroy restricted by >> #{by}" >> end >> end >> end >> >> >> > I have taken my first real plunge into the rails source and found a >> > number of places where I thought a transaction might go to prevent >> > this but to no avail. Actually my head is spinning a bit trying to >> > put all the pieces of active_record together. >> > >> > For this to work I think I need to put a transaction around the >> > entire destroy chain, but I have no idea how to do this. >> > >> > Any pointers anyone could give would be great. >> >> destroy and its callbacks are wrapped in a single transaction (see >> transactions.rb) so you can raise an exception to rollback. >> >> >> example: >> >> class ParentController < ApplicationController >> def destroy >> @parent.destroy >> redirect_to parent_url(@parent) >> rescue ActiveRecord::DestroyRestricted => restricted >> flash[:error] = restricted.message >> redirect_to :back >> end >> end >> >> >> Best, >> jeremy >> _______________________________________________ >> Rails-core mailing list >> Rails-core@lists.rubyonrails.org >> http://lists.rubyonrails.org/mailman/listinfo/rails-core >> > >Thanx Jeremy. This looks very different to what I had in mind. Lots >better :) > >I''ll get into trying to get this to work. > >Just a thought tho. By raising an exception, this would put destroy into a >different category, this should perhaps be destroy! to fit with the other >methods that raise errors. > >Would including an exception into the destory method mean that it would >break exisitng code? I guess you would only need to use a rescue if you >have declared a relationship as rectricted. I''ll have a go anyway and see >what ppl think. > >Thanx for you help > >Cheers > > >_______________________________________________ >Rails-core mailing list >Rails-core@lists.rubyonrails.org >http://lists.rubyonrails.org/mailman/listinfo/rails-core >Wouldn''t it make sense to have it call a function like ''can_destroy?'' before trying to do the work? Have that function recursively call itself on all the child objects, and then only return true if all dependent objects can be destroyed. If can_destroy? returns false, then... don''t destroy it. It might be a little more robust than relying on exceptions, since it will test everything before trying to destroy anything. _Kevin www.sciwerks.com -- Posted with http://DevLists.com. Sign up and save your mailbox.
On 7/30/06, Jeremy Kemper <jeremy@bitsweat.net> wrote:> > On Jul 28, 2006, at 8:29 PM, Daniel N wrote: > > Just a thought tho. By raising an exception, this would put > > destroy into a different category, this should perhaps be destroy! > > to fit with the other methods that raise errors. > > Good call. Really, destroy_with_callbacks should rollback the > transaction when a before_destroy callback is false rather than > returning false and committing. Then the implementation simplifies > back to > > case reflection.options[:dependent] > when :restrict > class_eval "before_destroy { |model| model.# > {reflection.name}.blank? }" > > jeremy > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >This is very similar to the solution that I came up with initially. Thinking that a return of false would roll back the destroy. case reflection.options[:dependent] when :restrict module_eval "before_destroy ''#{reflection.name}.count == 0''" As I understand it class_eval is an alias for module_eval so I think that the code you suggested and what I had were basically doing the same thing. This approach however didn''t work in the tests. When I had a has_many declared with :dependent => :destroy before the has_many with :restrict the first associations objects were destroyed before the destroy chain was halted. It didn''t seem to roll back in the tests The test that I had was num_association_1 = @obj.association_1.count num_associaton_2 = @obj.association_2.count @obj.destroy assert !@obj.frozen? assert @obj.association_1.count == num_association_1 # <= Test fails here assert @obj.association_2.count == num_association_2 The @obj was not frozen but association_1 no longer had the objects in it. I really don''t know how to proceed from here. It surely isnt'' as hard as I''ve made it, I''m positive I''m doing something not quite right. I just don''t understand why the destroy isn''t rolled right back to the start. I then took another approach of including a class variable and adding any restricted associations to it. Then on each call to before_destroy, I went through each restriced association and checked returning false if a restricted association was found. Trying a little overkill really. This broke a lot of existing tests in AR. This is a lot harder than I thought it was going to be. Thanx so much for your patience with this and for your help. Cheers Daniel _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 29 Jul 2006 20:04:41 -0000, Kevin Olbrich < devlists-rails-core@devlists.com> wrote:> > > On Saturday, July 29, 2006, at 1:29 PM, Daniel N wrote: > >On 7/27/06, Jeremy Kemper <jeremy@bitsweat.net> wrote: > >> > >> On Jul 26, 2006, at 6:06 AM, Daniel N wrote: > >> > I''ve had a crack at this one and it is as I feared. > >> > > >> > If I define the restrict association first, then the destroy chain > >> > is halted and all is well. But if I have an association where > >> > a :dependent => :destroy is declared before the association > >> > with :restrict then the first association is destroyed before the > >> > destroy chain is halted. > >> > >> You can wrap the destroy method to leapfrog the callback chain. > >> > >> associations.rb, configure_dependency_* > >> > >> case reflection.options[:dependent] > >> when :restrict > >> class_eval <<-end_eval > >> def destroy_with_has_many_#{reflection.name} > >> unless #{reflection.name}.blank? > >> raise DestroyRestricted.new(self, # > >> {reflection.name.inspect}) > >> end > >> end > >> > >> alias_method_chain :destroy, "has_many_#{reflection.name}" > >> end_eval > >> # ... > >> end > >> > >> > >> base.rb > >> > >> module ActiveRecord > >> class DestroyRestricted < ActiveRecordError > >> def initialize(model, by) > >> super "#{model.class.name} #{model.id} destroy restricted by > >> #{by}" > >> end > >> end > >> end > >> > >> > >> > I have taken my first real plunge into the rails source and found a > >> > number of places where I thought a transaction might go to prevent > >> > this but to no avail. Actually my head is spinning a bit trying to > >> > put all the pieces of active_record together. > >> > > >> > For this to work I think I need to put a transaction around the > >> > entire destroy chain, but I have no idea how to do this. > >> > > >> > Any pointers anyone could give would be great. > >> > >> destroy and its callbacks are wrapped in a single transaction (see > >> transactions.rb) so you can raise an exception to rollback. > >> > >> > >> example: > >> > >> class ParentController < ApplicationController > >> def destroy > >> @parent.destroy > >> redirect_to parent_url(@parent) > >> rescue ActiveRecord::DestroyRestricted => restricted > >> flash[:error] = restricted.message > >> redirect_to :back > >> end > >> end > >> > >> > >> Best, > >> jeremy > >> _______________________________________________ > >> Rails-core mailing list > >> Rails-core@lists.rubyonrails.org > >> http://lists.rubyonrails.org/mailman/listinfo/rails-core > >> > > > >Thanx Jeremy. This looks very different to what I had in mind. Lots > >better :) > > > >I''ll get into trying to get this to work. > > > >Just a thought tho. By raising an exception, this would put destroy into > a > >different category, this should perhaps be destroy! to fit with the other > >methods that raise errors. > > > >Would including an exception into the destory method mean that it would > >break exisitng code? I guess you would only need to use a rescue if you > >have declared a relationship as rectricted. I''ll have a go anyway and > see > >what ppl think. > > > >Thanx for you help > > > >Cheers > > > > > >_______________________________________________ > >Rails-core mailing list > >Rails-core@lists.rubyonrails.org > >http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > Wouldn''t it make sense to have it call a function like ''can_destroy?'' > before trying to do the work? Have that function recursively call > itself on all the child objects, and then only return true if all > dependent objects can be destroyed. > > If can_destroy? returns false, then... don''t destroy it. > > It might be a little more robust than relying on exceptions, since it > will test everything before trying to destroy anything. > > > _Kevin > www.sciwerks.com > > -- > Posted with http://DevLists.com. Sign up and save your mailbox. > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >Thanx for you help with this guys. I thought it was about time I posted something for some feedback. (And since I''m not sure about posting a patch to the rails site!) I''ve attached the diff file for active_record with :dependent => :restrict I ended up taking the suggestions and putting them together. There is a restricted? instance method for AR. This check first order associations only. I thought about having it check nth order associations but this could be huge. To do this all objects from every association, and associations associations would need to be loaded potentially. Or at least until a restricted one is detected. Even if this goes no further than this it has been good fun. Understanding alias_method_chain was worth it all by itself. Thanx again for you help. Please if you get the chance have a look. I hope it''s not too terrible. Cheers Daniel _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core