Opened 2 months ago
Last modified 2 months ago
#36480 new Cleanup/optimization
FieldError when referencing a nonexistent alias provides less information than nonexistent annotation
Reported by: | Jacob Walls | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | typo |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When migrating some querysets to use .alias()
rather than .annotate()
for performance purposes, I noticed when you typo a reference to an argument to .alias()
, the hint provided by the FieldError
does not include it.
(Whereas when using .annotate()
, the FieldError
hint does include what you meant.)
In [1]: from django.db.models import F In [2]: Tile.objects.alias(my_alias=F("pk")).order_by("typo") # notice "my_alias" missing from hint --------------------------------------------------------------------------- FieldError Traceback (most recent call last) Cell In[2], line 1 ----> 1 Tile.objects.alias(my_alias=F("pk")).order_by("typo") File ~/py313/lib/python3.13/site-packages/django/db/models/query.py:1722, in QuerySet.order_by(self, *field_names) 1720 obj = self._chain() 1721 obj.query.clear_ordering(force=True, clear_default=False) -> 1722 obj.query.add_ordering(*field_names) 1723 return obj File ~/py313/lib/python3.13/site-packages/django/db/models/sql/query.py:2291, in Query.add_ordering(self, *ordering) 2288 continue 2289 # names_to_path() validates the lookup. A descriptive 2290 # FieldError will be raise if it's not. -> 2291 self.names_to_path(item.split(LOOKUP_SEP), self.model._meta) 2292 elif not hasattr(item, "resolve_expression"): 2293 errors.append(item) File ~/py313/lib/python3.13/site-packages/django/db/models/sql/query.py:1805, in Query.names_to_path(self, names, opts, allow_many, fail_on_missing) 1797 if pos == -1 or fail_on_missing: 1798 available = sorted( 1799 [ 1800 *get_field_names_from_opts(opts), (...) 1803 ] 1804 ) -> 1805 raise FieldError( 1806 "Cannot resolve keyword '%s' into field. " 1807 "Choices are: %s" % (name, ", ".join(available)) 1808 ) 1809 break 1810 # Check if we need any joins for concrete inheritance cases (the 1811 # field lives in parent, but we are currently in one of its 1812 # children) FieldError: Cannot resolve keyword 'typo' into field. Choices are: child, data, file, geojsongeometry, nodegroup, nodegroup_id, parenttile, parenttile_id, provisionaledits, resourceinstance, resourceinstance_id, resxres, sortorder, tileid, vwannotation
I'm seeing this fixed with the following, but I stopped short of investigating the blame to see if it was explored before. (A contributor might have a look into that and report back?)
-
django/db/models/sql/query.py
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 20dbf7cfaa..3d9f39458b 100644
a b class Query(BaseExpression): 1820 1820 available = sorted( 1821 1821 [ 1822 1822 *get_field_names_from_opts(opts), 1823 *self.annotation _select,1823 *self.annotations, 1824 1824 *self._filtered_relations, 1825 1825 ] 1826 1826 )
(For anybody who followed #36380, you'll be glad to hear .alias()
let me prune my hundreds of useless annotations 😅)
Change History (5)
comment:1 by , 2 months ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 2 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:3 by , 2 months ago
I've tested the solution and it also lists for aggregate() which wouldn't be a valid choice
I think it would still be worth including the aliases among the choices for an aggregate()
call, so that you have the opportunity to fix a typo and receive the more informative:
FieldError: Cannot aggregate over the 'my_alias' alias. Use annotate() to promote it.
Also, I wonder if in the future we will add support for alias()
+ aggregate()
--the same logic holds that that querysets might be built incrementally, without knowledge of whether an aggregation will be attempted later--and if that scenario is possible then we might want to avoid adding complicated logic for the field error list.
comment:4 by , 2 months ago
Hi everyone, I’ve tested the change that the reporter suggested (replacing self.annotation_select with self.annotations). I think it works well — including alias names in the “Choices are:” part of the FieldError message makes the error more helpful and consistent with how .annotate()
behaves. This can really help catch typos or make things clearer when switching from .annotate()
to .alias()
.
I understand the concern about showing aliases in cases like .aggregate(),
where they aren’t valid. But I still think it’s helpful to include them — that seems better than hiding the alias completely.
comment:5 by , 2 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
Thank you! Agree any aliases should be listed if they are a valid option (so
order_by()
,filter()
etc). But not on the occasions that it's not valid, e.g. foraggregate()
I've tested the solution and it also lists for
aggregate()
which wouldn't be a valid choice so will unset "easy" as I think it needs a bit more looking into