litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.64k stars 382 forks source link

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

Open Alc-Alc opened 1 year ago

Alc-Alc commented 1 year ago

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

Fund with Polar

Goldziher commented 1 year ago

I'll have to test this out to see.

Goldziher commented 1 year ago

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.

tuukkamustonen commented 10 months ago

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?

Alc-Alc commented 10 months ago

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 🙂

tuukkamustonen commented 10 months ago

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 commented 10 months ago

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.

Alc-Alc commented 10 months ago

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.

ccrvlh commented 9 months ago

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).

guacs commented 9 months ago

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.

provinzkraut commented 9 months ago

@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?

guacs commented 9 months ago

@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?

provinzkraut commented 9 months ago

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!

grofte commented 8 months ago

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":"Expectedbool | null, gotMultiDict","key":"query"}]} is not enough for dumb dumbs like me.

Forceres commented 1 month ago

Issue was raised long time ago, so will it be solved at all?

provinzkraut commented 1 month ago

Hey @Forceres! It's unlikely this will land in 2.x, so if it comes, that would likely happen in 3.0.