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

Middleware API design doc #800

Closed
wants to merge 2 commits into from
Closed

Middleware API design doc #800

wants to merge 2 commits into from

Conversation

florimondmanca
Copy link
Member

Pushing a simplification of #783, focused on docs (and minimal code changes to allow example code snippets to work, i.e. hooking middleware into Client.get().)

Not sure we'd want to move forward with this whole middleware idea yet but I wanted to make a case for it here.

I'm pretty much convinced this is the correct API if we wanted to provide a general-purpose extension mechanism for clients.

It allows the following:

  • Intercepting/inspecting/modifying requests
  • Intercepting/inspecting/modifying responses
  • Sending multiple requests (w/o being coupled to the client or the sync/async environment).
  • Handling exceptions in a predictable fashion using try/except blocks.

An extension mechanism has benefits in itself, even if we don't immediately use it internally (e.g. for auth/redirects as initially implemented in #783). Mainly ,it would greatly reduce any tendency towards feature creep, and help us keep our core API/feature set under control. (Many "so I have this particular feature request…" issues could be addressed as "consider building a middleware".)

For example, retries (#784) and hooks (#790) can be implemented as middleware, with a much lower impact for us, and most likely more value for users (as focused third-party packages can afford going much deeper into the feature set, whereas we'd have a tendency to keep things minimal to reduce maintenance overhead).

(Eg. it's really super interesting to me how more powerful and versatile a retries middleware built on top of tenacity would be, compared to us bringing some very limited built-in functionality via #784… See example in the docs here.)

@florimondmanca florimondmanca added the enhancement New feature or request label Feb 2, 2020
@florimondmanca florimondmanca changed the title Middleware API Middleware API design doc Feb 2, 2020
@StephenBrown2
Copy link
Contributor

This is definitely cool. So this would only add on to the current "middleware" baked-in to httpx, and not replace it?

@florimondmanca
Copy link
Member Author

florimondmanca commented Feb 3, 2020

So this would only add on to the current "middleware" baked-in to httpx, and not replace it?

It’s not required that we switch the implementation for redirect/auth to using this middleware API, but we very much could eventually. (See the original #783 for how that might look. I left it out from this PR to focus on the API itself.)

def __init__(self, get_response):
self.get_response = get_response

def __call__(self, request, timeout, **kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

It just occurred to me now that we might want to make it so that timeout is buried inside **kwargs. It’s not useful for the vast majority of middleware, so exposing it as a positional argument in the API is somewhat clunky.

Client options: {'client_option': 'example'}
Sending request...
Request: <Request('GET', 'https://example.org')>
Request options: {'request_option': 'example'}
Copy link
Member Author

Choose a reason for hiding this comment

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

This might actually not be true, as auth=None and allow_redirects=True might end up in there.

@StephenBrown2
Copy link
Contributor

Yeah, I was thinking of the original #783 with that. It's good to be separated for now. In the future middleware_stack would just need to be aware of the builtins and possibly allow for overriding, but that's for the future.

@florimondmanca
Copy link
Member Author

Just found out about this Elixir HTTP client that’s completely built around the concept of middleware: https://github.com/teamon/tesla

Middleware there only allows sending a single request, but there’s a notion of context that allows passing state between recursive calls.

In fact, even adding in headers or a base_url is handled via middleware. Probably over engineered for these simple cases, but it occurred to me that, well, yes we could technically achieve that here too. (But we probably shouldn’t!)

Anyway dropping it here ‘cause I think it’s another example that this whole middleware thingy probably makes sense to have in some day.

@florimondmanca
Copy link
Member Author

I guess I'll close this for now, as this is not in our priorities for 1.0. Let's reconsider after we reached that milestone. :-)

@florimondmanca florimondmanca deleted the middleware-api branch March 9, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants