refactor 401 retrying into both auth methods #1910
Conversation
gmishkin
force-pushed
the
jira-session-retry
branch
from
1176bfd to
4428531
Compare
gmishkin
changed the title
gmishkin
changed the title
|
@ssbarnea not sure what to do about the check failures, can you advise? |
|
Hi @adehad! Can you please review this PR? Thank you! |
gmishkin
force-pushed
the
jira-session-retry
branch
from
4428531 to
58cd331
Compare
|
Hi, This is now impacting multiple workflows on our side. |
studioj
force-pushed
the
jira-session-retry
branch
from
c4d7b6f to
4b6ad60
Compare
| self._retry_counter_401 = 0 | ||
| self._max_allowed_401_retries = 1 # 401 aren't recoverable with retries really | ||
|
|
||
| def init_session(self): | ||
| """Auth mechanism specific code to re-initialize the Jira session.""" | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
| raise NotImplementedError() | |
| raise NotImplementedError() |
let's make this an abstract base class and define this as an abstract method. To allow us to pick up errors in static analysis rather than runtime.
| session_api_url (str): The session api url to use. | ||
| auth (Tuple[str, str]): The username, password tuple. | ||
| """ | ||
| def __init__(self, session: ResilientSession | None = None): |
There was a problem hiding this comment.
Let's remove the default of None here, and then remove as many if not all the plain asserts.
If there do end up being situations where the session is None when passing to the constructor of the subclasses then let us move to a subclass __init__ method override, and rather than asserts, either use warnings, or if it should really fail, let's use an Exception with suitable message.
I want to avoid asserts in the production codebase.
|
@adehad I agree with both of your requested changes but just wanted to give you a heads up that we have our internal Jira 10 upgrade planned for next month, and if you look at the linked issue, it appears that someone found an upstream issue that matches the symptoms I was seeing here, that's fixed in Jira 10. So, I may not get back to this PR, and honestly, you would be forgiven for closing this PR outright, since it's just a workaround for a bug that's resolved in the latest supported major version of the non-cloud Jira software (I don't know what your support policy is for older versions of the Jira server). |
|
Thanks @gmishkin for this update, I didn't catch that. Sorry for the delay in getting this merged when it would have made the most impact to you. I think there is still value in keeping it open and even merging, as I can imagine there are individuals affected that won't get the change to migrate as soon. Happy to leave that decision to you or any other budding contributors and happy to facilitate. I've just come back to open source contributions after a long pause, but now that I have time available I should be able to accelerate merging if it is ready. |
|
@adehad I don't know if this is a thing that exists on GitHub, but if you're able to you should "pin" this comment Adding a comment to the code describing what it's working around makes sense too 👍 P.S. no worries about not getting it merged, we have an internal pypi instance I've been publishing this so that our internal apps can use. |
See #1904 for details on why.
This change refactors 401 retrying into the common base class, and for token auth the retry is implemented by clearing the session cookie and retrying with the same PAT as recommended.
In a few cases, the code is kind of ugly because in order to implement the retry I need to have a reference to the session object, which is only available if the tokenauth is created through the normal client constructor. If the tokenauth is created directly then the session might not be available. I am not sure if this is a supported case, if it's not then I might be able to clean up the checks for session and the asserts.