WEEE-Open/tarallo
Move tree operations to the database (TreeDAO refactoring)
オープン
#24 opened on 2018/11/01
SQLhelp wantedvalidation and normalization
Repository metrics
- Stars
- (21 個のスター)
- PR merge metrics
- (PR metrics pending)
説明
Currently TreeDAO has some private methods with the most basic operations:
- addItemAsRoot
- setParent
- splitSubtree
and some public methods that call one or more of those:
- addToTree
- moveItem
- removeFromTree
And other methods that aren't really that important and shouldn't need any refactoring.
Main problems with this approach:
- addToTree wreaks havoc if it's called with an item already in the tree, while ideally it should be idempotent or turn into a move operation automatically
- removeFromTree doesn't really make sense on its own and has unknown effects on subtrees (has been tested only on leaves)
- The simple operations of "adding items" and "moving items" can be done only from PHP, but for bulk operations and fixing stuff they should be possible from the database, ideally with a single and somewhat short query.
- Moving an item into itself currently turns it into a root. Cool, but does it make sense?
- Error handling is not well thought, sometimes SQL exceptions (constraint violations) surface to API responses.
These point should be addressed:
- Move as much logic as possible to the database
- Try to keep public interfaces clean, e.g. a single method for useful operations (add, move, remove) both in PHP and database itself
- Better checks and safeguards against weird operations if needed: moving an item into itself, into one of its descendants (fails with a integrity constraint violation currently, please preserve its "operation failed" behavior so the tree stays a tree instead of becoming a graph of recursion and chaos, but feel free to give the user a better message), deleting a subtree, detaching a detached item, etc...
- Give a meaning to detached items that aren't deleted or link "tree removal" to "item deletion" in a more robust and clear way
- Leave a way to add roots, to have more than one root, to detach subtrees to create a new root. There's no need for these operation to work from PHP, though: running queries manually is enough, as long as they're robust and somewhat simple.
- Add more tests, this should help decide what to handle what and when and how and generally make the implementation more robust