[Django] #31570: Translations of one language in different territories can override each other
Django
other
------------------------------------------------+------------------------
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
The fix of #30439 created a new problem: Under some circumstances,
translations for one language in different territories can override each
other. In the case we've run into, users in New Zealand ({{{en-NZ}}})
received translations for South Africa ({{{en-ZA}}}). We've seen this
behavior on 2.2.12, and downgrading to 2.2.11 resolved it, hence marking
this as a bug in 2.2, but I believe the same issue exists in 3.0 and
master.
The circumstances where this happens are not trivial to reproduce; it
involves having applications with translations for the "bare" language
({{{en}}} in our case), and may depend on the order of loading of
different translations. I will try to come up with a test later.
The issue arises, AFAICT, when the {{{TranslationCatalog}}} installs a
reference to the bare-language dictionary ("catalog" in gettext terms) as
the first in the list of catalogs for a territorial variation (when an app
has only the bare-language translation), and is then willing to update it
with the contents of another territorial variant.
The following diff seems to resolve the issue, but I am far from certain
that it is the right solution.
{{{#!diff
diff --git a/django/utils/translation/trans_real.py
b/django/utils/translation/trans_real.py
index eed4705f6e..8042f6fdc4 100644
--- a/django/utils/translation/trans_real.py
+++ b/django/utils/translation/trans_real.py
@@ -96,7 +96,7 @@ class TranslationCatalog:
cat.update(trans._catalog)
break
else:
- self._catalogs.insert(0, trans._catalog)
+ self._catalogs.insert(0, trans._catalog.copy())
self._plurals.insert(0, trans.plural)
def get(self, key, default=None):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31570>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hey Shai. Thanks for the report. π€
> In the case we've run into, users in New Zealand (en-NZ) received
translations for South Africa (en-ZA).
> ...
> I will try to come up with a test later.
Yes, please. I'm struggling to see where the issue is.
I'm wondering if
[https://github.com/claudep/django/blob/6b840e55a29791529cb08faa8836fa4f6168642b/django/utils/translation/trans_real.py#L92-L97
the test for the equivalence of the plural forms] is robust enough:
{{{
def update(self, trans):
# Merge if plural function is the same, else prepend.
for cat, plural in zip(self._catalogs, self._plurals):
if trans.plural.__code__ == plural.__code__:
cat.update(trans._catalog)
break
else:
...
}}}
Q: What guarantees that the `__code__` object of the
[https://github.com/python/cpython/blob/a63c61168588937c482435d0432c753de4844c46/Lib/gettext.py#L176
generated plural functions] evaluates equal or not? (Current status: no
idea: it's not as simple as pointer address, but I can't yet find the
implementation so... )
There are a number of cases in the existing tests where we enter the
different plural forms `else`-block, beyond the test added for #30439, so
maybe there is a subtle behaviour change here. I'd be very grateful for
that test to be able to pin it down.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:1>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
I see that the problem description wasn't clear enough:
We are working on a website for the K-6 range of schools and students. The
"K" year (for "Kindergarten") is called "Year 1" in New Zealand, but
"Grade R" in South Africa, and some Kiwis were pretty confused when they
saw "Grade R" on the site.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:2>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hey Shai. Thanks for the clarification.
I was assuming that `en-NZ` and `en-ZA` have the same number of plural
forms? (Is that correct?)
If so, we shouldn't end up in the branch where your suggested/possible
edit is. i.e. the catalogs should be merged as they were in the past...
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:3>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
The whole {{{TranslationCatalog}}} class is new in the fix for #30439.
The issue is not about mis-comparing plural forms at all. The issue is
incorrect handling, in that code, of the "base language" ({{{en}}})
catalog. All the {{{en}}} variants have the same plural forms, of course,
and they do compare equal. But the code fails to account for the fact that
the {{{en}}} catalog is loaded for {{{en-NZ}}}. When the stars align
"correctly", it is loaded in such a way that {{{cat}}} in {{{cat.update}}}
in line 96 is the {{{en}}} catalog, and it is updated with the {{{NZ}}}
translations when those are loaded, and then with the {{{ZA}}}
translations when those are loaded. It is the same ({{{is}}}) dict that is
then used for all three languages.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:4>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> When the stars align "correctly", it is loaded in such a way that cat in
cat.update in line 96 is the en catalog, and it is updated with the NZ
translations when those are loaded, and then with the ZA translations when
those are loaded. It is the same (is) dict that is then used for all three
languages.
OK, it's that bit I haven't pinned down as yet. A reproduce project would
be super if you can. Otherwise I'll try again tomorrow.
(I'm not/wasn't seeing how the `copy()` in the else branch on ln99 comes
into play if the plurals compare equal...)
Thanks for your patience with me. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:5>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
The {{{else:}}} comes into play when the {{{TranslationCatalog}}} is still
empty.
And thanks for your own patience for all my hand-waving with no failing-
test code to back it up yet...
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:6>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> The else: comes into play when the TranslationCatalog is still empty.
Yes, of course. I think I had a dose of code blindness yesterday. π€―
Nonetheless, I've had another pop at this this morning and can't seem to
get it lined up to reproduce the issue.
I can keep trying but a sample project would be a big help.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:7>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Shai Berger):
* Attachment "test-31570.tgz" added.
Test project showing a problem. Depends on nothing but Django. `manage.py
test` passes on 2.2.11 and 3.0.4, fails on 2.2.12, 3.0.[56] and master.
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
Well, I could not make the stars align just right to reproduce the exact
behavior I saw in the large project, but there's enough in
attachment:test-31570.tgz to show that there is a regression.
The project includes three apps -- `bare_en` and `bare_en2` have their own
translations, but only for `en`; the translation for `trans` (into four
English dialects) is done in the project. In "real life", separate apps
with their own translations are 3rd-party apps, of course. There's two
such apps because I hoped to get an exact reproduction, and when I gave up
on that, I just left them there.
I was unable to get the regional dialects to trump each other -- I see
`en` being used instead of `en-nz` and `en-ca`, while `en-au` survives. I
don't yet fully understand this.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:8>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Release blocker | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hey Shai β Thank you! Super. π
I'll look at this again today.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:9>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
OK, super regression confirmed. Thank you Shai.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:10>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Carlton has a point with the plural function comparison broken. I cannot
even understand why I wrote that :-( If it worked, this bug wouldn't have
surface in this use case (as catalogs should be merged as before). Proper
comparison of plural equations is not trivial, I fear it's almost
impossible to do it perfectly.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:11>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Still, I understand I did it because it somewhat magically works in many
cases (I didn't dig either in C code to understand why, as it goes above
my abilities).
Another way to empirically compare the plural functions would be to
compare their result on numbers from 1 to 101 (or 105, orβ¦). It would not
be perfect but might work for our use case with a rather small performance
penalty.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:12>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Something like that:
{{{
diff --git a/django/utils/translation/trans_real.py
b/django/utils/translation/trans_real.py
index eed4705f6e..0dca4230d2 100644
--- a/django/utils/translation/trans_real.py
+++ b/django/utils/translation/trans_real.py
@@ -92,7 +92,7 @@ class TranslationCatalog:
def update(self, trans):
# Merge if plural function is the same, else prepend.
for cat, plural in zip(self._catalogs, self._plurals):
- if trans.plural.__code__ == plural.__code__:
+ if _plurals_equal(trans.plural, plural):
cat.update(trans._catalog)
break
else:
@@ -259,6 +259,17 @@ class
DjangoTranslation(gettext_module.GNUTranslations):
return tmsg
+def _plurals_equal(plur1, plur2):
+ """
+ Compare plural functions by comparing their result on 0 - 101.
+ Not perfect, but probably sufficient for our use case.
+ """
+ for num in range(102):
+ if plur1(num) != plur2(num):
+ return False
+ return True
+
+
def translation(language):
"""
Return a translation object in the default 'django' domain.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:13>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
Replying to [comment:11 Claude Paroz]:
> Carlton has a point with the plural function comparison broken. I cannot
even understand why I wrote that :-( If it worked, this bug wouldn't have
surface in this use case (as catalogs should be merged as before). Proper
comparison of plural equations is not trivial, I fear it's almost
impossible to do it perfectly.
As far as I can tell, this is not the issue here. See comment:4.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:14>
Django
other
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 2.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
Replying to [comment:11 Claude Paroz]:
> [...] plural function comparison broken. [...] If it worked, this bug
wouldn't have surface in this use case
After further investigation: This claim is technically correct, but is not
helpful for solving this bug.
To my surprise, I found that there are actually two different
implementations of the plural function in the English translation files.
From the apps in the default project template, {{{admin}}} and {{{auth}}}
have
{{{
"Plural-Forms: nplurals=2; plural=(n != 1);\n"
}}}
but this line is not found in the {{{en}}} translations (which exist) for
{{{contenttypes}}} and {{{sessions}}}, nor in the files initially
generated by {{{makemessages}}}. The {{{__code__}}} comparison considers
the default different from the explicit plural equation, but otherwise
works.
Now, given that the default plural function is {{{lambda n: int(n !=
1)}}}, it is indeed the case that if the plural function comparison worked
perfectly, all the plural functions involved would have been considered
equal, and the problem would not arise.
However, the point of #30439 was exactly the support of files with
different plural-functions for the same language. So the effect of the
comparison failure is just to allow us to debug with English, a problem
which could pop up in another language, where different functions are more
common.
As an aside, this was the major missing piece which allowed me to
reproduce exactly the behavior I saw in production: Adding the explicit
plural forms to my apps' translation files, and making the test run two
rounds of translations (as only when the first round ends, all languages
are loaded), that is
{{{#!diff
*** trans/tests.py~ 2020-05-13 12:38:30.028586931 +0300
--- trans/tests.py 2020-05-15 16:32:14.537889998 +0300
*************** class TestTranslations(SimpleTestCase):
*** 10,17 ****
'en_CA': 'canuck',
'en': 'local country person',
}
! for language, nick in country_people_nicknames.items():
! with self.subTest(language=language):
activate(language)
self.assertEqual(_('local country person'), nick)
--- 10,18 ----
'en_CA': 'canuck',
'en': 'local country person',
}
! for rnd in "12":
! for language, nick in country_people_nicknames.items():
! with self.subTest(language=language, round=rnd):
activate(language)
self.assertEqual(_('local country person'), nick)
}}}
Allowed me to get as output
{{{
======================================================================
FAIL: test_region_translations (trans.tests.TestTranslations)
(language='en', round='1')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/slate/tmp/test_31570/trans/tests.py", line 17, in
test_region_translations
self.assertEqual(_('local country person'), nick)
AssertionError: 'canuck' != 'local country person'
- canuck
+ local country person
======================================================================
FAIL: test_region_translations (trans.tests.TestTranslations)
(language='en_NZ', round='2')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/slate/tmp/test_31570/trans/tests.py", line 17, in
test_region_translations
self.assertEqual(_('local country person'), nick)
AssertionError: 'canuck' != 'kiwi'
- canuck
+ kiwi
======================================================================
FAIL: test_region_translations (trans.tests.TestTranslations)
(language='en', round='2')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/slate/tmp/test_31570/trans/tests.py", line 17, in
test_region_translations
self.assertEqual(_('local country person'), nick)
AssertionError: 'canuck' != 'local country person'
- canuck
+ local country person
}}}
That is, the {{{en-nz}}} (and {{{en}}}) dialect was overridden by {{{en-
ca}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:15>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Type: Bug | Status: assigned
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* owner: nobody => Carlton Gibson
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:16>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Type: Bug | Status: assigned
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/12990 PR]
I've spent far too long on this and still didn't get 100% to the bottom of
it, particularly with the fallbacks here.
I did learn some things though... π
The key is that once we have the app with only the subset of all the
supported languages, `_add_installed_apps_translations()` ends up getting
a translation
[https://github.com/python/cpython/blob/a63c61168588937c482435d0432c753de4844c46/Lib/gettext.py#L590-L598
who's `_catalog` is the exact same dictionary as has been previously
used]. This is what Shai said already. The suggested `copy()` resolves
that issue.
Thanks for the report Shai, and the reproduce: it's quite particular. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:17>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Type: Bug | Status: assigned
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:18>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@β¦>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"98fada7244f135022540ec5d2383fb0115844bb4" 98fada7]:
{{{
#!CommitTicketReference repository=""
revision="98fada7244f135022540ec5d2383fb0115844bb4"
[3.1.x] Fixed #31570 -- Corrected translation loading for apps providing
territorial language variants with different plural equations.
Regression in e3e48b00127c09eafe6439d980a82fc5c591b673.
Thanks to Shai Berger for report, reproduce and suggested fix.
Backport of dd1ca50b096bf0351819aabc862e91a9797ddaca from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:19>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Type: Bug | Status: closed
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@β¦>):
In [changeset:"2638627db45766a300279197b2d6f299a5ea841f" 2638627d]:
{{{
#!CommitTicketReference repository=""
revision="2638627db45766a300279197b2d6f299a5ea841f"
[3.0.x] Fixed #31570 -- Corrected translation loading for apps providing
territorial language variants with different plural equations.
Regression in e3e48b00127c09eafe6439d980a82fc5c591b673.
Thanks to Shai Berger for report, reproduce and suggested fix.
Backport of dd1ca50b096bf0351819aabc862e91a9797ddaca from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:20>
Django
other
Reporter: Shai Berger | Owner: Carlton
| Gibson
Type: Bug | Status: closed
Component: | Version: 2.2
Internationalization |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@β¦>):
In [changeset:"027840d7def56784b78ff0af656376043749200e" 027840d7]:
{{{
#!CommitTicketReference repository=""
revision="027840d7def56784b78ff0af656376043749200e"
[2.2.x] Fixed #31570 -- Corrected translation loading for apps providing
territorial language variants with different plural equations.
Regression in e3e48b00127c09eafe6439d980a82fc5c591b673.
Thanks to Shai Berger for report, reproduce and suggested fix.
Backport of dd1ca50b096bf0351819aabc862e91a9797ddaca from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31570#comment:21>