Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

List[...] as type for a Form field doesn't work as expected #3532

Closed
HitLuca opened this issue Jul 14, 2021 · 17 comments
Closed

List[...] as type for a Form field doesn't work as expected #3532

HitLuca opened this issue Jul 14, 2021 · 17 comments

Comments

@HitLuca
Copy link

HitLuca commented Jul 14, 2021

Opening a new issue since no action has been taken on #842 for more than a month.

Looks like we have an issue on how fastapi handles lists of elements passed as Form parameter (so anything that is type hinted List[...] = Form(...)). The issue doesn't arise when they are defined as Query parameters.
After some digging I managed to get to the root of the problem, which is how we pass the parameters when making an api call. fastapi indeed correctly understands that the input needs to be a list of elements, but doesn't parse the list if it comes from a single field.

Example using Query, works as expected

@app.post("/")
def api_test(l: List[int] = Query(...)) -> List[int]:
    return l

Api request generated using the swagger UI

curl -X 'GET' \
  'http://localhost:5009/?l=1&l=2' \
  -H 'accept: application/json'

note how the passed values are l=1&l=2

[
  1,
  2
]

Example using Form, doesn't work as expected

@app.post("/")
def api_test(l: List[int] = Form(...)) -> List[int]:
    return l

Api request generated using the swagger UI

curl -X 'POST' \
  'http://localhost:5009/' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -d 'l=1,2'

note how the passed values are l=1,2

{
  "detail": [
    {
      "loc": [
        "body",
        "l",
        0
      ],
      "msg": "value is not a valid integer",
      "type": "type_error.integer"
    }
  ]
}

Manual api request

curl -X 'POST' \
  'http://localhost:5009/' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -d 'l=1' \
  -d 'l=2'

Now the values are passed as -d 'l=1' and -d 'l=2'

[
  1,
  2
]

I am fairly sure that l=1,2 should be an accepted way of passing a list of values as parameter, but fastapi doesn't seem to like it. Also, if this isn't the case, the Swagger UI doesn't produce the correct curl requests for lists given as Form parameters.

Packages:

  • fastapi: 0.66.0
  • python: 3.7.4

Let me know if you need other details!

Thank you for coming to my TED talk

@HitLuca HitLuca added the question Question or problem label Jul 14, 2021
@uricholiveira
Copy link

For the media type application/x-www-form-urlencoded, you need to use like this:

Captura de tela de 2021-07-19 05-02-41

l=1,2 is only accepted by query params in GET requests. If you doens't like the way above, you can use JSON.

@HitLuca
Copy link
Author

HitLuca commented Jul 19, 2021

Then why does the Swagger UI produce curl requests which clearly don't work?

@uricholiveira
Copy link

uricholiveira commented Jul 22, 2021

You've got me! I don't know what is causing it. 😢

@HitLuca
Copy link
Author

HitLuca commented Jul 27, 2021

Found a new related issue, when using pydantic BaseModel as Form parameters:

class Color(BaseModel):
    r: float
    g: float
    b: float

@app.post("/")
def api_test(color: Color = Form(...)):
    print(color)
    print(type(color))

Request made using the Swagger UI:

curl -X 'POST' \
  'http://localhost:5010/' \
  -H 'accept: application/json' \
  -H 'Content-Type: multipart/form-data' \
  -F 'color={
  "r": 0,
  "g": 1,
  "b": 2
}'

Response:

{
  "detail": [
    {
      "loc": [
        "body",
        "color"
      ],
      "msg": "value is not a valid dict",
      "type": "type_error.dict"
    }
  ]
}

Again: is the implementation wrong? Is Swagger wrong? If the implementation is bad why isn't it catched as an issue?

Would really like to get some kind of feedback on this or even an acknowledgement that somebody is remotely planning to look into it, seems like nobody cares (@tiangolo ?)

@HitLuca
Copy link
Author

HitLuca commented Aug 4, 2021

Bump

@MarioIshac
Copy link

MarioIshac commented Aug 8, 2021

