-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Use httpx.Client Directly #202
Comments
Thanks for suggestion @kalzoo, I really appreciate you taking the time to think this through. Here are the concerns I have initially:
So I think those concerns rule out the first option of using It seems to be that the only really safe way to use Simple use case, same generated API as today, user just calls the function# No connection pooling, just like today
result = something.sync(client=client)
other = other.sync(client=client) User wants connection pooling or to manage httpx themselveswith client as httpx_client: # httpx_client is some new HttpxClient which inherits from Client
httpx_client.httpx # This is httpx.Client so do whatever you want
result = something.sync(client=httpx_client)
other = other.sync(client=httpx_client) Generated functions look like thisdef sync(*, client: Client):
with client as _client:
# if client was the base Client, this gives you a new HttpxClient which manages connection internally
# If client was already an HttpxClient (user controlling it themselves) then it returns itself
response = _client.httpx.post() Then for async variants I think we can declare Anyway, that's generally what I'm thinking so far. Definitely let me know what you think! |
Thanks for the thoughtful reply! You bet - I realized I had forgotten You make a good point that we can't just generate an Another possibility, then: accepting a callback to build a client. Thus, the |
A couple quick points:
A naive, simple approach would be to add an additional method, similar to what import httpx # We're already importing httpx so this doesn't seem like a big deal.
# ...
def httpx_request({{ arguments(endpoint) | indent(4) }}) -> Response[{{ return_string }}]:
{{ header_params(endpoint) | indent(4) }}
{{ query_params(endpoint) | indent(4) }}
{{ json_body(endpoint) | indent(4) }}
response = client.request(
{{ endpoint.method }},
{{ endpoint.path }},
{% if endpoint.json_body %}
json={{ "json_" + endpoint.json_body.python_name }},
{% endif %}
{% if endpoint.query_parameters %}
params=params,
{% endif %}
{% if endpoint.form_body_reference %}
"data": asdict(form_data),
{% endif %}
{% if endpoint.multipart_body_reference %}
"files": multipart_data.to_dict(),
{% endif %}
)
return _build_response(response=response) If this method is provided in addition to existing signatures, nothing breaks and httpx dependency is not pushed off to the client (they can use the existing methods). That said, if you no longer want to support httpx at a future date, the external API breaks, requiring a major version bump. Tangentially, I think it's possible users may want customize other parts of I think JupyterHub provides some potential guidance here. They allow users to customize their UI Jinja templates. They accomplish this by:
{% extends "templates/spawn_pending.html" %}
{% block message %}
{{ super() }}
<p>Patience is a virtue.</p>
{% endblock %} I'm not seeing Jinja blocks in this repo, so that second point may be a bigger lift than what's worthwhile. That said, providing a custom templates path seems like a fairly straightforward change, giving users more customization power and reducing responsibility of this repo to take a stance on ambiguous issues. |
@erichulburd I'm absolutely on board with having users be able to provide their own templates for either partial or full override of the generated client. It was brought up before in #171 though I think I only discussed particulars briefly. I'm going to leave a more thoughtful response on the matter in #231 . While custom templates are great for a wide variety of things, for this issue- I do think we still want to utilize httpx clients more effectively in the default template to benefit from connection pooling. |
+1 I do wait for this Enhancement , Here is my usecase. I like to do deep debugging with the clients generated using openapi-python-client. I would like to see request, response objects for every api call is logged/ dumped for analysis. From the httpx documentation, its possible using Event Hooks (https://www.python-httpx.org/advanced/). But I could not successfully able to hook the events. def log_httpx_request(request: httpx.Request) -> None:
print ("*** ", type(request))
print(f"+++Request event hook: {request.method} {request.url} - Waiting for response")
def log_httpx_response(response: httpx.Response) -> None:
print ("***", type(response))
request = response.request
print(f"+++Response event hook: {request.method} {request.url} - Status {response.status_code}")
...
client = Client(
base_url="https://codebeamer.xxx.com/cb",
headers={"Authorization": "Basic " + base64.b64encode(basic_auth_data).decode("ascii")},
timeout=10.0,
)
client.event_hooks={'request': [log_httpx_request], 'response': [log_httpx_response]}
version_data = get_server_version.sync(client=client)
... HTTPx event hook is not called at all. Environment:
|
I know a ton of use-cases are waiting on this issue—so if anyone wants to see my general thinking around it, you can check out #775. This will be a breaking change, sometimes subtly, to do it how I think is the "right" way (encourage consumers to always use context managers) |
As of version 0.6.1, the generated
Client
is somewhat configurable - headers, cookies, and timeout. However, these are all abstractions which have to then be handled explicitly within each generated API method.Would it be simpler to just make calls using an
httpx.Client
orhttpx.AsyncClient
instance, and allow consumers to configure that directly? Advantages:httpx
can be supported, and there's less likelihood that you'll have to change your package due to changes or new features inhttpx
.httpx.get
etc, and explicitly whathttpx
recommends in its documentation:of course, this package does use the context manager within API operations, but that doesn't allow multiple calls to share the same client and thus connection.
httpx
itself.AuthenticatedClient
andClient
can just each just become anhttpx.Client
configured with different headers.tl;dr: it decreases coupling between the two packages and lets you worry less about the client configuration and how to abstract it. More
httpx
functionality will be directly available to consumers, so you'll get fewer (actionable) feature requests. Future breaking changes here will be less likely. Seems like this alone would allow closing a couple currently pending issues (retries, different auth methods, response mimetypes), by putting them entirely in the hands of the consumer.Describe the solution you'd like
There are a few options.
httpx.Client
could be used directly (i.e. replaceclient.py
entirely). API methods would just accept the client and use it directly, and it would be up to the caller to configure and manage it. This is the simplest for sure, and meets the current use case. This is what I'd recommend.Client
could wrap anhttpx.Client
which allows you to add convenience methods as needed, and stay in control of theClient
object itself. This abstraction layer offers protected variation, but wouldn't be used for anything right now - headers, timeouts, and cookies can all be configured directly on anhttpx.Client
. However this need could also be met with configuration values passed directly to each API operation.Client
and proxy calls (with__getattr__
) to an inner client, or typecheckclient
on each API operation to see if you've got aClient
orhttpx.Client
. This allows them to be used interchangeably in API operations. This one's the most fragile and doesn't offer any advantages at the moment.Of course, this would all apply to
AsyncClient
for theasyncio
calls.Additional context
Happy to send a PR, can do it pretty quickly. Am looking to use this in production, and would love to stay on (and contribute to) mainline rather than a fork!
The text was updated successfully, but these errors were encountered: