Skip to content

Return the Response object when the content type is unspecified #141

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

Closed
emann opened this issue Aug 7, 2020 · 6 comments
Closed

Return the Response object when the content type is unspecified #141

emann opened this issue Aug 7, 2020 · 6 comments
Labels
✨ enhancement New feature or improvement
Milestone

Comments

@emann
Copy link
Collaborator

emann commented Aug 7, 2020

Is your feature request related to a problem? Please describe.
Sometimes the content type is unspecified in the OpenAPI doc (For instance, FastAPI's RedirectResponse doesn't convert to a proper response type), leading to functions accessing these endpoints to simply return None upon success.

Describe the solution you'd like
When an endpoint's content type is unspecified, the generated function should just return the Response object. Returning the Response in any case where a successful operation would return None might be a good idea as well as a catch-all. This is different from #115, as this only applies to certain endpoints and would just return the raw Response object as there is nothing else (schemas etc) to access.

Additional context
In my specific case, I have an endpoint that redirects to a download, and I am unable to use the generated function for said endpoint to download the file.

@emann emann added the ✨ enhancement New feature or improvement label Aug 7, 2020
@dbanty
Copy link
Collaborator

dbanty commented Aug 10, 2020

@emannguitar this feels like something we could cover with #115 if we designed it intelligently. I haven't put too much thought into a clean API for that. I'd ideally like to give access to the full response without having to generate a different client.

I thought about maybe making all endpoints return a custom Result object or something which includes both the actual payload (if any) and the Response info. Though I'd like to avoid tightly coupling our API to httpx if possible. Right now it's just an implementation detail that we can change if we want to.

@emann
Copy link
Collaborator Author

emann commented Aug 10, 2020

@dbanty With #146 I was thinking of adding custom headers to the Client instance itself to be passed along to the endpoint functions, while also having an optional param on each endpoint function for any per-call headers - maybe something similar could be done for the value of wrap_response?

You could specify a default value for the wrap_response param when creating a Client, but also have the ability to override it on a per-call basis by passing wrap_response directly when calling the function. This would give the best of both worlds for the desires of #115, allowing wrap_response to be defined (essentially) globally, but also addresses this issue by allowing me to just pass wrap_response=True to the endpoint function in question.

Thoughts?

@dbanty
Copy link
Collaborator

dbanty commented Aug 10, 2020

I don't see any easy way to make this typesafe, which is an objective of the project as a whole. Unless we subclass Client and have a RawClient or something. Then we could use the @overload decorator to declare one variant of each endpoint that returns the response. We should either put it in the client with a sub-type or provide a param to each endpoint, not both.

We still have the issue of deciding what to return. Do we make a custom Response class that surfaces both our parsed data and some other data (e.g. status code, headers, raw text/bytes)? Do we expose the httpx Response and just accept the fact that we're tied to httpx and we'll break compatibility any time they do?

@bowenwr
Copy link
Contributor

bowenwr commented Aug 10, 2020

Do we make a custom Response class that surfaces both our parsed data and some other data (e.g. status code, headers, raw text/bytes)

I like this idea, except it also be nice to return body() which would be the deserialized model that we return now. In the case of unspecified content-type, this could be bytes.

@dbanty dbanty added this to the 0.6.0 milestone Aug 10, 2020
@emann
Copy link
Collaborator Author

emann commented Aug 13, 2020

In case anyone is trying to do something similar (RedirectResponse redirecting to a file download): In my FastAPI app I now specify the content type as octet-stream which, combined with #157, solves the issue in this particular use case.

@dbanty
Copy link
Collaborator

dbanty commented Sep 21, 2020

This should be covered well enough by the upcoming 0.6.0 release (currently alpha), so I'm marking this one done.

@dbanty dbanty closed this as completed Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
Development

No branches or pull requests

3 participants