litestar icon indicating copy to clipboard operation
litestar copied to clipboard

Enhancement: Support for Typing the `query` keyword (and other reserved kwargs) to generate OpenAPI Spec

Open Alc-Alc opened this issue 1 year ago • 7 comments

Summary

Litestar supports query parameters that are validated as Pydantic models. As seen in the code under "Basic Example", validation of query parameters do indeed work as per the constraints set in the Pydantic model. While the route works as intended, the OpenAPI schema generated has no query parameters.

  "paths": {
    "/": {
      "get": {
        "summary": "Foo",
        "operationId": "Foo",
        "responses": {
          "200": {
            "description": "Request fulfilled, document follows",
            "headers": {},
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/EvenOdd"
                }
              }
            }
          }
        },
        "deprecated": false
      }
    }
  }

Basic Example

from typing import Annotated
from litestar import Litestar, get
from pydantic import BaseModel, Field


class EvenOdd(BaseModel):
    even: Annotated[str, Field(regex=r"^\d*[02468]$")]
    odd: Annotated[str, Field(regex=r"^\d*[13579]$")]


@get()
async def foo(query: EvenOdd) -> EvenOdd:
    return query


app = Litestar([foo])


from litestar.testing import TestClient

with TestClient(app) as test:
    data = {'even': 0, 'odd': 1}
    response = test.get('', params=data)
    # query parameter validation works as expected
    assert response.status_code == 200

    data = {'even': 1, 'odd': 0}
    response = test.get('', params=data)
    # query parameter validation fails as expected
    assert response.status_code == 400
    assert 'Validation failed for GET' in response.json()['detail']

Drawbacks and Impact

Users who are migrating from other frameworks could expect the same behavior.

Unresolved questions

There is no "parameters" present in the schema. Is this intended behavior, or can the schema generation be changed so that "pydantic models as query parameters" work?

Funding

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
Fund with Polar

Alc-Alc avatar Jul 20 '23 06:07 Alc-Alc

I'll have to test this out to see.

Goldziher avatar Jul 20 '23 15:07 Goldziher

Ok, the ask here is to allow typing the query kwarg in a way that generates OpenAPI specs.

The issue with this though is that query was intended to allow users to get access to a dictionary of values. Thus a user can declare query and also declare named query parameters. We will need to support this by validating that the user declares the named parameters correctly inside query as well.

For example:

from typing import Annotated
from litestar import Litestar, get
from pydantic import BaseModel, Field


class EvenOdd(BaseModel):
    even: Annotated[str, Field(regex=r"^\d*[02468]$")]
    odd: Annotated[str, Field(regex=r"^\d*[13579]$")]


@get()
async def foo(query: EvenOdd, extra_param: int) -> EvenOdd:  # <-- this should raise an exception
    return query

The above should raise an exception because extra_param, which is by default a query parameter, is not declared inside query.

The same kind of issue goes with typing any of the other reserved kwargs for parameter collections (headers and cookies).

Thus, while I do not object to this feature being added - it requires some complexity.

I am unassigning myself from it and opening it to people who want to take it over.

Goldziher avatar Aug 01 '23 16:08 Goldziher

Would it make sense to rename this ticket as "Support query/path/header/cookie/whatnot arguments as (Pydantic) models", as that's basically what the target is (using query is both in conflict with current spec and too inflexible in the longer run).

Or I can open a new one, too?

tuukkamustonen avatar Jan 23 '24 10:01 tuukkamustonen

Would it make sense to rename this ticket as "Support query/path/header/cookie/whatnot arguments as (Pydantic) models", as that's basically what the target is (using query is both in conflict with current spec and too inflexible in the longer run).

Or I can open a new one, too?

IMO the "and other reserved kwargs" wording in the title refers to what you mention, they are indeed reserved kwargs 🙂

Alc-Alc avatar Jan 23 '24 11:01 Alc-Alc

