Skip to content
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

Beta: Collapse HTTPClientAsync into HTTPClient #1239

Merged
merged 3 commits into from
Feb 17, 2024

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Feb 16, 2024

Summary

This

  • Eliminates HTTPClientAsync and adds its methods directly onto HTTPClient.
    • And HTTPClientBase too, as there is now only one class that inherits from it and it was introduced explicitly for HTTPClientAsync
  • Makes the HTTPClient constructor optionally accept a new parameter async_fallback_client. Calls to async methods will be handled by the fallback client if it is present, instead of raising "NotImplemented".
  • Changes api_requestor and stripe_client so that they do stripe.default_http_client = new_default_http_client(...) (as they do when the user provides no custom http client), a fallback client is provided (HTTPXClient or NoImportFoundAsyncClient).
  • Includes / builds on Testing: unify http client mock #1242 to simplify mocking infrastructure.

Details

I have chosen a design where

stripe.api_key = "sk_test_xyz"
async def main():
  # This works
  await stripe.Customer.create_async(...)

async def main2():
  stripe.default_http_client = stripe.HTTPXClient()
  # This works too
  await stripe.Customer.create_async(...)

async def main3():
  stripe.api_key = "sk_test_xyz"
  stripe.default_http_client = stripe.RequestsClient()
  # But this will fail with NotImplemented
  await stripe.Customer.create_async(...)

async def main4():
  stripe.default_http_client = stripe.new_default_http_client()
  # This also fails with NotImplemented
  await stripe.Customer.create_async(...)

async def main5():
  stripe.default_http_client = stripe.RequestsClient(async_fallback_client=stripe.HTTPXClient())
  # This would work
  await stripe.Customer.create_async(...)

This avoids some ambiguity in cases like

client = stripe.RequestsClient(timeout=5)

If this created a fallback client automatically, should the timeout=5 be forwarded through to the stripe.HTTPXClient? What if "timeout" means something different on stripe.RequestsClient than it does on stripe.HTTPXClient?

On the other hand, it does means fewer people will be able to upgrade their library, add await and "_async" to all their stripe method calls and have it Just Work™️ . (Things will Just Work™️ for people who use the defaults, but not for anybody making custom instances of Stripe's built-in http clients.)

Changelog

  • ⚠️ Removes the stripe.default_http_client_async global and the stripe.HTTPClientAsync class.
    • To set your own async-enabled http client, set stripe.default_http_client to a subclass of stripe.HTTPClient such as stripe.HTTPXClient that implements .request_async, .sleep_async, .request_stream_async, and .close_async.
    • The default http client of the library is still RequestsClient for synchronous methods, that "falls back" to a HTTPXClient when asynchronous methods are called.

@richardm-stripe richardm-stripe changed the base branch from master to beta February 16, 2024 03:23
@richardm-stripe richardm-stripe force-pushed the richardm-http-client-refactor branch 2 times, most recently from 48b6b04 to 5877097 Compare February 16, 2024 08:18
@richardm-stripe richardm-stripe requested review from a team, pakrym-stripe and helenye-stripe and removed request for a team and pakrym-stripe February 16, 2024 19:59
@richardm-stripe richardm-stripe marked this pull request as ready for review February 16, 2024 22:37
@richardm-stripe richardm-stripe changed the title [WIP] Beta: Collapse HTTPClientAsync into HTTPClient Beta: Collapse HTTPClientAsync into HTTPClient Feb 16, 2024
@helenye-stripe
Copy link
Contributor

LGTM though also tagging Annie. Just one clarifying question for my understanding, what would be a case where a user uses async_fallback_client versus just using an HTTPX client, or is this for case where they use a different async client?

@richardm-stripe
Copy link
Contributor Author

what would be a case where a user uses async_fallback_client versus just using an HTTPX client, or is this for case where they use a different async client?

I expect this to be rare but I can contrive an example: suppose you are a user migrating to async and you want to do it bit by bit instead of all at once -- maybe you are a large enterprise and a different team owns a different part of the Stripe integration. If you start with

stripe.default_http_client = stripe.RequestsClient(timeout=10)

then you can't use async.

If you switch to

stripe.default_http_client = stripe.HTTPXClient(timeout=10)

then you can begin to use async piece by piece, but you have inadvertently caused the entire sync integration to switch over to HTTPX instead of Requests. If you really want the migration to httpx/async to be done piecemeal you can

stripe.default_http_client = stripe.RequestsClient(timeout=10, async_client_fallback=stripe.HTTPXClient(timeout=10))

Should we make async_fallback_client private, considering this use case doesn't seem that common?

I lean towards keeping it public. It would feel weird to me if the stripe.default_http_client that we give users by default had behavior that it wasn't possible for users to recreate without reaching into internals.

@helenye-stripe
Copy link
Contributor

I agree, am peaceful with keeping it public. Thanks for the explanation!

Copy link
Contributor

@anniel-stripe anniel-stripe left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks for the thoughtful PR description with pros / cons and drive-by clean-up of the http_client_mock interface!

@richardm-stripe richardm-stripe merged commit be5b48a into beta Feb 17, 2024
15 checks passed
@richardm-stripe richardm-stripe deleted the richardm-http-client-refactor branch February 17, 2024 00:27
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.

None yet

3 participants