Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#25160 closed Cleanup/optimization (fixed)

Move unsaved model instance assignment check to model.save()

Reported by: Tim Graham Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: 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

As proposed in the last comments of #10811, some people wish to use the ORM with unsaved model instances so it's been proposed to move the data loss check to the model.save() method. Such a solution would require additional consideration for bulk_create() since save() isn't called in that scenario. I've attached an initial patch to give an idea of the changes required.

Attachments (1)

25160.diff (4.9 KB) - added by Tim Graham 2 years ago.
initial work

Download all attachments as: .zip

Change History (17)

Changed 2 years ago by Tim Graham

Attachment: 25160.diff added

initial work

comment:1 Changed 2 years ago by Tim Graham

See 5643a3b51be338196d0b292d5626ad43648448d3 for the original commit and 81e1a35c364e5353d2bf99368ad30a4184fbb653 which could be reverted.

comment:2 Changed 2 years ago by Carl Meyer

+1 to this change. I think the ability to use ORM model instances purely in memory is important for testing. In my own upgrade to 1.8 I simply disabled the new check by switching all my models to use a ForeignKey with the bypass class attribute set, which was a pain.

comment:3 Changed 2 years ago by Carl Meyer

Since the purpose of bulk_create is speed I think it may be preferable to just not have this check there than to check all instances. It could be benchmarked to see whether adding the check slows it down noticeably.

comment:4 Changed 2 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

I'll keep working on this. Carl, are you in favor of a backport to 1.8 as Anssi was in the linked ticket?

comment:5 in reply to:  4 Changed 2 years ago by Carl Meyer

Replying to timgraham:

I'll keep working on this. Carl, are you in favor of a backport to 1.8 as Anssi was in the linked ticket?

I think so. The original change was a backwards-incompatible regression, which should have had a deprecation path at the very least, if we even wanted to prohibit all in-memory use (which I don't think we should). So I think that regression justifies a backported fix.

comment:6 Changed 2 years ago by Tobias McNulty

+1 to this change as well. I discovered we were using related model objects for unsaved data while upgrading an old project and stumbled through several different options as workarounds before eventually settling on this: https://www.caktusgroup.com/blog/2015/07/28/using-unsaved-related-models-sample-data-django-18/

So, if you don't happen to backport to 1.8, there is a workaround, but in the end I do think it makes a lot more sense to validate this in save() rather then at the time of assignment.

comment:7 Changed 2 years ago by Tim Graham

Has patch: set

comment:8 Changed 2 years ago by Carl Meyer

Triage Stage: AcceptedReady for checkin

comment:9 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 5980b05c:

Fixed #25160 -- Moved unsaved model instance data loss check to Model.save()

This mostly reverts 5643a3b51be338196d0b292d5626ad43648448d3 and
81e1a35c364e5353d2bf99368ad30a4184fbb653.

Thanks Carl Meyer for review.

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

In e4b813c:

[1.8.x] Fixed #25160 -- Moved unsaved model instance data loss check to Model.save()

This mostly reverts 5643a3b51be338196d0b292d5626ad43648448d3 and
81e1a35c364e5353d2bf99368ad30a4184fbb653.

Thanks Carl Meyer for review.

Backport of 5980b05c1fad69eef907e0076aa2dc837edab529 from master

comment:11 Changed 2 years ago by Aymeric Augustin

Resolution: fixed
Status: closednew

For consistency, I think we should make the same change on reverse relations.

comment:12 Changed 2 years ago by Aymeric Augustin

Has patch: unset
Owner: changed from Tim Graham to Aymeric Augustin
Status: newassigned
Triage Stage: Ready for checkinAccepted

comment:13 Changed 2 years ago by Aymeric Augustin

Has patch: set

comment:14 Changed 2 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:15 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In c3904deb:

Fixed #25160 (again) -- Moved data loss check on reverse relations.

Moved data loss check when assigning to a reverse one-to-one relation on
an unsaved instance to Model.save(). This is exactly the same change as
e4b813c but for reverse relations.

comment:16 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 1abd177:

[1.8.x] Fixed #25160 (again) -- Moved data loss check on reverse relations.

Moved data loss check when assigning to a reverse one-to-one relation on
an unsaved instance to Model.save(). This is exactly the same change as
e4b813c but for reverse relations.

Backport of c3904de from master

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