[BUG] [python-fastapi] A bundle of minor issues

TBBle commented 2 years ago

Bug Report Checklist

[ ] Have you provided a full/minimal spec to reproduce the issue?
[ ] Have you validated the input using an OpenAPI validator (example)?
[ ] Have you tested with the latest master to confirm the issue still exists?
[x] Have you searched for related issues/PRs?
[ ] What's the actual output vs expected output?
[ ] [Optional] Sponsorship to speed up the bug fix or feature request (example)

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 the packageVersion property, as other Python generators use. This breaks because the API spec version is a string, where the setup.cfg version is more constrained.

Dockerfile COPYs wrong venv

The Dockerfile's final COPY

COPY --from=test_runner /venv /venv

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:

description = This is a sample server Petstore server. For this sample, you can use the api key &#x60;special-key&#x60; to test the authorization filters.

should be

description = "This is a sample server Petstore server. For this sample, you can use the api key 'special-key' to test the authorization filters."

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

uvloop = { markers = "sys_platform != 'win32' and sys_platform != 'cygwin' and sys_platform != 'cli'", version = "0.14.0" }

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 mention

Endpoints 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

```yaml components: schemas: FastAPIHTTPValidationError: description: |- Structure generated by FastAPI when request validation fails. Returned with error code 422. See `fastapi.exception_handlers.request_validation_exception_handler`. Specification per auto-generated OAS 3 of FastAPI 0.7.0. required: [] type: object properties: detail: description: Detail type: array items: $ref: '#/components/schemas/FastAPIValidationError' FastAPIValidationError: description: "" required: - loc - msg - type type: object properties: loc: description: Location type: array items: type: string msg: description: Message type: string type: description: Error Type type: string FastAPIHTTPException: title: Root Type for FailureResponse description: |- Structure generated by FastAPI when [a `HTTPException` is raised](https://fastapi.tiangolo.com/tutorial/handling-errors/?h=error). See `fastapi.exception_handlers.http_exception_handler`. required: [] type: object properties: detail: description: "" additionalProperties: false example: detail: {} responses: FastAPIFailureResponse: content: application/json: schema: $ref: '#/components/schemas/FastAPIHTTPException' description: The default failure response for FastAPI-based projects. FastAPI500Response: content: application/json: schema: $ref: '#/components/schemas/FastAPIHTTPException' text/plain: {} text/html: {} description: |- Union of FastAPIFailureResponse and the two extra responses that may be generated by Starlette with a 500 error. Failures raised by endpoints will be `application/json`. `text/plain` and `text/html` will be raised for unhandled exceptions and similar codebase issues. `text/html` will only be seen from a server running in 'debug' mode. FastAPIHTTPValidationErrorResponse: content: application/json: schema: $ref: '#/components/schemas/FastAPIHTTPValidationError' description: FastAPI request validation failure response. ```

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, but ~~pending some upstream changes in FastAPI~~ up until FastAPI 0.79.0, that case also needs to have the response_type kwarg overridden to Response so that it doesn't end up converting Python None into JSON null 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 and 5XX 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:

class FastAPIHTTPException(BaseModel):
    """NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).

    Do not edit the class manually.

    FastAPIHTTPException - a model defined in OpenAPI

        detail: The detail of this FastAPIHTTPException [Optional].
    """

    detail: Optional[object] = None

but needs this added as an inner class so that Pydantic will accept it:

    class Config:
        arbitrary_types_allowed = True

I'm hoping this is a simple thing to do when a model class is generated with an object or Optional[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.

docker run --rm -v ${PWD}:/local -v ${PWD}/../generated:/generated openapitools/openapi-generator-cli:v5.3.0 generate --input-spec /generated/service.latest.oas3.yaml --generator-name python-fastapi --output /local/service/ --additional-properties packageName=service --additional-properties packageVersion=1.0 --additional-properties disallowAdditionalPropertiesIfNotPresent=false --additional-properties legacyDiscriminatorBehavior=false

Steps to reproduce

Related issues/PRs

@TBBle: TODO a PR.

Suggest a fix

thisislj commented 2 years ago

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 filterBy parameter

---
openapi: 3.0.2
info:
  title: test spec
  version: "0.1"
  description: This is a test spec
paths:
  /request:
    get:
      tags:
      - Request
      parameters:
      - name: "filterBy"
        description: "filter condition"
        schema:
          type: string
        in: query
      responses:
        "200":
          description: Alive
      summary: Get request
      description: |
        Get requests

generation command:

docker run --rm -v ${PWD}:/local -v ${PWD}/../generated:/generated openapitools/openapi-generator-cli:v5.3.0 generate --input-spec /generated/bug_repro.yml --generator-name python-fastapi --output /local/test_service/ --additional-properties packageName=test_service --additional-properties packageVersion=1.0 --additional-properties disallowAdditionalPropertiesIfNotPresent=false --additional-properties legacyDiscriminatorBehavior=false

Generated request_api.py

@router.get(
    "/request",
    responses={
        200: {"description": "Alive"},
    },
    tags=["Request"],
    summary="Get request",
)
async def request_get(
    filter_by: str = Query(None, description="filter condition"),
) -> None:
    """Get requests """
    ...

Note hat filterBy has been converted to filter_by.

TBBle commented 2 years ago

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 alias keyword parameter for Query must be provided in order to correctly match incoming requests.

wing328 commented 2 years ago

@TBBle thanks for reporting the issue

cc @krjakbrjak

srl295 commented 2 years ago

204 issue may be related to https://github.com/OpenAPITools/openapi-generator/issues/6842

OpenAPITools / openapi-generator