Code

Opened 6 years ago

Last modified 5 years ago

#8033 new New feature

Better error handling for gettext and gettext_lazy issues

Reported by: msaelices Owned by:
Component: Internationalization Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

It's very common get confused in use of gettext and gettext_lazy functions. An usual error was to use gettext at import time, for example, in any of this cases:

  • In a parameter of settings.py file:
    from django.utils.translation import ugettext as _
    ...
    LANGUAGE_CHOICES = (('es', _('Spanish')),
                        ('en', _('English')),)
    
  • In class or field definition of a models.py file:
    from django.db import models
    from django.utils.translation import ugettext as _
    
    class Author(models.Model):
        name = models.CharField(_(u'Name'), max_length=100)
    

In both cases, if you get an error, the error will be an ImportError usually in another part of code... This is very difficult to debug, because the real error should be "Incorrect use of gettext. You can't use gettext at import time".

The error is due to the fact of not all applications has been imported yet, and gettext resolution need to have all possible translation files loaded (django.po files), which implies loading of all applications. This causes a circular import error problem.

In this thread there is a discussion about this.

We are thinking in check in every call to gettext if django apps has been loaded (using django.db.models.loading.app_cache_ready()) and take one of this options:

  • raise an representative error.
  • return a dummy string.

I personally prefers first one, because gettext should be always called inside functions (at run time), and never at import time. I don't imagine a case of we need use gettext before that.

Attachments (5)

better_error_handling_for_ugettext_r8138.2.diff (2.2 KB) - added by msaelices 6 years ago.
A better patch... but It's seems to have problems in apache detection of app_cache_ready
better_error_handling_for_ugettext_r8138.diff (2.2 KB) - added by msaelices 6 years ago.
A better patch... but It's seems to have problems in apache detection of app_cache_ready
better_error_handling_for_ugettext_r8138.3.diff (2.8 KB) - added by msaelices 6 years ago.
This patch fixes apache problems, forcing apps loading just in request processing
testproj.tgz (8.0 KB) - added by msaelices 6 years ago.
Test project
better_error_handling_for_ugettext_r8377.diff (3.9 KB) - added by msaelices 6 years ago.
Using a flag for no try translation while django apps is still loading

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by msaelices

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Previous patch pass all tests, but if you test in an apache deployment, it fails in rendering of 500 template, with this error:

Mod_python error: "PythonHandler django.core.handlers.modpython"

Traceback (most recent call last):

  File "/usr/lib/python2.5/site-packages/mod_python/apache.py", line 299, in HandlerDispatch
    result = object(req)

  File "/var/lib/python-support/python2.5/django/core/handlers/modpython.py", line 211, in handler
    return ModPythonHandler()(req)

  File "/var/lib/python-support/python2.5/django/core/handlers/modpython.py", line 184, in __call__
    response = self.get_response(request)

  File "/var/lib/python-support/python2.5/django/core/handlers/base.py", line 126, in get_response
    return self.handle_uncaught_exception(request, resolver, exc_info)

  File "/var/lib/python-support/python2.5/django/core/handlers/base.py", line 146, in handle_uncaught_exception
    return debug.technical_500_response(request, *exc_info)

  File "/home/lin/django_src/django/views/debug.py", line 39, in technical_500_response
    html = reporter.get_traceback_html()

  File "/home/lin/django_src/django/views/debug.py", line 113, in get_traceback_html
    return t.render(c)

  File "/var/lib/python-support/python2.5/django/template/__init__.py", line 176, in render
    return self.nodelist.render(context)

  File "/var/lib/python-support/python2.5/django/template/__init__.py", line 751, in render
    bits.append(self.render_node(node, context))

  File "/var/lib/python-support/python2.5/django/template/debug.py", line 81, in render_node
    raise wrapped

TemplateSyntaxError: Caught an exception while rendering: django.utils.translation.gettext function called at import time to perform a translation of 'Tue'. gettext function must be calledcalled after applications loading

