Skip to content

Report Qobuz API failures cleanly instead of crashing; keep credentials out of errors - #1037

Open
sjbrownrigg wants to merge 1 commit into
nathom:devfrom
sjbrownrigg:fix/qobuz-api-errors
Open

sjbrownrigg wants to merge 1 commit into
nathom:devfrom
sjbrownrigg:fix/qobuz-api-errors

Conversation

@sjbrownrigg

Copy link
Copy Markdown

Problem

rip search qobuz artist <name> can crash with a bare traceback ending in:

AssertionError: 400

The 400 comes from Qobuz's own search backend, and its message explains it:

Impossible to connect, please check your Algolia Application Id.

It is transient. Probing the same search three times, 20 seconds apart, it failed once and succeeded twice, with the same credentials and app id throughout. _paginate() asserts on the status and discards the message, so the user gets neither an explanation nor a second chance.

Change

A graceful exit instead of a traceback. Requests that must succeed go through a new _request_ok(). When a failure looks transient — a 5xx, or that Algolia message — it retries once after a short pause. Otherwise it raises a new APIError carrying Qobuz's own message, and the search commands print it and return:

Search failed: Qobuz artist/search failed (HTTP 400): Impossible to connect, please check your Algolia Application Id.

The 3-second pause is a guess; I only observed recovery at the 20-second mark. The retry is a mitigation, and the clear message is the main fix.

A login that returns something other than 200/400/401 now raises APIError too, instead of failing later on a missing resp["user"].

Credentials in error output

Qobuz takes user_id and user_auth_token as URL query parameters, and they currently reach error output in three ways:

  • HTML error pages. When Qobuz's edge returns an HTML page (a 502 happened during the same testing), response.json() raises aiohttp's ContentTypeError, whose message quotes the full request URL, token included. _api_request() now returns the status and content type for non-JSON responses instead.
  • Failed logins. Invalid credentials from params {params} puts the whole params dict, token included, into the exception. See [BUG] <title>Invalid Credentials from params #796, where it ended up in a public issue. Params are now masked in those messages.
  • Debug logging printed params and the full login response. Both are now masked.

Relationship to #955

#955 already redacts the login payload (_redact_auth_payload) and rewrites these authentication messages as part of its new login flow — credit to @mikelandzelo173. The two overlap on the login block. If #955 lands first, I'm happy to rebase so that its login block wins and this PR keeps only the non-200 check there; the _api_request masking, the non-JSON handling and the search changes are independent of it.

Merge order

I trial-merged this branch against every open PR touching qobuz.py, main.py or exceptions.py.

Merge cleanly: #847, #944, #982, #1007, #1013, #1024, #1031, #1036.

Conflict, with the resolution:

PR Where Resolution
#955 qobuz.py login block Take #955's login block; keep only the elif status != 200: raise APIError(...) branch from this PR.
#955 main.py imports Import both: from ..exceptions import APIError, AuthenticationError
#1005 qobuz.py login block #1005 wraps the login body in try to close the session. Keep its structure and apply this PR's _redacted(params) calls and the non-200 branch inside it.
#967 qobuz.py _api_request #967 rewrites _api_request with its own non-JSON handling, which raises without quoting the URL. Take #967's version and keep _redacted(params) in its debug log line. The retry then won't cover HTML error pages, which is acceptable.
#1023 exceptions.py Both append a class at the end of the file. Keep both.
#1025, #1029 main.py imports Both add an import on the same line. Keep both lines.

Whichever of these lands after this one, I'm happy to rebase.

Testing

tests/test_qobuz_errors.py (6 tests):

  • a transient Algolia 400 is retried once and then succeeds
  • a persistent failure raises APIError with Qobuz's message
  • other errors are not retried
  • an HTML error page is reported by status, without the token
  • a failed login's exception does not contain the token
  • rip search prints Search failed: … and returns instead of raising

The same mocked failing search, before and after:

before:  AssertionError: 400
after:   APIError: Qobuz artist/search failed (HTTP 400): Impossible to connect, please check your Algolia Application Id.

A failed login with a known token: the token appears in the exception message on dev, and does not with this change.

Full suite: 65 passed, 7 skipped; tests/test_meta.py::test_album_metadata_qobuz fails identically on unmodified dev. A live rip search --output-file against Qobuz succeeded twice, with no credentials in the output.

…of errors

rip search crashed with a bare "AssertionError: 400". The 400 was
Qobuz's own search backend failing -- "Impossible to connect, please
check your Algolia Application Id." -- and it cleared on its own: the
same search succeeded 20 and 40 seconds later. _paginate asserted on
the status and threw the message away.

Requests that must succeed now go through _request_ok(), which retries
once after a short pause when the failure looks transient (a 5xx, or
that Algolia message) and otherwise raises APIError carrying Qobuz's
own message. The search commands print it instead of a traceback. The
3-second pause is a guess: recovery was only observed at 20 seconds.

Credentials also reached error output. Qobuz takes user_id and
user_auth_token as URL query parameters, and an HTML error page -- a
502 from Qobuz's edge, seen during the same session -- made aiohttp
raise a ContentTypeError quoting the full URL, token included. A
failed login interpolated the whole params dict into the exception,
and debug logging printed params and the login response. Non-JSON
responses are now reported by status, a login answering anything but
200/400/401 raises APIError instead of failing on a missing key, and
params are masked wherever they reach a log line or an error.
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.

1 participant