Re: [Django] #8892: ForeignKey relation not saved as expected
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: new
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Ready for | Keywords:
checkin | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by blacklwhite):
* has_patch: 0 => 1
* ui_ux: => 0
* easy: => 0
* stage: Accepted => Ready for checkin
Comment:
I fixed the bug as follows. If you save a model with a foreign key to a
model which is not saved until this point, django will show an error:
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> a.b = b
>>> a.save()
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "django\db\models\base.py", line 492, in save
self.save_base(using=using, force_insert=force_insert,
force_update=force_update)
File "django\db\models\base.py", line 521, in save_base
self.refresh_foreign_keys()
File "django\db\models\base.py", line 475, in refresh_foreign_keys
". Have you already saved the " + str(fk_ref) + " object?")
ValueError: Cannot find a primary key for b as a foreign key in an
instance
of ModelA. Have you already saved the b object?
}}}
If you save a related model '''after''' setting the related one to the
first object, it works as expected.
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> a.b = b
>>> b.save()
>>> a.save()
}}}
----
Problems I've considered (sometimes I use id as synonym for the foreign
key of an object):
* Compatible to users accessing to the foreign key field as follows:
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> b.save()
>>> a.b = b
>>> a.b_id
}}}
So it is neccessary to set the id to a_id during the __set__ method of a.b
= b. However the assertion that the id of b is the same as a.b_id must be
before writing a to the database.
* Not loading the related object from the database.
{{{
>>> a = ModelA.objects.get(name="a")
>>> a.name = "b"
>>> a.save()
}}}
The instance of Object b will not be loaded from the database to reduce
db-queries.
* This patch does only try to set the primary key of the related object,
if no primary key is set and a related object exists. In this way it will
never do an additional database query. Of course it produces an overhead
for every call of the save() method, but by comparison to the following
database-query it is negligible.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Version: 1.0 | (models, ORM)
checkin | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by blacklwhite):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Version: 1.0 | (models, ORM)
checkin | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by blacklwhite):
* status: closed => reopened
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Needs documentation: 0 | Has patch: 1
Patch needs improvement: 0 | Needs tests: 1
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* needs_tests: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Please don't mark your own patches as RFC — see the contribution
guidelines.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 1
Patch needs improvement: 0 | Needs tests: 1
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Comment (by blacklwhite):
Sorry, I misinterpreted the guideline chart.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
@blacklwhite:
Thanks for having a go at this. I think this patch is the wrong approach,
however. It is unnecessarily complicated, and changes the semantics of
saving an object in some significant ways, by adding a new point where the
object's fields are set from the related object.
We want a smaller fix, which only changes existing behaviour for the case
when a developer has made a mistake - as in the example above. We already
have checking at the point of assignment, such as checking that the object
is of the right type, which is in `SingleRelatedObjectDescriptor`, and
that's where the new check should be made. It should be a two liner.
Of course, this also needs tests.
On a point of style, you should use % interpolation for building up
strings.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
Thank you for your feedback. Your hint made it much more simpler to find
the correct piece of code to bugfix it. And - as you said - it fits
formerly as a two liner into the existing code :-)
The patch will now raise an error and results in the following behaviour:
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> a.b = b
ValueError: Cannot assign "<ModelB: ModelB object>": "ModelB.b" must have
a primary key.
}}}
It does the same the reversed scenario.
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> b.a_set.add(a)
}}}
Is the error message ok or would it be better to append "You have to save
<ModelB: ModelB object> before assignment." or something like that?
Are the both tests in the right class / method?
What about compatibility to people using a workaround like follows; don't
considering about that?
{{{
>>> a = ModelA(name="a")
>>> b = ModelB(name="b")
>>> a.b = b
>>> print a.b.name
>>> b.save()
>>> a.b_id = a.b.id
>>> a.save()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
This is much better, but the tests don't test some of the new code. If you
remove the hunk of the patch for `SingleRelatedObjectDescriptor`, the
tests added still pass. (Hint: the cycle described on
[http://en.wikipedia.org/wiki/Test-driven_development Test-driven
development] would have prevented this - always test first and make sure
the test fails).
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner: blacklwhite
Type: Bug | Status: reopened
Milestone: | Component: Database layer
Version: 1.0 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 1
Patch needs improvement: 1 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Comment (by blacklwhite):
Replying to [comment:16 lukeplant]:
> This is much better, but the tests don't test some of the new code. If
you remove the hunk of the patch for `SingleRelatedObjectDescriptor`, the
tests added still pass. (Hint: the cycle described on
[http://en.wikipedia.org/wiki/Test-driven_development Test-driven
development] would have prevented this - always test first and make sure
the test fails).
I used the following command `runtests.py --settings=test_sqlite
many_to_one` to test django. I've tried it again with the current django
trunk and the first added assertion ends in a failed test. The second one
ends in an errored test because of raising an IntegrityError.
After installing the patch the tests work fine.
I used TDD so your statement about the passing tests is curious for me.
Where could my mistake be?
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Component: Database layer | Status: reopened
(models, ORM) | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
-------------------------------------+-------------------------------------
Comment (by akaariai):
I wonder if the set is the proper place to raise the error. One could
construct model instances without ever saving them. Isn't the error really
happening at save time?
The check would be to go though foreign key fields in model.save(). If the
instance's `__dict__` contains a model for the foreign key field's name,
but the value for field.attname in self.`__dict__` is missing or is None,
then raise an error.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Type: Bug | blacklwhite
Component: Database layer | Status: reopened
(models, ORM) | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anonymous):
Here's a [http://stackoverflow.com/questions/13248994/django-assigning-
foreign-key-before-target-model-is-saved StackOverflow question] about
this
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Type: Bug | blacklwhite
(models, ORM) | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by altlist):
I am a little confused why the original example isn't supported.
{{{
>>> a = ModelA(name='foo')
>>> b = ModelB(name='bar')
>>> b.a = a # Set relation *before* saving related object
>>> a.save() # Save related object
>>> a.pk
123
>>> b.a.pk
123
>>> b.save()
>>> b = ModelB.objects.get(name='bar')
>>> b.a
<NoneType: None>
}}}
While I agree the docs suggests auto-increment keys can be determined
upfront, the django model above shows a.pk is already known BEFORE I save
b. Hence I don't see why Django can't save object b correctly.
Instead, I have to use the below suggested hack in the StackOverlow
question. Yet this seems cumbersome and unnecessary.
{{{
>>> b.a = b.a
>>> b.save()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Type: Bug | blacklwhite
Component: Database layer | Status: new
(models, ORM) | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
The original example isn't supported because, deep inside the ORM, `b.a =
a` is implemented as `b.a_id = a.id`.
Changing this design would require rewriting most of the related fields
code, which isn't for the faint of heart ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Type: Bug | blacklwhite
Component: Database layer | Status: new
(models, ORM) | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
What we could possibly do is check that b.a.id == b.a_id. If not, then
throw an error. This would break some code for sure, but on the other hand
it would be a nice protection. Instead of saving corrupted data the ORM
layer forces you to write code where both b.a.id and b.a_id agree on the
value.
There are a couple of other options, too, like when a.save() is called,
then b.a_id is set (requires that a knows it is assigned to b), or on
b.save() use the value of b.a.id instead of b.a_id (this will break code
that relies on saving b.a_id).
Raising an exception whenever b.a_id doesn't agree on b.a.id's value is
worth considering. I believe exception is a lot better than the
possibility for bugs that lead to silent data corruption.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: julien | Owner:
Type: Bug | blacklwhite
(models, ORM) | Version: 1.0
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
Looks like a duplicate of #10811.
--
Ticket URL: <https://code.djangoproject.com/ticket/8892#comment:24>