#34894 closed Cleanup/optimization (needsinfo)
Query.change_aliases() has several significant bugs
Reported by: | Aqua Penguin | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Normal | Keywords: | Query change_aliases |
Cc: | Aqua Penguin, Simon Charette | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When using the Query.change_aliases(change_map)
method (on an extended class), some changes are not done and errors occur:
- 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;
- even if an entry of
Query.alias_map
is not in thechange_map
, its alias_data may still have to be relabeled ifalias_data.parent_alias
is in thechange_map
, but it is not done and the generated nested joins that are modified bychange_map
have errors in the generated ON clauses;
- the cached property
Query.base_table
is not modified if it is in thechange_map
, it results in error when compiling the query, the old alias is searched inQuery.alias_refcount
, resulting in a KeyError.
*Solutions:*
- change the for loop to iterate on the
Query.alias_map
instead of thechange_map
, and rebuild it in the same order with updates;
- in addition to relabel the entries of
Query.alias_map
that are in thechange_map
, relabel for those whose thealias_data.parent_alias
are in theQuery.alias_map
;
- change the cached property
Query.base_table
if it is in thechange_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 ?
Attachments (1)
Change History (5)
by , 13 months ago
Attachment: | patch_34894.diff added |
---|
comment:1 by , 13 months ago
Cc: | added |
---|
comment:2 by , 13 months ago
comment:3 by , 13 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
Type: | Bug → Cleanup/optimization |
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.
comment:4 by , 13 months ago
Cc: | added |
---|
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.
Here the attached patch.
django/db/models/sql/query.py
del self.alias_map[old_alias]tests/queries/test_query.py