vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
7.26k stars 431 forks source link

[BUG] Query parameters typed as Optional[List[...]] fail when at least one value provided #538

Open OtherBarry opened 2 years ago

OtherBarry commented 2 years ago

Problem NB: A very similar issue was reported in #181, however it appears it was only resolved for non-optional types.

When using an query parameter typed as an optional list, e.g. Optional[List[str]], any values provided give the error:

{
  "detail": [
    {
      "loc": [
        "query",
        "<param name>"
      ],
      "msg": "value is not a valid list",
      "type": "type_error.list"
    }
  ]
}

Example

@router.get("/test")
def test(request, values: Optional[List[str]] = Query(None)) -> str:
    if values is None:
        return "No values"
    return "|".join(values)

A GET request to /test will return "No values" as expected, however a GET request to /test?values=foo or /test?values=foo&values=bar will result in a 422 response, with the error listed above.

Workaround There is a workaround that I found, however it is not an ideal solution.

class QueryParams(Schema):
    values: Optional[List[str]] = Field(None)

@router.get("/test")
def test(request, params: QueryParams = Query(...)) -> str:
    if params.values is None:
        return "No values"
    return "|".join(params.values)

Versions

vitalik commented 2 years ago

Hi @OtherBarry

I think it some general issue with Optional marking - for you now you can skip it by passing [] empty list as default value:

@router.get("/test")
def test(request, values: list[str] = Query([])) -> str:
    if not values:
        return "No values"
    return "|".join(values)
Andrioden commented 2 years ago

I have a similar problem

@router.get("/")
def get_many(request: WSGIRequest, id: Optional[int] = None) -> Iterator[Hero]:
    pass

GET /id=

django ninja responds with

{"detail": [{"loc": ["query", "id"], "msg": "value is not a valid integer", "type": "type_error.integer"}]}

I assume the problem is that the query param id= is read as a string and django ninja tries to convert "" to int. However, that makes no sense as it should be converted to None.

As far as I can think, there exist no logical reasons for an client to input a blank query int param, and not want it to interpreted as None.

OtherBarry commented 2 years ago

@vitalik yeah that's a cleaner way to do it. I always forget you can use mutable defaults with pydantic.

@Andrioden not including the id param at all would be the standard way to indicate None I would think. You can probably create a custom type as a workaround, see here.

vitalik commented 2 years ago

@Andrioden

try change : Optional[int] = None to : int = None (Remove the optional - I think generally Optional is currently have some bug)

Andrioden commented 2 years ago

@Andrioden not including the id param at all would be the standard way to indicate None I would think. You can probably create a custom type as a workaround, see https://github.com/pydantic/pydantic/discussions/2687.

@OtherBarry - Thanks for this tip

try change : Optional[int] = None to : int = None (Remove the optional - I think generally Optional is currently have some bug)

@vitalik - I tried this, and several other variations, farily certain there is no way around it with normal types.


All things considered, i do still suggest the default behavior of django ninja is that a blank int typed input query param is interpreted as None. If this is not possible or wanted due to django ninja using native pydantic type conversion, then i suggest documenting the behavior here.

I did end up solving this for my case by filtering this out on the client side, but i still generally believe backends should be as flexible as possible as long as there is no chance of confusion. = P

shughes-uk commented 1 year ago

The latest ninja beta made this a problem for us. This used to be fine, but now throws an error. Seems like the issue might be in how is_collection_type works? __ninja_collection_fields__ is simply empty for this schema.

    state: Optional[List[int]] = None