Opened 6 years ago

Closed 5 years ago

#29545 closed Bug (fixed)

Nested OuterRef not looking on the right model for the field.

Reported by: Aaron Lisman Owned by: Simon Charette
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: outerref, subquery
Cc: Can Sarıgöl, Oskar Persson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aaron Lisman)

I ran into this and made a test case for it. It seems similar to the test case above it. I'm not sure what's different about it and causing it to fail.

Let me know if the query is just built wrong.

diff --git a/tests/expressions/models.py b/tests/expressions/models.py
index 42e4a37bb0..164bda3b3d 100644
--- a/tests/expressions/models.py
+++ b/tests/expressions/models.py
@@ -83,6 +83,28 @@ class SimulationRun(models.Model):
         return "%s (%s to %s)" % (self.midpoint, self.start, self.end)
 
 
+class Customer(models.Model):
+    name = models.TextField()
+
+
+class Item(models.Model):
+    pass
+
+
+class Invoice(models.Model):
+    INVOICE = 'invoice'
+    EXPENSE = 'expense'
+
+    KIND_CHOICES = (
+        (INVOICE, 'Invoice'),
+        (EXPENSE, 'Expense'),
+    )
+
+    kind = models.CharField(choices=KIND_CHOICES, max_length=255, default=None)
+    owner = models.ForeignKey(Customer, models.CASCADE)
+    items = models.ManyToManyField(Item, related_name='invoices')
+
+
 class UUIDPK(models.Model):
     id = models.UUIDField(primary_key=True, default=uuid.uuid4)
 
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
index d1e622a2d4..2c50744fc8 100644
--- a/tests/expressions/tests.py
+++ b/tests/expressions/tests.py
@@ -24,7 +24,7 @@ from django.test.utils import Approximate
 
 from .models import (
     UUID, UUIDPK, Company, Employee, Experiment, Number, Result, SimulationRun,
-    Time,
+    Time, Customer, Item, Invoice
 )
 
 
@@ -536,6 +536,55 @@ class BasicExpressionsTests(TestCase):
         # This is a contrived example. It exercises the double OuterRef form.
         self.assertCountEqual(outer, [first, second, third])
 
+    def test_nested_subquery_outer_ref_3(self):
+        customer = Customer.objects.create(name='Test Customer')
+        other_customer = Customer.objects.create(name='Ohter Customer')
+
+        unexpensed_invoice = Invoice.objects.create(kind=Invoice.INVOICE, owner=customer)
+        unexpensed_item_1 = Item.objects.create()
+        unexpensed_invoice.items.add(unexpensed_item_1)
+
+        partially_expensed_invoice = Invoice.objects.create(kind=Invoice.INVOICE, owner=customer)
+        expense_1 = Invoice.objects.create(kind=Invoice.EXPENSE, owner=customer)
+        expensed_item_1 = Item.objects.create()
+        unexpensed_item_2 = Item.objects.create()
+        partially_expensed_invoice.items.add(expensed_item_1, unexpensed_item_2)
+        expense_1.items.add(expensed_item_1)
+
+        expensed_invoice = Invoice.objects.create(kind=Invoice.INVOICE, owner=customer)
+        Invoice.objects.create(kind=Invoice.EXPENSE, owner=customer)
+        expensed_item_2 = Item.objects.create()
+        expensed_invoice.items.add(expensed_item_2)
+        expense_1.items.add(expensed_item_2)
+
+        other_invoice = Invoice.objects.create(kind=Invoice.INVOICE, owner=other_customer)
+        other_invoice.items.add(unexpensed_item_1)
+        other_expense = Invoice.objects.create(kind=Invoice.EXPENSE, owner=other_customer)
+        other_expense.items.add(unexpensed_item_1)
+
+        inner = Invoice.objects.filter(
+            kind=Invoice.EXPENSE,
+            owner=OuterRef(OuterRef('owner')),
+            items=OuterRef('id'),
+        )
+        middle = Item.objects.filter(
+            invoices=OuterRef('id'),
+        ).annotate(
+            expensed=Exists(inner),
+        ).filter(
+            expensed=False,
+        )
+        outer = Invoice.objects.filter(
+            kind=Invoice.INVOICE,
+            owner=customer,
+        ).annotate(
+            unexpensed=Exists(middle),
+        )
+
+        self.assertTrue(outer.get(pk=unexpensed_invoice.pk).unexpensed)
+        self.assertTrue(outer.get(pk=partially_expensed_invoice.pk).unexpensed)
+        self.assertFalse(outer.get(pk=expensed_invoice.pk).unexpensed)
+
     def test_nested_subquery_outer_ref_with_autofield(self):
         first = Time.objects.create(time='09:00')
         second = Time.objects.create(time='17:00')
