Re: [Django] #10811: Assigning unsaved model to a ForeignKey leads to silent failures
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
I prefer failing early and loudly, by raising an exception when an unsaved
object is assigned to a related field.
I could listen to an argument for trying to re-fetch the pk from the
cached related instance in save(), but that feels like action-
at-a-distance: the actual problem usually happened earlier.
Automatically saving related objects is too magic; I'm sure it would be
considered unexpected and undesirable in some circumstances.
----
This is a bug that may lead to data loss ; it's pretty bad. Given that it
hasn't been fixed in three years I'm strongly in favor of the simplest
solution, raising an exception.
It would also be consistent with 3190abcd75b1fcd660353da4001885ef82cbc596
— a fix for the same problem for reverse one-to-one relations.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => aaugustin
Comment:
#11811 should be fixed in the same fashion at the same time.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
I have a patch for this. Unfortunately it causes 6 unrelated test failures
in the test suite, some of which look non-trivial.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Note the existence of the opposite ticket: #8070 :'(
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Honza points out that he takes advantage of this ability in tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
I'm attaching a new patch that fixes a few test failures:
- by saving the objects before assigning them to foreign keys,
- by removing explicit tests for the behavior this ticket wants to
prevent, introduced (among other more reasonable changes) in #8070.
With this patch, there are still two tests failures related to admin
inlines.
{{{
======================================================================
ERROR: test_create_inlines_on_inherited_model
(admin_inlines.tests.TestInline)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/myk/Documents/dev/django/tests/admin_inlines/tests.py",
line 192, in test_create_inlines_on_inherited_model
response =
self.client.post('/admin/admin_inlines/extraterrestrial/add/', data)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 460,
in post
response = super(Client, self).post(path, data=data,
content_type=content_type, **extra)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 289,
in post
return self.generic('POST', path, post_data, content_type, **extra)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 339,
in generic
return self.request(**r)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 421,
in request
six.reraise(*exc_info)
File "/Users/myk/Documents/dev/django/django/utils/six.py", line 491, in
reraise
raise value
File "/Users/myk/Documents/dev/django/django/core/handlers/base.py",
line 114, in get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py",
line 467, in wrapper
return self.admin_site.admin_view(view)(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
99, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/views/decorators/cache.py",
line 52, in _wrapped_view_func
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/sites.py",
line 198, in inner
return view(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
29, in _wrapper
return bound_func(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
99, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
25, in bound_func
return func(self, *args2, **kwargs2)
File "/Users/myk/Documents/dev/django/django/db/transaction.py", line
360, in inner
return func(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py",
line 1161, in add_view
if all_valid(formsets) and form_validated:
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
415, in all_valid
if not formset.is_valid():
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
292, in is_valid
err = self.errors
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
267, in errors
self.full_clean()
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
315, in full_clean
self._errors.append(form.errors)
File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 121,
in errors
self.full_clean()
File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 275,
in full_clean
self._post_clean()
File "/Users/myk/Documents/dev/django/django/forms/models.py", line 387,
in _post_clean
self.instance = construct_instance(self, self.instance, opts.fields,
opts.exclude)
File "/Users/myk/Documents/dev/django/django/forms/models.py", line 57,
in construct_instance
f.save_form_data(instance, cleaned_data[f.name])
File
"/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py",
line 637, in save_form_data
setattr(instance, self.name, data)
File
"/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line
337, in __set__
(value, self.field.rel.to._meta.object_name))
ValueError: Cannot assign "<ExtraTerrestrial: ExtraTerrestrial object>":
"ExtraTerrestrial" instance isn't saved in the database.
======================================================================
ERROR: testInline (admin_views.tests.AdminInheritedInlinesTest)
Ensure that inline models which inherit from a common parent are correctly
handled by admin.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/myk/Documents/dev/django/tests/admin_views/tests.py", line
2278, in testInline
response =
self.client.post('/test_admin/admin/admin_views/persona/add/', post_data)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 460,
in post
response = super(Client, self).post(path, data=data,
content_type=content_type, **extra)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 289,
in post
return self.generic('POST', path, post_data, content_type, **extra)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 339,
in generic
return self.request(**r)
File "/Users/myk/Documents/dev/django/django/test/client.py", line 421,
in request
six.reraise(*exc_info)
File "/Users/myk/Documents/dev/django/django/utils/six.py", line 491, in
reraise
raise value
File "/Users/myk/Documents/dev/django/django/core/handlers/base.py",
line 114, in get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py",
line 467, in wrapper
return self.admin_site.admin_view(view)(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
99, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/views/decorators/cache.py",
line 52, in _wrapped_view_func
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/sites.py",
line 198, in inner
return view(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
29, in _wrapper
return bound_func(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
99, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line
25, in bound_func
return func(self, *args2, **kwargs2)
File "/Users/myk/Documents/dev/django/django/db/transaction.py", line
360, in inner
return func(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py",
line 1161, in add_view
if all_valid(formsets) and form_validated:
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
415, in all_valid
if not formset.is_valid():
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
292, in is_valid
err = self.errors
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
267, in errors
self.full_clean()
File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line
315, in full_clean
self._errors.append(form.errors)
File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 121,
in errors
self.full_clean()
File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 275,
in full_clean
self._post_clean()
File "/Users/myk/Documents/dev/django/django/forms/models.py", line 387,
in _post_clean
self.instance = construct_instance(self, self.instance, opts.fields,
opts.exclude)
File "/Users/myk/Documents/dev/django/django/forms/models.py", line 57,
in construct_instance
f.save_form_data(instance, cleaned_data[f.name])
File
"/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py",
line 637, in save_form_data
setattr(instance, self.name, data)
File
"/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line
337, in __set__
(value, self.field.rel.to._meta.object_name))
ValueError: Cannot assign "<Persona: Test Name>": "Persona" instance isn't
saved in the database.
}}}
During the validation phase, in `BaseModelForm._post_clean`, the model
formset attemps to build an in memory representation of the object created
by the inline and to attach the parent. This fails as soon as you can't
attach unsaved objects to foreign keys.
----
Russell helped me track this down inside `InlineModelAdmin`, but I
couldn't find a fix. Here's what we understood.
1) To create the formset for the admin inline, Django calls
`InlineModelAdmin.get_formset` without a `fields` keyword argument.
2) `get_formset` build the list of fields for the form based on the
`fieldsets` defined for the `InlineModelAdmin`: `fields =
flatten_fieldsets(self.get_fieldsets(request, obj))`
3) When the `InlineModelAdmin` doesn't contain an `fieldsets` attribute,
`BaseModelAdmin.get_fieldsets` returns `[(None, {'fields':
self.get_fields(request, obj)})]`
4) `InlineModelAdmin.get_fields` attempts to obtain the list of fields
from its `formset`'s `form`, which it obtains with `form =
self.get_formset(request, obj, fields=None).form`.
5) At this point we're extremely close to an infinite recursion, but
passing the `fields` keyword argument avoids it. This looks very fragile
and I wouldn't be surprised if there was a design error in this area.
Since `fields` is explicitly set to `None`, the inner `get_formset` uses
`forms.ALL_FIELDS` instead.
6) `InlineModelAdmin.get_fields` returns `form.base_fields` which contains
all fields if the `InlineModelAdmin.form` wasn't set to a particular form
class.
6) We're back in the outer `InlineModelAdmin.get_formset` where this list
of fields gets passed explicitly to `inlineformset_factory`.
Thus, the field representing the foreign key to the parent ends up being
explicitly included in the formset's form, which is the root cause of
these test failures.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Jacob thinks that wontfixing on the basis that a) the problem is
complicated b) the current behavior might be useful isn't the answer,
because it's still a bug that can result in data loss, no matter how much
we document it.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner: aaugustin
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* owner: aaugustin =>
* status: assigned => new
Comment:
I'm admitting defeat. If you want to tackle this, my advice in to untangle
the loop in InlineModelAdmin.
The patch also needs release notes as it's probably going to break at
least a few people's code. People who relied on this possibility in tests
should switch to mocking.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Let the following code:
{{{
a = A.objects.create(...)
a.b = B()
a.b.save()
a.save()
}}}
Isn't it as simple as checking if `a.b` is not None and `a.b_id` is None?
If this happens, raise an exception. For instance (surely it must use
getattr and setattr):
{{{
if a.b is not None and a.b_id is None:
if a.b.id is not None:
a.b_id = b.id
else:
raise Exception()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by anubhav9042):
* status: new => closed
* resolution: => worksforme
Comment:
Well I use Django 1.6 and there is no such problem, so I am closing it.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: worksforme =>
Comment:
Given that the problem still existed 4 months ago, that I failed to fix it
after a non-negligible amount of effort (meaning it isn't trivial), and
that I'm not aware of any changes in this area since then (and I would
probably have noticed), I tend to think it isn't fixed.
How did you determine the issue had vanished?
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
ok i realized that i did something wrong, the problem still exists.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: anubhav9042@… (added)
Comment:
Well I found a way to raise warning in Django 1.6.
If it is correct then please tell me how to do the same in dev.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
See line 439.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Changes (by anubhav9042):
* owner: => anubhav9042
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Actually I did something, please review:
https://github.com/django/django/pull/2162
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
It looks like you got to roughly the same place as Aymeric did in his
attempt to solve this (running into problems with InlineModelAdmin). I'm
not really inclined to look into it myself since Aymeric already tried and
it's not clear fixing this ticket will yield much benefit -- from an
earlier comment: "Jacob thinks that wontfixing on the basis that a) the
problem is complicated b) the current behavior might be useful isn't the
answer, because it's still a bug that can result in data loss, no matter
how much we document it."
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
#20554 was a duplicate. See there for some comments from Anssi including
"This might be surprisingly hard to solve correctly."
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Well I went through the entire problem today thoroughly.
The problem is that when in the `inline` test(which causes errors), we are
trying to create an object on the fly and then since it has no `id` so it
cannot be used. Following Aymeric's comments above line by line its true
that form gets fields: `['et', 'place']` out of which `et` which is FK
creates problem in form validation step in `_post_clean` when
`construct_instance` is called.
We need a saved object of `ExtraTerrestrial` class for its `id` to be
saved in `Sighting`.
As far as I think, there are other tests in same test file with models
using the FK, why not try to implement the error causing test case in a
similar way/ or create an object before and send it(dunno if the second
part is possible)
Or
Is it possible to write a check in `_post_clean` and prevent calling of
`construct_instance`?
Need some guidance here.
A similarity between failing tests: Both are dealing with inherited
models. Although I don't think that might be a problem here but still I
thought it better to post.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
Looks like #8892 is another duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by altlist):
One could do this and it does what I expect in Django 1.6
{{{
book.author.save()
book.author = book.author # refresh id
book.save()
}}}
So I extended the original patch in #8892 (ticket) and wrote a
save_with_refresh(obj) function.
{{{
book.author.save()
refresh.save_with_refresh(book)
book.save()
}}}
One could modify models.base.save() to accept a "refresh" keyword that
would do the above.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
I have found out some things regarding the failure of tests, and it is due
to inherited models.
In case of `admin_inline` test(look into
https://github.com/django/django/blob/master/tests/admin_inlines/tests.py#L191):
the Extraterrestrial object is never assigned a `pk`, this failure of
tests is not due to the fix of #10811 but there is some other problem
because a similar test(see
https://github.com/django/django/blob/master/tests/admin_inlines/tests.py#L65)
works fine.
If you look at models, there is:
{{{
class Lifeform():
pass
class ExtraTe...(Lifeform):
pass
class Sighting():
some_fk = ExtraTerr...
}}}
In database instead of `id` in ExtraTerrestrial there is a field
`lifeform_ptr_id`, which never gets a value. I have tried out printing out
`id` of the ET object before and after save is called but it comes out
`None`. So the problem lies there.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Even before the fix for this bug was written, I don't know how the tests
passes because the id has always been None, so I do not understand how
were the tests passing.
Are ids pulled out again at a later point??
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
There is some problem in saving objects in inherited models in case of
inlines as far as I think. It might very well be related to #19524.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Patch complete: https://github.com/django/django/pull/2697
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: altlist@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by altlist):
Any reason why we can't do something like the save_with_refresh()
function?
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
We won't add a new API for an edge case and let developers figure out
which API to use in which context.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by altlist):
Replying to [comment:33 aaugustin]:
> We won't add a new API for an edge case and let developers figure out
which API to use in which context.
I was suggesting an extension to the save() API, where one could pass a
"refresh=True" option. In the meantime, using my own routine.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Replying to [comment:34 altlist]:
> I was suggesting an extension to the save() API, where one could pass a
"refresh=True" option. In the meantime, using my own routine.
Raising an error and notifying that the action is wrong is better approach
than allowing the user to do wrong and correct later.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
(models, ORM) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5643a3b51be338196d0b292d5626ad43648448d3"]:
{{{
#!CommitTicketReference repository=""
revision="5643a3b51be338196d0b292d5626ad43648448d3"
Fixed #10811 -- Made assigning unsaved objects to FK, O2O, and GFK raise
ValueError.
This prevents silent data loss.
Thanks Aymeric Augustin for the initial patch and Loic Bistuer for the
review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | anubhav9042
Component: Database layer | Status: closed
(models, ORM) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"fd5897193f0bd8fa211885be5726f8e5613f3c08"]:
{{{
#!CommitTicketReference repository=""
revision="fd5897193f0bd8fa211885be5726f8e5613f3c08"
Fixed problem with refs #10811.
When 'to_field' is specified with a FK, then we need to check the pk value
the object.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by riklaunim):
Just a note that this change broke factory_boy factories a lot with Django
1.8 - now .build() or .attributes() can't be used if model has a
subfactory (FK or OneToOne relation).
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:38>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ryanhiebert):
I wonder if it would have been possible and better to raise the error when
the model was saved rather than when the related model was assigned. It
would both prevent the data loss issue and allow completely unsaved models
to be used, such as how .build() and .attributes() work for factory_boy
factories.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:39>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I believe the discussion of whether or not the ORM should be usable with
completely in-memory models was answered as "no", but I don't recall the
reasoning and can't cite any discussion. Feel free to start a thread on
the DevelopersMailingList as that's a better place to have a discussion
about this.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:40>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
I must say I believe doing the check at save() time is better than
disallowing assignment completely. This should result in the same data-
loss-preventation effect, while giving better backwards compatibility for
users.
The check in save() would essentially be "if 'fk' in self.__dict__ and
self.fk.id is None -> raise Error" (with suitable replacements to get the
from and to fields dynamically). I believe this is safe to backpatch to
1.8, as currently assigning anything with pk = None to fk will raise an
error. So, there shouldn't be any code where the check is True in released
1.8 versions.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:41>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by mvantellingen):
Replying to [comment:41 akaariai]:
> I must say I believe doing the check at save() time is better than
disallowing assignment completely. This should result in the same data-
loss-preventation effect, while giving better backwards compatibility for
users.
>
> The check in save() would essentially be "if 'fk' in self.__dict__ and
self.fk.id is None -> raise Error" (with suitable replacements to get the
from and to fields dynamically). I believe this is safe to backpatch to
1.8, as currently assigning anything with pk = None to fk will raise an
error. So, there shouldn't be any code where the check is True in released
1.8 versions.
I agree that moving the check to the save() method seems the best
solution. This change really caused our unittests to run much slower since
now we need to create objects in the database to test properties unrelated
to the fk's. How can we get this in 1.9 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:42>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Writing a patch would be a good start. Note that we added a flag to bypass
the check in #24495. I guess that would be reverted then.
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:43>
Django
-------------------------------------+-------------------------------------
Reporter: Glenn | Owner:
| anubhav9042
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Created #25160 for moving the check to save().
--
Ticket URL: <https://code.djangoproject.com/ticket/10811#comment:44>