fix: when EJR tokens are invalid a refresh should be attempted - #526
fix: when EJR tokens are invalid a refresh should be attempted#526pvbouwel wants to merge 2 commits into
Conversation
| self._log.exception(f"Failed to do EJR API request `{method} {url}`: {e!r}") | ||
| raise EjrApiError(f"Failed to do EJR API request `{method} {url}`") from e | ||
| self._log.debug(f"EJR response on `{method} {path}`: {response.status_code!r}") | ||
| if response.status_code == 401 and use_auth: |
There was a problem hiding this comment.
I think this must go under the try body above, right?
There was a problem hiding this comment.
Makes me also wonder about the need for that reretry.retry_call stuff, instead of the http retry utilities we're more used to
There was a problem hiding this comment.
Why do you think it goes there? Exceptions are for scenarios that are not handled as part of HTTP responses spec (e.g. connection issues like connection resets, are client side timeout triggering). In this case the server replies with a 401 status code so a valid HTTP response. So it is further client-side handling that should determine what to do with this response.
I was trying to make a minimal change. If you prefer to have retry.retry_call refactored out that is fine by me but I would create a separate issue for that. Because for a change like that I would think it be good if the scope is:
- Add an AGENTS.md file stating the preferred way of making http calls
- Have code refactor
Then future agent reviews would also help to streamline usage
There was a problem hiding this comment.
Ah or you mean to have handling of exceptions of the extra do_request for that it would actually make sense, digesting further :)
There was a problem hiding this comment.
OK, moved it because it could help avoid confusion if the request after clearing cache does get one of those connection type of issues. I guess that is a very rare case but Murphy is always around the corner and in this case either placement has similar complexity so I don't see any negative implications.
2 participants
No description provided.