Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#15129 closed (fixed)

Invalid order of applying formats

Reported by: Artem Skoretskiy Owned by: nobody
Component: Internationalization Version: 1.2
Severity: Keywords:
Cc: Artem Skoretskiy, <tonn81@…> Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I found that DATE_INPUT_FORMATS used for JS I18N is not format I defined in app/formats/*/formats.py file (FORMAT_MODULE_PATH = 'app.formats')

Django version is 1.2.4

I found that get_formats uses invalid order of formats - so format I defined is being overwritten by Django's default ones.

That is serious bug as Django JS uses different formatting than server-side code.

To prove that I changed file django/views/i18n.py to debug the issue:

def get_formats():
    """
    Returns all formats strings required for i18n to work
    """
    FORMAT_SETTINGS = (
        'DATE_FORMAT', 'DATETIME_FORMAT', 'TIME_FORMAT',
        'YEAR_MONTH_FORMAT', 'MONTH_DAY_FORMAT', 'SHORT_DATE_FORMAT',
        'SHORT_DATETIME_FORMAT', 'FIRST_DAY_OF_WEEK', 'DECIMAL_SEPARATOR',
        'THOUSAND_SEPARATOR', 'NUMBER_GROUPING',
        'DATE_INPUT_FORMATS', 'TIME_INPUT_FORMATS', 'DATETIME_INPUT_FORMATS'
    )
    result = {}
    modules = [settings] + get_format_modules(reverse=True) # DEBUG
    log = '%r:\n' % modules # DEBUG
    for module in modules:
        for attr in FORMAT_SETTINGS:
            try:
                if attr == 'DATE_INPUT_FORMATS': # DEBUG
                    log += "    %r from %r\n" % (getattr(module, attr, None), module) #DEBUG
                result[attr] = getattr(module, attr)
            except AttributeError:
                pass
    log += "=> %r from %r" % (result.get('DATE_INPUT_FORMATS'), module) # DEBUG
    if result.get('DATE_INPUT_FORMATS') != ('%Y-%m-%d', '%m/%d/%Y', '%m/%d/%y'): # DEBUG
        logging.getLogger('coupon.localebug').error(log) # DEBUG

    # remaining code is cut

That's the output I got:

[<django.conf.LazySettings object at 0x2359fd0>, <module 'app.formats.fr.formats' from '/XXX/app/formats/fr/formats.pyc'>, <module 'django.conf.locale.fr.formats' from '/XXX/pyenv/lib/python2.5/site-packages/django/conf/locale/fr/formats.pyc'>]:
    ('%Y-%m-%d', '%m/%d/%Y', '%m/%d/%y', '%b %d %Y', '%b %d, %Y', '%d %b %Y', '%d %b, %Y', '%B %d %Y', '%B %d, %Y', '%d %B %Y', '%d %B, %Y') from <django.conf.LazySettings object at 0x2359fd0>
    ('%Y-%m-%d', '%m/%d/%Y', '%m/%d/%y') from <module 'app.formats.fr.formats' from '/XXX/app/formats/fr/formats.pyc'>
    ('%d/%m/%Y', '%d/%m/%y', '%d.%m.%Y', '%d.%m.%y', '%Y-%m-%d', '%y-%m-%d') from <module 'django.conf.locale.fr.formats' from '/XXX/pyenv/lib/python2.5/site-packages/django/conf/locale/fr/formats.pyc'>
=>  ('%d/%m/%Y', '%d/%m/%y', '%d.%m.%Y', '%d.%m.%y', '%Y-%m-%d', '%y-%m-%d') from <module 'django.conf.locale.fr.formats' from '/XXX/pyenv/lib/python2.5/site-packages/django/conf/locale/fr/formats.pyc'>

That means that my format is used, but it is not the last in the order, so it is being overwritten.

Attachments (3)

formats.patch (554 bytes ) - added by Artem Skoretskiy 13 years ago.
Patch that does the fix
15129.1.diff (2.1 KB ) - added by Ramiro Morales 13 years ago.
tonnzor's patch wth tests integrated with our test suite. Implementation of fix simply chanegs get_format_modules docstring to say it doesn
15129.2.diff (2.1 KB ) - added by Artem Skoretskiy 13 years ago.
Ramiro patch with fixed iterator -> list

Download all attachments as: .zip

Change History (16)

comment:1 by Artem Skoretskiy, 13 years ago

It seems I had found the issue in django/utils/formats.py:

def get_format_modules(reverse=False):
    """
    Returns an iterator over the format modules found
    """
    lang = get_language()
    modules = _format_modules_cache.setdefault(lang, list(iter_format_modules(lang)))
    if reverse:
        modules.reverse()
    return modules

Modules are stored in _format_modules_cache variable and get reversed in place. So if we call the same method multiple times - order will be incorrect.

It seems all we need is just to reverse in memory, not in place:

def get_format_modules(reverse=False):
    """
    Returns an iterator over the format modules found
    """
    lang = get_language()
    modules = _format_modules_cache.setdefault(lang, list(iter_format_modules(lang)))
    if reverse:
        return list(reversed(modules)) # FIX
    return modules

comment:2 by Artem Skoretskiy, 13 years ago

Yeah, that fixed the problem. Please, include it into Django as soon as possible.

by Artem Skoretskiy, 13 years ago

Attachment: formats.patch added

Patch that does the fix

comment:3 by Artem Skoretskiy, 13 years ago

Has patch: set

comment:4 by Ramiro Morales, 13 years ago

Needs tests: set

comment:5 by Russell Keith-Magee, 13 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:6 by Artem Skoretskiy, 13 years ago

What's wrong with current patch? I may improve it if needed.

The only thing I found inconsistent is that new code returns list, not an iterator. However, previous version returned list too and making it return iterator would require to fix django/views/i18n.py file too:

--- a/django/views/i18n.py
+++ b/django/views/i18n.py
@@ -1,4 +1,5 @@
 import os
+import itertools
 import gettext as gettext_module
 
 from django import http
@@ -47,7 +48,7 @@
         'DATE_INPUT_FORMATS', 'TIME_INPUT_FORMATS', 'DATETIME_INPUT_FORMATS'
     )
     result = {}
-    for module in [settings] + get_format_modules(reverse=True):
+    for module in itertools.chain([settings], get_format_modules(reverse=True)):
         for attr in FORMAT_SETTINGS:
             try:
                 result[attr] = getattr(module, attr)

--- a/django/utils/formats.py
+++ b/django/utils/formats.py
@@ -39,7 +39,7 @@
     lang = get_language()
     modules = _format_modules_cache.setdefault(lang, list(iter_format_modules(lang)))
     if reverse:
-        modules.reverse()
+        return reversed(modules)
     return modules
 
 def get_format(format_type):

As of the test - we'll need a project with FORMAT_MODULE_PATH set and formats file existing. Then test is quite simple:

from django.utils import unittest
from django.utils.formats import get_format_modules

class MyTestCase(unittest.TestCase):
    def setUp(self):
        pass

    def testPreservedOrder(self):
        old = "%r" % get_format_modules(reverse=True) # serialize to prevent changing
        new = "%r" % get_format_modules(reverse=True) # second try
        self.assertEqual(new, old, 'get_formats_modules result must be the same during calls')

That check (that result changes during consequent calls) proves the issue on non-patched system.

comment:7 by Artem Skoretskiy, 13 years ago

Cc: Artem Skoretskiy <tonn81@…> added

by Ramiro Morales, 13 years ago

Attachment: 15129.1.diff added

tonnzor's patch wth tests integrated with our test suite. Implementation of fix simply chanegs get_format_modules docstring to say it doesn

comment:8 by Ramiro Morales, 13 years ago

Needs tests: unset

I'e attached a version of tonnzor's patch with tests integrated with our test suite. Implementation of fix simply changes get_format_modules() docstring to not say it returns an iterator, and so describe the real current code behavior.

in reply to:  8 comment:9 by Artem Skoretskiy, 13 years ago

Replying to ramiro:

I'e attached a version of tonnzor's patch with tests integrated with our test suite.

Thanks for patch, ramiro!

Please replace

    return reversed(modules)

with

    return list(reversed(modules))

because:

  • reversed function returns iterator, not a list as described in docstring
  • as reversed return iterator - that would lead to crash in django/views/i18n.py file on this line:
        for module in [settings] + get_format_modules(reverse=True):
    

by Artem Skoretskiy, 13 years ago

Attachment: 15129.2.diff added

Ramiro patch with fixed iterator -> list

comment:10 by Artem Skoretskiy, 13 years ago

Triage Stage: AcceptedReady for checkin

I suppose that is ready for check-in

comment:11 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: newclosed

In [15402]:

Fixed stability of data input/output L10N format modules priority order. Thanks tonnzor for the report and fix.

comment:12 by Ramiro Morales, 13 years ago

In [15403]:

[1.2.X] Fixed #15129 -- Fixed stability of data input/output L10N format modules priority order. Thanks tonnzor for the report and fix.

Backport of [15402] from trunk

comment:13 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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