Saving models with unsaved ForeignKey

[Patrick Robetson]

Previous tickets relating to this are #10811 and #8892 (which have been closed in lastest Master/1.8)
Some work has already been done to deal with assigning unsaved foreign keys to models (a ValueError is now raised when assigning) but I still think there is more work to be done.

First off, I do not think it is strictly 'right' that a ValueError is raised if you assign a ForeignKey with a null ID to a model. Something like this might be useful:

class ModelA(models.Model):
    name = models.CharField(max_length=10)

class ModelB(models.Model):
    name = models.CharField(max_length=10)
    a = models.ForeignKey(ModelA) # note the lack of null=True. Which I believe more common than the edge case when null=True is defined

b = ModelB(name='me', a=ModelA())
if b.conditional_test():
   b.save()
else:
   # discard b and don't save

I don't think any checks should be done until a ModelB object is actually saved to the database - when creating a ModelB object there is no requirement that it be saved to the database, so why should a database-related check (that a.id != None) be made?

I am aware of the chance of data loss, but this is also an edge case - if ModelB is defined as having null=True for the ModelA ForeignKey (if null=False then an IntegrityError is raised upon save as expected)
So in this case, perhaps a ValueError should *only* be raised if null=True on the ForeignKey field.

In my view, this is a regression - a model is just a Python object, and there is no requirement to save it to the database. It should be treated this way (database agnostic) until it is actually saved.

Further to this - Django 1.7 has several issues with saving models that have a ForeignKey with a null id (most likely why this ValueError implementation was introduced.

# Django 1.7, with null=False set on the Foreign Key field
>>> b = ModelB(name='b', a=ModelA(name='a'))
>>> b.a.save()
>>> print b.a.pk
7 # a now has a pk
>>> b.save()
Integrity Error raised... a_id is null

# re-assiging post save fixes this:
>>> b = ModelB(name='b', a=ModelA(name='a'))
>>> b.a.save()
>>> b.a = b.a
>>> print b.a.pk
7 # a now has a pk
>>> b.save()
# works!

In light of this, I think that the ValueError check should either be reverted (or left only for when null=True on the ForeignKey - the only case when data loss is possible) AND in other cases, when saving a model with a foreign key, any 'backwards' relationships should have their _id value set (so as to avoid having to re-assign the foreign key). I'm not too sure of the internals of Dj models, but perhaps a ForeignKey field could add some sort of post_save() signal to itself, so that when a is saved

>>> b = ModelB(name='b', a=ModelA(name='a'))
>>> b.a.save()
# behind the scenes, in a.save() (or wherever)
... self.save()
... self.send_post_save_signal()
... # in ForeignKey
... def deal_with_foreign_key_post_save(obj):
...     modelb.a_id = obj.id

[Tim Graham]

It may be worth revisiting this.

[Anssi Kääriäinen]

Is this really a release blocker?

[Patrick Robetson]

@akaariai - OK I did create the issue, but I think it definitely is.

For anybody else that has code with the same workflow as mine - create a django model.Model in memory, then save it later on - with this change our code will just break straight away. It also means that creating a ForeignKey('something', null=True) is essentially pointless as the problematic changes discussed here won't allow it.

Finally, and an example I've just thought of now: bulk_create will no longer work:
(Based on ModelA and ModelB in this issue:

model_bs = [ModelB(name=str(i)) for i in xrange(0,10)]
ModelB.objects.bulk_create(model_bs)

[Anubhav Joshi]

From #10811
'..prefer failing early and loudly, by raising an exception when an unsaved object is assigned to a related field...'

• First, it does not matter if null=True is set or not, if user is setting an unsaved object to a FK, its wrong so the user must be told that what he is doing is wrong.
• Secondly, I don't think that bulk_create problem occurs because of that in the example you have given, its because null=True is not set so a FK value is required there and you haven't specified one.(correct me if I am wrong, p.s. the check for id runs only when FK object is not None.)
• Thirdly, it is consistent with 3190abc — a fix for the same problem for reverse one-to-one relations.

[Patrick Robetson]

@coder9042 - you're right about the bulk_create reference. I hadn't quite thought that through, and the example I gave wasn't great.

So if we assume that this change *is* right, and that what I'm suggesting *isn't* right, what would be the suggested method for trying to achieve what I want? This change is forwards incompatible, and I cannot see any way of migrating code to work with this change, other than forcing potentially 100s of 'wasted' objects to be stored in the database, *just* to meet a Django validation.

Again, to reiterate incase you can come up with any forwards compatible solution, here's my scenario (fleshed out a bit more):

mods = []
for xml_element in xml.findall('item'):
     a = ModelA(name=xml_element.findtext('name')) # and other things
     b = ModelB(name='me', a=a)
     mods.append(b)

values_to_save = [c for c in mods if b.conditional_test_based_on_a_and_b()]

# save the values_to_save, discard the rest

Currently the only forwards compatible solution I see is to create another object class that mirrors my ModelB class, do all the logic with that, then 'convert' it to a ModelB object and save it if required. Not very DRY

[Thomas C]

I'd rather use something like this which makes the code more explicit :

mods = []
for xml_element in xml.findall('item'):
     a = ModelA(name=xml_element.findtext('name')) # and other things
     b = ModelB(name='me')
     mods.append((a, b))

# save the values_to_save, discard the rest
for a, b in mods:
    if ...some condition...:
        a.save()
        b.a = a
        b.save()

However, I agree that if assigning an unsaved instance of ModelA to b.a was allowed before and no longer is (do I understand correctly ?), that might break some code...

[Aymeric Augustin]

Previously, running b.a = a where a is an unsaved model instance was silently ignored. Not raising an error was a data loss bug.

The reporter asks to delay that error until one tries saving the model, which I considered at the time and rejected, because:

• It's more common to save the model instances you create than throw them away; if you don't intend saving something to the database, there's no point in using Django models;
• It's better to raise an error at the point where there's (likely) a mistake in the code than at an unrelated point later on;
• The proposal would be awfully complicated to implement properly.

[Anubhav Joshi]

@pjrobertson

I think you have got the answer to your question, thanks to @tchaumeny and @aaugustin.

[Patrick Robetson]

@tchaumenmy - thanks for giving your recommendations, I think it's probably been concluded that this will not be changed, and I can see that my case is probably in the minority - even if there is no solution for me.

The problem I have with @tchaumenmy's solution is that my if ...some condition... relies on the foreign keys of b.

Let's say that A/B are expanded to be something like:

class ModelA(models.Model):
    name = models.CharField(max_length=10)
    price = models.IntegerField()

class ModelB(models.Model):
    name = models.CharField(max_length=10)
    sub_item = models.ForeignKey(ModelA) # note the lack of null=True. Which I believe more common than the edge case when null=True is defined
    price = models.IntegerField()
    
    @property
    def total_price(self):
        return self.price + sub_item.price

Now if I used tchaumenmy's solution here, it would not work.

for a,b in mods:
    if b.total_price > 100:
        a.save()
        b.a = a
        b.save()

Granted, I could do:

for a,b in mods:
    if b.price + a.price > 100:
         # save

But this is a not very practical (and this example is an oversimiplified case)

I expect my only solution is going to be to create another class which mirrors exactly the Django model classes, then when I want to actually save one of the objects I will just 'transfer' it over to the Mobdel class and save it.

I agree with aaugustin that models are saved more often than not, so going out of your way to make it work is probably not work it for the core team.