This is also a problem with Tuple, not just List. Additionally, now in FastAPI 0.68.0 (but not 0.67.0), the Swagger UI doesn't load for endpoints with Form parameters of type Tuple. See #3665.

@francoposa
Copy link

Bump on this, still experiencing this with lists of form params

@StevRamos
Copy link

StevRamos commented May 26, 2022

Why hasn't it been solved? I am experiencing it too. @tiangolo any comment about this? Thank you in advance

The service:

@router.post(
    path="/solicitud-inscripcion/registrar",
    tags=["identificacion"]
)
async def registrar_solicitud_inscripcion(
    imagenes: UploadFile = File(...),
    esParcial: bool = Form(
        ...,
        example=True
    ),
    idSolicitante: int = Form(
        ...,
        gt=0,
        example=1
    ),
    idSede: int = Form(
        ...,
        gt=0,
        example=1
    ),
    idPropietario: int = Form(
        ...,
        gt=0,
        example=1
    ),
    bienes: List[BienOutModel] = Form(...)
):
    return {"resp": "it works"}
curl -X 'POST' \
  'http://127.0.0.1:8000/identificacion/solicitud-inscripcion/registrar' \
  -H 'accept: application/json' \
  -H 'Content-Type: multipart/form-data' \
  -F '[email protected];type=application/pdf' \
  -F 'esParcial=false' \
  -F 'idSolicitante=1' \
  -F 'idSede=1' \
  -F 'idPropietario=1' \
  -F 'bienes={
  "id": 1,
  "nombre": "Ramos",
  "descripcion": "Ramos",
  "sumilla": "Ramos",
  "imagenes": [
    {
      "id": 1,
      "nombre": "Ramos",
      "url": "Ramosurl",
      "idOrientacion": 1,
      "idBien": 1,
      "activo": false
    }
  ]
}'

and the output is this

{
  "detail": [
    {
      "loc": [
        "body",
        "bienes",
        0
      ],
      "msg": "value is not a valid dict",
      "type": "type_error.dict"
    }
  ]
}

@phillipuniverse
Copy link

phillipuniverse commented Aug 25, 2022

Then why does the Swagger UI produce curl requests which clearly don't work?

You've got me! I don't know what is causing it. 😢

@HitLuca @uricholiveira probably the miss here is that FastAPI/Pydantic need to use the explode keyword in the OpenAPI specification. It looks like the default is simple in the OpenAPI spec which yields the CSV form.

EDIT - this is a bug in swagger-ui see swagger-api/swagger-js#2713

That linked doc talks about query parameters but there is a mention of it when describing request bodies too. Looks like the same rules apply.

@falkben
Copy link
Contributor

falkben commented Nov 13, 2022

@phillipuniverse Do you override the OpenAPI generation to fix this? Any chance you'd submit a PR to fix this here or in pydantic?

@falkben
Copy link
Contributor

falkben commented Nov 16, 2022

I'd like to do a bit more with this, but it's getting late here, and wanted to put something down because I did make a tiny bit of progress with this and wanted to share.

This page describes where the explode needs to go in the OpenAPI file

It goes under the content / multipart-form (or x-www-form-urlencoded) section in it's own encoding section ("beside" schema).

@phillipuniverse is correct -- once adding the explode: true configuration to the openapi schema, swaggerUI correctly produces requests into the API which don't treat the param as just a singular string but as a list of values.

As an example of what the openapi schema should look like ... if you had a param that was a form list (my_list_param: List[str] = Form(...)) the openapi.json to get the swaggerUI to work correctly with it would be:

"requestBody": {
  "content": {
    "multipart/form-data": {
      "schema": {
        "$ref": "#/components/schemas/Body_update_movie_movie__movie_id__patch"
      },
      // this is the section to add:
      "encoding": {
        "my_list_param": {
          "style": "form",
          "explode": true
        }
      }
    }
  }
}

@phillipuniverse
Copy link

@falkben looking at this again, I think that there is a bug in Swagger UI. According to the 'Form Data' section of describing request bodies, it says:

By default, arrays are serialized as array_name=value1&array_name=value2 and objects as prop1=value1&prop=value2, but you can use other serialization strategies as defined by the OpenAPI 3.0 Specification

So the default should work the exact same as query parameters. If there are changes to code, it should be within Swagger UI, not in FastAPI or Pydantic (although perhaps FastAPI could make it easier to extend the encoding).

I messed around with a very rough way to override this with monkeypatching:

    import fastapi.openapi.utils
    orig_get_request_body = fastapi.openapi.utils.get_openapi_operation_request_body

    from typing import Optional
    from pydantic.fields import ModelField
    from typing import Dict
    from typing import Union
    from typing import Type
    from pydantic import BaseModel
    from enum import Enum
    from typing import Any

    def get_request_body_with_explode(*,
        body_field: Optional[ModelField],
        model_name_map: Dict[Union[Type[BaseModel], Type[Enum]], str],
    ) -> Optional[Dict[str, Any]]:
        original = orig_get_request_body(body_field=body_field, model_name_map=model_name_map)
        if not original:
            return original
        content = original.get("content", {})
        form_patch = content.get("application/x-www-form-urlencoded", {})
        if not form_patch:
            form_patch = content.get("multipart/form-data", {})
        if form_patch:

            form_patch["encoding"] = {
                "vals": {  # TODO: change to be more dynamic
                    "style": "form",
                    "explode": True
                }
            }

        return original

    fastapi.openapi.utils.get_openapi_operation_request_body = get_request_body_with_explode

You would monkeypatch that in the same sort of place you instantiate FastAPI, or just any time before you invoke the app.openapi() function.

This works great with an endpoint defined like:

@router.post("/withlistparams")
async def with_list_params(vals: list[str] = Form()):
    raise NotImplementedError()

The annoying part in the spec is that you have to specify the "encoding" on a per-request-property basis. But you could use the given body_field to look up the properties in the schema that are lists and ensure all of those properties have the right encoding set on the operation.

Hope that helps!

@falkben
Copy link
Contributor

falkben commented Nov 17, 2022

Thanks @phillipuniverse, that is very helpful. The code I had was directly modifying the schema dictionary, so I think this would be an improvement, although I think I want to come up with a way to generalize it to all List/form params. Unfortunately, that typing information doesn't seem to be available in the get_request_body_with_explode function so I'm still thinking about this.

If there are changes to code, it should be within Swagger UI, not in FastAPI or Pydantic (although perhaps FastAPI could make it easier to extend the encoding).

I'm curious why you say that. It seems that what SwaggerUI produces is fine, generally speaking -- it's just that FastAPI doesn't parse lists of params with CSV so it cannot use the default that SwaggerUI produces.

Since that is the case, I would think the fix would naturally go into FastAPI where it generates the openapi scheme. Since it knows that List params can only be correctly parsed with the "exploded" duplicated params style, fastapi should generate an openapi scheme that has the explode: true option on by default.

Re: easier extending into the encoding section, I did see some interesting options there. For example, I know that FastAPI can't easily convert complex, nested Pydantic models to Form fields. However, there is an option in openapi called "Complex Serialization in Form Data" (here) where a form param will parse a JSON payload. This could be a "natural" way to enable nested models in Forms.

@phillipuniverse
Copy link

Unfortunately, that typing information doesn't seem to be available in the get_request_body_with_explode function so I'm still thinking about this.

☹️ bummer.

I'm curious why you say that. It seems that what SwaggerUI produces is fine, generally speaking -- it's just that FastAPI doesn't parse lists of params with CSV so it cannot use the default that SwaggerUI produces.

@falkben whether or not FastAPI should or should not parse CSV values is a bit separate from this discussion about Swagger UI.

Since that is the case, I would think the fix would naturally go into FastAPI where it generates the openapi scheme. Since it knows that List params can only be correctly parsed with the "exploded" duplicated params style, fastapi should generate an openapi scheme that has the explode: true option on by default.

