fastapi
✨ Support for mixed and multiple Pydantic models for parameters using Query, Cookie and Header
From #12199 onwards, Pydantic models are supported for query, cookie and header parameters. When one parameter is present, the model is flattened in the OpenAPI spec, but when multiple are defined, they aren't.
This is confusing, and results in a confusing OpenAPI spec. Since these arguments are used in flattened form anyway, it makes more sense to flatten all of them.
Thanks for the PR! As you consider this to be a bug, can you commit a unit test that shows the behaviour of this functionality on this PR, which would break on master?
@svlandeg I've updated the PR with more tests, and fixed something uncovered by those tests :+1:
Looking forward to this, it fixes the issue described in this comment https://github.com/fastapi/fastapi/pull/12199#issuecomment-2436442713
Reflecting on this, I believe we could classify this as a feature rather than a bug. For the first test case, i.e. async def with_depends(model1: Model = Query(), dependency=Depends(dependency)):, this PR may constitute a bug fix (as it handled properly), but now that everything is properly flattened, this would constitute a new (and rather useful) feature.
It now properly handles multiple Pydantic models, dependencies and direct arguments. I've updated the PR title to reflect this.
Would love to see this merged, the query param models are mostly unusable for us in their current state
Hi! We really appreciate the time and effort spent to submit this PR. We have a high volume of PRs and we're working hard to review and classify them. We'll get back to you once we've been able to review this in detail - thanks for your patience! 🙏
A conflict with PR #13515 has been resolved, and this PR should be able to be merged directly.
We would love to move forward with this PR, as there appears to be clear support for this feature. If there's anything we can do to help, please let us know @svlandeg
@JSCU-CNI, thank you for your work!
I haven't delved into the implementation details yet, just looked at it briefly.
As I can see, the case with BaseModel that has extra="forbid" is not handled, right?
If we have model with extra="forbid" and add more single parameters or another model, the validation of the model will fail and the error message might be not clear to understand what is happening.
I think we should handle this and show warning (or even error) on startup if such model (with extra="forbid" is combined with another model or single parameters).
What do you think?
One more thing to think about: if we have 2 models and both of them have field with the same name, will this be handled well? Intuitively, everything should be Ok, but better check it and probably add test to make it explicit.
Thank you for your comment @YuriiMotov. The implementation closely follows the original code, and only makes some necessary adjustments. It's no coincidence that PR #12944 closely resembles this code, as they probably did a similar thing.
Regarding your comment on extra=forbid, it is hard to detect whether the use-case is valid, as all models may share fully overlapping arguments. In these cases, everything will function as intended, and a warning should not be emitted. Only when models are not fully overlapping, this should trigger. I've at least added a couple of tests to cover this. I'm on the fence on whether this PR should address this more elaborately rather than leaving if unaddressed, as extra=forbid is arguably a niche edge case regardless. We could probably solve this by stating this fact in the documentation.
You're correct that everything functions properly when there are overlapping field names in different models, but I've added several test cases to verify this as well.
I've rebased and squashed all commits, and added some comments to the test cases to explain them a bit better. I've also added a test where I mix various models, just to fully cover our bases.
@JSCU-CNI, thanks for the explanation!
I think I agree about extra=forbid. Actually it's not that hard to find the cause of "Extra inputs are not permitted" error.
One more thing. There are no tests for convert_underscores. And it seems that it doesn't work correctly:
class Model3(BaseModel):
field_1: str
class Model4(BaseModel):
field_2: str
@app.get("/headers2")
async def get_hhh(
h1: Annotated[Model3, Header(convert_underscores=False)],
h2: Annotated[Model4, Header(convert_underscores=False)],
):
return {"h1": h1, "h2": h2}
Shows parameters as param-1 and param-2 in schema.
@YuriiMotov You're absolutely right, tests were lacking for convert_underscore=False. This regression was introduced when rebasing the PR back in March. The issue has been resolved and appropriate tests were added. It was an issue with OpenAPI spec generation, which needed the same treatment as was done previously for the dependencies.
While I was at it, I realized that the (new) code at dependencies/utils.py line 754-ish was almost duplicated with the new _get_flat_fields_from_params. Since I needed it again for openapi/utils.py, I wrapped it into a _get_flat_fields_from_params_with_origin function, and rewrote _get_flat_fields_from_params to use the new (more elaborate) function.
@YuriiMotov @svlandeg If there's anything else you'd like to see happen in this PR, please let me know. I'm happy to resolve any further feedback you may have.
There seems to be clear and broad support for this feature, given the fact several issues, discussions and PRs mention the same underlying issue. Therefore, I would love to finally move forward with this PR. Please let me know what to do next.
I did see that in issue #13400 @tiangolo commented that he was unsure about a feature like this. I would interject that this seems to be a very clear iteration on what is available and possible now, and that the current implementation is arguably more broken and confusing. Perhaps they could weigh in?
@JSCU-CNI, thanks again for all your work on this!
I can definitely see how the current implementation (& limitations) are confusing/surprising. But to implement this kind of feature we need to make sure we think of all the edge-cases, implications on other parts of the code base, and consistencies with other existing features (like how it works for multiple Body models). To this end, we're internally discussing this with Tiangolo before we can proceed here. We'll keep you up-to-date on this PR. Thanks for your patience! 🙏
I think one of the use cases I find would be cumbersome is this:
from typing import Annotated
from fastapi import FastAPI, Query
from pydantic import BaseModel
app = FastAPI()
class RangeParams(BaseModel):
skip: int = 0
limit: int = 100
@app.get("/")
def handle_items(range: Annotated[RangeParams, Query()], filter: str):
return {"range": range, "filter": filter}
Here all the parameters in RangeParams and filter would be at the same level, even though in the function there are some that are "embeded" in a model (RangeParams) and one that is put there directly (filter). But the end result would be that all of them are at the same level.
Another related inconsistency is that even though that code above would have all the fields flattened, if it was all the same but in body parameters, like this:
from typing import Annotated
from fastapi import Body, FastAPI
from pydantic import BaseModel
app = FastAPI()
class RangeParams(BaseModel):
skip: int = 0
limit: int = 100
@app.post("/")
def handle_items(range: RangeParams, filter: Annotated[str, Body()]):
return {"range": range, "filter": filter}
Then the fields would not be flattened, but the RangeParams would be embedded in a range key.
Nevertheless, there's already the quirk that even when declaring body data, a Pydantic model would be at the top level or embedded in a key, depending on the other parameters. So, it behaves in one way or another depending on the rest.
So, a similar tradeoff between explicitness and convenience.
Given that, I think the convenience of being able to use more than one Pydantic model for Query, Header, Cookie params wins against these cumbersome quirks for other cases.
So, yep, I think this feature should be supported. I haven't reviewed this PR, haven't even checked the code yet, but I wanted to share my current views on the whole idea of the feature (and API design) itself. :nerd_face:
Side note:
Regarding the parameters in RangeParams and filter being on the same level, we should probably think about how it will work with other serialization methods (mainly DeepObject serialization) if (when) we decide to support them.
I think we can add a parameter like Query(serialization_method=...) (or Query(flatten=...), not sure about name). But do we need to add it now (to avoid breaking changes later) or we can introduce it later?
I'll think about this more a bit later.
This pull request has a merge conflict that needs to be resolved.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@JSCU-CNI: there's a bit of a tricky merge conflict, do you have time to resolve it?
Thank you for your consideration, @tiangolo @YuriiMotov
@svlandeg Luckily, the conflict looked more complicated than it actually was. It should have been resolved now.
Could this solution be extended to cover Form() parameters also? We're building an app using HTMX that is intended to degrade gracefully in situations without Javascript (so no AJAX, no JSON) and it would be extremely convenient to POST one flat form and have the backend automatically generate separate Pydantic models for us.
Looking forward to this being merged. I expected this to work as described going off of the documentation. Thanks for the work to all involved.
This pull request has a merge conflict that needs to be resolved.