Fixed #29697 -- Changed Query.trim_start for avoid removing the aux tables #10498

cansarigol · 2018-10-10T09:51:43Z

timgraham

Please try to reuse or modify existing models. Adding more models slows the tests.

timgraham · 2018-10-10T18:20:02Z

tests/queries/tests.py

+            ~Q(company__employees__permissions__contains=Concat(Value('records.'), 'key', Value('.denied'))),
+            company__employees__pk=4).order_by('id')
+
+        self.assertQuerysetEqual(


prefer assertSequenceEqual

timgraham · 2018-10-10T18:21:08Z

tests/queries/tests.py

@@ -3918,3 +3920,31 @@ def test_ticket_23622(self):
            set(Ticket23605A.objects.filter(qy).values_list('pk', flat=True))
        )
        self.assertSequenceEqual(Ticket23605A.objects.filter(qx), [a2])
+
+
+class Ticket29697Tests(TestCase):


Better to use a descriptive name than "Ticket29697" (or at least add a docstring that explains). It doesn't seem like a separate class is needed if there's only 1 test.

timgraham · 2018-10-10T18:21:25Z

tests/queries/models.py

+class Ticket29697Status(models.Model):
+    company = models.ForeignKey(
+        Ticket29697Company,
+        on_delete=models.CASCADE)


Pass on_delete as a positional argument.

charettes

I'd also appreciate if you could describe why this is needed in the issue description. A brief summary of your investigation would be appreciated. Altering trim_start can have a lot of ramifications and cause unnecessary JOINs that the suite might not be catching.

charettes · 2018-10-10T18:23:47Z

django/db/models/sql/query.py

+        from_opts_tables = [
+            path.from_opts.db_table for path in all_paths
+            if path.from_opts.db_table != self.base_table
+        ]


Use a set since it's only used for containment checks.

charettes · 2018-10-10T18:25:58Z

tests/queries/tests.py

+        status = Ticket29697Status.objects.filter(
+            Q(company__employees__is_superuser=True) |
+            ~Q(company__employees__permissions__contains=Concat(Value('records.'), 'key', Value('.denied'))),
+            company__employees__pk=4).order_by('id')


I'd be great if you could make the test case more concise somehow. I doubt all of these criterias are required to hit the issue and they add a lot of noise which makes future debugging harder.

cansarigol · 2018-10-11T00:07:06Z

I tried to apply your recommendations. Could you check again?

charettes

Hey @cansarigol I don't have time to go through an extensive review now but I just wanted to say that the extra time you took to refine your patch and reduce it to the minimal changes required to address the issue is greatly appreciated.

charettes · 2018-10-11T00:07:23Z

django/db/models/sql/query.py

+                if path.from_opts.db_table in alias_joined_list and
+                path.from_opts.db_table != path.to_opts.db_table
+            ]
+        ])


Use set literals, {...} here and above.

charettes · 2018-10-11T00:07:48Z

django/db/models/sql/query.py

+        alias_refcount might be changed hence need alias_joined_list
+        to check available from join
+        """
+        alias_joined_list = set([k for k, v in self.alias_refcount.items() if v > 0])


Might want to rename to something else since it's not a list?

charettes · 2018-10-11T00:08:45Z

django/db/models/sql/query.py

@@ -2076,13 +2094,17 @@ def trim_start(self, names_with_path):
            t for t in self.alias_map
            if t in self._lookup_joins or t == self.base_table
        ]
+        # from_opts_tables for manage to define unref_alias table.
+        # before unref, make sure the table wasn't added
+        # in another from_opts as reference.


I guess this comment can be removed now?

charettes · 2018-10-11T00:09:35Z

tests/queries/tests.py

@@ -1199,6 +1199,16 @@ def test_common_mixed_case_foreign_keys(self):
        )
        self.assertTrue(qs.first())

+    def test_ticket_29697_complex_query_crashes(self):


test_excluded_intermediary_m2m_table_joined?

cansarigol · 2018-10-11T07:30:54Z

@charettes thanks a lot. sorry about these shortcomings and thank again for your help.

timgraham · 2018-10-31T15:26:00Z

django/db/models/sql/query.py

@@ -2082,7 +2100,8 @@ def trim_start(self, names_with_path):
            if self.alias_map[lookup_tables[trimmed_paths + 1]].join_type == LOUTER:
                contains_louter = True
            alias = lookup_tables[trimmed_paths]
-            self.unref_alias(alias)
+            if self.is_alias_available_join(all_paths, alias):


I don't see a test failure with this change reverted. If it's needed, could you add a test for it?

I'm not sure whether is_alias_available_join is useful. the tables I am interested at will not be there. so I removed it

charettes

The changes require a few adjustments before being merged. They might happen to work for the tested case and pass on the suite it's not dealing with aliases appropriately and will be really inefficient when dealing with large querysets.

charettes · 2018-10-31T20:42:53Z

django/db/models/sql/query.py

+        If so shouldn't be removed
+        alias_refcount might be changed hence need joined_alias
+        to check available from join
+        """


