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

Descendant `update` queries do not include SQL for the CTE #71

Closed rebeccacremona closed 4 weeks ago

rebeccacremona commented 3 months ago

First, thank you for this terrific library!

When migrating a project from django-mptt, I discovered that with Django 4.2.13 and django-tree-queries 0.19.0, node.descendants().update(...) raises django.db.utils.ProgrammingError: the generated SQL is an UPDATE that references the __tree table, but does not include the SQL for building it. In comparison, node.ancestors().update(...) splits the work into two queries, issuing a SELECT then an UPDATE.

Reproduction

# 
# Minimal setup
#

# Use the model definition from the docs.
from tree_queries.models import TreeNode
class Node(TreeNode):
    name = models.CharField(max_length=100)

# Run migrations, then create a node.
node = Node(name='The Only Node')
node.save()
another_node = Node(name='Another Node', parent=node)
another_node.save()

# Verify that node.descendants() and node.ancestors() work as expected
node.descendants()
node.ancestors()
another_node.descendants()
another_node.ancestors()

#
# Attempt to update descendant queryset, and log the SQL
#

from django.db.utils import ProgrammingError

try:
    node.descendants().update(name='A New Name')
except ProgrammingError as e:
    # ProgrammingError: missing FROM-clause entry for table "__tree"
    # STATEMENT:  UPDATE "sample_node" SET "name" = 'A New Name' WHERE ((1 = ANY(__tree.tree_path)) AND NOT ("sample_node"."id" = 1))
    print(e)

#
# For comparison, update ancestor queryset.
#

assert another_node.ancestors().update(name='A New Name') == 1

# It works, in an unexpected way:
# it first does a SELECT that includes the `WITH RECURSIVE __rank_table` clause,
# and then does an UPDATE with the found ids
# STATEMENT:  UPDATE "sample_node" SET "name" = 'A New Name' WHERE "sample_node"."id" IN (1)

Workaround

We were able to work around this easily in our application by manually taking the same two-query approach as with ancestors:

descendant_ids = list(node.descendants().values_list('id', flat=True))
Node.objects.filter(ids__in=descendant_ids).update(name='A New Name')
matthiask commented 3 months ago

Hi

Thanks for the nice words!

The problem is that we're interfering with some internal parts of the Django ORM when we assing our own TreeQuery as the query class which should be used.

.update() runs query.chain(...) which in turn overrides the query class used by Django: https://github.com/django/django/blob/adae619426b6f50046b3daaa744db52989c9d6db/django/db/models/query.py#L1227

We could try replacing .update() with our own method, but we'd probably require a TreeUpdateQuery or something for that. This sounds quite involved to me (I may be wrong) so maybe a better approach would be to detect these conditions and error out in a more obvious way.

matthiask commented 3 months ago

Addendum: I wonder if you could keep it in the database by using a subquery. This seems to work nicely:

>>> Page.objects.filter(id__in=p.descendants(include_self=True).values("id")).update(title="blub")
11
rebeccacremona commented 3 months ago

Nice that that works!

In our very specific situation, I ended up wanting to keep around a list of IDs I could reference later, so making two separate queries within a transaction ended up being perfect, but I have to imagine that the subquery approach would work best for others!

matthiask commented 4 weeks ago

I have added tests which verify the behavior with .descendants().update() in 73ee469f0b54c8434172cb56d4e232818af6b76f

It still doesn't work but at least we'll know if anything changes.

matthiask commented 4 weeks ago

I have added an example to the docs describing how to work around this issue, and I'm going to close it as wontfix. I hope someone finds the time and energy to fix it for real (feel free to submit PRs!) but having a documented workaround is good enough for me.