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 , 10 years ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:3 by , 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 , 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.
comment:5 by , 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 , 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 , 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 , 10 years ago
@pjrobertson
I think you have got the answer to your question, thanks to @tchaumeny and @aaugustin.
comment:9 by , 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 , 10 years ago
Resolution: | → wontfix |
---|---|
Severity: | Release blocker → Normal |
Status: | new → closed |
It may be worth revisiting this.