Opened 15 years ago

Closed 7 years ago

Last modified 4 years ago

#11390 closed Bug (fixed)

If you use a callable as default value on a model field, it gets called 3 times.

Reported by: espen.nettbruk@… Owned by:
Component: Forms Version: 1.0
Severity: Normal Keywords: models
Cc: danny.adair@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I belive i found a bug in the Django code, im truly sorry if it isnt. I just added a callable (a function) as the default value to a models field. And when i then initate the model, e.x by refreshing the admin edit page, I can se that the function runs three (3) times. There is no reason this should happen and it could lead to performance issues if the calulations done in this function is though.

Change History (21)

comment:1 by Russell Keith-Magee, 15 years ago

Triage Stage: UnreviewedAccepted

I've verified that this is actually happening. Without digging into details, I'm pretty sure the three calls are caused by:

  1. Instantiating an empty Model() instance to pass to the form
  2. Rendering the visible form field, and therefore the value displayed
  3. Rendering the hidden form field providing a comparison initial-value.

It might be possible to collapse these calls into a single usage, but it will take some spelunking to sort out the details. In the meantime, the workaround is to keep your default callables lightweight (or, if they can't be lightweight, cache the computed values).

comment:2 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:3 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:4 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:5 by danny.adair@…, 12 years ago

Cc: danny.adair@… added

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/

comment:6 by Aymeric Augustin, 11 years ago

Component: Core (Other)Forms

comment:7 by Jeremy Dunck, 9 years ago

Possibly fixed in #24391

comment:8 by Lorenzo Pascucci, 9 years ago

Owner: changed from nobody to Lorenzo Pascucci
Status: newassigned

in reply to:  7 comment:9 by Lorenzo Pascucci, 9 years ago

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

comment:10 by Lorenzo Pascucci, 9 years ago

Owner: Lorenzo Pascucci removed
Status: assignednew

comment:11 by Tom V, 9 years ago

Owner: set to Tom V
Status: newassigned

comment:12 by Tom V, 9 years ago

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:

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 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 sample Django project is here, and includes stack traces from the 3 calls.

comment:13 by Tom V, 9 years ago

Has patch: set

comment:14 by Tim Graham <timograham@…>, 9 years ago

In a4a58811:

Removed redundant model instantiation in contrib.admin; refs #11390.

comment:15 by Tim Graham, 9 years ago

Has patch: unset

comment:16 by Tom V, 9 years ago

Owner: Tom V removed
Status: assignednew

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.

comment:17 by MaartenPI, 7 years ago

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()
Last edited 7 years ago by Tim Graham (previous) (diff)

comment:18 by MaartenPI, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:19 by Tim Graham, 7 years ago

Resolution: fixed
Status: newclosed
Triage Stage: Ready for checkinAccepted

comment:20 by GitHub <noreply@…>, 4 years ago

In 0bf627f0:

Refs #11390 -- Clarified dual-calling of ChoiceField.choices callable.

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 8178c4f:

[3.1.x] Refs #11390 -- Clarified dual-calling of ChoiceField.choices callable.

Backport of 0bf627f0b2f868cdcc53ac12cc7f390901d4b83d from master

Note: See TracTickets for help on using tickets.
Back to Top