Delete nodes from leafs up to root

[kbrock]

When destroying dependents, start with the furthest down leafs and work your way up to the current node

#90

@miguelgraz @brendon You still using ancestry? Do you remember if this is the solution you used?

This seems like a good change, but want some feedback in terms of what unintended side effects will be introduced.
Current destroy does not specify an order. The database will tend towards insert order, and that tends to be parent down to children.

[brendon]

This is my current strategy:

    # Unfortunately ancestry deletes by calling descendants and deleting in an arbitrary order, and belongs_to
    # :dependent => :destroy destroys the component_instance before the instance thus we override the orphan
    # strategy below:
    define_method :apply_orphan_strategy do
      unless new_record?
        children.all.each { |child| child.destroy }
        instance.destroy
      end
    end

It's slow because it steps through the tree deleting one at a time. I probably cut some corners there as I determined that some checks weren't relevant in my situation.

[kbrock]

1. Thanks for pointer on reverse. I will change to reverse_order and go from there.
2. So you basically do an N+1 for deletes. heh. thought you'd opt for arrange or something ;)
3. How did you get your define_method into there? Is this a common mixin?

[brendon]

Oh sorry, yes #1 looks good, #2 yes not too efficient I'm sure. I'm not usually dealing with a lot of nodes when deleting though. #3 sorry that's because this is coming from a Rails Concern so that's why.

[kbrock]

re 2: what ever works and is the easiest to support
re 3: No. I was glad you had something interesting there. I'm trying to define the patterns for extending the apply_orphan_strategy

I wanted to make sure in #632 that I handle most of the use cases around orphan_strategies

I'm trying to go more towards the dynamic route.
and doing something more like this

[brendon]

Haha, That code in ActsAsList! That was done by a contractor I had in the past, it drives me nuts but what's done is done :D

Personally I don't like the way it abstracts the configuration process away into many files. It's difficult to grok, but I get why one would want to clean it up that way.

Ideally if you cover off most overridden orphan strategies then there won't be a need to override them anymore. Perhaps in #632 you could adjust the code to allow one to use one of your built-in strategies or to nominate one of their own via a symbol that calls a method on the model. Essentially you'd have a constant with a hash of built in orphan strategy names as keys with the values being the method names, then you'd just check if the orphan strategy is one of those set a before_destroy to the associated method, or if it doesn't exist, just make a before_destroy to the literal symbol value that was passed in. You could then still enforce that something is set, but it then allows people to use their own methods without any hoops to jump through.

The main problem with that is that a typo would lead to the code trying to call a non-existent method on destroy which could be confusing. You could instead have two config settings, orphan_stratgegy: (one of the built in ones, or :custom), then custom_orphan_strategy_method: (the symbol of the method, and this gets enforced if the strategy is :custom).

Just some thoughts an away :D

[kbrock]

Personally I don't like the way it abstracts the configuration process away into many files. It's difficult to grok, but I get why one would want to clean it up that way.

Thanks, good point. I will take that into account
The way attributes are passed in does make that very complicated and feels a little over engineered.
But I like the idea of adding more modules, one for each of the various strategies. Was thinking about putting all (not each) orphan strategies into a single module. But yea, a comment in front of the 5 methods may work just as well.

---

I'm glad your ideas around :orphan_strategy are similar to mine.
I was thinking of adding :none and just letting people add their own before_destroy callback, but I wrote it off. Now I'm seriously considering it. Implementation couldn't be any simpler.

I'm concerned around the current code defining a method inline. If you added your concern before running has_anestry it will ignore your code. But maybe the original code has the same issue.

---

Thanks again for the great input. Ping any time you need help with acts_as_list or others.

[brendon]

Thanks @kbrock, will do! :) I actually had a stab at writing my own positioning Concern and it's working quite well. It works more like acts_as_list but has much more focus on correct and failproof reordering within transactions so that a consistent state is maintained without gaps between items. For now I'm completely flat out so can only focus on reviewing other's PR's.

:none could be a great option as long as there's documentation saying what to do next. Enforcing the name of a custom callback could help make the knife less sharp?

[kbrock]

update:

• rebased

[brendon]

Just wondering if this needs a test case to confirm that it fixes the problem? Hard to re-create though.

[kbrock]

When we don't specify the order, it most likely would bring back the data in id order, depending upon the index that the database uses. And we typically create parents then children. So the parents have an id lower / earlier in the indexes (and definitely in the ancestry indexes), so it was probably always deleting from root to child.

Not sure how to write a red-green tests here.
Maybe just have a before_destroy that verifies that the parent record still exists?
Yea, I think I'm good with the non-test here.

[brendon]

I think an after_destroy would do the trick. Something that tries to manipulate the parent as you say :) Certainly it would show that it works now whereas before it was non-deterministic.