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

Unions, intersections and differences with tree fields do not work #55

Open
bbbergh opened this issue May 27, 2023 · 3 comments
Open

Unions, intersections and differences with tree fields do not work #55

bbbergh opened this issue May 27, 2023 · 3 comments

Comments

@bbbergh
Copy link

bbbergh commented May 27, 2023

Unions, intersections and differences do not seem to work when tree fields are ussed. Since this is not mentioned in the limitations I am not sure if this is deliberate. Specifically, multiple errors are thrown when performing these queries when they include tree fields. I am using django-tree-queries 0.14 and django 4.2.1.

Minimal example:

from tree_queries.models import TreeNode
class TestClass(TreeNode):
    class Meta:
        app_label = "test"

TestClass.objects.with_tree_fields().union(TestClass.objects.with_tree_fields())
# .intersection(...) and .difference(...) throw the same errors

This throws an error:

File ".../venv/lib/python3.11/site-packages/django/db/models/sql/compiler.py", line 550, in <listcomp>
    query.get_compiler(self.using, self.connection, self.elide_empty)
TypeError: TreeQuery.get_compiler() takes from 1 to 3 positional arguments but 4 were given

which seems to be related to the elide_empty not being passed as a keyword argument by this django code. Changing the signature of TreeQuery.get_compiler to get_compiler(self, using=None, connection=None, elide_empty = True, **kwargs) fixes this issue but raises another one. Instead, I get:

File ".../venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 2151, in add_fields
    raise FieldError(
django.core.exceptions.FieldError: Cannot resolve keyword 'tree_depth' into field. Choices are: __orderbycol1, id, parent, parent_id

It seems like a custom implementation of get_combinator_sql(self, combinator, all) would probably be required on TreeQuery to make this work.

@matthiask
Copy link
Member

Thanks for the report!

elide_empty was added in django/django@f3112fd , its absence hasn't been noticed yet it seems.

It would definitely be nice of django-tree-queries supported those types of queries.

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Aug 28, 2024

Hi!

Just encountered this myself. There's a comment that says:

def get_compiler(self, using=None, connection=None, **kwargs):
        ...
        # **kwargs passes on elide_empty from Django 4.0 onwards
        return TreeCompiler(self, connection, using, **kwargs)

But Django has never sent elide_empty as kwargs, only as positional args. Proof of this is the commit django/django@f3112fd where it was a positional argument, which was added 3 years ago, before there were no argument at all.

So, to be compiler compliant, django-tree-queries should use the same footprint as the regular Query.get_compiler:

    def get_compiler(self, using=None, connection=None, elide_empty=True):
        if using is None and connection is None:
            raise ValueError("Need either using or connection")
        if using:
            connection = connections[using]
        return connection.ops.compiler(self.compiler)(
            self, connection, using, elide_empty
        )

which should then pass elide_empty into TreeCompiler.

I'll be glad to create a PR with this change as soon as I find some spare time!
Hopefully, this is all that's needed to support unions and such, but I wouldn't know since I don't know the internals.

@matthiask
Copy link
Member

I'll be glad to create a PR with this change as soon as I find some spare time!
Hopefully, this is all that's needed to support unions and such, but I wouldn't know since I don't know the internals.

Would be great if that was the case! I'd appreciate a PR.

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

No branches or pull requests

3 participants