Skip to content

[improvement] Wrap HTTPError in ConjureHTTPError with SerializableError details #22

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

Merged
merged 6 commits into from
Oct 17, 2018

Conversation

ahggns
Copy link

@ahggns ahggns commented Oct 16, 2018

Before this PR

HTTPError response content was parsed hoping to find SerializableError contents, though didn't extract all contents and relied on deprecated fields such as message. The original HTTPError and a message were then wrapped in a new HTTPError and reraised.

After this PR

  1. The original HTTPError is wrapped in a way (raise_from) that is understandable to Python 3's improved traceback functionality.
  2. The new ConjureHTTPError can be separately detected as such while maintaining a reference to its wrapped cause.
  3. The new ConjureHTTPError contains attributes to access fields from SerializableError, as well as the X-B3-TraceId from response headers when possible.

See also #21.

@@ -2,3 +2,4 @@
check_untyped_defs = True
verbosity = 0
show_column_numbers = True
ignore_missing_imports = True
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy was having a really hard time finding the future module, haven't been able to figure out why

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems wrong to me, I think we should fail if we can't get all of our dependencies. can we remove?

Copy link
Author

@ahggns ahggns Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently mypy just isn't about validating your use of third party libraries: python/mypy#1339 (comment). The two options presented are to build our own stubs where we care or just ignore missing imports.

@ahggns ahggns requested review from ferozco and iamdanfox October 17, 2018 10:12
@@ -142,3 +131,70 @@ def init_poolmanager(
ssl_context=ssl_context,
**pool_kwargs
)


class ConjureHTTPError(Exception):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For naming, maybe we should be throwing a RemoteException so it's similar to what you'd get with Java clients? This internally has a SerializableError with these fields and the HTTP error code?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should this extend HttpError? If people had previously been catching `HttpError in their code this'd now slip through right?

@ahggns
Copy link
Author

ahggns commented Oct 17, 2018

The tests are passing locally but failing here because they can't find the future module. Guessing this has to do with the cache only being based on tox.ini and not Pipfile (or Pipfile.lock?) or perhaps tox environments aren't being rebuilt with dependency updates? As far as I can tell that should all just flow from setup.py.

@ahggns ahggns merged commit 190cdd1 into develop Oct 17, 2018
@ahggns ahggns deleted the ah/conjure-http-error branch October 17, 2018 18:08
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.

3 participants