Opened 10 years ago

Closed 10 years ago

#23449 closed Bug (wontfix)

Saving models with unsaved ForeignKey

Reported by: Patrick Robetson Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: ANUBHAV JOSHI Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (10)

comment:1 by Tim Graham, 10 years ago

Cc: ANUBHAV JOSHI added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

It may be worth revisiting this.

comment:2 by Anssi Kääriäinen, 10 years ago

Is this really a release blocker?

comment:3 by Patrick Robetson, 10 years ago

@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)

comment:4 by Anubhav Joshi, 10 years ago

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.

I think that there is nothing wrong with the current state as the test runs only when object provided is not None, so when null=False and no FK object is provided, things should work correctly if that's the problem here.
IMO if an object is provided, then irrespective of null=True, it should be checked if that object is saved or not.

Last edited 10 years ago by Anubhav Joshi (previous) (diff)

comment:5 by Patrick Robetson, 10 years ago

@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

comment:6 by Thomas C, 10 years ago

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

comment:7 by Aymeric Augustin, 10 years ago

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.

comment:8 by Anubhav Joshi, 10 years ago

@pjrobertson

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

comment:9 by Patrick Robetson, 10 years ago

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

comment:10 by Tim Graham, 10 years ago

Resolution: wontfix
Severity: Release blockerNormal
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top