5.2.3 regression with `.annotate` that forbids overriding existing fields

[sobolevn]

Refs #2797

Quoting @asottile-sentry from #2797 (comment)

---

I believe this might not be correct for the .annotate(...) case

here's an example from sentry:

src/sentry/api/serializers/models/rule.py:207: error: Attribute "date_added" already defined on "sentry.models.rulefirehistory.RuleFireHistory"  [no-redef]

https://github.com/getsentry/sentry/blob/e1829e8cbd0fe4179e137afbbee23b06d35d3eee/src/sentry/api/serializers/models/rule.py#L209

(there's a few other examples in sentry as well where particular aggregations are used to produce a value with the same name as an existing field when making a grouped query)

---

Quoting myself from #2797 (comment)

---

Hm, interesting, I haven't consider this use-case. So, basically, if we annotate an some existing IntegerField field with int value, there's no type error here. So, basically, we should instead detect type mismatches in annotated fields and report them.

[UnknownPlatypus]

I believe this might not be correct for the .annotate(...) case

Django does raise an internal error on such case but only if the field was not filtered by a prior .values("rule_id").
So I think the issue might only be for cases with a prior values / values_list

from django.contrib.auth.models import User
from django.db.models import Max

User.objects.annotate(username=Max("last_login")) # ValueError: The annotation 'username' conflicts with a field on the model.

User.objects.values("id").annotate(username=Max("last_login")) # OK

(there's a few other examples in sentry as well where particular aggregations are used to produce a value with the same name as an existing field when making a grouped query)

@asottile-sentry could you check such cases also have prior values / values_list calls to confirm my intuition ? (I have a patch ready if that's the case)

[asottile-sentry]

let me check each of these:

src/sentry/api/serializers/models/rule.py:207: error: Attribute "date_added" already defined on "sentry.models.rulefirehistory.RuleFireHistory"  [no-redef]
src/sentry/api/serializers/models/rule.py:225: error: Attribute "date_added" already defined on "sentry.workflow_engine.models.workflow_fire_history.WorkflowFireHistory"  [no-redef]
src/sentry/workflow_engine/endpoints/serializers.py:561: error: Attribute "event_id" already defined on "sentry.workflow_engine.models.workflow_fire_history.WorkflowFireHistory@AnnotatedWith[TypedDict({'count': Any})]"  [no-redef]
src/sentry/workflow_engine/endpoints/serializers.py:561: error: Attribute "detector_id" already defined on "sentry.workflow_engine.models.workflow_fire_history.WorkflowFireHistory@AnnotatedWith[TypedDict({'count': Any, 'last_triggered': Any})]"  [no-redef]
src/sentry/rules/history/backends/postgres.py:138: error: Attribute "event_id" already defined on "sentry.models.rulefirehistory.RuleFireHistory@AnnotatedWith[TypedDict({'count': Any})]"  [no-redef]

[asottile-sentry]

yeah each of those come after a .values(...) call

[UnknownPlatypus]

Thanks, I'll submit a fix hopefully tomorrow, (got caught up with a DDoS attack today unfortunately :/ )

[AllexVeldman]

@UnknownPlatypus I found a false-positive in the case of an empty .values() call:

In [5]:  User.objects.all().values().annotate(username=Value("foo")).values("username")
Out[5]: <QuerySet [{'username': 'foo'}, {'username': 'foo'}]>

mypy:

tmp.py:7: error: Attribute "username" already defined on "django.contrib.auth.models.User"  [no-redef]

I could not find documentation on the Django side that this is intended behaviour though.

[UnknownPlatypus]

Interesting, it does feel very weird to me since .values() is documented as a shortcut vs listing every possible fields but in one case it would crash but not the other

https://docs.djangoproject.com/en/5.2/ref/models/querysets/#values

If you don’t specify the fields, each dictionary will contain a key and value for every field in the database table.

We can probably support this but out of curiosity, did you have actual use cases for that ?

[AllexVeldman]

We can probably support this but out of curiosity, did you have actual use cases for that ?

I found it in one of our codebases where it was used explicitly to re-define the same name, where we only need that single name.
But the same could easily be accomplished with a .values('id') instead of an empty .values(), we just don't use the 'id' later.
The Django behaviour also seems inconsitent to me so I'm changing the codebase.

So from my perspective there is no need to support this, although if the goal is to be consistent with Django behaviour you might consider it.

[UnknownPlatypus]

So from my perspective there is no need to support this, although if the goal is to be consistent with Django behaviour you might consider it.

I think it's ok to difer it it helps promote better patterns, I'm not completely clear here if there are legit cases where this helps with no easy workaround.

It feels to me that if we want to do processing to compute a field, it make sense to be explicit like so:

User.objects.all().annotate(computed_username=Value("foo")).values("computed_username")

But I guess if we really care about the "username" key, the .values('id') kinda make sense. The bare .values() is a bit of a weird workaround because it should mean all fields hence still causing a naming conflict.

I'm not opposed to support it, just need some motivation