Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle tree queryset .values() correctly #57

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

glennmatthews
Copy link
Contributor

Thanks for this great library!

We had a report (nautobot/nautobot#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
Copy link
Member

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 matthiask merged commit 80feec8 into feincms:main Nov 29, 2023
15 checks passed
@matthiask
Copy link
Member

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

@glennmatthews glennmatthews deleted the fix-values-handling branch November 29, 2023 13:52
@glennmatthews
Copy link
Contributor Author

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
Copy link
Member

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants