Translations of one language in different territories can override each other

[Shai Berger]

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.

django/utils/translation/trans_real.py

@@ -96 +96 @@
                 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):

[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 ​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 ​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... )

Update:

• ​https://github.com/python/cpython/blob/master/Objects/codeobject.c
• ​https://github.com/python/cpython/blob/master/Objects/funcobject.c#L236

Are the relevant sources, but that's not to have followed the flow fully.
endupdate

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.

[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.

[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...

[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.

[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. :)

[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...

[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.

[Shai Berger]

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.

[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.

[Carlton Gibson]

Hey Shai — Thank you! Super. 👍
I'll look at this again today.

[Carlton Gibson]

OK, super regression confirmed. Thank you Shai.

[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.

[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 beyond 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.

[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.

[Shai Berger]

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

[Shai Berger]

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

*** 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.

[Carlton Gibson]

​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 ​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. :)

[Mariusz Felisiak <felisiak.mariusz@…>]

In 98fada7:

[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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2638627d:

[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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 027840d7:

[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.