-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[BUG] [python-fastapi] A bundle of minor issues #11006
Comments
Adding to this mega-bug another issue we found: Fastapi generator converts camelCase query parameter to snake_case. example spec file to reproduce the problem, note the camelCase
generation command:
Generated request_api.py
Note hat |
For that last issue, I think the conversion of the parameter name is probably fine to do (it prevents issues with fields that are not valid Python identifiers and avoids conflicts with linters) but in this case an |
@TBBle thanks for reporting the issue cc @krjakbrjak |
204 issue may be related to #6842 |
Bug Report Checklist
Description
A bunch of minor issues noticed in the python-fastapi generated output. I'm putting them here for visibility, and hope to actually produce a PR to address them, but cannot currently commit to doing so immediately, and don't want them to be lost, bitrotting in a text file on my desktop.
They could also be broken out into separate issues if they are seen as more-complex than just "a minor template change", or need more discussion, or simply have more-complex examples.
setup.cfg ignores
packageVersion
The generated setup.cfg used the API spec version
{{appVersion}}
as the package version, instead of thepackageVersion
property, as other Python generators use. This breaks because the API spec version is astring
, where the setup.cfg version is more constrained.Dockerfile COPYs wrong venv
The Dockerfile's final
COPY
should probably be copying from
builder
, assuming the intent is to not include pytest.Issues with quotes in text
A couple of things appeared here:
A lot of the strings are being SGML-encoded, i.e.
'
becomes`
, for places that are not HTML. Particularly, the description in setup.cfg and the response-code descriptions in the generated Python got this treatment. ` got the same treatment when I tried to rephrase my descriptions to avoid'
.This can be seen in the Petstore sample in master. It's correct in the block comment but wrong in the
FastAPI
construction.Also, the setup.cfg description is not quoted, which meant that combined with the SGML-encoding, a description with
'
cuts off there as the resulting#
is seen as the start of a same-line comment.This issue is also visible in the Petstore sample in master: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/server/petstore/python-fastapi/setup.cfg#L4:
should be
uvloop isn't supported on Windows
uvloop in requirements.txt needs to be qualified to not install on Windows. Based on uvloop's setup.py, I ended up with
but that's in the Poetry syntax (I had already converted requirements.txt to Poetry before I noticed this issue), but the same markers should apply.
Endpoints without a 200 response are mishandled
If you don't provide a 200 response to FastAPI, it still assumes a 200 response exists. The generator is also assuming this, since its generated tests check for and produce a 200 response.
I was using a spec with only a 204 success response in this case. To make this work, the generated decorator needs to include a
status_code
kwarg targeting the correct, default, response.I suspect the default response code should be the first 2XX, if possible, but (per the next point) preferring non-204 response. If there's no 2XX response codes, then... I dunno.
This is made somewhat more complex because FastAPI really wants a 'success' result code to default to, but the in OpenAPI 3.0 Spec, that's only a should. Petstore unhelpfully has a couple of endpoints, e.g.,
DELETE /pet/{petId}
, which only provide error responses. FastAPI will, given this spec, assume you also have a 200 response you just forgot to mentionEndpoints that only provide error codes are hard to do right, because FastAPI idiomatically expects you to handle failure cases by raising an exception which it converts into a response elsewhere, and will happily add a 200 response to your API if you don't tell it not to.
####### Sub-thought on error handling
It might be a nice feature if the generated code handled error cases with a custom exception and registered an exception-handler to suit, since FastAPI's default exception handling has a specific return structure (which they helpfully don't document except the 422 request validation failure), and which bypasses response validation (because it generates a
JSONResponse
object directly) so effectively ignores the spec's idea of what that error's structure should be.I locally worked around this by writing the FastAPI HTTP error handler's output structure into my API spec, and used then as 422 (
FastAPIHTTPValidationErrorResponse
), 500 (FastAPI500Response
), and 4XX and 5XX (FastAPIFailureResponse
) responses in my end-points. This is more of an OpenAPI spec-writing VS FastAPI case though, not an OpenAPI Generator issue.My attempt to document the FastAPI default error handling behaviours
Endpoints with default response 204 need extra handling
For my use-case the code-generator did correctly determine that my end-point with only 204 (plus errors) was
-> None
, butpending some upstream changes in FastAPIup until FastAPI 0.79.0, that case also needs to have theresponse_type
kwarg overridden toResponse
so that it doesn't end up converting PythonNone
into JSONnull
and breaking the spec for 204 responses. (That in future may be the express-for-purpose)EmptyResponse
instead, which would implement the necessary special-casing for the various HTTP response codes that do not allow response bodies, like 204.I didn't see in the
responses
dictionary a way to specify the response type per-response, sadly, or this would be easier to automate. This kind-of makes sense though, as different response types are generally under the user's control by returning a subclass of Response (which also bypasses response format validation, it turns out).I don't yet know how the code-generator deals with return type when there's multiple possible responses (or even any response with a body, all my endpoints were 204-or-4XX in my initial use-case), but because of this, when multiple 2XX response options exist, it probably should only choose 204 as the 'default', so that a bodyful response is preferred.
In that case, it probably makes sense to include the
request
parameter in the generated handler, so that the response code can be overridden easily for bodiful cases. Hopefully this will eventually also do the right thing when used with a non-default 204 response.When I am actually looking into this, I'll put together some examples that illustrate what I mean here.
Next steps for my use-case may introduce non-204 success results to this API, so I hope to have more practical experience here soon.
Responses dictionary does not quote its keys when necessary
The endpoints I was creating were all
204|4XX|5XX
, and in the generated responses dictionary in the decorator,4XX
and5XX
were not quoted, and hence caused Syntax Errors straight out of the box.Models without a spec are rejected by pydantic by default
In my FastAPI error structure spec above, there's an optional field of type
object
, because it could be literally anything (string, list, dict, or anything else that'll pass the JSON encoder)However, Pydantic won't accept types for fields it doesn't recognise, including
object
. So the generated class looks like:but needs this added as an inner class so that Pydantic will accept it:
I'm hoping this is a simple thing to do when a model class is generated with an
object
orOptional[object]
field, as I expect any other case would either be another known model class, or a primitive type.openapi-generator version
openapitools/openapi-generator-cli:v5.3.0
was used when I saw these. I intend to, but have not yet, verified them with a master release.OpenAPI declaration file content or url
@TBBle: TODO.
Generation Details
From the directory where the new service will live, with the spec in
../generated/service.latest.oas3.yaml
.Steps to reproduce
Related issues/PRs
@TBBle: TODO a PR.
Suggest a fix
The text was updated successfully, but these errors were encountered: