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

Fixed#32398--Fixed negated annotation filter on build_filter #14065

Closed
wants to merge 1 commit into from

Conversation

baidoosik
Copy link
Sponsor Contributor

ticket-32398

I fixed the reported bug. but when the driving table has left join, I can't handle it right now. it is difficult on the current structure.
(this code -> self.alias_map[join_list[-1]].join_type == LOUTER))
If there are any issues, please tell me. and frankly, I don't know where is the good place to put the test code...

@@ -91,6 +93,35 @@ def test_negated_nullable(self):
self.assertIsInstance(lookup, IsNull)
self.assertEqual(lookup.lhs.target, Item._meta.get_field('modified'))

def test_negated_annotation_nullable(self):
query = Query(Item.objects.annotate(year=ExtractYear('modified')).exclude(modified='2021'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greetings, and thanks for your patch. The format of 2021 here is causing a test failure. To make this easier to review could you correct it? Thanks.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls I added other test cases. if another test cases are needed, tell me.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls could you retry this test 'Tests / JavaScript tests (pull_request) '? I don't know why it was failed.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @baidoosik, thanks for getting started here. Don't worry about the JS test -- it will rerun when you push again. Speaking of which--it's time to rewrite your branch history. Can you rebase and squash your commits to just two, one for the new test that increases coverage, and one for your patch and regression test?

Comment on lines 182 to 178
def test_equality_with_negated_nullable(self):
# nullable field
self.assertNotEqual(
Item.objects.exclude(modified__year='2021').values('id').query,
Item.objects.annotate(year=ExtractYear('modified')).exclude(year='2021').values('id').query,
)
# not nullable field
self.assertNotEqual(
Item.objects.exclude(created__year='2021').values('id').query,
Item.objects.annotate(year=ExtractYear('created')).exclude(year='2021').values('id').query,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes without applying your patch -- we like tests that increase coverage to be in separate commits. (what if we have to revert the new behavior? we can keep the test increasing coverage.)

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to check this. I will fix this.
self.assertEqual(
str(Item.objects.exclude(modified__year='2021').values('id').query),
str(Item.objects.annotate(year=ExtractYear('modified')).exclude(year='2021').values('id').query),
)

def test_negated_nullable_with_excluding_on_annotation(self):
# nullable field
query = str(Item.objects.annotate(year=ExtractYear('modified')).exclude(year='2021').values('id').query)
self.assertTrue('IS NOT NULL' in query)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think evaluating the result of a query (such as with assertQuerysetEqual) is much better than checking whether there's a random IS NOT NULL somewhere in the SQL. This would involve creating some objects.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls okay. Thanks for your review.

Comment on lines 1308 to 1298
target = targets[0].target
lookup_type = condition.lookup_name
clause.add(condition, AND)
if current_negated and lookup_type != 'isnull' and condition.rhs is not None \
and self.is_nullable(target):
# The condition added here will be SQL like this:
# NOT (col IS NOT NULL), where the first NOT is added in
# upper layers of code. The reason for addition is that if col
# is null, then col != someval will result in SQL "unknown"
# which isn't the same as in Python. The Python None handling
# is wanted, and it can be gotten by
# (col IS NULL OR col != someval)
# <=>
# NOT (col IS NOT NULL AND col = someval).
lookup_class = target.get_lookup('isnull')
col = self._get_col(target, target, alias)
clause.add(lookup_class(col, False), AND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you explored a way to resolve the DRY issues here?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally i try to use the below logic for checking negated null column.

        if current_negated and (lookup_type != 'isnull' or condition.rhs is False) and condition.rhs is not None:
            require_outer = True
            if lookup_type != 'isnull':
                # The condition added here will be SQL like this:
                # NOT (col IS NOT NULL), where the first NOT is added in
                # upper layers of code. The reason for addition is that if col
                # is null, then col != someval will result in SQL "unknown"
                # which isn't the same as in Python. The Python None handling
                # is wanted, and it can be gotten by
                # (col IS NULL OR col != someval)
                #   <=>
                # NOT (col IS NOT NULL AND col = someval).
                if (
                    self.is_nullable(targets[0]) or
                    self.alias_map[join_list[-1]].join_type == LOUTER
                ):
                    lookup_class = targets[0].get_lookup('isnull')
                    col = self._get_col(targets[0], join_info.targets[0], alias)
                    clause.add(lookup_class(col, False), AND)
                # If someval is a nullable column, someval IS NOT NULL is
                # added.
                if isinstance(value, Col) and self.is_nullable(value.target):
                    lookup_class = value.target.get_lookup('isnull')
                    clause.add(lookup_class(value, False), AND)

but in my view, other logic on below that code is not designed for reffed_expression.
so, I can't resolve DRY issues.
If you have any good idea, please tell me.

        try:
            join_info = self.setup_joins(
                parts, opts, alias, can_reuse=can_reuse, allow_many=allow_many,
                reuse_with_filtered_relation=reuse_with_filtered_relation,
            )

            # Prevent iterator from being consumed by check_related_objects()
            if isinstance(value, Iterator):
                value = list(value)
            self.check_related_objects(join_info.final_field, value, join_info.opts)

            # split_exclude() needs to know which joins were generated for the
            # lookup parts
            self._lookup_joins = join_info.joins
        except MultiJoin as e:
            return self.split_exclude(filter_expr, can_reuse, e.names_with_path)

@baidoosik baidoosik force-pushed the ticket-32398-query branch 3 times, most recently from c9c9566 to 02c4386 Compare March 3, 2021 16:47
Comment on lines 780 to 783
def assertQueryEqual(self, query1, query2, msg=None):
"""
Assert that two queries generate same sql. .
The arguments must be valid Query.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't provide a better tip -- I was talking about Django's own testing tool for this: https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.TransactionTestCase.assertQuerysetEqual

Have a look at the other test cases that use this test method.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls okay! Thanks!

Copy link
Sponsor Contributor Author

@baidoosik baidoosik Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls

I wanna make two testcases.

  1. for proving case about the reported issue.
    it is enough to write code like below, right?
    self.assertEqual(
    str(Item.objects.exclude(modified__year='2021').values('id').query),
    str(tem.objects.annotate(year=ExtractYear('modified')).exclude(year='2021').values('id').query)
    )

  2. check the IS NOT NULL includes or not in about each cases.
    about this cases I will use 'TransactionTestCase.assertQuerysetEqual' this.

Is it okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go for it. Another reviewer may let you know if it looks unnecessary to test the raw SQL, but we do have other tests that do this:

django/tests/queries/tests.py

Lines 3137 to 3139 in a948d9d

def test_null_join_demotion(self):
qs = ModelA.objects.filter(Q(b__name__isnull=False) & Q(b__name__isnull=True))
self.assertIn(' INNER JOIN ', str(qs.query))

I was just trying to encourage you to create and evaluate querysets. 👍🏻

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'TransactionTestCase.assertQuerysetEqual' compare query result(return data from db) with iterable. that is not test i want. because i wanna check to include 'IS NOT NULL' or not according to each condition. give me your opinion :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just one voice -- if other reviewers think testing the raw SQL is fine, we can roll on. Sure, let's get your test of the raw SQL in place and then hear from other reviewers.

What makes me nervous about only verifying the raw SQL instead of checking that the query got the right objects from the db is that these queries are complex -- how do I know the IS NOT NULL is in the right place? Won't there be a false negative if there's an IS NOT NULL on some other column? Then to review your patch I have to print out the raw SQL and check it myself, if you get where I'm going.

Copy link
Sponsor Contributor Author

@baidoosik baidoosik Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobtylerwalls I think so, too.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added only case1. I think it's enough to prove the issue. if you or other reviewers think we need more tests, tell me.

Comment on lines 1311 to 1312
if current_negated and lookup_type != 'isnull' and condition.rhs is not None \
and self.is_nullable(target):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid line continuation characters.

Suggested change
if current_negated and lookup_type != 'isnull' and condition.rhs is not None \
and self.is_nullable(target):
if (
current_negated and
lookup_type != 'isnull' and
condition.rhs is not None and
self.is_nullable(target)
):

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have resurfaced.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I changed this by felixxm suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just meant code style. The suggestion didn't have a backslash, and we avoid them in Django.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay! Thanks!

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baidoosik Thanks for this patch 👍 I left comments.

self.assertEqual(
str(Item.objects.exclude(created__year='2021').values('id').query),
str(Item.objects.annotate(year=ExtractYear('created')).exclude(year='2021').values('id').query),
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing raw queries is the last resort, in this case we should be able to create a queryset that doesn't return the correct results without a fix:

self.assertCountEqual(
    queryset,
    [....],
)

# nullable field
self.assertEqual(
str(Item.objects.exclude(modified__year='2021').values('id').query),
str(Item.objects.annotate(year=ExtractYear('modified')).exclude(year='2021').values('id').query),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExtractYear() is not important for this test, I would use a simpler function, e.g. Lower().

Comment on lines 1306 to 1310
targets = reffed_expression.get_source_expressions()
if targets and isinstance(targets[0], Col):
target = targets[0].target
lookup_type = condition.lookup_name
clause.add(condition, AND)
if (
current_negated and
lookup_type != 'isnull' and
condition.rhs is not None and
self.is_nullable(target)
):
# The condition added here will be SQL like this:
# NOT (col IS NOT NULL), where the first NOT is added in
# upper layers of code. The reason for addition is that if col
# is null, then col != someval will result in SQL "unknown"
# which isn't the same as in Python. The Python None handling
# is wanted, and it can be gotten by
# (col IS NULL OR col != someval)
# <=>
# NOT (col IS NOT NULL AND col = someval).
lookup_class = target.get_lookup('isnull')
col = self._get_col(target, target, alias)
clause.add(lookup_class(col, False), AND)
return clause, ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try to refactor existing code and add some internal hook instead of having similar (identical?) logic in two places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't think it's a complete solution as annotations on the LHS still don't work properly, see failing test:

    def test_exclude_nullable_fields_annotation(self):
        from django.db.models.functions import Abs
        number = Number.objects.create(num=1, other_num=1)
        Number.objects.create(num=2, other_num=2, another_num=2)
        self.assertSequenceEqual(
            Number.objects.annotate(x=Abs('another_num')).exclude(other_num=F('x')),
            [number],
        )   
        self.assertSequenceEqual(
            Number.objects.annotate(x=Abs('another_num')).exclude(num=F('x')),
            [number],
        )   

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm in my view, below test code is our expected result.

    def test_exclude_nullable_fields_annotation(self):
        from django.db.models.functions import Abs
        number = Number.objects.create(num=1, other_num=1)
        Number.objects.create(num=2, other_num=2, another_num=2)
        self.assertSequenceEqual(
            Number.objects.annotate(x=Abs('another_num')).exclude(other_num=F('x')),
            [],
        )
        self.assertSequenceEqual(
            Number.objects.annotate(x=Abs('another_num')).exclude(num=F('x')),
            [],
        )

Because I expect that query makes like below SQL. I think another_sum is nullable, so need to add is not null.
it means return 0 rows.

-- First Queryset
SELECT
	num,
	other_num,
	another_num
FROM
	number
WHERE
	NOT(another_num IS NOT NULL AND other_num = abs(another_sum))
	
-- Second Queryset
SELECT
	num,
	other_num,
	another_num
FROM
	number
WHERE
	NOT(another_num IS NOT NULL AND num = abs(another_sum))	

Let me know if I'm misunderstanding.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm At the first, I wanna reuse existing code but I can't. Because for reusing that code, we have to use some code such as self.setup_joins, self.trim_joins. but setup_joins is not designed for reffed_expression.(just my opinion, as you know that this is my second fix, so I may not understand the code because I am not familiar with it yet.)

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have some tip about this, please give me. i also wanna improve this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm in my view, below test code is our expected result.

No, it's not. 1 != NULL so why it's expected that number is excluded? see 512da9d and ticket-23797 for more details.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm You're right. but it seems to fail in the 'master' branch, too. and that query is not used changed code.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my changs affect when LHS is annotation field and RHS is not field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my changs affect when LHS is annotation field and RHS is not field.

I know, but this is also an issue with nullable annotation so we should fix it in this PR. If it is a different branch in build_filter() then we have another reason to add some hook.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm Ok. it makes sense. I will check this also.

@@ -165,6 +165,18 @@ def test_equality(self):
Author.objects.filter(Q(item__name='foo')).query,
)

def test_equality_with_negated_nullable(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this test to tests.queries.tests.ExcludeTests.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm okay! Thanks for review.

Base automatically changed from master to main March 9, 2021 06:22
@baidoosik baidoosik force-pushed the ticket-32398-query branch 3 times, most recently from 8325706 to bea28ae Compare March 21, 2021 12:48
@baidoosik
Copy link
Sponsor Contributor Author

baidoosik commented Mar 21, 2021

@felixxm I reflected your feedback. I try to refactor the existing code.

@baidoosik baidoosik requested a review from felixxm March 24, 2021 04:38
@baidoosik
Copy link
Sponsor Contributor Author

@felixxm When you have time, please review this :)

@jacobtylerwalls
Copy link
Member

Don't forget to keep the "Needs..." flags on the ticket up to date, otherwise your PR won't be visible in the review queue.

@baidoosik
Copy link
Sponsor Contributor Author

@jacobtylerwalls could you know me how to keep the 'Need...' flags?
image

@jacobtylerwalls
Copy link
Member

The checkboxes on the ticket. Just log in to Trac (presumably with your GitHub creds) and you'll see the last action on the ticket was when a reviewer used those boxes to indicate tests and improvements still needed. If you're caught up on review feedback, it's time to let people know on the ticket by unsetting the flags.

@jacobtylerwalls
Copy link
Member

Oh sorry, didn't see you included a screenshot. Uncheck "needs tests" and "patch needs improvement" and save your changes.

@baidoosik
Copy link
Sponsor Contributor Author

@jacobtylerwalls Big Thanks!!

@baidoosik
Copy link
Sponsor Contributor Author

@jacobtylerwalls I think i seem to miss something.(to receive review) If you know about this, please let me know.

@felixxm
Copy link
Member

felixxm commented Mar 30, 2021

@baidoosik Thanks for updates 👍 You didn't miss anything. There are ~30 PRs waiting for a review, please be patient.

@baidoosik
Copy link
Sponsor Contributor Author

@felixxm Sorry I didn't rush you. It was my first time to open source, so I misunderstood because I didn't know the process well! Thank you for your answer!!

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baidoosik Thanks for updates 👍 I think we should start from adding Expression.nullable in a separate PR. Please check my comments.

Comment on lines 60 to 66
def get_col_from_expression(expression):
# get Col from expression
targets = expression.get_source_expressions()
if targets and isinstance(targets[0], Col):
return targets[0]
else:
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it will work in all cases, you could use _get_cols() but it doesn't give you much. We need to know whether an expression is nullable or not, so we cannot take only columns into account. Some expressions are non-nullable even if all the columns passed are.

Simon proposed to add a nullable to the BaseExpression (see #11600 (comment)), it would help solving multiple issues (e.g. ticket-25946, ticket-29291) and fits perfectly with this patch. Can you prepare a patch in advance with a new nullable property?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm What you mean is to fix this bug after proceeding with inserting nullable property first?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm Okay. I will try to add nullable to the BaseExpression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm What you mean is to fix this bug after proceeding with inserting nullable property first?

Yes.

Copy link
Sponsor Contributor Author

@baidoosik baidoosik Jul 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm Currently i'm trying to add a nullable to BaseExpression.

I think there are two cases in this situation.

  1. Expression which depends on the column.

    ex) ExtractYear('created_at') -> depends on 'create_at' field is nullable or not.

  2. Expression which doesn't depends on 'column'.

    ex) Subquery -> it's always can be null.

I think I have to divide two cases. how about your opinion?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw your comment in the previous PR.

This flag should be set properly for all expressions, e.g. in `Col` should contain the same logic as in is_nullable(), many text functions are not nullable on Oracle because it interprets empty strings as NULL's, e.g. MD5, Concat() is not nullable, etc.

Do I need to check all of the expressions? and if we need to check column, we have to add is_nullable() function, right?

target = col.target
else:
return clause, ()
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like that we increase indentation here, it make code less readable, and it's complicated even without this 😞 We could reduce the number of changes significantly with:

        if reffed_expression:
            condition = self.build_lookup(lookups, reffed_expression, value)
            clause.add(condition, AND)
            # When col is nullable, add IS NOT NULL.
            col = self._gen_cols(reffed_expression)
            if col:
                lookup_type = condition.lookup_name
                target = col.target
                if current_negated and (lookup_type != 'isnull' or condition.rhs is False) and condition.rhs is not None and self.is_nullable(target):
                    lookup_class = target.get_lookup('isnull')
                    col = self._get_col(target, target, alias)
                    clause.add(lookup_class(col, False), AND)
            return clause, ()

and reverting related adjustments.

Copy link
Sponsor Contributor Author

@baidoosik baidoosik May 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm Thank you for reviewing. When you said above, we also have to handle when reffed_expression is 'Subquery' or some expressions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this case? Currently all tests work with my proposition.

Copy link
Sponsor Contributor Author

@baidoosik baidoosik Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixxm
It doesn't work when reffed_expression is the subquery.
below case is failed.

    def test_exclude_nullable_subquery_annotation(self):
        number = Number.objects.create(num=1, other_num=1)
        subquery_with_null = Subquery(Number.objects.filter(num=1).values('another_num')[:1])
        self.assertSequenceEqual(
            Number.objects.annotate(current_num=subquery_with_null).exclude(current_num=1),
            [number]
        )

Ran 1 test in 0.005s

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code according to your suggestion. my test case(for ticket-32864) doesn't work. if there is any missing point, please tell me. and many errors occurred with "target = col.target
AttributeError: 'generator' object has no attribute 'target'".

Copy link
Sponsor Contributor Author

@baidoosik baidoosik Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to handle when reffed_expression is the subquery. but so many other tests are still failed with AttributeError: 'generator' object has no attribute 'target'".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I ran into the same issue of AttributeError: 'generator' object has no attribute 'target'". with this test checking if this PR fixed the issue when using exclude with an alias (flagged up in duplicate ticket-32896):

    # ExcludeTests

    def test_exclude_aliased_nullable_fields(self):
        number = Number.objects.create(num=1, other_num=1)
        Number.objects.create(num=2, other_num=2, another_num=2)
        qs = Number.objects.alias(aliased_num=F('num'))
        self.assertSequenceEqual(
            qs.exclude(another_num=F('aliased_num')),
            [number],
        )
        self.assertSequenceEqual(
            qs.exclude(aliased_num=F('another_num')),
            [number],
        )

@felixxm
Copy link
Member

felixxm commented May 19, 2021

Can we also add a test for a nullable subquery annotation (see ticket-32684 that was marked as a duplicate)?

@baidoosik baidoosik force-pushed the ticket-32398-query branch 3 times, most recently from 3730e54 to e53a1da Compare June 1, 2021 13:48
@baidoosik baidoosik force-pushed the ticket-32398-query branch 2 times, most recently from 7d6bdec to ae71755 Compare June 1, 2021 15:17
@nessita
Copy link
Contributor

nessita commented May 12, 2023

Hello @baidoosik! I'm doing some PR cleanup. Would you have time to keep working on this? Specifically, if you could rebase onto the latest main, and also resolve conflicts, that would be great. I can then try to push further the review process, since it seems quite advanced already.

Thank you!

@baidoosik
Copy link
Sponsor Contributor Author

@nessita Hi ~! sorry for late response. i think it's hard to keep this PR. You can close this. i will try to make new PR when i have a enough time. Thanks for asking!

@nessita
Copy link
Contributor

nessita commented May 23, 2023

Closing as discussed with the reporter.

@nessita nessita closed this May 23, 2023
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.

5 participants