Original Traceback (most recent call last):
  File "/var/lib/python-support/python2.5/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/var/lib/python-support/python2.5/django/template/debug.py", line 87, in render
    output = force_unicode(self.filter_expression.resolve(context))
  File "/var/lib/python-support/python2.5/django/template/__init__.py", line 542, in resolve
    new_obj = func(obj, *arg_vals)
  File "/var/lib/python-support/python2.5/django/template/defaultfilters.py", line 627, in date
    return format(value, arg)
  File "/var/lib/python-support/python2.5/django/utils/dateformat.py", line 264, in format
    return df.format(format_string)
  File "/var/lib/python-support/python2.5/django/utils/dateformat.py", line 29, in format
    pieces.append(force_unicode(getattr(self, piece)()))
  File "/var/lib/python-support/python2.5/django/utils/dateformat.py", line 174, in r
    return self.format('D, j M Y H:i:s O')
  File "/var/lib/python-support/python2.5/django/utils/dateformat.py", line 29, in format
    pieces.append(force_unicode(getattr(self, piece)()))
  File "/var/lib/python-support/python2.5/django/utils/encoding.py", line 49, in force_unicode
    s = unicode(s)
  File "/var/lib/python-support/python2.5/django/utils/functional.py", line 201, in __unicode_cast
    return self.__func(*self.__args, **self.__kw)
  File "/var/lib/python-support/python2.5/django/utils/translation/__init__.py", line 62, in ugettext
    return real_ugettext(message)
  File "/var/lib/python-support/python2.5/django/utils/translation/trans_real.py", line 295, in ugettext
    return do_translate(message, 'ugettext')
  File "/var/lib/python-support/python2.5/django/utils/translation/trans_real.py", line 278, in do_translate
    "called after applications loading" % message)
ImproperlyImplemented: django.utils.translation.gettext function called at import time to perform a translation of 'Tue'. gettext function must be calledcalled after applications loading

comment:2 follow-up: Changed 6 years ago by msaelices

I've found that previous real error was: TemplateDoesNotExist: 500.html but in DEBUG mode, fails in variables and context rendering.

Changed 6 years ago by msaelices

A better patch... but It's seems to have problems in apache detection of app_cache_ready

Changed 6 years ago by msaelices

A better patch... but It's seems to have problems in apache detection of app_cache_ready

comment:3 Changed 6 years ago by msaelices

Now, the problem I get in apache is that always raise error, even when call gettext at run time. But the problem seems to be that app_cache_ready() doesnt work in Apache. Always returns False, even inside a django view.

comment:4 Changed 6 years ago by msaelices

I think I find an error with apps installation. I don't find any code point that forces a django apps loading, and apps loading occurs if some things happends, like gettext call. For example, if in your first line of your django view, in a production server with mod_python, you call to django.db.models.loading.app_cache_ready() you get False.

I will attach a patch that fixes this problem, but changes Django core parts (django.core.handlers.base module). Ok, I can say that this core changes passes all tests and works in my dev and production server.

Changed 6 years ago by msaelices

This patch fixes apache problems, forcing apps loading just in request processing

comment:5 follow-up: Changed 6 years ago by mtredinnick

  • Owner changed from msaelices to mtredinnick
  • Triage Stage changed from Unreviewed to Accepted

