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

[Dave Howorth]

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.

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, 
what's the rationale for the values that are returned?

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. 
The method could also take a list of new children.

attach_sibling - same issue with name and possible list of arguments.


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.

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

position_column - this method is not listed.

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

_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!


Minor typo stuff:

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


General Comments

There're no traversal methods (depth-first or breadth-first, all nodes 
at level 5 etc) or classification predicates (is_leaf etc). I guess you 
plan to add those?

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

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.

Cheers, Dave