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

tree_ordering by char fields - not working/supported? #41

Closed glennmatthews closed 2 years ago

glennmatthews commented 2 years ago

It looks like the tree_ordering implementation (as well as order_siblings_by() assumes the field being ordered by is an integer value, since it left-pads it with zeros to solve the "110" < "20" problem (replacing it with "00000000000000000110" > "00000000000000000020"). While I see there's some code in tree_queries/compiler.py that appears to be intended to handle text differently from integers for PostgreSQL, in MySQL at least there's no such logic and so a tree ordered by a CharField is inappropriately sorted, e.g.:

>>> Location.objects.all()[4].tree_ordering
['0000000000Milky Way', '000000000Sol System', '00000000000000Earth', '0000000000000Africa']
>>> Location.objects.all()[5].tree_ordering
['0000000000Milky Way', '000000000Sol System', '00000000000000Earth', '000000000000Eurasia']
>>> Location.objects.all()[6].tree_ordering
['0000000000Milky Way', '000000000Sol System', '00000000000000Earth', '0000000000Australia']
>>> Location.objects.all()[7].tree_ordering
['0000000000Milky Way', '000000000Sol System', '00000000000000Earth', '000000North America']

(note that "Eurasia" is sorted between "Africa" and "Australia", apparently because its string length falls between them.)

Additionally the tree_ordering is calculated incorrectly if the field being ordered by exceeds 20 characters in length:

>>> Location.objects.get(name="Various Islands in the Pacific Ocean").tree_ordering
['0000000000Milky Way', '000000000Sol System', '00000000000000Earth']

(the tree_ordering is simply omitting the object's name altogether from the array in this case.)

Is this a known limitation of this library?

matthiask commented 2 years ago

Hi

Thanks for the report! I'm not surprised that it doesn't work but I'm surprised that this hasn't been detected before.

I changed the test_string_ordering test to use strings of varying lengths and it is even worse than I thought. Ordering by string fields only works when using fixed-length fields on all databases, even on PostgreSQL. Oh my...

matthiask commented 2 years ago

I hope 0.10 fixes this issue.

glennmatthews commented 2 years ago

Wow, that was fast! 😃 Thank you, I'll take it for another spin.