Subject: [patch][pgAdmin] Fix for pgadmin4-linux-qa #1651 failure

From: Rahul Shirsat <[email-masked]>
Date: Mon, 28 Jun 2021 21:26:18 +0530

Hi Hackers,

Please find the attached patch for fixation of jenkins failure.

-- 
*Rahul Shirsat*
Senior Software Engineer | EnterpriseDB Corporation.

---

From: Dave Page <[email-masked]>
Date: Mon, 28 Jun 2021 17:03:28 +0100

Hi

> Please find the attached patch for fixation of jenkins failure.

That won't work - you can't include variables (or string building
operations) in the first argument to gettext calls, as there won't be any
way to extract a complete message into the catalogs. The way it's being
done at the moment is correct (I don't know why it's failing, but it's the
correct way to structure the gettext calls).

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

---

From: Rahul Shirsat <[email-masked]>
Date: Tue, 29 Jun 2021 09:08:13 +0530

I feel gettext sometimes won't escape the characters as it should be.

I now tried to escape those using some utils.

Please find the updated patch.

---

From: Dave Page <[email-masked]>
Date: Tue, 29 Jun 2021 09:26:07 +0100

Hi

> I now tried to escape those using some utils.

That won't work either. The string being passed to gettext() *must* be in
the gettext call.

When gettext extracts strings to create/update the catalogs, it will search
the code for all gettext calls, and then extract a string constant from the
first argument. You cannot have variables, function calls or expressions in
there. It *must* be a string constant.

Keep in mind that msgextract is scanning the source code; it's not
executing it. There are many examples in the code, e.g. (from node.js):

title = gettext('Drop %s?', obj.label);

I don't see anything obviously wrong with the existing code. Are you sure
there are no weird unprintable characters in there?

---

From: Aditya Toshniwal <[email-masked]>
Date: Tue, 29 Jun 2021 16:30:27 +0530

Hi Rahul,

I did "make msg-extract" and "make msg-update" and looking at the PO files
I think it is not updated correctly.
For instance, the below message has msgstr without %s. I corrected it and
the error was gone.

#: pgadmin/browser/server_groups/servers/roles/static/js/role.js:766
#, fuzzy, python-format
msgid "%s Objects"
msgstr "Obiekty"

The one below had 2 %s in msgstr and I corrected it to fix the error.

#: pgadmin/browser/server_groups/servers/roles/static/js/role.js:767
#, fuzzy, python-format
msgid "Are you sure you wish to %s all the objects owned by the selected
role?"
msgstr "Czy na pewno skasować %s \"%s\" i wszystkie obiekty zależne od
niego?"

You have to update the .po files to match the total %s and send the patch.

On Tue, Jun 29, 2021 at 1:56 PM Dave Page <[email-masked]> wrote:

> When gettext extracts strings to create/update the catalogs, it will
> search the code for all gettext calls, and then extract a string constant
> from the first argument. You cannot have variables, function calls or
> expressions in there. It *must* be a string constant.
>

-- 
Thanks,
Aditya Toshniwal
pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
<http://edbpostgres.com>
"Don't Complain about Heat, Plant a TREE"

---

From: Rahul Shirsat <[email-masked]>
Date: Tue, 29 Jun 2021 18:29:57 +0530

Hi Hackers,

Thanks Aditya for pointing out the issue. Please find the attached patch
which contains all the .po files corrected with %s.

Regards,
Rahul Shirsat.

On Tue, Jun 29, 2021 at 4:31 PM Aditya Toshniwal <
[email-masked]> wrote:

> Hi Rahul,
>
> I did "make msg-extract" and "make msg-update" and looking at the PO
> files I think it is not updated correctly.

---

From: Dave Page <[email-masked]>
Date: Tue, 29 Jun 2021 14:31:42 +0100

Hi

Please send the patch without updates to the po files. Those get updated as
part of the release process.

Thanks.

On Tue, Jun 29, 2021 at 2:00 PM Rahul Shirsat <
[email-masked]> wrote:

>>> That won't work either. The string being passed to gettext() *must* be
>>> in the gettext call.
>>>
>>> I don't see anything obviously wrong with the existing code. Are you
>>> sure there are no weird unprintable characters in there?
>>>
>>>
>>>>

---

From: Aditya Toshniwal <[email-masked]>
Date: Tue, 29 Jun 2021 19:10:55 +0530

Dave,

Somehow, the new text strings are added to PO with incorrect translations.
That is causing the issue.
Either they should be empty or fixed.

On Tue, Jun 29, 2021 at 7:01 PM Dave Page <[email-masked]> wrote:

> Hi
>
> Please send the patch without updates to the po files. Those get updated
> as part of the release process.
>
> Thanks.
>
>>> For instance, the below message has msgstr without %s. I corrected it
>>> and the error was gone.
>>>

---

From: Dave Page <[email-masked]>
Date: Tue, 29 Jun 2021 14:44:00 +0100

> Either they should be empty or fixed.

Then the source problem should be fixed. There's no point at all in putting
fixes directly in the PO files as they'll be overwritten prior to release
anyway.

