[Dbix-class] RFC: DBIx-Class-Tree

[Aran Deltac]

On 4/4/2006, "Dave Howorth" <dhoworth at mrc-lmb.cam.ac.uk> wrote:

>Aran Deltac wrote:
>> Hi all, I've been hacking away at a set of components for representing,
>> modifying, and traversing trees of DBIx::Class objects.  The API was
>> hammered out with some help from Matt and others on #dbix-class.
>>
>> Before a first release is ever made I wanted to give everyone some time
>> to take a look at the first of the Tree modules,
>> DBIC:Tree::AdjacencyList and DBIC:Tree::AdjacencyList::Ordered.  I have
>> plans to add Nested Set type trees (and other lesser known types) and
>> provide a way to traverse the trees in a similar fashion as
>> Tree::Simple::Visitor does it.  I'm hoping to have a final API that can
>> be consistent across all Tree types, so you can plug-n-play Tree
>> components as your needs change.
>
>I don't use DBIC yet - I'll likely change from CDBI next time I revisit
>my model code - but I do use trees, so I was interested to see this.
>
>AdjacencyList
>
>parent - what does the code actually do if you try to make an object the
>root? The Description says the root is the 'row with a Parent ID of 0'
>but I can't see how that would ever get set.

Its up to you.  For example, if you parent column is parent_id then you
could do $obj->parent_id( 0 ); or $obj->parent( 0 ), or if you're
creating a new object you could either specify it in the ->create({
parent_id=>0 }) or in the table definition as in "parent_id INTEGER NOT
NULL DEFAULT 0".

I've added a small comment to this effect under the parent() method.

>method return values - the docs don't state the return values. Since
>accessor/mutators normally always return the current attribute value,
>the fact that these don't really needs to be made VERY clear. BTW,

Good point - I've modified the docs to include this information where I
had missed putting it in.

>what's the rationale for the values that are returned?

Return 1 on success and 0 on minor errors, throw exceptions (croak) on
harder errors.

>attach_child - DBIC follows the CDBI convention of add_to_*. So why not
>call this method add_to_children? The description could also be clearer.

Because Matt asked me to follow the DOM naming conventions, which I think
I have to the best of my ability.  Some of the method names I've chosen
are not directly from the DOM methods, but are heavly influenced by them.

>The method could also take a list of new children.
>attach_sibling - same issue with name and possible list of arguments.

Done - both methods now accept more than one object.

>Ordered
>
>position_column & parent_column - since they both have to be called
>exactly once in a specific order, it might be worth considering
>combining them into a single method.

That could be.  No good name pops right off the top of my head, so I'll
let this one lie - it can be tacked on any time.

>Is the ordering really restricted to a single column rather than some
>predicate? Could that be relaxed?

It could be relaxed.  But I'm not sure exactly how best to handle it. 
Its a good idea - I'll keep it in mind.

>position_column - this method is not listed.

Its in the Ordered component.  Any method that exist in
Tree::AdjacencyList or Ordered components is not shown in
Tree::AdjacencyList::Ordered component.  I've added a section listing
the methods that are made available via these other two components.  You
would still need to read the related component's POD for detailed docs
on the methods.

>attach_before - $this->attach_before($that) reads as "this attach before
>that" but attach_before actually attaches that rather than this so IMHO
>a different name would be better. I can't think of one I like -
>add_senior_sibling?
>
>attach_after - same issue as attach_before

Right.  I can't think of one I like either.  Added to the TODO.

>_grouping_clause - this is deeply worrying. It overrides a private
>method of a base class that is marked in that class as "These methods
>are used internally. You should never have the need to use them." So
>there's something wrong somewhere!

The _grouping_clause method was designed specifically for people that
want to subclass Ordered (as Tree::AdjacencyList::Ordered does) and want
to extend how Ordered groups the objects.  That being said, I think the
whole idea can be scrapped.  Done.

>Minor typo stuff:
>
>"add .. to the top of the .. list" => "add .. to the front of the ..
>list" or is that a US => UK thing?

Good catch.  Nah, this is a we used to call before/after up/down thing
and there are still some remnants of the up/down concept.  Fixed.

>General Comments
>
>There're no traversal methods (depth-first or breadth-first, all nodes

That will be implimented with a seperate DBIC::Tree::Visitor set of
components such as how Tree::Simple::Visitor does it.

>at level 5 etc) or classification predicates (is_leaf etc). I guess you
>plan to add those?

Yes, I plan to add thos.  is_leaf is a good one, which makes me think
is_parent, is_branch, and is_root are good ones as well.  Any more?

>When you generalize the API for other representations, it would be nice
>if it could also handle DAGs.

A DAG is a bit of a different beast than a Tree, but I'll keep it in
mind.  Sounds like a deffinate possibility.

>I must confess to a prejudice. I have a wariness about trees/graphs in
>databases. I tend to build the tree in memory and interrogate/manipulate
>it there, using the database only as a backing store. The tree structure
>itself is usually quite small compared to current memory sizes, even if
>it contains 100,000 nodes or so. So something that automatically cached
>the tree and knew which nodes needed writing back at a checkpoint would
>probably be more use to me.

Good idea, maybe we'll have a Tree::Cached component one day.

I've commited all of the above changes.
Thanks for all the input Dave!

Aran

>Cheers, Dave
>
>_______________________________________________
>List: http://lists.rawmode.org/cgi-bin/mailman/listinfo/dbix-class
>Wiki: http://dbix-class.shadowcatsystems.co.uk/
>IRC: irc.perl.org#dbix-class
>SVN: http://dev.catalyst.perl.org/repos/bast/trunk/DBIx-Class/