Opened 5 months ago

Closed 5 months ago

Last modified 4 months ago

#33952 closed Bug (fixed)

Too aggressive pk control in create_reverse_many_to_one_manager

Reported by: Claude Paroz Owned by: David Wobrock
Component: Database layer (models, ORM) Version: 4.1
Severity: Release blocker Keywords:
Cc: David Wobrock Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the context of #19580, Django now requires an instance pk to even instanciate a related manager [7ba6ebe9149a].

Now I have a use case where I need to introspect the model used by a related manager (MyModel().related_set.model) and Django 4.1 refuses that with ValueError: 'MyModel' instance needs to have a primary key value before this relationship can be used.

My opinion is that is is too aggressive of a check and would suggest to let the __init__ succeed even if the instance has no pk. Other calls to _check_fk_val in the class seems sufficient to me to safeguard against shooting in the foot.

Change History (9)

comment:1 Changed 5 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

I think this worth doing, having get_queryset perform self._check_fk_val() should catch the currently tested .all() case as well.

In the mean time I think you should be able to use MyModel._meta.get_field("related_set") for your introspection needs.

Last edited 5 months ago by Simon Charette (previous) (diff)

comment:2 in reply to:  1 Changed 5 months ago by Claude Paroz

Replying to Simon Charette:
...

In the mean time I think you should be able to use MyModel._meta.get_field("related_set") for your introspection needs.

Thanks for the tip! I just needed to remove the _set as it's not part of the ManyToOneRel field name in _meta.

comment:3 Changed 5 months ago by David Wobrock

Cc: David Wobrock added
Has patch: set
Owner: changed from nobody to David Wobrock
Status: newassigned

I provided a small PR with the changes discussed by Simon.
There a few behaviour changes, tell me what you think! :)

comment:4 Changed 5 months ago by Mariusz Felisiak

Severity: NormalRelease blocker

comment:5 Changed 5 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:6 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 806e9e2d:

Fixed #33952 -- Reallowed creating reverse foreign key managers on unsaved instances.

Thanks Claude Paroz for the report.

Regression in 7ba6ebe9149ae38257d70100e8bfbfd0da189862.

comment:7 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In fca05531:

[4.1.x] Fixed #33952 -- Reallowed creating reverse foreign key managers on unsaved instances.

Thanks Claude Paroz for the report.

Regression in 7ba6ebe9149ae38257d70100e8bfbfd0da189862.

Backport of 806e9e2d0dcf8f58e376fb7e2a8b9771e2a9ce16 from main

comment:8 Changed 5 months ago by Claude Paroz

Many thanks David for the fix!

comment:9 in reply to:  8 Changed 4 months ago by Steven Mapes

Replying to Claude Paroz:

Many thanks David for the fix!

Are we sure this is fixed as I still get these errors in all 4.1 versions? I posted an example on another ticket, #33984, which I thought was more related to the issue. you can find that report here

Note: See TracTickets for help on using tickets.
Back to Top