[Django] #34894: Query.change_aliases() has several significant bugs
Django
-------------------------------------+-------------------------------------
Reporter: Aqua | Owner: nobody
Penguin |
Type: Bug | Status: new
Component: Database | Version: 4.2
layer (models, ORM) | Keywords: Query
Severity: Normal | change_aliases
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When using the {{{Query.change_aliases(change_map)}}} method (on an
extended class), some changes are not done and errors occur:
1. the insert order of {{{Query.alias_map}}} is modified, and it is used
by the compiler to compile the FROM parts in the correct order, this can
results in an inconsistant order of the JOIN clauses;
2. even if an entry of {{{Query.alias_map}}} is not in the
{{{change_map}}}, its alias_data may still have to be relabeled if
{{{alias_data.parent_alias}}} is in the {{{change_map}}}, but it is not
done and the generated nested joins that are modified by {{{change_map}}}
have errors in the generated ON clauses;
3. the cached property {{{Query.base_table}}} is not modified if it is in
the {{{change_map}}}, it results in error when compiling the query, the
old alias is searched in {{{Query.alias_refcount}}}, resulting in a
KeyError.
*Solutions:*
1. change the for loop to iterate on the {{{Query.alias_map}}} instead of
the {{{change_map}}}, and rebuild it in the same order with updates;
2. in addition to relabel the entries of {{{Query.alias_map}}} that are in
the {{{change_map}}}, relabel for those whose the
{{{alias_data.parent_alias}}} are in the {{{Query.alias_map}}};
3. change the cached property {{{Query.base_table}}} if it is in the
{{{change_map}}}.
Attached files: patch with fixes and tests
Question: The bugs has been tested on 4.2 and main, I have to do a PR on
all current versions or only on the main branch ?
--
Ticket URL: <https://code.djangoproject.com/ticket/34894>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Query | Triage Stage:
change_aliases | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "patch_34894.diff" added.
Django
-------------------------------------+-------------------------------------
Reporter: Aqua Penguin | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Query | Triage Stage:
change_aliases | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Aqua Penguin):
* cc: Aqua Penguin (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34894#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Aqua Penguin | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Query | Triage Stage:
change_aliases | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aqua Penguin):
Here the attached patch.
{{{
#!diff
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index a7839ccb4d..95458a9102 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -988,14 +988,27 @@ class Query(BaseExpression):
}
# 2. Rename the alias in the internal table/alias datastructures.
- for old_alias, new_alias in change_map.items():
- if old_alias not in self.alias_map:
+ # - The order of insertion in alias_map has great importance for
+ # future iterations, so we need to rebuild it in the original
order.
+ # - alias_data must be relabeled if it contains a change to do in
+ # alias_data.parent_alias, even if its alias does not change.
+ # - Cached property base_table need to change if necessary.
+ base_table_cached = self.__dict__.get("base_table", None)
+ old_alias_map, self.alias_map = self.alias_map, {}
+ for old_alias, alias_data in old_alias_map.items():
+ if old_alias not in change_map:
+ self.alias_map[old_alias] = (
+ alias_data.relabeled_clone(change_map)
+ if alias_data.parent_alias in change_map
+ else alias_data
+ )
continue
- alias_data =
self.alias_map[old_alias].relabeled_clone(change_map)
- self.alias_map[new_alias] = alias_data
+ new_alias = change_map[old_alias]
+ if old_alias == base_table_cached:
+ self.__dict__["base_table"] = new_alias
+ self.alias_map[new_alias] =
alias_data.relabeled_clone(change_map)
self.alias_refcount[new_alias] =
self.alias_refcount[old_alias]
del self.alias_refcount[old_alias]
- del self.alias_map[old_alias]
table_aliases = self.table_map[alias_data.table_name]
for pos, alias in enumerate(table_aliases):
diff --git a/tests/queries/test_query.py b/tests/queries/test_query.py
index 99d0e32427..f5a17c56e5 100644
--- a/tests/queries/test_query.py
+++ b/tests/queries/test_query.py
@@ -89,6 +89,45 @@ class TestQuery(SimpleTestCase):
self.assertIsNone(lookup.lhs.lhs.alias)
self.assertEqual(lookup.lhs.lhs.target,
Author._meta.get_field("name"))
+ def test_change_aliases_alias_map_order(self):
+ query = Query(Item)
+ # force to create a join between Item and Author
+ query.add_q(Q(creator__num__gt=2))
+ # force to initialize all joins and caches
+ sql = str(query)
+ order = [
+ "item_alias" if alias == Item._meta.db_table else alias
+ for alias in list(query.alias_map)
+ ]
+ query.change_aliases({
+ Item._meta.db_table: "item_alias",
+ })
+ self.assertEqual(list(query.alias_map), order)
+
+ def test_change_aliases_nested_joins(self):
+ query = Query(Item)
+ # force to create a join between Item and Author
+ query.add_q(Q(creator__num__gt=2))
+ # force to initialize all joins and caches
+ sql = str(query)
+
self.assertEqual(query.alias_map[Author._meta.db_table].parent_alias,
Item._meta.db_table)
+ query.change_aliases({
+ Item._meta.db_table: "item_alias",
+ })
+
self.assertEqual(query.alias_map[Author._meta.db_table].parent_alias,
"item_alias")
+
+ def test_change_aliases_base_table(self):
+ query = Query(Item)
+ # force to create a join between Item and Author
+ query.add_q(Q(creator__num__gt=2))
+ # force to initialize all joins and caches
+ sql = str(query)
+ self.assertEqual(query.base_table, Item._meta.db_table)
+ query.change_aliases({
+ Item._meta.db_table: "item_alias",
+ })
+ self.assertEqual(query.base_table, "item_alias")
+
def test_negated_nullable(self):
query = Query(Item)
where = query.build_where(~Q(modified__lt=datetime(2017, 1, 1)))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34894#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Aqua Penguin | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: Query | Triage Stage:
change_aliases | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* type: Bug => Cleanup/optimization
* resolution: => needsinfo
Comment:
`Query.change_aliases()` is a private API and, as far as I'm aware, it
works fine for internal use. Can you explain what are you trying to
implement? Did you find any existing
[https://code.djangoproject.com/query?status=assigned&status=new&component=Database+layer+(models%2C+ORM)&stage=Accepted&col=id&col=summary&col=status&col=component&col=owner&col=type&col=version&desc=1&order=id
bug] that could be fix by these changes? Can you prepare failing tests
using querysets rather than testing internal tools?
> Question: The bugs has been tested on 4.2 and main, I have to do a PR on
all current versions or only on the main branch ?
Mergers will handle backports, if needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/34894#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Aqua Penguin | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: Query | Triage Stage:
change_aliases | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
As Mariusz said it'd be great to learn what kind of ''significant bugs''
this addresses as this method is extensively used by the ORM and making
changes to it, even if they appear right from an API perspective, can have
unintended side effects.
--
Ticket URL: <https://code.djangoproject.com/ticket/34894#comment:4>