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

Handle tree queryset .values() correctly #57

Closed glennmatthews closed 9 months ago

glennmatthews commented 9 months ago

Thanks for this great library!

We had a report (https://github.com/nautobot/nautobot/issues/4812) that doing a .values() call on a tree queryset was returning mixed-up data where the values were being reported for the wrong fields, i.e. instead of {field_a: value_a, field_b: value_b, field_c: value_c, field_d: value_d} we were getting {field_a: <tree_depth>, field_b: <tree_path>, field_c: <tree_ordering>, field_d: value_a}.

The attached change appears to fix the issue, and I've added a corresponding test case.

matthiask commented 9 months ago

Thanks! Tests are failing unfortunately.

I'd move the or self.query.values_select before the any(annotation.is_summary...) check but apart from that it looks like an improvement.

matthiask commented 9 months ago

Thanks! I replaced the hardcoded primary key values and released 0.16 containing your fix.

glennmatthews commented 9 months ago

I didn't catch this in my testing earlier but this appears to have had the side effect that calling values() or values_list() on the base queryset now returns the objects in ordering order rather than in tree traversal order. I can possibly work around it in our project but it may be an undesired change in general - would appreciate your thoughts?

(Pdb) Location.objects.first().name
'Campus-01'  # root object, as expected
(Pdb) Location.objects.all()[:3].values_list("name", flat=True)
<LocationQuerySet ['Aisle-06', 'Aisle-13', 'Aisle-20']>    # alphabetical order by name
(Pdb) Location.objects.all()[:3].values("name")
<LocationQuerySet [{'name': 'Aisle-06'}, {'name': 'Aisle-13'}, {'name': 'Aisle-20'}]>  # alphabetical order by name
matthiask commented 9 months ago

Yes, I thought about that as well. Thanks for the new patch, this one looks betterè

(It's obvious in hindsight that dropping the CTE would change the ordering of results...)