I have read a bit more into the OpenAPI spec and opened a bug ticket in swagger-js. In the bug ticket I included a bit more information and relevant parts of the OpenAPI spec, specifically that if not provided, the encoding object should have a default style of form for form-based request bodies, which then means that the default value of explode becomes true. So as far as the OpenAPI specification is concerned, the spec that FastAPI generates (no encoding at all) is consistent with its behavior to not parse arrays in the CSV form. So no change to FastAPI should be required.

If you look at the bug ticket that should convince you that the default the SwaggerUI produces is inconsistent with the OpenAPI specification. Let me know if I'm misreading!

If I'm not misreading then we should close this ticket in FastAPI as effectively a duplicate of #50 (comment).

@falkben
Copy link
Contributor

falkben commented Nov 17, 2022

the encoding object should have a default style of form for form-based request bodies

ah, that would explain some of my confusion when reading through the spec and then comparing to the behavior of SwaggerUI. The spec does state that form data should have a default of explode: true. I guess I just assumed you had to explicitly include the style: "form" but I agree it should be able to be determined by the content section.

I'm definitely in favor of this being fixed in SwaggerUI if they agree it's a bug.

Otherwise, I think it should be possible to look into the model when creating the schema to determine if it needs an encoding section. It probably won't be pretty, but I suspect there's a way to get the information we need through the model_name_map

@phillipuniverse
Copy link

@falkben I couldn't help myself, here's the fully dynamic version of my monkeypatch!!

    import fastapi.openapi.utils
    orig_get_request_body = fastapi.openapi.utils.get_openapi_operation_request_body

    from typing import Optional
    from pydantic.fields import ModelField
    from pydantic.schema import field_schema
    from typing import Any, Dict, Union, Type
    from pydantic import BaseModel
    from fastapi.openapi.constants import REF_PREFIX

    from enum import Enum
    def get_request_body_with_explode(*,
        body_field: Optional[ModelField],
        model_name_map: Dict[Union[Type[BaseModel], Type[Enum]], str],
    ) -> Optional[Dict[str, Any]]:
        original = orig_get_request_body(body_field=body_field, model_name_map=model_name_map)
        if not original:
            return original
        content = original.get("content", {})
        form_patch = content.get("application/x-www-form-urlencoded", {})
        if not form_patch:
            form_patch = content.get("multipart/form-data", {})
        if form_patch:
            schema_reference, schemas, _ = field_schema(
                body_field, model_name_map=model_name_map, ref_prefix=REF_PREFIX
            )
            array_props = []
            for schema in schemas.values():  # type: Dict[str, Any]
                for prop, prop_schema in schema.get("properties", {}).items():
                    if prop_schema.get("type") == "array":
                        array_props.append(prop)

            form_patch["encoding"] = {prop: {"style": "form"} for prop in array_props} # could include "explode": True but not necessary in swagger-ui

        return original

    fastapi.openapi.utils.get_openapi_operation_request_body = get_request_body_with_explode

I think it should be possible to look into the model when creating the schema to determine if it needs an encoding section

Yup, exactly! The key part to my above code is to get the actual OpenAPI schema from Pydantic, not just a reference to it. The schema comes back in the 2nd part of the tuple in the field_schema call:

   schema_reference, schemas, _ = field_schema(
                body_field, model_name_map=model_name_map, ref_prefix=REF_PREFIX
            )

Once you've got that it's trivial to determine the array properties. Shouldn't be too bad to refactor that to a recursive version either.

Let me know if that works for you!

@falkben
Copy link
Contributor

falkben commented Nov 18, 2022

This worked perfectly @phillipuniverse!

I modified it slightly to use the walrus operator (my first time using the walrus operator, in fact)

    if form_patch := (
        content.get("application/x-www-form-urlencoded")
        or content.get("multipart/form-data")
    ):

@fastapi fastapi locked and limited conversation to collaborators Feb 28, 2023
@tiangolo tiangolo converted this issue into discussion #8741 Feb 28, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

8 participants