This is pretty evil. :-(

We intentionally do lazy apps loading for a reason (but I cannot, for the life of me, remember why that is). I'll try to remember the reason, but I don't like forcing it like this. It's certainly not a bug that it happens lazily. I'd prefer to work around requiring it to be loaded. All that being said, your patch is a good start, so I'll have a read of it tonight.

comment:6 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by msaelices

Replying to msaelices:

I've found that previous real error was: TemplateDoesNotExist: 500.html but in DEBUG mode, fails in variables and context rendering.

The final problem was Django tries to translate server_time to a spanish date, for rendering 500 template, in django.core.debug module. For fix that, I deactivated all translation in 500 error handling and I checked in do_translate that translations are activated, before raise an ImproperlyImplemented exception if is the case.

comment:7 in reply to: ↑ 6 Changed 6 years ago by msaelices

Replying to msaelices:

Replying to msaelices:

I've found that previous real error was: TemplateDoesNotExist: 500.html but in DEBUG mode, fails in variables and context rendering.

The final problem was Django tries to translate server_time to a spanish date, for rendering 500 template, in django.core.debug module. For fix that, I deactivated all translation in 500 error handling and I checked in do_translate that translations are activated, before raise an ImproperlyImplemented exception if is the case.

do you refer to do in do_translate something like this?:

from django.db.models.loading import get_apps

def do_translate(message, translation_function):
    global _default, _active
    t = _active.get(currentThread(), None)
    try:
       get_apps()
    except ImportError:
       raise ImproperlyImplemented(
           "ImportError occurs, maybe it was caused because"
           "django.utils.translation.%(func_name)s function was called at "
           "import time to perform a translation of '%(message)s'. "
           "%(func_name)s function must be called after "
           "applications loading." % {'func_name': translation_function,
                                      'message': message})

comment:8 in reply to: ↑ 5 Changed 6 years ago by msaelices

Replying to mtredinnick:

This is pretty evil. :-(

We intentionally do lazy apps loading for a reason (but I cannot, for the life of me, remember why that is). I'll try to remember the reason, but I don't like forcing it like this. It's certainly not a bug that it happens lazily. I'd prefer to work around requiring it to be loaded. All that being said, your patch is a good start, so I'll have a read of it tonight.

Sorry for my previous comment... was in bad thread.

About your work around... do you refer to do in do_translate something like this?:

from django.db.models.loading import get_apps

def do_translate(message, translation_function):
    global _default, _active
    t = _active.get(currentThread(), None)
    try:
       get_apps()
    except ImportError:
       raise ImproperlyImplemented(
           "ImportError occurs, maybe it was caused because"
           "django.utils.translation.%(func_name)s function was called at "
           "import time to perform a translation of '%(message)s'. "
           "%(func_name)s function must be called after "
           "applications loading." % {'func_name': translation_function,
                                      'message': message})
    ...

comment:9 Changed 6 years ago by msaelices

And with respect lazy apps loading. I dont mean is a bug, but the fail may be in let apps rely on app_cache_ready, because maybe never get loaded. Maybe the reason for lazy apps loading was performance. If you count time of loading templatetags, etc. When you do a simple HttpResponseRedirect, may not have to import any apps.

comment:10 Changed 6 years ago by mtredinnick

@msaelices: how are you checking things with this patch? I realise it's pretty hard to write automated tests and I'm not really worried about that. I just want to make sure I'm covering all the possibilities you are when I make changes. So I'm using ugettext() in the __init__.py file of an app and in a model field definition. Anything else?

comment:11 Changed 6 years ago by msaelices

It's very strange thing. I dont get to make fails django trunk, but I remember fixed several related issues by gettext_lazy. We also take a one of my company project and test it with actual django version and works. Also I try to update to a previous django revision (r8100) and works too.

What happening?

I will attach my test project, with uses gettext in forms.py and models.py, with two apps inside and one uses the other.

Changed 6 years ago by msaelices

Test project

comment:12 Changed 6 years ago by mtredinnick

  • Has patch set
  • Patch needs improvement set

I haven't had as much time to look at this as I would have liked, but I just gave it another read. My feeling is that it is over-engineered. Firstly, it's not always wrong to use ugettext() in a module at import time (because of dynamic import possibilities), but it usually is.

I also feel that the patch is trying to solve too large a problem. The real problem that we want to avoid is calling the translation machinery whilst apps are being imported. We have a way to tell when the app loading is finished. So probably all that we need is a way to tell is the app loading is in progress. How about adding an extra method/flag to the AppCacheclass to indicate that loading is in progress? Then we can test that before trying to access a translation and, if it's true, raise an error. The number of lines of code should be less and it catches the really nasty problem case.

Adding lots and lots of lines of code to handle an error situation that is a developer error is adding complexity that shouldn't be necessary. A clear error message to describe the problem is good, though, but I think we've gone a bit too far in trying to hold the hand of the developer in this patch.

comment:13 Changed 6 years ago by msaelices

You're completely right. I'll try to make a better patch with your idea.

Changed 6 years ago by msaelices

Using a flag for no try translation while django apps is still loading

comment:14 Changed 6 years ago by msaelices

@mtredinnick, I added a patch with your idea. But, I think we do not solve all kind of problems. Still there is a possibility to call ugettext before first app was tried to load. In this case, ugettext was no check any problem, and a circular import error will occurs (because this translation would begin apps loading).

What is your opinion?

comment:15 Changed 6 years ago by mtredinnick

  • milestone set to 1.0 maybe

comment:16 Changed 6 years ago by jacob

  • milestone changed from 1.0 maybe to 1.0

comment:17 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0

This can wait until after 1.0. The patch still needs some work (we're aren't going to raise an exception saying Django isn't properly implemented, since that isn't true: this feature is about errors in the user's code). I'm also fairly sure it would be safe to use ugettext() before importing all models and that could be a genuine use-case (e.g. in cronjobs), but it needs testing and mistakes now would be stabilising.

Since this is only a diagnostic check and only triggered for code that is already broken, we can push it to later.

comment:18 Changed 6 years ago by mtredinnick

  • Owner mtredinnick deleted

comment:19 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:20 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:21 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:22 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.