django.core.exceptions.FieldError: Cannot resolve keyword 'owner' into field. Choices are: expensed, id, invoices

Change History (10)

comment:1 by Aaron Lisman, 6 years ago

Description: modified (diff)

comment:2 by Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

I can't definitely say that it's a bug. Accepting for investigation.

comment:3 by Can Sarıgöl, 5 years ago

Cc: Can Sarıgöl added

IMO this query join order should be like this:

inner = Item.objects.filter(

middle = Invoice.objects.filter(
        
outer = Item.objects.filter(

If you want to use nested outerref, middle query has to contain foreign keys. Otherwise, this usage needs to include deep related field.

middle = Item.objects.filter(invoices=OuterRef('id'),).annotate(
            expensed=Exists(inner),
            owner=F('invoices__owner')
        ).filter(expensed=False,)

or maybe not necessary nested outerref

inner = Invoice.objects.filter(
            kind=Invoice.EXPENSE,
            owner=OuterRef('invoices__owner'),
            items=OuterRef('id'),
        )

comment:4 by Oskar Persson, 5 years ago

I also ran into this just now and can confirm that the bug exists in both 2.2.4 and master, tested with the models in tests/queries/models.py:

def test_nested_outerref(self):
    from .models import Item, Node, Number

    Number.objects.create(num=5)
    Node.objects.create(num=5)

    qs = Number.objects.annotate(
        has_item=Exists(
            Item.objects.annotate(
                has_tag=Exists(
                    Node.objects.filter(num=OuterRef(OuterRef('num')))
                )
            ).filter(has_tag=True)
        )
    )
    print(qs)

This results in the following exception

django.core.exceptions.FieldError: Cannot resolve keyword 'num' into field. Choices are: cover, created,
creator, creator_id, has_tag, id, modified, name, note, note_id, tags

comment:5 by Oskar Persson, 5 years ago

Cc: Oskar Persson added
Version: 2.0master

comment:6 by Simon Charette, 5 years ago

Thanks for the simplified test case Oskar, that's really valuable.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:7 by Simon Charette, 5 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

This is more of a brain dump than a final resolution but I think I might have nailed it down. I thought I'd share how I came with what I believe is the solution to share a bit of knowledge about how the ORM resolves expressions.

By breaking the query construction in multiple statements and adding breakpoints in OuterRef.resolve_expression and ResolvedOuterRef.resolve_expression I noticed that the latter was being called after the complete query construction when it was being executed which should not happen. In short all expressions should be resolved to their final expression at this point and in the case of OuterRef and ResolvedOuterRef they should be Col instances.

I then assumed that something must not be have been esolved properly so I added a breakpoint before the print(qs) and dug into qs.query internals. By walking the chain of qs.query.annotations['has_item'].query I noticed that .where.children[0].lhs was and Exist instance and that its .query.where.children[0].rhs happened to be a ResolvedOuterRef instance. I then knew that this was the culprit as it should have been a Col by this point. I also noticed that the copy of the Exists present as the .has_tag expression had its .query.where.children[0].rhs correctly resolved to a Col so I adjusted the test to replace the unresolved reference to the Col and got the test passing.

def test_nested_outerref(self):
    number = Number.objects.create(num=5)
    Node.objects.create(num=5)
    nested_query = Node.objects.filter(num=OuterRef(OuterRef('num')))
    nested_exist = Exists(nested_query)
    query = Item.objects.annotate(
        has_tag=nested_exist,
    ).filter(has_tag=True)
    exists = Exists(query)
    qs = Number.objects.annotate(
        has_item=exists
    )
    col = qs.query.annotations['has_item'].query.annotations['has_tag'].query.where.children[0].rhs
    qs.query.annotations['has_item'].query.where.children[0].lhs.query.where.children[0].rhs = col
    print(str(qs.query))
    self.assertEqual(qs.get(), number)

From that point I knew that there was an issue resolving qs.query.annotations['has_item'].query.where.children[0].lhs and then I remembered that sql.Where.resolve_expression was performing resolving for .rhs of lookups but not for .lhs which brought me to this still incomplete patch that happens to solve this issue and pass the test suite on SQLite so far.

I've assigned the ticket to myself as I plan to submit the above branch for review in the next days. I wouldn't be surprised if it fixed a other issues related to subqueries and OuterRef.

comment:9 by Can Sarıgöl, 5 years ago

Sorry I misinterpreted. Simon is right. Thanks for the explanation.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 8b4a43d:

Fixed #29545 -- Fixed using filter lookups againts nested subquery expressions.

Made sql.Where resolve lhs of its child nodes. This is necessary to
allow filter lookups against nested subquery expressions to properly
resolve their OuterRefs to Cols.

Thanks Oskar Persson for the simplified test case.

Note: See TracTickets for help on using tickets.
Back to Top