-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
AIP-84 Migrate GET Dag Runs endpoint to FastAPI #43506
base: main
Are you sure you want to change the base?
Conversation
…AIP-84/list_dag_runs
), | ||
], | ||
session: Annotated[Session, Depends(get_session)], | ||
# fields: list[str] | None = Query(None), |
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.
@pierrejeambrun, @bugraoz93 , looking for ideas on how to implement fields to return.
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.
Due to its type and purpose, this should be similar to update_mask
(even though it is reversed regarding masking
). We will get in explode=true
, similar to update_mask again ....?fields=item1&fields=item2
.
As far as I see there isn't a way to exclude fields during validation
and doing dump
and validate
won't work until we have a dynamically generated Pydantic
model with the selected fields making the validation on return. I saw a discussion about including this capability. There are also some workarounds for people there. Maybe we can also have a generic exclusion method for Pydantic
models until it is provided fully.
pydantic/pydantic#9573
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.
For now you can just skip fields
. Because it's an issue to implement and dynamically change the returned type. (sub fields etc...), which isn't great for the client.
This is mentioned here #43378 (comment), as one of the breaking change.
We can either fix that later before 3.0 release, or accept that breaking change. In the mean time I would suggest to just remove it and don't bother with it.
related to #42701