litestar-org / litestar

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

Enhancement: Adding extra validation on path/query/header/cookie arguments #3018

Closed tuukkamustonen closed 10 months ago

tuukkamustonen commented 10 months ago

Summary

At the moment, it's not possible to "directly" inject extra validations for path, query, header and cookie parameters. Only what Parameter() supports is allowed. Extra validation needs to be done within the handler function, or via DI quite manually.

Pydantic allows to attach extra validations to fields with AfterValidator (and similar):

def ensure_uppercase(value: Any):
    if ...something...:
        raise ValueError("...")

class MyModel(BaseModel):
    foo: Annotated[str, Field(), AfterValidator(ensure_uppsercase)]

This syntax is short and neat. Allow doing something similar at the endpoint level:

@get(path="/users/{user_id:str}")
def endpoint(user_id: Annotated[str, Parameter(...), AfterValidator(ensure_uppercase)]): ...

The current workaround in Litestar (shared by @guacs in Discord) is something like:

class ParamsIn(BaseModel):
    user_id: Annotated[str, Field(), AfterValidator(ensure_uppsercase)]

def provide_paramsin(user_id: str) -> ParamsIn:
    return ParamsIn(user_id=user_id)

@get(dependencies={"args": Provide(provide_paramsin, sync_to_thread=False)
def endpoint(args: ParamsIn): ...

But this is quite cluttered, and requires extra code for passing the values along to the model. More brief and intuitive syntax should be allowed.

Basic Example

No response

Drawbacks and Impact

No response

Unresolved questions

No response


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

provinzkraut commented 10 months ago

Closing this as a duplicate of #2979 / #2015.

When those are implemented, you'd be able to define your query params with a Pydantic model, which is a reasonable fix for this as well.

The syntax you proposed won't be supported, since it would require to generate an extra Pydantic model under the hood, which could grow quite complex considering we'd have to support this throughout the full dependency tree.

guacs commented 10 months ago

Yeah, I agree. I think #2015 is the way to go. This would allow the use of not just pydantic, but msgspec Structs, attrs or any other such libraries.

tuukkamustonen commented 10 months ago

The syntax there (as separate class) is a bit more verbose and the direct inline syntax proposed here is what a developer expects.

Though, I guess often times input parameters are simple(ish) and the standard OpenAPI validators are mostly sufficient. So the need here is rare(ish), and doesn't necessitate complex solutions, I agree.

tuukkamustonen commented 9 months ago

Coming back to this... declaring the advanced scenarios via Pydantic models is fine.

However, could supporting something similar to Pydantic Field(json_schema_extra=...) be supported also for Parameter, though? It would allow local hacks, ie. injecting what-you-need, when the system is fighting back?

And if I understood right, a Pydantic model is constructed underneath anyway, so the value be just passed there.

provinzkraut commented 9 months ago

However, could supporting something similar to Pydantic Field(json_schema_extra=...) be supported also for Parameter, though? It would allow local hacks, ie. injecting what-you-need, when the system is fighting back?

To be honest, I'm not sure if we should support Pydantic's json_schema_extra, since we don't use that to generate the OpenAPI schema. Having an independent schema_extra for Parameter seems fine though.

And if I understood right, a Pydantic model is constructed underneath anyway, so the value be just passed there.

This is not correct, no. As stated above, Litestar doesn't use Pydantic to generate the OpenAPI schema and doesn't create internal Pydantic models. It used to work that way back in Starlite 1.x, but we're completely independent of Pydantic now.

tuukkamustonen commented 9 months ago

To be honest, I'm not sure if we should support Pydantic's json_schema_extra, since we don't use that to generate the OpenAPI schema. Having an independent schema_extra for Parameter seems fine though.

People (like me) would expect Pydantic's json_schema_extra to work (at both model_config and Field level).

It's a maybe a corner-case, needing to adjust the schema, but when you do it rather belongs to the model/fields, rather than adjusting from-the-outside (leads to repetition):

# Rather than once...
class SpecialBody(BaseModel):
    model_config = ConfigDict(json_schema_extra=CUSTOM1)

    field: Field(json_schema_extra=CUSTOM2)

# You'd have to always...
@get()
def endpoint1(data: Annotated[SpecialBody, Parameter(schema_extra=CUSTOM1 + CUSTOM2)]): ...

@get()
def endpoint2(data: Annotated[SpecialBody, Parameter(schema_extra=CUSTOM1 + CUSTOM2)]): ...

@get()
def endpoint3(data: Annotated[SpecialBody, Parameter(schema_extra=CUSTOM1 + CUSTOM2)]): ...

Even if it is possible to:

SpecialBodyParam = Annotated[SpecialBody, Parameter(schema_extra=CUSTOM1 + CUSTOM2)]

It's just not as nice... 🤔

In any case, I would argue that people except pretty much anything to work, unless explicitly denied/raised.

This is not correct, no. As stated above, Litestar doesn't use Pydantic to generate the OpenAPI schema and doesn't create internal Pydantic models. It used to work that way back in Starlite 1.x, but we're completely independent of Pydantic now.

Thanks for the correction!

guacs commented 9 months ago

@tuukkamustonen I think the feature you want should be possible if we offload the schema generation of models that define the query/header etc. parameters to the corresponding plugin though this might require some substantial changes in how we handle them (I'd have to look at the code to be sure). However, I don't think it would make sense to support this for Parameter since that should not be linked to pydantic, msgpsec or any other such libraries.

mtvx commented 9 months ago

@guacs The later proposal here was about extending the Litestar generated OpenAPI schema, though. Ie. different from adding custom validations there.

I think you should support easy extension of the schema (probably possible via custom Operation now, but it could be even easier).

Just, read Parameter(schema_extra=...), even Pydantic's json_schema_extra (from both ConfigDict and Field) in the Pydantic plugin, why not. Just recursively merge, perhaps.

But yeah, this ticket was about extending validation, and I opened a new one for extending the generated OpenAPI scheam in https://github.com/litestar-org/litestar/issues/3022 (only proposing Parameter(schema_extra=...) now, but as mentioned you could support also other sources).

guacs commented 9 months ago

@mtvx sorry I may have not been clear. I think offloading to the plugins would allow for extending the schema as suggested in #3022 with whatever is supported by the specific library (pydantic for example) as well as the validation would also take place when instantiating the object via whatever validation library you're using. The ideal case after fixing #2015 would be the following:

class ParamsIn(BaseModel):
    user_id: Annotated[str, Field(), AfterValidator(ensure_uppsercase)]

@get("/")
def handler(params_in: ParamsIn) -> None:
    ...

This way the validation will take place properly and any pydantic specific features you have for extending the schema can be used which will/should be handled by the PydanticSchemaPlugin.

tuukkamustonen commented 9 months ago

Ah yeah, so two birds with one stone 👍🏻

Could still support Parameter(schema_extra=...) for simple cases / query parameters / whatnot, where you don't want to clutter the code by creating yet another class, though.

I believe https://github.com/litestar-org/litestar/issues/2015 is the ticket for supporting "parameters as classes".