[dev] Major flaw in DataTree::getLast() implementation

[Chuck Hagenbuch]

Quoting Jan Schneider <jan at horde.org>:

> It took me a while to find this out, but it's easy to reproduce: simply
> start a new thread in Agora and return to the forums listing directly
> after that.
>
> DataTree::getLast() assumes that the last object in a tree branch is
> the one with the highest ID. But that's wrong because in add() we
> *first* retrieve a new sequence ID and *then* check if we already have
> a parent object and create it if not (retrieving another, higher
> sequence ID). This logic could probably be changed in add(), but it
> exists since ever, so getLast() won't work with any old DataTree data,
> which is a bad thing IMO.
>
> Any ideas how to proceed?

I remember wondering about the implementation when it was first 
proposed. Looking at the schema I don't think there's any way it can 
work reliably as-is. We'd need to either reorder existing ids and 
change add(), or add a new field - datatree_updated won't work because 
if an existing row is touched, it'll get updated and suddenly be "last".

With adding a new field we could create a very arbitrary script to 
assign values that'd work for old data, sort of walking the tree... I 
don't think it'd hurt to add datatree_created and start using 
datatree_updated explicitly. Or we can just  drop datatree_updated from 
the schema since we don't use it - wouldn't hurt to have less data in 
that table.

Thinking further, we can't reorder existing data because it might break 
applications using getById().

We can make it work for future data, but not existing data - I don't 
think there's a way around it.

-chuck

-- 
"But she goes not abroad in search of monsters to destroy." - John 
Quincy Adams