True, I guess "and other reserved kwargs" covers path/headers/cookies.

The other part of the request is how to connect those - you suggested passing the model as query named keyword argument, but it doesn't need to be declared that tightly. Consider:

class DateRanges: ...
class Pagination: ...

@get()
def endpoint(
    dates: DateRanges,
    pagination: Pagination,
): ...

That would combine arguments from 2 (or more) models, with whatever argument names, instead of only being able to use single model and query as name. (I believe this is supported in FastAPI, actually.)

tuukkamustonen avatar Jan 23 '24 13:01 tuukkamustonen

Oh, by the way. The description says:

Litestar supports query parameters that are validated as Pydantic models.

Where in the documentation is that pointed out? I don't see it.

If it's this bit:

These parameters will be parsed from the function signature and used to generate a pydantic model. This model in turn will be used to validate the parameters and generate the OpenAPI schema.

This means that you can also use any pydantic type in the signature, and it will follow the same kind of validation and parsing as you would get from pydantic.

Then it at least needs an example. It's much too vague now.

tuukkamustonen avatar Jan 24 '24 09:01 tuukkamustonen

Litestar supports query parameters that are validated as Pydantic models.

Where in the documentation is that pointed out? I don't see it.

I don't think my initial statement was in reference to the docs, it is just an observation based on the minimal example which at the time (seems to still be the case, replace regex with pattern for pydantic V2) made the asserts pass, indicating that validation does indeed happen.

Alc-Alc avatar Jan 24 '24 09:01 Alc-Alc

Would it make sense to add a new reserved kwarg eg. query_params or params that would take a Pydantic/msgspec/attrs model to perform validation on the params? I see value on being able to get the raw query as query: dict, and it seems that trying to support both cases (raw params and pre-defined model for validation) means more complexity (same kwarg, two different behaviors).

ccrvlh avatar Feb 24 '24 04:02 ccrvlh

Would it make sense to add a new reserved kwarg eg. query_params or params that would take a Pydantic/msgspec/attrs model to perform validation on the params? I see value on being able to get the raw query as query: dict, and it seems that trying to support both cases (raw params and pre-defined model for validation) means more complexity (same kwarg, two different behaviors).

As seen in #3053, we're trying to move away from the use of reserved kwargs. Also, there shouldn't be any added complexity regarding the validation since msgspec is the one that's going to validate it (it'll happily handle the case of both query: dict as well query: Params.

guacs avatar Feb 27 '24 04:02 guacs

@guacs Can we create a new ticket that encompasses both this and #3053 so that we don't have to track the progress on these individually?

provinzkraut avatar Feb 27 '24 09:02 provinzkraut

@guacs Can we create a new ticket that encompasses both this and #3053 so that we don't have to track the progress on these individually?

Aren't they two different issues though?

guacs avatar Feb 28 '24 00:02 guacs

Yeah, but the fix should be the same, i.e. with the same feature of the "new DI". We should probably just open a ticket for that. I'll do it when I've got the time!

provinzkraut avatar Feb 28 '24 10:02 provinzkraut

Please just add a warning or error message. To a newbie this makes sense but it doesn't work:

from dataclasses import dataclass

from litestar import Litestar, get


@dataclass
class TodoItem:
    title: str
    done: bool


TODO_LIST: list[TodoItem] = [
    TodoItem(title="Start writing TODO list", done=True),
    TodoItem(title="???", done=False),
    TodoItem(title="Profit", done=False),
]


@get("/")
async def get_list(query: bool | None = None) -> list[TodoItem]:
    if query is None:
        return TODO_LIST
    return [item for item in TODO_LIST if item.done == query]


app = Litestar([get_list])

I'm the newbie, it made sense to me

{"status_code":400,"detail":"Validation failed for GET /","extra":[{"message":"Expected bool | null, got MultiDict","key":"query"}]} is not enough for dumb dumbs like me.

grofte avatar Mar 26 '24 14:03 grofte