-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add specific exceptions for authentication issues #10633
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
base: master
Are you sure you want to change the base?
Add specific exceptions for authentication issues #10633
Conversation
CodSpeed Performance ReportMerging #10633 will not alter performanceComparing Summary
|
a816ee5
to
ec5371c
Compare
ec5371c
to
cdaeb38
Compare
# avoid crashing down the line | ||
try: | ||
auth_from_url.encode() | ||
except UnicodeEncodeError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a breaking change for anyone trapping UnicodeEncodeError
currently since the new exceptions don't include it in the inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid the breaking change by making the new exceptions also derived from UnicodeEncodeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but hypothetically a case could be added later where these exceptions no longer stem from a UnicodeEncodeError
so that would also make it weird.
If we don't want to make the exceptions as generic as possible now, I'd include UnicodeEncodeError
in the exception names in addition to the inheritance to avoid someone reusing it for some other root error down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use InvalidRedirectUrlAuthClientUnicodeEncodeError
and InvalidUrlAuthClientUnicodeEncodeError
, which inherit from both.
I know, they're a bit of a mouthful 😉
or maybe just InvalidRedirectUrlAuthClientUnicodeError
and InvalidUrlAuthClientUnicodeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a shorter [Redirect]UrlAuthUnicodeEncodingError
?
What do these changes do?
They add more specific exceptions for authentication issues to avoid getting a blanket
UnicodeEncodeError
when authentication credentials cannot be encoded.Are there changes in behavior for the user?
Yes, but for the better as they will get clearer exceptions. Instead of getting a
UnicodeEncodeError
in such cases, users will be met with one of the following:InvalidAuthClientError
if it's the authentication passed via theauth
parameter that cannot be encodedInvalidUrlAuthClientError
(subclass ofInvalidURL
) if it's the authentication that is present in the input URL that cannot be encodedInvalidRedirectUrlAuthClientError
(subclass ofInvalidUrlRedirectClientError
) if it's the authentication that is present in a redirect URL that cannot be encodedI initially was going to go with a single exception, but I believe differentiating them makes sense, as:
Each of these could warrant a different response from the user depending on their need, but at the same time the ones for which the user is potentially not responsible can still be caught by a blanket
except InvalidURL
block.For a bit more context, in my implementation I currently would need to catch
UnicodeEncodeError
on a block that does way more than just do theGET
call via aiohttp, and it could very well be that at some point unrelated code in this block raises aUnicodeEncodeError
, which I would need to know about. There are of course ways around that, but having a more specific exception is just cleaner overall.Is it a substantial burden for the maintainers to support this?
No.
Related issue number
N/A
Checklist
CONTRIBUTORS.txt
CHANGES/
folder => waiting for a go-ahead first since this is not linked to an open issuename it
<issue_or_pr_num>.<type>.rst
(e.g.588.bugfix.rst
)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix
: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature
: A new behavior, public APIs. That sort of stuff..deprecation
: A declaration of future API removals and breakingchanges in behavior.
.breaking
: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc
: Notable updates to the documentation structure or buildprocess.
.packaging
: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib
: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc
: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.