Opened 15 months ago

Closed 15 months ago

Last modified 7 weeks 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 (12)

comment:1 Changed 15 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 15 months ago by Simon Charette (previous) (diff)

comment:2 in reply to:  1 Changed 15 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 15 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 15 months ago by Mariusz Felisiak

Severity: NormalRelease blocker

comment:5 Changed 15 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:6 Changed 15 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 15 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 15 months ago by Claude Paroz

Many thanks David for the fix!

comment:9 in reply to:  8 Changed 14 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

comment:10 Changed 7 weeks ago by Alex Matos

I already made a comment on #19580, but since this ticket is related, I'm gonna comment here as well. I believe the changes on these two tickets should be fully reverted. I'm upgrading a large Django codebase from version 3.2 to 4.2 and a lot of code broke due to these changes, where code that was once returning an empty queryset, now raises a ValueError. In the name of Django's stability, I think changing user code behavior without a deprecation warning should be avoided at all costs.

comment:11 Changed 7 weeks ago by Andrey Ferriyan

Any news on this?.

Several codes broke due to the migration to the stable LTS version 4.2. At least we have like temporary solution if it is possible.

comment:12 in reply to:  11 Changed 7 weeks ago by Mariusz Felisiak

Replying to Andrey Ferriyan:

Any news on this?.

Several codes broke due to the migration to the stable LTS version 4.2. At least we have like temporary solution if it is possible.

I'm not sure what kind of news do you expect. This change was made in Django 4.1 (which is already in extended support). Nobody started a discussion as I requested 20 months ago, so there is nothing new here that could change a previous decision. Also, please don't spread comments across multiple tickets. If you have something to add, leave a comment in #19580.

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