Connections API is able to patch all fields correctly #48109
Conversation
jscheffl
requested review from
ephraimbuddy,
pierrejeambrun and
rawwar
as code owners
| # Not all fields match, therefore copy manually | ||
| if not update_mask or "conn_type" in update_mask: | ||
| connection.conn_type = patch_body.conn_type | ||
| if not update_mask or "description" in update_mask: | ||
| connection.description = patch_body.description | ||
| if not update_mask or "host" in update_mask: | ||
| connection.host = patch_body.host | ||
| if not update_mask or "schema" in update_mask: | ||
| connection.schema = patch_body.schema_ | ||
| if not update_mask or "login" in update_mask: | ||
| connection.login = patch_body.login | ||
| if not update_mask or "password" in update_mask: | ||
| connection._password = patch_body.password | ||
| if not update_mask or "port" in update_mask: | ||
| connection.port = patch_body.port | ||
| if not update_mask or "extra" in update_mask: | ||
| connection._extra = patch_body.extra |
There was a problem hiding this comment.
I wonder if instead of hard-coding this list here, we should make use of Pydantic's aliases so that the mapping can be maintained in the datamodel and handled for us by pydantic?
There was a problem hiding this comment.
That was before. And not working.
But we need a mapping in both directions unfortunately, schema is aliased to schema_ and on the other hand _password and _extra need to be aliased with underscore in the front :-(
There was a problem hiding this comment.
@ashb okay even worse. Took a look deeper into code. The model_dump()/property-transfer is only working if you create a new DB connection object (as this maps the internals in __init__()). Otherwise for password and extra because of encryption you need to walk via the official setters to properly update the fields.
Hope this is okay?
Also adjusted the Bulk API service, this had the same problem. Also consolidated the Pydantic to ORM mapping to one helper function.
airflow-core/src/airflow/api_fastapi/core_api/services/public/connections.py
Outdated
Show resolved
Hide resolved
jscheffl
force-pushed
the
bugfix/connections-api-patch-all-fields
branch
from
0ec69f1 to
1441339
Compare
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/public/connections.py
Outdated
Show resolved
Hide resolved
| @@ -569,7 +608,7 @@ def test_patch_should_respond_404(self, test_client, body): | |||
|
|
|||
| @pytest.mark.enable_redact | |||
| @pytest.mark.parametrize( | |||
| "body, expected_response", | |||
| "body, expected_response, update_mask", | |||
There was a problem hiding this comment.
I don't think we need the update_mask update.
All fields that are wet will be updated.
There was a problem hiding this comment.
Oh, we should. Especially because of the Alias and logic you requested to make "better" the current pytest found a regression. Actually needed to scratch my head 3-times until I found the bug.
The more I look on the revised code comparing to the previous list the more I hate it. Over generlized. And error prone because of aliases. So we SHOULD keep this pytest with update mask.
jscheffl
force-pushed
the
bugfix/connections-api-patch-all-fields
branch
from
1441339 to
7f187e5
Compare
There was a problem hiding this comment.
I think we removed the model_fields_set and this is the source of confusion.
When no 'mask' is passed to the function, only fields that are explicitely specified in the payload (even if passed to 'None') should be set. Other missing fields from the partial body should not be updated. I tried locally and we observe the opposite in this current state.
airflow-core/src/airflow/api_fastapi/core_api/services/public/connections.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py
Show resolved
Hide resolved
jscheffl
force-pushed
the
bugfix/connections-api-patch-all-fields
branch
from
7f187e5 to
a7ff401
Compare
jscheffl
force-pushed
the
bugfix/connections-api-patch-all-fields
branch
from
a7ff401 to
dca8763
Compare
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py
Show resolved
Hide resolved
| { | ||
| "connection_id": TEST_CONN_ID, | ||
| "conn_type": TEST_CONN_TYPE, | ||
| "extra": '{"key": "var"}', | ||
| "login": None, | ||
| "port": None, | ||
| }, |
There was a problem hiding this comment.
Nit: Such test case would be awesome for test_patch_should_respond_200 where no mask is provided. Just to be sure that it behaves as we expect. (setting to None login and port)
During implementation of #48102 we saw that the patch API for connections is not working correctly.
This PR fixes the problem - reason is that pydantic fields to not map 1:1 to API fields - as well as extends pytests to cover the difference