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
415 stars 27 forks source link

Docs appear wrong: Children not ordered by ID by default #73

Closed boosh closed 1 month ago

boosh commented 1 month ago

From the docs:

Nodes with the same parent may be ordered among themselves. The default is to order siblings by their primary key

That's not what I'm seeing with Postgres 16.3. I have to explicitly call mymodel.children.order_siblings_by("id") to order children by ID.

matthiask commented 1 month ago

Thanks! It's possible that this broke while adding support for ordering by several fields (resp. the introduction of the rank table). I'll look into it if nobody beats me to it.

matthiask commented 1 month ago

I'm not sure what's going on.

I have tested it by removing the ordering so that Page._meta.ordering == []

Querying this produces the following CTE:

WITH RECURSIVE __rank_table( "id", "parent_id", "rank_order" ) AS (
        SELECT "pages_page"."id",
               "pages_page"."parent_id",
               ROW_NUMBER() OVER (ORDER BY "pages_page"."id") AS "rank_order"
          FROM "pages_page"
       ),
       __tree ( "tree_depth", "tree_path", "tree_ordering", "tree_pk" ) AS (
        SELECT 0, array[T.id], array[T.rank_order], T."id"
          FROM __rank_table T
         WHERE T."parent_id" IS NULL
     UNION ALL SELECT __tree.tree_depth + 1,
               __tree.tree_path || T.id,
               __tree.tree_ordering || T.rank_order,
               T."id"
          FROM __rank_table T
          JOIN __tree
            ON T."parent_id" = __tree.tree_pk
       ) 
...

The ROW_NUMBER values are generated ordered by the ID field as they should. The tree_ordering values look correct as well.

I have also added a unittest here c4faadd968ccb2facc1abc5478eca9e2c3c21ff1 and cannot find any unexpected behavior. Can you recheck please if your ordering attribute has the expected value and if you're using .with_tree_fields()? The tree query and the corresponding BFS ordering doesn't happen by default. If you always want tree queries you have to opt in to this behavior, e.g. by using TreeQuerySet.as_manager(with_tree_fields=True).

boosh commented 1 month ago

Yes I'm definitely using with_tree_fields(). I only found this issue affecting maybe fewer than 5% of trees. Let me see if I can reproduce it again and update this for a failing query.

boosh commented 1 month ago

Yeah I've reproduced it, definitely using .with_tree_fields(). How can I print that full query above? Printing the query just gives me this:

print(task.children.all().query)
SELECT "core_jobtask"."id", ... FROM "core_jobtask" WHERE "core_jobtask"."parent_id" = 38765
boosh commented 1 month ago

Ah I've just spotted:

Basic usage, disregards the tree structure completely. nodes = Node.objects.all()

I guess that's the issue. If I use task.children.with_tree_fields() it is ordered, so I guess I just didn't spot that...

matthiask commented 1 month ago

I'm using django-debug-toolbar to inspect queries, it has always worked well for me.

task.children.all() might not actually use tree queries!

boosh commented 1 month ago

Yeah that must be it. OK thanks for your help.

matthiask commented 1 month ago

Ah I've just spotted:

Basic usage, disregards the tree structure completely. nodes = Node.objects.all()

I guess that's the issue. If I use task.children.with_tree_fields() it is ordered, so I guess I just didn't spot that...

Yeah, quite certainly. If you use objects = TreeQuerySet.as_manager(with_tree_fields=True) then queries like .children should also automatically use tree queries, I think.

matthiask commented 1 month ago

Yeah that must be it. OK thanks for your help.

My pleasure!