Ticket #30382 force_insert flag is not passed when saving parents on inherited models

[Akash Sen]

[Looking for an alternate approach]

Ticket link : https://code.djangoproject.com/ticket/30382
Problem : 
When saving we pass force_insert=True to prevent the extra UPDATE statement that precedes the INSERT. The force_insert flag is respected on the child table but not on the parent.

The main problem : On our first go this issue would seem an easy picking as we just have to pass the force_insert flag while saving the parents, but as mentioned here it is going to create a disaster of integrity errors in the Model.objects.create() method with non-abstract inherited parents as the create method assumes force_insert=True.

Approach 1: Set the force_insert=False inside create method if that contains a non-abstract parent model. 

The problem with it is it generates an extra query while creating the non abstract child model when the parent exists, more details can be found here... (Tried in the PR)

Approach 2: Pass an extra flag to the save method and if the method call is coming from create then prevent passing force_insert flag(i.e set it to false) while saving parents when called via create, but as we cannot change the signature of the save method we cannot simply take an extra flag, so this comes down to a very simple question : 

How to pass a flag to a python method without changing its signature:

I can think of three approaches for this,

1. Inspecting a stack frame(Deprecated because of performance issues) - Tried in the PR
2. Using a global variable (Deprecated because of performance issues)

3. Using a class variable (Deprecated because of backward compatibility as names can collide with method, variable, property name of a model) - Tried in the PR

PR link : https://github.com/django/django/pull/16830

If someone can suggest an alternate approach that would be great!

[charettes]

I left some notes on the PR but I think the crux of the issue here is that you are trying to change the meaning of Model.save(force_insert=True) from force the insert of the current model to force the insert of the model and all its bases.

This is a problem not only for QuerySet.create but likely for a many other use cases in the wild that need a way to only force the creation of the leaf table but not others. In other words, the change you are proposing take one feature way (only insert leaf model) to introduce a new one (insert all bases) and thus is fundamentally backward incompatible.

To me the only way forward here is to introduce a new feature but as you've mentioned changing the signature of Model.save() is comple.

The only viable option I can think of is to change the accepted type for force_insert from bool to Type[Model] | Tuple[Type[Model]] to allow save_parent to determine which one should be forced inserted and which shouldn't.

In the case of the reported usage case

```

class ParentModel(models.Model):

    id = models.BigIntegerField(primary_key=True)

class ChildModel(ParentModel): pass

```

That would mean that force_insert=ParentModel must be passed to opt-in into the full insertion mode (both issubclass(ChildModel, ParentModel) and issubclass(ParentModel, ParentModel))

```

ChildModel(id=1).save(force_insert=ParentModel)

```

That seems  like a viable option to me because

- An empty tuple is falsy which is coherent with force_insert=False

- A non-empty tuple is truthy and which is coherent with the fact the current leaf model must be inserted (current behavior)

- Complex MTI scenario forced insertion needs can be targeted with specially crafted base tuples (think of diamond inheritance)

- If a deep ancestor must be inserted then it means that all its children must also be inserted (makes sense due to how children must reference to parent)

- force_insert=models.Model can be used as a generic way to specify that all bases must be inserted independently of the chain of models involved

Hope that makes sense!

Cheers,

Simon

[Sarah Boyce]

+1

Looking at the PR it looks like a boolean is still an accepted value for force_insert which is nice as the feature looks to be used quite a bit (just a rough idea https://github.com/search?type=code&auto_enroll=true&q=force_insert%3DTrue). 

Might be a stupid question, reading the docs https://docs.djangoproject.com/en/4.2/ref/models/instances/#ref-models-force-insert, force_update is currently the opposite of force_insert. Would/should force_update accept the same pattern? 

[charettes]

Sarah, not a stupid question at all.

While `force_update` is a somewhat opposite of `force_insert` there are nuances in how they are handled in the face of MTI (multi-table inheritance).

The proposed issubclass'esque additional signature (ModelBase | tuple[ModelBase]) works in the case of `force_insert` because if a base has to be inserted then all of its subclasses must be as well. In other words, if a row must be inserted all rows that depend on it for referential integrity must as well. This property doesn't hold for `force_update` as you might want to update a base but not all its children.

We already have a way to specify which base should be  `force_update`d via `update_fields` which even offer per-base-field granularity.

To summarize, while `force_update` and `force_insert` are mutually exclusive on a per-base basis I don't think the latter should accept the same pattern particularly because it can already be achieved by using `update_fields`.

Cheers,

Simon

[Akash Sen]

Also the current method initially tries to UPDATE, if not UPDATED then it tries to INSERT. So the case of having consecutive UPDATES is already optimized with minimum possible number of queries.