sanic-org / sanic-ext

Extended Sanic functionality
https://sanic.dev/en/plugins/sanic-ext/getting-started.html
MIT License
50 stars 35 forks source link

[Bug] Pydantic query validation with extra data not defined in model raise's KeyError #247

Open sagarsabu-tait opened 4 months ago

sagarsabu-tait commented 4 months ago

Describe the bug Using a pydantic model for query parameter validation fails if extra query parameters are passed to the route. This is not a pydantic validation failure (as by default pydantic ignores extra data passed to the model initializer).

Screenshots image

To Reproduce

from sanic import Sanic, response, request
from pydantic import BaseModel
from sanic_ext import validate

class MyQueryData(BaseModel):
    name: str

app = Sanic("Example")

@app.get('/test')
@validate(query=MyQueryData)
async def handler(request: request.Request, query: MyQueryData):
    print(f"{query=}")
    return response.empty()
# Works as expected. Return 200 OK
curl <the-url>/test?name=abcd 

# Works as expected. validation error as require params not set
curl <the-url>/test

# Unexpected failure. Return 500
curl <the-url>/test?name=abcd&another=abcd

Expected behavior

Pydantic has model based settings for handling extra data being passed to model initializer. Let it handle the extra data accordingly https://docs.pydantic.dev/2.7/api/config/#pydantic.config.ConfigDict.extra

# Expected behviour is to return 200 OK via silently ignoring extra data
curl <the-url>/test?name=abcd&another=abcd

Environment (please complete the following information):

chinu27 commented 3 months ago

Any updates on this issue?. Urgent resolution is required.

sagarsabu-tait commented 2 months ago

Should be a one liner fix.

# In sanic_ext/extras/validation/clean.py
def clean_data(model: Type[object], data: Dict[str, Any]) -> Dict[str, Any]:
    hints = get_type_hints(model)
    # was - return {key: _coerce(hints[key], value) for key, value in data.items()}
    return { key: _coerce(hints[key], value) for key, value in data.items() if key in hints }
Panaetius commented 1 month ago

I do not think that the proposed one-liner fix above fully solves the problem, as it'd just ignore all extra fields.

While this is the expected behavior for models like

class MyQueryData(BaseModel):
    model_config = ConfigDict(extra='ignore')

    name: str

it wouldn't be correct for

class MyQueryData(BaseModel):
    model_config = ConfigDict(extra='forbid')

    name: str

which should fail with a validation error for unknown properties.

Panaetius commented 1 month ago

I took a stab at fixing this in #259

Do note that this fix only solves these issues for models inheriting from pydantic.BaseModel, NOT when using pydantic.dataclass.

In the dataclass case, the is_pydantic() method is returns False, so regular validation (_validate_annotations()) is used, which has a separate bug. Namely, line 165 of check.py sig.bind(**data) will fail because it gets an extra argument from the querystring that isn't in the type signature. This is not affected by pydantic configuration like @pydantid.dataclass(config=pydantic.ConfigDict(extra="ignore")) as the code in question fails before any pydantic code is run. For me this isn't an issue, as we don't use pydantic.dataclass, but probably warrants a follow-up issue.

shaoerkuai commented 1 month ago

And not work for pydantic v2.