-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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 patch a connection to FastAPI API #43102
base: main
Are you sure you want to change the base?
AIP-84 Migrate patch a connection to FastAPI API #43102
Conversation
extra: str | None = Field(default=None) | ||
|
||
|
||
class ConnectionResponse(ConnectionBase): |
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.
Unfortunately I don't think they can share the same base.
Because that works for the ConnectionBody
, but it doesn't for the Response. Here in the documentation, all those field will appear as optional
, like they could be missing from the body
but that's not possible for the Response.
I think we have no other choice than having 2 full distinct Schema ConnectionResponse
(no fields are missing, but nullable) ConnectionBody
(most fields can be missing from the body)
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.
Agree. We have no other choice. Having the distinction would also help with the model/database distinctions, for example, for variables. I have also adjusted this approach for the Variable model and realised that it is flaky if we don't provide a separate strategy for aliases for request and response due to the Pydandic model_dump only generating the proper attributes to update in this case.
In addition, while doing these changes and testing the update_mask
parameter in the Patch endpoints, I realised that some updates are needed for the update_mask field. Even though it is stated as a comma-separated string list, it is behaving differently in FastAPI. This is because the distinction in passing list from query param is to use the following HTTP call string.
HTTP Call String
http://localhost:8000/items/?q=foo&q=bar
https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#query-parameter-list-multiple-values
I believe this is not the case for our legacy api. Please check the test case for connection. I think indeed it is properly cast as a sequence in legacy api but when we use this approach in FastAPI, the value is going as a single-item list rather than a distinct list where it includes all the items. Otherwise, the FastAPI example should be used in the request stated above.
https://github.com/apache/airflow/blob/main/tests/api_connexion/endpoints/test_connection_endpoint.py#L386
@pierrejeambrun This is why It took me some time to make the changes. I had thoughtful times for making everyone happy in pre-commit and FastAPI 😅 What do you think? Please correct me if I am missing something.
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.
Hello @bugraoz93,
TLDR: Yes fastapi use the exploded=True
way for passing list, and we should use that and not try to pass comma separated value as before q=item1,item2
. That's a breaking change and we can document it.
Indeed fastapi use the exploded=True
form where the legacy one does not. You can take a look at the other tests on the update_mask
in fastapi. You can either pass an object with a list to the params
parameters of the starlette test client, and let him handle the formatting and encoding of query parameters. (what we are doing). i.e client.get(..., params={"q": [item1, item2]})
.
Or you can do that manually (don't recommend) by doing "?q=item1&q=item2
".
More info on the spec here:
https://swagger.io/docs/specification/v3_0/serialization/
I can add a significant
newsfragment to warn users that this changed.
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.
Hello @pierrejeambrun,
Thanks for the details! I forgot to add TLDR into my previous message (it was an essay) my bad 😓
I saw the way along with tests and implemented everything similar to those. Then this bugged me when I saw legacy api
tests because I thought everything in Public API would be backwards compatible. If we can do breaking changes, all good.
I will convert the update_mask
parameters again into list[str]
. I can document these changes into a significant
fragment.
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.
I have adjusted the code accordingly and included the significant
fragment. Please check again when you have time :)
8f6c415
to
c4bfddb
Compare
…ameter Field default value for backward compatibility
…on for backward compat and fix for variables serialisation won't update val in setattr()
…date_mask breaking change
f33126e
to
e58dae7
Compare
The ``update_mask`` parameter in the PATCH endpoints has been updated to explode (true) by default. | ||
Before: ``http://<URL>:<PORT>/<PATH>?update_mask=item1,item2`` |
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.
I don't think this should be specific to the PATCH
endpoint. This is how fast API works, and all list parameters in the API will be impacted, where this suggests that only this specific query parameter is impacted.
closes: #42592.
Include, making fields nullable for patch endpoints for backward compatibility.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.