feincms / django-tree-queries

Adjacency-list trees for Django using recursive common table expressions. Supports PostgreSQL, sqlite, MySQL and MariaDB.
https://django-tree-queries.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
433 stars 27 forks source link

TreeNode model inheriting from concrete class raises Operational Error #1

Closed bkfox closed 3 months ago

bkfox commented 5 years ago

When a model subclasses another concrete model, an Operational Error is thrown using django's admin website: no such column: T.position.

I've tried with feincms3's, with concrete Page subclassing AbstractPage, and DiffusionPage subclassing Page. It seems that is because DiffusionPage holds a fk to Page table and do not contains itself the position column.

Here is a copy of the generated SQL (sqlite) expression:

('\n'
 '    WITH RECURSIVE __tree(tree_depth, tree_path, tree_ordering, tree_pk) AS '
 '(\n'
 '        SELECT\n'
 '            0 tree_depth,\n'
 '            printf("\x1f%%s\x1f", page_ptr_id) tree_path,\n'
 '            printf("\x1f%%020s\x1f", position) tree_ordering,\n'
 '            T."page_ptr_id" tree_pk\n'
 '        FROM app_diffusionpage T\n'
 '        WHERE T."parent_id" IS NULL\n'
 '\n'
 '        UNION ALL\n'
 '\n'
 '        SELECT\n'
 '            __tree.tree_depth + 1,\n'
 '            __tree.tree_path || printf("%%s\x1f", T.page_ptr_id),\n'
 '            __tree.tree_ordering || printf("%%020s\x1f", T.position),\n'
 '            T."page_ptr_id"\n'
 '        FROM app_diffusionpage T\n'
 '        JOIN __tree ON T."parent_id" = __tree.tree_pk\n'
 '    )\n'
 '    SELECT (1) AS "a" FROM "app_diffusionpage" , "__tree" WHERE '
 '("app_diffusionpage"."diffusion_id" = %s AND (__tree.tree_pk = '
 'app_diffusionpage.page_ptr_id)) ORDER BY ("__tree".tree_ordering) '
 'ASC  LIMIT 1')
matthiask commented 5 years ago

Yes, that does not work right now -- the SQL would have to contain a JOIN but does not.

There are other projects trying to add CTE support to Django in a more generic way resp. in a way that better leverages the Django ORM's existing features e.g. https://github.com/dimagi/django-cte . Time would probably be better invested there; this library is probably almost too simplistic for this.

matthiask commented 2 years ago

This ia a bug; TreeNode could ship with a system check or something which would detect this situation and emit a django.core.checks.Error.

Maybe the check could even be implemented on the queryset so that it also works if someone doesn't use the TreeQuerySet

I think this shouldn't be too hard to implement.

matthiask commented 3 months ago

Fixed in 7bf2a32a6904a7c385b0f12704492d6642eb8553