Re: [Django] #11390: If you use a callable as default value on a model field, it gets called 3 times.
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Changes (by danny.adair@…):
* cc: danny.adair@… (added)
Comment:
I would like to add that this not just a performance issue.
I believe it prevents any kind of reliable "counter" fields, includig an
!IntegerField primary key where some control is needed (i.e. not just
autoincrement). See http://stackoverflow.com/a/9354703/640759 (postgresql-
only solution)
Another example would be per-foreignkey counters, such as invoice numbers
which increase per customer account. A workaround there would be to
postpone the pulling of a new counter number until save(). See
http://djangosnippets.org/snippets/1574/
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* component: Core (Other) => Forms
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:6>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by jdunck):
Possibly fixed in #24391
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:7>
Django
times.
Reporter: espen.nettbruk@… | Owner: lorenzo-pasa
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by lorenzo-pasa):
* status: new => assigned
* owner: nobody => lorenzo-pasa
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:8>
Django
times.
----------------------------------+----------------------------------------
Reporter: espen.nettbruk@… | Owner: lorenzo-pasa
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+----------------------------------------
Comment (by lorenzo-pasa):
Replying to [comment:7 jdunck]:
> Possibly fixed in #24391
This bug is still there.
I just checked using the latest Django version, "pip installed" from the
master branch (i.e. Django==1.9.dev20150322063655).
I created a model with a single field, and just added a function as the
default value to this field.
Every time I access its ''add page'' in the admin, the method is called 3
times.
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:9>
Django
times.
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* owner: lorenzo-pasa =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:10>
Django
times.
----------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* owner: => tomviner
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:11>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: tomviner
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by tomviner):
ok, I've located the 3 calls to get the default value, that are made when
you view the admin "add item" page. 2 are from the view and the 3rd from
BoundField, when the form is rendered.
Regarding 2 calls in the view:
[https://github.com/django/django/blob/master/django/contrib/admin/options.py#L1389
django/contrib/admin/options.py: ModelAdmin.changeform_view]
{{{
line 1389: form = ModelForm(initial=initial)
line 1390: formsets, inline_instances = self._create_formsets(request,
self.model(), change=False)
}}}
actually both these lines end up calling model(). ModelForm does it when
setting ModelForm().instance and as you can see the 2nd line there has an
explicit self.model().
So my thought is to simply replace the call to self.model() with
form.instance. i.e.:
{{{
line 1389: form = ModelForm(initial=initial)
line 1390: formsets, inline_instances = self._create_formsets(request,
form.instance, change=False)
}}}
See [https://github.com/django/django/pull/4384 one line PR] with this
change. But it's hard to know if this has unintended effects? So feedback
appreciated on that please.
As for the 3rd call in BoundField. I have no idea how to merge this with
the one above, or if that would be desired anyway. The fact is, an in
memory model object gets created with the default value, and a ModelForm
field/widget also gets created with a fresh call to get the default.
My [https://github.com/tomviner/callable_as_default sample Django project
is here], and includes stack traces from the 3 calls.
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:12>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: tomviner
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:13>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: tomviner
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"a4a58811b9729ed1ab2417d72e5d07ebb1a30886" a4a58811]:
{{{
#!CommitTicketReference repository=""
revision="a4a58811b9729ed1ab2417d72e5d07ebb1a30886"
Removed redundant model instantiation in contrib.admin; refs #11390.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:14>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: tomviner
Type: Bug | Status: assigned
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:15>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner:
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
* owner: tomviner =>
* status: assigned => new
Comment:
Thanks for the merge.
One down one to go. But as I mentioned earlier, I'm not sure how or if
it's possible to combine the two remaining calls.
This simple example shows the 2 calls:
{{{
In [1]: from django.forms.models import modelform_factory
In [2]: from cad.models import Article
In [3]: article_modelform = modelform_factory(Article, fields=('title',))
In [4]: form = article_modelform()
get_default_title call #1
In [5]: form.as_p()
get_default_title call #2
Out[5]: u'<p>...
}}}
One's the in memory model being instantiated and the other's the ModelForm
field/widget populating it's values. They seem separate and I'm not sure
where any memoisation of this value would go or if it would be a great
idea.
I'll unassign myself so others can either take a stab, or chime in that
it's not a worthy goal.
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:16>
Django
times.
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by MaartenPI):
I created stack traces and don't really see a clean way to optimize this
any further. I'll close the ticket, if anyone feels it shouldn't. Please
re-open.
As reference here are the stack traces:
from django.forms.models import modelform_factory
from cad.models import Article
article_modelform = modelform_factory(Article, fields=('title',))
form = article_modelform()
==> gives the following stacktrace:
~/local/lib/python2.7/site-packages/django/forms/models.py(278)__init__()
-> self.instance = opts.model()
~/local/lib/python2.7/site-
packages/django/db/models/base.py(529)__init__()
-> val = field.get_default()
~/local/lib/python2.7/site-
packages/django/db/models/fields/__init__.py(769)get_default()
-> return self.default()
~/callable_as_default/sample_project/cad/models.py(9)get_title()
form.as_p()
==> gives the following stacktrace
~/local/lib/python2.7/site-packages/django/forms/forms.py(289)as_p()
-> errors_on_separate_row=True)
~/local/lib/python2.7/site-
packages/django/forms/forms.py(226)_html_output()
-> 'field': six.text_type(bf),
~/local/lib/python2.7/site-packages/django/utils/html.py(382)<lambda>()
-> klass.__unicode__ = lambda self: mark_safe(klass_unicode(self))
~/local/lib/python2.7/site-
packages/django/forms/boundfield.py(42)__str__()
-> return self.as_widget() + self.as_hidden(only_initial=True)
~/local/lib/python2.7/site-
packages/django/forms/boundfield.py(89)as_widget()
-> attrs = self.build_widget_attrs(attrs, widget)
~/local/lib/python2.7/site-
packages/django/forms/boundfield.py(238)build_widget_attrs()
-> if widget.use_required_attribute(self.initial) and self.field.required
and self.form.use_required_attribute:
~/local/lib/python2.7/site-
packages/django/forms/boundfield.py(225)initial()
-> data = data()
~/callable_as_default/sample_project/cad/models.py(9)get_title()
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:17>
Django
times.
Reporter: espen.nettbruk@… | Owner: (none)
Type: Bug | Status: new
Component: Forms | Version: 1.0
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MaartenPI):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:18>
Django
times.
Reporter: espen.nettbruk@… | Owner: (none)
Type: Bug | Status: closed
Component: Forms | Version: 1.0
Severity: Normal | Resolution: fixed
Keywords: models | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:19>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: (none)
Type: Bug | Status: closed
Component: Forms | Version: 1.0
Severity: Normal | Resolution: fixed
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"0bf627f0b2f868cdcc53ac12cc7f390901d4b83d" 0bf627f0]:
{{{
#!CommitTicketReference repository=""
revision="0bf627f0b2f868cdcc53ac12cc7f390901d4b83d"
Refs #11390 -- Clarified dual-calling of ChoiceField.choices callable.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:20>
Django
times.
----------------------------------+------------------------------------
Reporter: espen.nettbruk@… | Owner: (none)
Type: Bug | Status: closed
Component: Forms | Version: 1.0
Severity: Normal | Resolution: fixed
Keywords: models | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"8178c4fbfba8c425ce2591662dc915722224f7f1" 8178c4f]:
{{{
#!CommitTicketReference repository=""
revision="8178c4fbfba8c425ce2591662dc915722224f7f1"
[3.1.x] Refs #11390 -- Clarified dual-calling of ChoiceField.choices
callable.
Backport of 0bf627f0b2f868cdcc53ac12cc7f390901d4b83d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11390#comment:21>