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):  
    18201820                    available = sorted(
    18211821                        [
    18221822                            *get_field_names_from_opts(opts),
    1823                             *self.annotation_select,
     1823                            *self.annotations,
    18241824                            *self._filtered_relations,
    18251825                        ]
    18261826                    )

(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 Sarah Boyce, 2 months ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted

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. for aggregate()
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

comment:2 by houston0222, 2 months ago

Owner: set to houston0222
Status: newassigned

comment:3 by Jacob Walls, 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 houston0222, 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 houston0222, 2 months ago

Owner: houston0222 removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top