-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
🐛 Fix tagged discriminated union not recognized as body field #12942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
868e06f to
8b292f1
Compare
1fdf0b6 to
83e02fd
Compare
|
Rebased/updated and modified the original comment to better explain the purpose of this PR 😄 |
YuriiMotov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Actually, this problem is related to Pydantic V1 as well.
It can be tested using the following dummy test (in details)
Details
from typing import Annotated, Union
from fastapi import FastAPI
from fastapi.testclient import TestClient
from pydantic import BaseModel, Field
class Item(BaseModel):
value: str
ItemUnion = Union[
Annotated[Item, Field()],
Annotated[Item, Field()],
]
app = FastAPI()
@app.post("/items/")
def save(item: ItemUnion): # Changing this to `item: ItemUnion = Body()` would fix
return {"item": item}
# Tests
def test_() -> None:
client = TestClient(app)
response = client.post("/items/", json={"value": "item"})
assert response.status_code == 200, response.text
assert response.json() == {"item": {"value": "item"}}But I don't think we need to fix this for Pydantic V1 now. I can't imagine any real-life use case with Pydantic V1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested this in our codebase and it works!
I also checked that the new tests were not passing on the main branch 😊
EDIT: Also updated the test to make sure query params still work when using Annotated
|
@frankie567 I've update this to use a different style of defining the discriminator, which also makes the OpenAPI schema better 😊 Previous schema: {
"openapi": "3.1.0",
"info": {"title": "FastAPI", "version": "0.1.0"},
"paths": {
"/items/": {
"post": {
"summary": "Save Union Body Discriminator",
"operationId": "save_union_body_discriminator_items__post",
"requestBody": {
"content": {
"application/json": {
"schema": {
"oneOf": [
{"$ref": "#/components/schemas/FirstItem"},
{"$ref": "#/components/schemas/OtherItem"},
],
"title": "Item", # No discriminator here
}
}
},
"required": True,
},
"responses": {
"200": {
"description": "Successful Response",
"content": {
"application/json": {
"schema": {
"additionalProperties": True,
"type": "object",
"title": "Response Save Union Body Discriminator Items Post",
}
}
},
},
"422": {
"description": "Validation Error",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/HTTPValidationError"
}
}
},
},
},
}
}
},
"components": {
"schemas": {
"FirstItem": {
"properties": {
"value": {"type": "string", "title": "Value"},
"price": {"type": "integer", "title": "Price"},
},
"type": "object",
"required": ["value", "price"],
"title": "FirstItem",
},
"HTTPValidationError": {
"properties": {
"detail": {
"items": {"$ref": "#/components/schemas/ValidationError"},
"type": "array",
"title": "Detail",
}
},
"type": "object",
"title": "HTTPValidationError",
},
"OtherItem": {
"properties": {
"value": {"type": "string", "title": "Value"},
"price": {"type": "number", "title": "Price"},
},
"type": "object",
"required": ["value", "price"],
"title": "OtherItem",
},
"ValidationError": {
"properties": {
"loc": {
"items": {"anyOf": [{"type": "string"}, {"type": "integer"}]},
"type": "array",
"title": "Location",
},
"msg": {"type": "string", "title": "Message"},
"type": {"type": "string", "title": "Error Type"},
},
"type": "object",
"required": ["loc", "msg", "type"],
"title": "ValidationError",
},
}
},
}Current schema {
"openapi": "3.1.0",
"info": {"title": "FastAPI", "version": "0.1.0"},
"paths": {
"/items/": {
"post": {
"summary": "Save Union Body Discriminator",
"operationId": "save_union_body_discriminator_items__post",
"parameters": [
{
"name": "q",
"in": "query",
"required": True,
"schema": {
"type": "string",
"description": "Query string",
"title": "Q",
},
}
],
"requestBody": {
"required": True,
"content": {
"application/json": {
"schema": {
"oneOf": [
{"$ref": "#/components/schemas/FirstItem"},
{"$ref": "#/components/schemas/OtherItem"},
],
"discriminator": { # 👈 Discriminator
"propertyName": "value",
"mapping": {
"first": "#/components/schemas/FirstItem",
"other": "#/components/schemas/OtherItem",
},
},
"title": "Item",
}
}
},
},
"responses": {
"200": {
"description": "Successful Response",
"content": {
"application/json": {
"schema": {
"type": "object",
"additionalProperties": True,
"title": "Response Save Union Body Discriminator Items Post",
}
}
},
},
"422": {
"description": "Validation Error",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/HTTPValidationError"
}
}
},
},
},
}
}
},
"components": {
"schemas": {
"FirstItem": {
"properties": {
"value": {"type": "string", "const": "first", "title": "Value"},
"price": {"type": "integer", "title": "Price"},
},
"type": "object",
"required": ["value", "price"],
"title": "FirstItem",
},
"HTTPValidationError": {
"properties": {
"detail": {
"items": {"$ref": "#/components/schemas/ValidationError"},
"type": "array",
"title": "Detail",
}
},
"type": "object",
"title": "HTTPValidationError",
},
"OtherItem": {
"properties": {
"value": {"type": "string", "const": "other", "title": "Value"},
"price": {"type": "number", "title": "Price"},
},
"type": "object",
"required": ["value", "price"],
"title": "OtherItem",
},
"ValidationError": {
"properties": {
"loc": {
"items": {"anyOf": [{"type": "string"}, {"type": "integer"}]},
"type": "array",
"title": "Location",
},
"msg": {"type": "string", "title": "Message"},
"type": {"type": "string", "title": "Error Type"},
},
"type": "object",
"required": ["loc", "msg", "type"],
"title": "ValidationError",
},
}
},
}but the code works in both cases, the fix is mostly around using |
|
Thank you @patrick91, good point 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks @frankie567! 🚀
And thanks @YuriiMotov and @patrick91 for the help!
I realized that the tests were also passing on master 😅
So, I updated them to make them fail on master (adding the Tag) and get fixed by this change.
This will be released in FastAPI 0.118.2 in the next hours. 🎉
Ups, it seems I broke the tests, let me check that first. Fixed. 😅
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@YuriiMotov No, I don't think that's the case. I have a similar problem: class CommunicateViaSmsDTO(BaseModel):
message: str
scheduled_at: Optional[datetime.datetime] = None
@dataclass
class SearchCustomerDTO:
order_by: Optional[OrderByStatement] = None
phone_number: Optional[str] = None
max_last_active_datetime: Optional[datetime.datetime] = None
@communicate_router.post("/via_sms")
def communicate_via_sms(
dto: CommunicateViaSmsDTO,
search_params: SearchCustomerDTO = Depends(),
customer_ids: list[int] = Query(default=None),
) -> APIResult[SendBatchMessageResultDTO]:
# Implementation
passAnd after upgrading to FastAPI 118.2 from 118.1, FastAPI doesn't see the body I send: admin_client = TestClient(.....)
admin_client.post(
f"/customers/communicate/via_email?max_last_active_datetime={last_active_datetime}",
json={
"subject": "hello",
"message": "world",
},
)It throws: For some reason it treats the dto as a scalar of the body (judging by the error) and changing the testing code to: admin_client.post(
f"/customers/communicate/via_email?max_last_active_datetime={last_active_datetime}",
json={
"dto": {
"subject": "hello",
"message": "world",
}
},
)Fixes the problem |
That's because FASTAPI treats one of other parameters (likely See https://fastapi.tiangolo.com/tutorial/body-multiple-params/#multiple-body-parameters |
|
@YuriiMotov But it used to work correctly. Why does it treat an order_by (which is located in the SearchCustomerDTO accessed via Depends()) as body? Here is the order_by definition: from typing import Annotated, TypeVar, Sequence, Generic, Type, Literal
import pydantic
from fastapi import Query
from pydantic import AfterValidator, PlainValidator, WithJsonSchema, ConfigDict
OrderByList = list[tuple[str, Literal["desc", "asc"]]]
def _validate_order_by_str(v):
if isinstance(v, list):
couples = v
elif isinstance(v, str):
couples = [couple.split(":") for couple in v.split(";")]
else:
raise TypeError()
result = []
for couple in couples:
column, order = couple
if not isinstance(column, str):
raise TypeError("column should be str")
if order not in ("desc", "asc"):
raise TypeError("Order should be either desc or asc")
result.append(couple)
return result
OrderByStatement = Annotated[
OrderByList,
PlainValidator(_validate_order_by_str),
WithJsonSchema(
{
"pattern": "column_name:desc|asc;",
"examples": ["amount:desc", "sum:asc;experience:desc"],
"type": "string",
},
mode="validation",
),
]Marking order_by as Query doesn't help: order_by: Annotated[Optional[OrderByStatement], Query] = NoneBut you're right - commenting out order_by fixes the issue. Due to the changes made in this PR, OrderByStatement is now considered complex. Though it's not clear how to bring back the old behavior. |
|
The fix was: if TYPE_CHECKING:
OrderByStatement = OrderByList
else:
OrderByStatement = Annotated[
str,
PlainValidator(_validate_order_by_str),
WithJsonSchema(
{
"pattern": "column_name:desc|asc;",
"examples": ["amount:desc", "sum:asc;experience:desc"],
"type": "string",
},
mode="validation",
),
]I'd personally mention it as a breaking change in the changelog |
Problem
When using a union type models with a tagged discriminator as an endpoint parameter, like the following:
FastAPI fails to identify it correctly as body payload. It incorrectly considers it's a query parameter, which then causes a validation error. The workaround currently is to explicitly tell FastAPI it's a body parameter using
dessert: Annotated[Dessert, Body()]Fix
The solution is to improve the function handling type annotations by handling the case where we can have
Annotatedtypes inside an
Annotatedtype.Discussed in #12941