Fix AttributeError: 'ClientConnectorCertificateError' object has no attribute '_os_error'.#12136
Fix AttributeError: 'ClientConnectorCertificateError' object has no attribute '_os_error'.#12136themylogin wants to merge 1 commit intoaio-libs:masterfrom
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12136 +/- ##
==========================================
- Coverage 98.78% 98.77% -0.01%
==========================================
Files 128 128
Lines 45297 45302 +5
Branches 2403 2403
==========================================
+ Hits 44747 44749 +2
- Misses 390 393 +3
Partials 160 160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CHANGES/12136.bugfix.rst
Outdated
| @@ -0,0 +1,2 @@ | |||
| Fixed ``ClientConnectorCertificateError.os_error`` raising ``AttributeError``. | |||
There was a problem hiding this comment.
| Fixed ``ClientConnectorCertificateError.os_error`` raising ``AttributeError``. | |
| ``ClientConnectorCertificateError.os_error`` no longer raises :exc:`AttributeError`. |
tests/test_client_exceptions.py
Outdated
| err = client.ClientConnectorCertificateError( | ||
| connection_key=self.connection_key, certificate_error=certificate_error | ||
| ) | ||
| assert err.os_error is not None |
There was a problem hiding this comment.
This would have to check for an actual error code, not that it's something.
|
I think this is an example of bugs getting introduced due to supporting non-ssl builds. I'm thinking it might be time to drop support for non-ssl builds soon. |
Fair. I think "missing openssl" as a thing is coming from the late Python 2 epoch. OTOH, nowadays this might be a property of less common runtimes (pyodide etc). |
CHANGES/12136.bugfix.rst
Outdated
| @@ -0,0 +1,2 @@ | |||
| ``ClientConnectorCertificateError.os_error`` no longer raises :exc:`AttributeError`. | |||
There was a problem hiding this comment.
Broken render.
| ``ClientConnectorCertificateError.os_error`` no longer raises :exc:`AttributeError`. | |
| ``ClientConnectorCertificateError.os_error`` no longer raises :exc:`AttributeError` |
| err = client.ClientConnectorCertificateError( | ||
| connection_key=self.connection_key, certificate_error=certificate_error | ||
| ) | ||
| assert err.os_error == OSError() |
There was a problem hiding this comment.
We could go for isinstance() here, I suppose.
Though, I feel weird about the empty exception object as a hack.
cc @Dreamsorcerer WDYT?
…ttribute '_os_error'. ClientConnectorCertificateError is a subclass of ClientConnectorError and should have all of its attributes (LSP). However, when I try to access the os_error attribute, I get the mentioned exception.
| self, connection_key: ConnectionKey, certificate_error: Exception | ||
| ) -> None: | ||
| self._conn_key = connection_key | ||
| super().__init__(connection_key, OSError()) |
There was a problem hiding this comment.
I think certificate_error should be passed here:
| super().__init__(connection_key, OSError()) | |
| super().__init__(connection_key, certificate_error) |
The parent class will need the annotation updated to allow it.
There was a problem hiding this comment.
The parent class will need the annotation updated to allow it.
No, it shouldn't actually. The certificate error should be a subclass of OSError.
3 participants
What do these changes do?
ClientConnectorCertificateErroris a subclass ofClientConnectorErrorand should have all of its attributes (Liskov substitution principle). However, when I try to access theos_errorattribute in my exception handler, I get the following exception:Are there changes in behavior for the user?
Users that use
hasattr(e, 'os_error')to ensure thateis not an instance ofClientConnectorCertificateError(or its subclasses), will have their code broken.Is it a substantial burden for the maintainers to support this?
There should be none.
Checklist
CONTRIBUTORS.txtCHANGES/foldername 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.