Opened 16 years ago
Closed 13 years ago
#10498 closed Bug (fixed)
Passing ugettext_lazy to related object's create() doesn't work
Reported by: | Malcolm Tredinnick | Owned by: | Jonas Obrist |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | dceu2011 |
Cc: | anssi.kaariainen@… | Triage Stage: | Design decision needed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As originally reported in #10491, doing this appears to fail (unconfirmed by me at the moment):
user.message_set.create(message=ugettext_lazy(u'xxx'))
It should work.
Attachments (4)
Change History (19)
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 16 years ago
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:4 by , 14 years ago
Easy pickings: | unset |
---|---|
Owner: | changed from | to
UI/UX: | unset |
comment:5 by , 14 years ago
Possible solution: In django.db.models.base.Model.init, check if 'val's that are set onto the model are django.utils.functional.Promise instances, and if so, force_unicode them before setting the attribute.
comment:6 by , 14 years ago
Has patch: | set |
---|---|
Keywords: | dceu2011 added |
comment:7 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Tested that performance are not affected:
Running benchmarks: django_test_postgresql:many_to_one Control: Django 1.4a1 (in git branch master) Experiment: Django 1.4a1 (in git branch 10498) Running 'django_test_postgresql' benchmark ... Min: 0.245787 -> 0.249683: 1.0159x slower Avg: 0.256881 -> 0.257157: 1.0011x slower Not significant Stddev: 0.00550 -> 0.00291: 1.8910x smaller (N = 20) Running benchmarks: django_test_sqlite:many_to_one Control: Django 1.4a1 (in git branch master) Experiment: Django 1.4a1 (in git branch 10498) Running 'django_test_sqlite' benchmark ... Min: 0.053780 -> 0.053740: 1.0007x faster Avg: 0.054773 -> 0.053980: 1.0147x faster Not significant Stddev: 0.00186 -> 0.00040: 4.6798x smaller (N = 20)
comment:9 by , 13 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Severity: | Normal → Release blocker |
Status: | closed → reopened |
While working on #17030 I noticed the isinstance(val, Proxy) calls in the model.__init__
. I did some benchmarking, and confirmed the patch applied in this ticket causes some performance regressions. Using djangobench:
Running 'query_all' benchmark ... Min: 0.021516 -> 0.024808: 1.1530x slower Avg: 0.026032 -> 0.030944: 1.1887x slower Significant (t=-5.668286) Stddev: 0.00516 -> 0.00696: 1.3479x larger (N = 100) Running 'query_all_multifield' benchmark ... Min: 0.043492 -> 0.053683: 1.2343x slower Avg: 0.049113 -> 0.061782: 1.2580x slower Significant (t=-11.048640) Stddev: 0.00686 -> 0.00919: 1.3382x larger (N = 100)
When repeating the query_all_multifield the result seems to stay around 20% slower (mentioning this because for some reason or other djangobench hasn't been that reliable).
I am going to reopen and mark this ticket as a release blocker. The reason is that I think this should be reconsidered in the light of the 20% regression for fetching objects from the db. It seems pretty clear that the performance regression wasn't taken in account when doing the commit, so reconsidering this in light of the regression should be done before 1.4. I am not saying that this must be reverted, just that reconsidering the value of the patch with the new info available should be done.
My initial feeling is that the patch should be reverted, as using ugettext_lazy in models doesn't sound more important than the 20% regression. There are also a couple of other ways forward:
- revert just the part of the commit which touches the args based setattr "fast path" loop. This leaves a bit of inconsistency, but args based
__init__
+ ugettext_lazy isn't that common really. - check isinstance(Promise) when saving the model. That way .create() would work with ugettext lazy objects. I think there is still an unfixed problem if you just assign a Promise object to some field, then save (no, I haven't tested this).
Attached is the full benchmark log. (Note: most of the other results aren't repeatable. url_resolving regression is real, though).
comment:10 by , 13 years ago
Triage Stage: | Ready for checkin → Design decision needed |
---|
The attached patch does the Promise -> unicode conversion much later in the saving cycle. It fixes numerous other ways to pass Promise objects to the DB layer, and in addition the slowdown to model init is now removed.
by , 13 years ago
Attachment: | ticket_10498_alt.diff added |
---|
Alternate way to fix Promise object handling
comment:11 by , 13 years ago
Sorry about my benchmarks in comment:7, I was probably not targeting enough the object creation part in the executed tests. I can confirm Anssi's benchmarks, even if for query_all tests, I only get about 7% slower results.
comment:12 by , 13 years ago
It would be nice to have continuous benchmarking system...
The 5-20% slowdown is somewhat important to deal with, but now that I look at this, the _real_ reason to fix this in the subqueries module is that it is a better place to do the fix. It fixes some situations the original patch doesn't fix. I wonder if the proper place would be in database adapters actually. I don't think now is the time to do that change anyways.
My opinion is that the alternate patch should be committed both because it fixes a performance regression, and also because it fixes more cases to begin with.
comment:14 by , 13 years ago
The new patch looks good to me. Seems reasonable and djangobench reports it fixes the performance regression.
note that the problem is:
something like
<django.utils.functional.__proxy__ object at 0x0110ED10>
shows when i'm trying to access messages in template contextmessages
.so i think there's some connection with #10491 and i put it in #10491 at first.