Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packages: fix up parse_accept_language() #19527

Merged

Conversation

allisonkarlitskaya
Copy link
Member

Make several fixes to our handling of the Accept-Language: header:

  • we used to crash if q= contained invalid values due to an unchecked cast with float(). Catch that error and ignore the entry in that case.

  • our handling of English was incorrect. We would look for and fail to find po.en.js, and move on to the next item in the list. That means if the user listed English first, followed by another language which we did support, they'd see Cockpit in that other language, which is unexpected. This is a regression introduced by f4be906. Now we drop all items that sort after English.

  • our handling of fallbacks (ie: 'de' from 'de-de') was incorrect. RFC4647 §3.4 says that a "Lookup" should consider items in the order they're found, stripping each item down to its base form, before considering the next item. This passes a gut check, as well: a user who lists de-de, nl probably expects to see a German translation before a Dutch one.

We also now mark the parser code as @lru_cache. This makes sense: within a given run of cockpit-bridge, we're likely to see very many copies of the Accept-Language header, and they're practically always going to be identical. Make sure we now accept and return immutable types to prevent weird side-effects.

While we're at it, up our testing game a bit to make sure we don't mistreat requests for 'English' in the future.

Closes #19526

@allisonkarlitskaya allisonkarlitskaya temporarily deployed to gh-cockpituous October 25, 2023 17:10 — with GitHub Actions Inactive
@allisonkarlitskaya
Copy link
Member Author

Note: this change means that a language list like en-ca, en-gb, en is entirely pointless. After en-ca is considered, en will be directly considered after it, effectively ending the search.

It also means that lists like de-at, de-ch are almost pointless: after we fail to find de-at we'll immediately try to find de (which will typically be there). de-ch won't be considered in that case. This is the correct behaviour per RFC 4647.

@allisonkarlitskaya
Copy link
Member Author

Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/verify/check-storage-stratis", line 869, in testBasic
    m.execute(f"stratis pool stop {self.stop_type_opt} pool0")
                                   ^^^^^^^^^^^^^^^^^^
AttributeError: 'TestStorageStratisNBDE' object has no attribute 'stop_type_opt'

hihi #19525

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks for fixing this! I'd like to see some more edge cases covered in tests, but code looks good!


document = packages.load_path('/one/po.js', {'Accept-Language': 'de;q=0.9, fr, en-ca'})
assert '/javascript' in document.content_type
assert document.data.read() == b''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting test case would be en;q=0.9, de. Intuitively that should result in "de", but given how my intuition was already fooled before by the "insert fallbacks with no country" before (which feels actively wrong for some cases), we better pin it down?

src/cockpit/packages.py Show resolved Hide resolved
src/cockpit/packages.py Show resolved Hide resolved
src/cockpit/packages.py Show resolved Hide resolved
test/pytest/test_packages.py Show resolved Hide resolved
Make several fixes to our handling of the `Accept-Language:` header:

 - we used to crash if `q=` contained invalid values due to an unchecked
   cast with `float()`.  Catch that error and ignore the entry in that
   case.

 - our handling of English was incorrect.  We would look for and fail to
   find po.en.js, and move on to the next item in the list.  That means
   if the user listed English first, followed by another language which
   we did support, they'd see Cockpit in that other language, which is
   unexpected.  This is a regression introduced by f4be906.  Now we
   drop all items that sort after English.

 - our handling of fallbacks (ie: 'de' from 'de-de') was incorrect.
   RFC4647 §3.4 says that a "Lookup" should consider items in the order
   they're found, stripping each item down to its base form, before
   considering the next item.  This passes a gut check, as well: a user
   who lists `de-de, nl` probably expects to see a German translation
   before a Dutch one.

We also now mark the parser code as `@lru_cache`.  This makes sense:
within a given run of cockpit-bridge, we're likely to see very many
copies of the Accept-Language header, and they're practically always
going to be identical.  Make sure we now accept and return immutable
types to prevent weird side-effects.  We also take the time to remove
duplicate items from the list.

While we're at it, up our testing game a bit to make sure we don't
mistreat requests for 'English' in the future.

Closes cockpit-project#19526
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

@allisonkarlitskaya allisonkarlitskaya merged commit b6564b2 into cockpit-project:main Oct 26, 2023
95 of 96 checks passed
@allisonkarlitskaya allisonkarlitskaya deleted the english-po-js branch October 26, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Locale selection code in Python bridge is broken
2 participants