tree_ordering: returns int instead of string values after upgrade from 0.16.1 to 0.18

[nuarhu]

Hi,

after upgrading to 0.18 my tests are failing at the usages of tree_ordering. I tried different versions, and the failures do not show up with 0.16.1 but with 0.17 and 0.18.

Model:

class TranslationKey(TreeNode):
    label = CharField(
        null=False,
        blank=False,
        max_length=255,
    )
    objects = TreeQuerySet.as_manager(with_tree_fields=True)

    class Meta:
        ordering = ['label']
        unique_together = ['parent', 'label']

    def __str__(self):
        return self.label

    @property
    def full_label(self):
        tree_ordering = getattr(self, 'tree_ordering', None)
        if tree_ordering:
            # this is only present for objects fetched via TranslationKey.objects
            return '.'.join(tree_ordering)
        return '.'.join([tk.label for tk in self.ancestors(include_self=True)])

full_label returns a translation label like global.button.save where global, button, and save are 3 instances of TranslationKey.

This code is running fine up until 0.16.1 but fails after upgrading to 0.17 when running as part of a Django test - which means that the DB is setup with migrations from scratch before each test. In this case, my tests run on SQLite (the actual app runs on Postgres).

I have not found any release notes that indicate I need to change something (which would be completely fine), and I have not found any issues in this github repo about this. Am I the only one facing this issue?

Thanks for this library!

[matthiask]

Hi @nuarhu

Thanks! I'm sorry to hear that recent changes which were added to support more ways of ordering siblings broke your code. The tree_ordering and tree_path field contents are not supposed to be a part of the public interface of the package since its early days (56004f4), but I now see that this is only mentioned in the changelog and the code, and not in the more obvious places. So, you should always fall back to the self.ancestors() case. If that's too slow, either denormalize the full label and save it on each node or build the node tree in the Python code and traverse the ancestry yourself? I know that those sound like worse choices than what you had. I have been thinking on and off about allowing additional CTE fields but haven't gotten around to doing that just yet.

[nuarhu]

Thank you for the speedy answer!

Ok, I'll remove the usage then. Thanks for your suggestion of denormalizing the label. I just need to think through what is necessary to keep them up to date.

[jmbarbier]

Just to mention that @nuarhu is not alone being hit by this change, i was heavily using those 2 fields, so, for now, sticking to 0.16 is my only workaround in a reasonable amount of work.

[matthiask]

Yeah, I hear you. By the way, tree_path hasn't changed, only tree_ordering.

I think the additional functionality provided by recent changes is a good thing, but I can see that it's breaking other use cases. Would it help if there was a way to include additional fields in the recursive CTE? I don't know how something like this could be implemented on sqlite3 and mysql/mariadb without the same ugly hacks as the current tree_ordering and tree_path uses, but that's how it is.

[matthiask]

Here's a terrible hack which works for a very basic test case on PostgreSQL:
https://github.com/feincms/django-tree-queries/compare/mk/65-recursive-fields-hack

Something like this could work, but it would obviously have to be cleaned up a lot.

[jmbarbier]

@matthiask that was exactly what i planned to do (i'm on pg-only projects), and it is really a great addition when you need to retrieve other fields (permissions, status, ...) for all ancestors, without having to loop with get_ancestors. You did it faster... thanks a lot 👏 ❤️

[matthiask]

@jmbarbier Great! It's not finished yet btw, so if you want to contribute there's still work to do :-)

I won't be able to finish this in the next ~10 days.

[matthiask]

I've committed the extra_fields() hack to main (see #67), if you have a chance to test it out I'd appreciate some feedback. It's postgres-only at the moment, but I will probably get around cleaning it up.

[matthiask]

Check this out:

django-tree-queries/tests/testapp/test_queries.py

Lines 928 to 952 in 42e797c

	def test_tree_fields(self):
	self.create_tree()
	names = [obj.tree_names for obj in Model.objects.tree_fields(tree_names="name")]
	self.assertEqual(
	names,
	[
	["root"],
	["root", "1"],
	["root", "1", "1-1"],
	["root", "2"],
	["root", "2", "2-1"],
	["root", "2", "2-2"],
	],
	)
	orders = [
	obj.tree_orders for obj in Model.objects.tree_fields(tree_orders="order")
	]
	self.assertEqual(orders, [[0], [0, 0], [0, 0, 0], [0, 1], [0, 1, 0], [0, 1, 1]])
	# ids = [obj.tree_pks for obj in Model.objects.tree_fields(tree_pks="custom_id")]
	# self.assertIsInstance(ids[0][0], int)
	# ids = [obj.tree_pks for obj in Model.objects.tree_fields(tree_pks="parent_id")]
	# self.assertEqual(ids[0], [""])