This docstring needs rewording. It should focus on what the function is doing and not on what the caller should do with the returned value.

charettes · 2018-10-31T20:47:58Z

django/db/models/sql/query.py

+                path.from_opts.db_table != path.to_opts.db_table
+            }
+        }
+        return alias not in from_to_tables


This seems overly complicated and probably also wrong with tables that are JOIN'ed more than once as the following JOINs will have aliases that are not db_table.

> from django.contrib.auth.models import User > User.objects.filter(groups__name='foo').query.alias_refcount {'auth_group': 1, 'auth_user': 1, 'auth_user_groups': 1} > User.objects.filter(groups__name='foo').filter(groups__name='bar').query.alias_refcount {'T4': 1, 'T5': 1, 'auth_group': 1, 'auth_user': 2, 'auth_user_groups': 1}

Also you probably don't need to build the from_to_tables set as it's really inneficent. You should use a loop and exit the function as soon as a matching alias is found.

cansarigol · 2018-11-01T07:44:32Z

thanks for your reviews. @charettes you're right about alias, I think so too but I couldn't find a better solution, it exceeded me. I changed is_alias_available_join

charettes · 2018-11-01T14:48:09Z

you're right about alias, I think so too but I couldn't find a better solution, it exceeded me.

This part of the ORM is definitely tricky, it's normal that you are feeling overwhelmed. I still think we should find a way to test the multi-alias case and address it before merging this change though.

…ables

cansarigol · 2019-03-18T09:09:32Z

hi @charettes, pushed a commit about maintaining is available check func before unref_alias and test for multi-alias queries.

felixxm · 2019-03-29T15:01:04Z

The added test passes without changes in Query. Bug was probably fixed by Simon refactorization or test doesn't cover this issue. I believe in Simon 😄 , so I will check which commit has fixed this.

felixxm · 2019-03-29T21:18:11Z

Bug was fixed by f19a494.

Tests updated in #11142.

cansarigol · 2019-03-29T22:53:19Z

that's a great solution. @charettes 👍

timgraham reviewed Oct 10, 2018

View reviewed changes

charettes reviewed Oct 10, 2018

View reviewed changes

charettes reviewed Oct 11, 2018

View reviewed changes

timgraham reviewed Oct 31, 2018

View reviewed changes

charettes suggested changes Oct 31, 2018

View reviewed changes

cansarigol force-pushed the ticket_29697 branch from efd2dc6 to 6d0721a Compare November 17, 2018 15:44

Fixed #29697 -- Changed Query.trim_start for avoid removing the aux t…

0595465

…ables

cansarigol force-pushed the ticket_29697 branch from 6d0721a to 0595465 Compare March 18, 2019 09:04

felixxm self-assigned this Mar 29, 2019

felixxm closed this Mar 29, 2019

felixxm removed their assignment Mar 29, 2019

cansarigol deleted the ticket_29697 branch March 29, 2019 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #29697 -- Changed Query.trim_start for avoid removing the aux tables #10498

Fixed #29697 -- Changed Query.trim_start for avoid removing the aux tables #10498

cansarigol commented Oct 10, 2018 •

edited

timgraham left a comment

timgraham Oct 10, 2018

timgraham Oct 10, 2018

timgraham Oct 10, 2018

charettes left a comment

charettes Oct 10, 2018

charettes Oct 10, 2018

cansarigol commented Oct 11, 2018

charettes left a comment

charettes Oct 11, 2018

charettes Oct 11, 2018

charettes Oct 11, 2018

charettes Oct 11, 2018

cansarigol commented Oct 11, 2018

timgraham Oct 31, 2018

cansarigol Oct 31, 2018

charettes left a comment

charettes Oct 31, 2018 •

edited

charettes Oct 31, 2018 •

edited

cansarigol commented Nov 1, 2018

charettes commented Nov 1, 2018

cansarigol commented Mar 18, 2019

felixxm commented Mar 29, 2019

felixxm commented Mar 29, 2019

cansarigol commented Mar 29, 2019

Fixed #29697 -- Changed Query.trim_start for avoid removing the aux tables #10498

Fixed #29697 -- Changed Query.trim_start for avoid removing the aux tables #10498

Conversation

cansarigol commented Oct 10, 2018 • edited

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cansarigol commented Oct 11, 2018

charettes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cansarigol commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes left a comment

Choose a reason for hiding this comment

charettes Oct 31, 2018 • edited

Choose a reason for hiding this comment

charettes Oct 31, 2018 • edited

Choose a reason for hiding this comment

cansarigol commented Nov 1, 2018

charettes commented Nov 1, 2018

cansarigol commented Mar 18, 2019

felixxm commented Mar 29, 2019

felixxm commented Mar 29, 2019

cansarigol commented Mar 29, 2019

cansarigol commented Oct 10, 2018 •

edited

charettes Oct 31, 2018 •

edited

charettes Oct 31, 2018 •

edited