>>>> msgid "Are you sure you wish to %s all the objects owned by the
>>>> selected role?"

---

From: Aditya Toshniwal <[email-masked]>
Date: Tue, 29 Jun 2021 19:24:33 +0530

Hi,

On Tue, Jun 29, 2021 at 7:14 PM Dave Page <[email-masked]> wrote:

>> Somehow, the new text strings are added to PO with incorrect
>> translations. That is causing the issue.
> Then the source problem should be fixed. There's no point at all in
> putting fixes directly in the PO files as they'll be overwritten prior to
> release anyway.
>
The translations submitted by translators are updated in the PO file right
? And then they're compiled to MO ?
It's the same like Rahul will be submitting the translations. Please
correct me if I'm wrong.

>>>> Thanks Aditya for pointing out the issue. Please find the attached
>>>> patch which contains all the .po files corrected with %s.
>>>>
>>>> Regards,
>>>> Rahul Shirsat.
>>>>
>>>>>> That won't work either. The string being passed to gettext() *must*
>>>>>> be in the gettext call.
>>>>>>

---

From: Dave Page <[email-masked]>
Date: Tue, 29 Jun 2021 15:00:55 +0100

> ? And then they're compiled to MO ?

Right.

> It's the same like Rahul will be submitting the translations. Please

That depends if he's changed the original message or the translated
message. It's impossible to see without reading megabytes of text.

And in any case, he's updated translations that haven't been touched in
ages - why would they suddenly have broken? (hint: if the upstream message
has changed, it'll be marked as fuzzy or untranslated - in other words,
gettext knows how to handle that).

>>>> Please send the patch without updates to the po files. Those get
>>>> updated as part of the release process.
>>>>
>>>> Thanks.
>>>>
>>>>>> msgstr "Czy na pewno skasować %s \"%s\" i wszystkie obiekty zależne
>>>>>> od niego?"
>>>>>>
>>>>>>

---

From: Rahul Shirsat <[email-masked]>
Date: Tue, 29 Jun 2021 21:20:10 +0530

Hi Dave / Aditya,

For a time being, Let's make a call to gettext conditional instead of
passing dynamic parameters for this scenario at least. With this, we won't
be touching the *.po files and translations will do its task smoothly.
I have already checked for the string with weird unprintable characters,
and there were none.

e.g.
Instead of -  *title = gettext('%s objects?', obj.label);*

*if (drop) {*
*   title = gettext('Drop objects?');*
*}*
*else {*
   *title = gettext('Reassign objects?');*
*}*

I have tried the above code and it looks good to me.

Please suggest.

On Tue, Jun 29, 2021 at 7:31 PM Dave Page <[email-masked]> wrote:

>> The translations submitted by translators are updated in the PO file
>> right ? And then they're compiled to MO ?
>>
>
> Right.
>
>
>>>>>>> I did "make msg-extract" and "make msg-update" and looking at the
>>>>>>> PO files I think it is not updated correctly.
>>>>>>> For instance, the below message has msgstr without %s. I corrected
>>>>>>> it and the error was gone.
>>>>>>>
>>>>>>>> When gettext extracts strings to create/update the catalogs, it
>>>>>>>> will search the code for all gettext calls, and then extract a string
>>>>>>>> constant from the first argument. You cannot have variables, function calls
>>>>>>>> or expressions in there. It *must* be a string constant.
>>>>>>>>
>>>>>>>> I don't see anything obviously wrong with the existing code. Are
>>>>>>>> you sure there are no weird unprintable characters in there?
>>>>>>>>
>>>>>>>>
>>>>>>>>>

---

From: Rahul Shirsat <[email-masked]>
Date: Wed, 30 Jun 2021 12:58:11 +0530

Hi All,

Please find the attached patch for resolving this issue wrt above
suggestion.

Let me know if anyone has any queries.

On Tue, Jun 29, 2021 at 9:20 PM Rahul Shirsat <
[email-masked]> wrote:

