Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
pierrejeambrun commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2608136825 Nice guys, thanks for highlighting that @shubhamraj-git. Indeed that's a big part and the implementation is a little bit rough but i'm sure we will improve it iteratively. (sounds like we have a plan for that, I'll gladly review any PR on the front 😄) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
shubhamraj-git commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2608073934 Thanks @bugraoz93 , Sounds great. For now, I have solved the bug by removing the action_on_existence from BaseModel, and included that differently for create and (delete & update). This can be later unified when you are working on https://github.com/apache/airflow/issues/45816 . Let me know if you need to do this in other way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
bugraoz93 commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2608090615 >For now, based upon above comments, I have included that in https://github.com/apache/airflow/pull/45939 I just saw this update after sending my message :) Feel free to change that part in your MR, I don't want to block you on that. Thanks! >For now, I have solved the bug by removing the action_on_existence from BaseModel, and included that differently for create and (delete & update). This solves all the three issues. This can be later unified when you are working on https://github.com/apache/airflow/issues/45816 . Let me know if you need to do this in other way. > >Also, I did this since the Pool Bulk PR https://github.com/apache/airflow/pull/45939 is ready, and this seems to be earliest solution, didn't brainstorm more, since you are already working on https://github.com/apache/airflow/issues/45816 which will anyways refactor these all. Amazing, thanks! Looks good! I agree, fixing is enough and no need for brainstorming. I was trying to not put the work on you (mostly updating tests since they are unified now) :) I can move from there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
shubhamraj-git commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2608081214 Thanks @bugraoz93 , Sounds great. For now, I have solved the bug by removing the action_on_existence from BaseModel, and included that differently for create and (delete & update). This solves all the three issues. This can be later unified when you are working on https://github.com/apache/airflow/issues/45816 . Let me know if you need to do this in other way. Also, I did this since the Pool Bulk PR https://github.com/apache/airflow/pull/45939 is ready, and this seems to be earliest solution, didn't brainstorm more, since you are already working on https://github.com/apache/airflow/issues/45816 which will anyways refactor these all. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
bugraoz93 commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2608067378 > Hey @bugraoz93 , we replaced `action_if_exists ` and `action_if_not_exists` with `action_on_existence` > > 1. This contains, "overwrite", which would be not relevant in case of delete and update, and will fail. > 2. Also, `action_on_existence` seems misleading in case of update and delete, since the operations here were for if the value is not present and not on existence, when either it fails or skip. > > Was this intentional? Or just a miss, in case I can rectify this in upcoming PR for bulk pool. Hey @shubhamraj-git , This was the baseline for making things common for the bulk endpoints so implementation is still from surface at the moment. I think things are getting clearer while we implement more use cases/endpoints. On the documentation perspective `overwrite` is not relevant and it could be a side effect/bug since we are allowing to call the endpoints with `overwrite` in this case. I think we can drive from a parent `enum class` to make documentation nice and separate the `enum classes` according to action types. I am planning to do more unification for the datamodels in #45816. I can cover this one over there, it seems relevant. Please bring up anything in that issue to discuss more. Thanks for pointing out! I would be happy to bounce ideas and am going to tag you in the next PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
shubhamraj-git commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2607684237 Hey @bugraoz93 , we replaced `action_if_exists ` and `action_if_not_exists` with `action_on_existence` 1. This contains, "overwrite", which would be not relevant in case of delete and update, and will fail. 2. Also, `action_on_existence` seems misleading in case of update and delete, since the operations here were for if the value is not present and not on existence, then either it fails or skip. Was this intentional? Or just a miss, in case I can rectify this in upcoming PR for bulk connection. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
potiuk commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2606860463 NICE! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
pierrejeambrun merged PR #45715: URL: https://github.com/apache/airflow/pull/45715 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
pierrejeambrun commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2606799580 Nice -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
bugraoz93 commented on code in PR #45715: URL: https://github.com/apache/airflow/pull/45715#discussion_r1922851860 ## airflow/api_fastapi/core_api/datamodels/connections.py: ## @@ -90,8 +91,74 @@ class ConnectionBody(BaseModel): extra: str | None = Field(default=None) -class ConnectionBulkBody(BaseModel): -"""Connections Serializer for requests body.""" +class ConnectionBulkCreateAction(BaseModel): +"""Bulk Create Variable serializer for request bodies.""" + +action: Literal["create"] = "create" Review Comment: Updated as an Enum and applied the changes to `variables` too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
bugraoz93 commented on code in PR #45715:
URL: https://github.com/apache/airflow/pull/45715#discussion_r1922850608
##
tests/api_fastapi/core_api/routes/public/test_connections.py:
##
@@ -1058,3 +635,304 @@ def test_should_call_db_create_default_connections(self,
mock_db_create_default_
response = test_client.post("/public/connections/defaults")
assert response.status_code == 204
mock_db_create_default_connections.assert_called_once()
+
+
+class TestBulkConnections(TestConnectionEndpoint):
+@pytest.mark.parametrize(
+"actions, expected_results",
+[
+# Test successful create
+(
+{
+"actions": [
+{
+"action": "create",
+"connections": [
+{
+"connection_id": "NOT_EXISTING_CONN_ID",
+"conn_type": "NOT_EXISTING_CONN_TYPE",
+}
+],
+"action_if_exists": "skip",
+}
+]
+},
+{
+"create": {
+"success": ["NOT_EXISTING_CONN_ID"],
+"errors": [],
+}
+},
+),
+# Test successful create with skip
+(
+{
+"actions": [
+{
+"action": "create",
+"connections": [
+{
+"connection_id": TEST_CONN_ID,
+"conn_type": TEST_CONN_TYPE,
+},
+{
+"connection_id": "NOT_EXISTING_CONN_ID",
+"conn_type": "NOT_EXISTING_CONN_TYPE",
+},
+],
+"action_if_exists": "skip",
+}
+]
+},
+{
+"create": {
+"success": ["NOT_EXISTING_CONN_ID"],
+"errors": [],
+}
+},
+),
+# Test create with overwrite
+(
+{
+"actions": [
+{
+"action": "create",
+"connections": [
+{
+"connection_id": TEST_CONN_ID,
+"conn_type": TEST_CONN_TYPE,
+"description": "new_description",
+}
+],
+"action_if_exists": "overwrite",
+}
+]
+},
+{
+"create": {
+"success": [TEST_CONN_ID],
+"errors": [],
+}
+},
+),
+# Test create conflict
+(
+{
+"actions": [
+{
+"action": "create",
+"connections": [
+{
+"connection_id": TEST_CONN_ID,
+"conn_type": TEST_CONN_TYPE,
+"description": TEST_CONN_DESCRIPTION,
+"host": TEST_CONN_HOST,
+"port": TEST_CONN_PORT,
+"login": TEST_CONN_LOGIN,
+},
+],
+"action_if_exists": "fail",
+}
+]
+},
+{
+"create": {
+"success": [],
+"errors": [
+{
+"error": "The connections with these
connection_ids: {'test_connection_id'} already exist.",
+"status_code": 409,
+},
+],
+}
+},
+),
+# Test successful update
+(
+{
+"actions": [
+{
+"action": "update",
+"connections": [
+{
+"connection_id": TEST_CONN_ID,
+
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
bugraoz93 commented on PR #45715: URL: https://github.com/apache/airflow/pull/45715#issuecomment-2603239451 > Nice > > A few suggestions, and hint for follow up PRs. Almost ready to merge. Thanks for the quick review! I updated the code according to the comments. I agree with all the other comments and created an issue for it too #45816. While making fields `enums`, I tried to create a baseline for unifying data models for bulk operations in general. I moved them in `common.py`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI) [airflow]
pierrejeambrun commented on code in PR #45715: URL: https://github.com/apache/airflow/pull/45715#discussion_r1922453886 ## airflow/api_fastapi/core_api/datamodels/connections.py: ## @@ -90,8 +91,74 @@ class ConnectionBody(BaseModel): extra: str | None = Field(default=None) -class ConnectionBulkBody(BaseModel): -"""Connections Serializer for requests body.""" +class ConnectionBulkCreateAction(BaseModel): +"""Bulk Create Variable serializer for request bodies.""" + +action: Literal["create"] = "create" +connections: list[ConnectionBody] = Field(..., description="A list of connections to be created.") +action_if_exists: Literal["skip", "overwrite", "fail"] = "fail" + + +class ConnectionBulkUpdateAction(BaseModel): +"""Bulk Update Connection serializer for request bodies.""" + +action: Literal["update"] = "update" +connections: list[ConnectionBody] = Field(..., description="A list of connections to be updated.") +action_if_not_exists: Literal["skip", "fail"] = "fail" -connections: list[ConnectionBody] -overwrite: bool | None = Field(default=False) + +class ConnectionBulkDeleteAction(BaseModel): +"""Bulk Delete Connection serializer for request bodies.""" + +action: Literal["delete"] = "delete" +connection_ids: list[str] = Field(..., description="A list of connection IDs to be deleted.") +action_if_not_exists: Literal["skip", "fail"] = "fail" + + +class ConnectionBulkBody(BaseModel): +"""Request body for bulk Connection operations (create, update, delete).""" + +actions: list[ConnectionBulkCreateAction | ConnectionBulkUpdateAction | ConnectionBulkDeleteAction] = ( Review Comment: I think we can add a pydantic discriminator to save time at the serialization time. Basically the `action` field tells us what model should be use to deserialize an action. ## airflow/api_fastapi/core_api/datamodels/connections.py: ## @@ -90,8 +91,74 @@ class ConnectionBody(BaseModel): extra: str | None = Field(default=None) -class ConnectionBulkBody(BaseModel): -"""Connections Serializer for requests body.""" +class ConnectionBulkCreateAction(BaseModel): +"""Bulk Create Variable serializer for request bodies.""" + +action: Literal["create"] = "create" Review Comment: Maybe we should switch to an enum for the `action`. To avoid hardcoding everywhere `create` `delete` ... strings. ## airflow/api_fastapi/core_api/datamodels/connections.py: ## @@ -90,8 +91,74 @@ class ConnectionBody(BaseModel): extra: str | None = Field(default=None) -class ConnectionBulkBody(BaseModel): -"""Connections Serializer for requests body.""" +class ConnectionBulkCreateAction(BaseModel): +"""Bulk Create Variable serializer for request bodies.""" + +action: Literal["create"] = "create" +connections: list[ConnectionBody] = Field(..., description="A list of connections to be created.") +action_if_exists: Literal["skip", "overwrite", "fail"] = "fail" + + +class ConnectionBulkUpdateAction(BaseModel): +"""Bulk Update Connection serializer for request bodies.""" + +action: Literal["update"] = "update" +connections: list[ConnectionBody] = Field(..., description="A list of connections to be updated.") +action_if_not_exists: Literal["skip", "fail"] = "fail" -connections: list[ConnectionBody] -overwrite: bool | None = Field(default=False) + +class ConnectionBulkDeleteAction(BaseModel): +"""Bulk Delete Connection serializer for request bodies.""" + +action: Literal["delete"] = "delete" +connection_ids: list[str] = Field(..., description="A list of connection IDs to be deleted.") +action_if_not_exists: Literal["skip", "fail"] = "fail" + + +class ConnectionBulkBody(BaseModel): +"""Request body for bulk Connection operations (create, update, delete).""" + +actions: list[ConnectionBulkCreateAction | ConnectionBulkUpdateAction | ConnectionBulkDeleteAction] = ( +Field(..., description="A list of Connection actions to perform.") +) + + +class ConnectionBulkActionResponse(BaseModel): +""" +Serializer for individual bulk action responses. + +Represents the outcome of a single bulk operation (create, update, or delete). +The response includes a list of successful connection_ids and any errors encountered during the operation. +This structure helps users understand which key actions succeeded and which failed. +""" + +success: list[str] = Field( +default_factory=list, description="A list of connection_ids representing successful operations." +) +errors: list[dict[str, Any]] = Field( +default_factory=list, +description="A list of errors encountered during the operation, each containing details about the issue.", +) Review Comment: For instance this can almost be used for all resources (connection, variables etc..), we just need to update the `suecces
