Fix #3472: allow None for nullable FK/OneToOne _id lookups - #3476
Fix #3472: allow None for nullable FK/OneToOne _id lookups#3476usamashehab wants to merge 3 commits into
Conversation
The _id attname of a ForeignKey or OneToOneField (e.g. prospect_id) was not correctly resolved when the field name differed from the related model's lowercase name. The previous check compared field_part against model_name + '_id', which only worked when the FK name matched the model name (e.g. publisher -> Publisher). Fixed by checking field.attname instead, which is the actual column name Django uses for the _id variant. Also preserves the original FK's nullability: the underlying PK field is always null=False, but the column is nullable when the FK has null=True, so None is now accepted for nullable _id lookups. ManyToManyField is excluded because its attname equals its name (no _id column variant exists).
federicobond
left a comment
federicobond
left a comment
There was a problem hiding this comment.
The bare positional bool returned from _resolve_field_from_parts reads a bit awkwardly, and is discarded at most call sites. It also carries a fairly narrow special-case name.
What do you think about introducing a small namedtuple ResolvedLookupField(field, model, is_nullable), where is_nullable is the effective nullability of the looked-up column? That way the resolver owns the nullability decision and hands it down as a general fact.
…dtuple Per review feedback from @federicobond: replace the bare positional bool with a ResolvedLookupField NamedTuple that owns the nullability decision. The resolver now hands down is_nullable as a general fact instead of a narrow is_nullable_fk_id special-case.
This comment was marked as outdated.
This comment was marked as outdated.
|
Look, if you want to continue contributing to the project I suggest you re-read the AI policy you agreed to. If I have to respond to a comment written entirely by an LLM I would rather work with the model directly instead of through a slow human intermediary. It's not bad to use AI but if you are not willing to put in the time to write a short reply in your own words it's just not worth it to take your contribution seriously. |
my skills in writing is not good, that's why i am using AI to write a good response but seems that is not the best behavior, will try to write on my own |
yes pls, it's not an issue if your writing is not perfect, and practice is the only way to make it better. |
| # The lookup uses the FK's ``_id`` suffix (e.g. ``prospect_id``). | ||
| # Django's ``get_field`` returns the ``RelatedField`` for this | ||
| # attname, but semantically it refers to the underlying column, | ||
| # so resolve to the related model's primary key field instead. | ||
| # Preserve the original FK's nullability — the PK field itself | ||
| # is always ``null=False`` but the column is nullable when the | ||
| # FK is. | ||
| # ``field_part != field.name`` excludes ManyToManyField, whose | ||
| # ``attname`` equals its ``name`` (no ``_id`` column variant). |
There was a problem hiding this comment.
Can you simplify this comment ? lot's of noise
It should focus on explaining the if conditional, mostly the last 2 lines I think ?
Also let the comment take more width and avoid breaking in the middle of sentences, it should be readable for a human as much as possible
3 participants
Related issues
Fixes #3472
Problem
Filtering on the
_idcolumn of a nullable ForeignKey or OneToOneField withNonewas incorrectly rejected by mypy:This has two root causes in
_resolve_field_from_parts:Field name detection: The old check compared
field_partagainstmodel_name + "_id"(the related model's lowercase name +"_id"). This only worked when the FK field name happened to match the related model name (e.g.publisher→Publisher). When the field name differs (e.g.prospect→OtherModel), the check"prospect_id" == "othermodel_id"failed, and the field stayed as theRelatedFieldinstead of being resolved to the PK column field.Nullability: Even when the
_idvariant was correctly detected (e.g. forpublisher_id), the resolved PK field always hasnull=False. Soget_field_lookup_exact_typereturnedstr | intwithoutNone, even for nullable FKs whereprospect_id=Noneis valid at the database level.Fix
model_name + "_id"check withfield.attname, which is the actual column name Django assigns to the FK's_idvariant. This works regardless of whether the field name matches the model name.is_nullable_fk_idtracking: when a FK_idattname is detected, the original FK'snullattribute is preserved and passed through toget_field_lookup_exact_type, soNoneis accepted for nullable_idlookups.ManyToManyFieldviafield_part != field.name(M2M fields haveattname == name, no_idcolumn variant exists).AI Policy