>>>>>>>>> That won't work either. The string being passed to gettext()
>>>>>>>>> *must* be in the gettext call.
>>>>>>>>>
>>>>>>>>>>> That won't work - you can't include variables (or string
>>>>>>>>>>> building operations) in the first argument to gettext calls, as there won't
>>>>>>>>>>> be any way to extract a complete message into the catalogs. The way it's
>>>>>>>>>>> being done at the moment is correct (I don't know why it's failing, but
>>>>>>>>>>> it's the correct way to structure the gettext calls).
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Dave Page

---

From: Dave Page <[email-masked]>
Date: Wed, 30 Jun 2021 09:22:53 +0100

Hi

> Please find the attached patch for resolving this issue wrt above

Well that may fix the problem (and is a reasonable change), however, I
think it's important that we understand the root cause. Why is this failing
on Linux only? Why does the following from node.js (which follows the same
pattern) work fine?

var type_label = gettext('%s Script',stype.toUpperCase());

>>>>>>>>> #:
>>>>>>>>> pgadmin/browser/server_groups/servers/roles/static/js/role.js:766
>>>>>>>>> #, fuzzy, python-format
>>>>>>>>> msgid "%s Objects"
>>>>>>>>> msgstr "Obiekty"
>>>>>>>>>
>>>>>>>>> #:
>>>>>>>>> pgadmin/browser/server_groups/servers/roles/static/js/role.js:767
>>>>>>>>> #, fuzzy, python-format
>>>>>>>>> msgstr "Czy na pewno skasować %s \"%s\" i wszystkie obiekty
>>>>>>>>> zależne od niego?"
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Keep in mind that msgextract is scanning the source code; it's
>>>>>>>>>> not executing it. There are many examples in the code, e.g. (from node.js):
>>>>>>>>>>

---

From: Dave Page <[email-masked]>
Date: Wed, 30 Jun 2021 14:25:12 +0100

Hi

> var type_label = gettext('%s Script',stype.toUpperCase());

Rahul and I figured out the root cause. The issue is occuring because the
previous string had no parameters (i.e. no %s's). Because fuzzy matching is
used for the translations, when updating the catalogs it was matching with
the old translation, which at runtime would likely have caused a crash
because the catalogs would have contained something like:

#: pgadmin/browser/static/js/node.js:209
#, fuzzy, python-format
msgid "Search %s Objects"
msgstr "Typy obiektów"

There are a few of ways around this:

- Manually fix the translations in each catalog. This is not a good idea
because we don't speak all those languages and will probably mess the
translations up.

- Run something like 'make msg-extract && pybabel update
--no-fuzzy-matching -i web/pgadmin/messages.pot -d web/pgadmin/translations
&& make msg-compile', then commit the results. This will remove all fuzzy
matches from the catalogs, which means more work for the translators on the
next release, but will likely also result in them becoming much cleaner.

- Change the code to use the conditional fix.

I'm leaning towards the second option. In the worst case, it'll lose about
58 fuzzy translations (meaning 58 messages to re-translate), but at least
we'll know they are clean. See
https://www.pgadmin.org/development/translations/ for current stats.

Thoughts?

---

From: Aditya Toshniwal <[email-masked]>
Date: Wed, 30 Jun 2021 19:12:02 +0530

Hi,

> https://www.pgadmin.org/development/translations/ for current stats.
Right. I'm in favor of removing the incorrect translations. For which, the
right way is the second option.
Shouldn't we add the no fuzzy flag in the makefile ? This problem can occur
again.

---

From: Dave Page <[email-masked]>
Date: Wed, 30 Jun 2021 14:56:35 +0100

On Wed, Jun 30, 2021 at 2:42 PM Aditya Toshniwal <
[email-masked]> wrote:

>> I'm leaning towards the second option. In the worst case, it'll lose
>> about 58 fuzzy translations (meaning 58 messages to re-translate), but at
>> least we'll know they are clean. See
> Shouldn't we add the no fuzzy flag in the makefile ? This problem can
> occur again.
>

Maybe :-). That's the point of this email - to gather such opinions.

---

From: Khushboo Vashi <[email-masked]>
Date: Thu, 1 Jul 2021 11:17:56 +0530

On Wed, Jun 30, 2021 at 6:55 PM Dave Page <[email-masked]> wrote:

> +1 for pybabel update with -N option

---

From: Rahul Shirsat <[email-masked]>
Date: Sat, 3 Jul 2021 01:10:57 +0530

+1 for --no-fuzzy-matching for updating translations.

---

From: Dave Page <[email-masked]>
Date: Mon, 5 Jul 2021 09:10:46 +0100

On Fri, Jul 2, 2021 at 8:41 PM Rahul Shirsat <[email-masked]>
wrote:

>>> Rahul and I figured out the root cause. The issue is occuring because
>>> the previous string had no parameters (i.e. no %s's). Because fuzzy
>>> matching is used for the translations, when updating the catalogs it was
>>> matching with the old translation, which at runtime would likely have
>>> caused a crash because the catalogs would have contained something like:
>>>
Akshay?

---

From: Akshay Joshi <[email-masked]>
Date: Mon, 5 Jul 2021 13:52:30 +0530

Hi

On Mon, Jul 5, 2021 at 1:40 PM Dave Page <[email-masked]> wrote:

>
>
> On Fri, Jul 2, 2021 at 8:41 PM Rahul Shirsat <
>>>> - Manually fix the translations in each catalog. This is not a good
>>>> idea because we don't speak all those languages and will probably mess the
>>>> translations up.
>>>>

    Yes, I'll also go with the second option  "--no-fuzzy-matching".

-- 
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*

---

From: Dave Page <[email-masked]>
Date: Mon, 5 Jul 2021 09:51:17 +0100

On Mon, Jul 5, 2021 at 9:22 AM Akshay Joshi <[email-masked]>
wrote:

>>>>>> Well that may fix the problem (and is a reasonable change), however,
>>>>>> I think it's important that we understand the root cause. Why is this
>>>>>> failing on Linux only? Why does the following from node.js (which follows
>>>>>> the same pattern) work fine?
>>>>>>

OK, I've pushed that change. Let's see what happens. For reference, here's