Query.change_aliases() has several significant bugs

[Aqua Penguin]

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 ?

[Aqua Penguin]

Here the attached patch.

django/db/models/sql/query.py

@@ -988 +988 @@
         }
 
         # 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):

tests/queries/test_query.py

@@ -89 +89 @@
         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)))

[Mariusz Felisiak]

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 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.

[Simon Charette]

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.