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

[Tim Graham]

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.

[Tim Graham]

initial work

[Tim Graham]

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

[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.

[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.

[Tim Graham]

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

[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.

[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.

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

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.

[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

[Aymeric Augustin]

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

[Aymeric Augustin]

​https://github.com/django/django/pull/5317

[Aymeric Augustin <aymeric.augustin@…>]

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.

[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