[Django] #33952: Too aggressive pk control in create_reverse_many_to_one_manager
Django
-------------------------------------+-------------------------------------
Reporter: Claude | Owner: nobody
Paroz |
Type: Bug | Status: new
Component: Database | Version: 4.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Replying to [comment:1 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:2>
Django
-------------------------------------+-------------------------------------
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* has_patch: 0 => 1
* status: new => assigned
Comment:
I provided a small [https://github.com/django/django/pull/15995 PR] with
the changes discussed by Simon.
There a few behaviour changes, tell me what you think! :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
Comment:
Regression in 7ba6ebe9149ae38257d70100e8bfbfd0da189862.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"806e9e2d0dcf8f58e376fb7e2a8b9771e2a9ce16" 806e9e2d]:
{{{
#!CommitTicketReference repository=""
revision="806e9e2d0dcf8f58e376fb7e2a8b9771e2a9ce16"
Fixed #33952 -- Reallowed creating reverse foreign key managers on unsaved
instances.
Thanks Claude Paroz for the report.
Regression in 7ba6ebe9149ae38257d70100e8bfbfd0da189862.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"fca055315e9bb6cb1212953478dbe971b0bb47fb" fca05531]:
{{{
#!CommitTicketReference repository=""
revision="fca055315e9bb6cb1212953478dbe971b0bb47fb"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Many thanks David for the fix!
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Steven Mapes):
Replying to [comment:8 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
[https://code.djangoproject.com/ticket/33984#comment:18 here]
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alex Matos):
I already made a [https://code.djangoproject.com/ticket/19580#comment:58
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:11 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 [https://code.djangoproject.com/ticket/19580#comment:57
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Claude Paroz | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I started a community thread for the requested design discussion about
reverting this:
https://forum.djangoproject.com/t/allow-querying-relatedmanagers-on-
unsaved-objects/43962
--
Ticket URL: <https://code.djangoproject.com/ticket/33952#comment:13>