Unified Bulk Patch Endpoint for Connections in Rest API (FastAPI)

[bugraoz93]

relates: #45601

• Implementing the unified approach for bulk endpoints.

---

^ 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.

[pierrejeambrun]

Nice

A few suggestions, and hint for follow up PRs. Almost ready to merge.

[bugraoz93]

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 which could be applied for any unification for data models. I moved them in common.py. I have included TODO for further unification for datamodels from this story.

[pierrejeambrun]

Nice

[pierrejeambrun]

Thanks 🎉

[potiuk]

NIIIIICE!

[shubhamraj-git]

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.
3. [In context of Pools bulk API] Since the PoolBulkCreateAction and PoolBulkUpdateAction models share the same structure (both have an action field and a pools list), pydantic may incorrectly parse an update action as a create action because it matches the first type (PoolBulkCreateAction) in the union. This issue here is because the PoolBulkCreateAction and PoolBulkUpdateAction have pools type as different list[PoolPostBody] and list[PoolPatchBody] respectively. [Can refer the below attached PR]

Was this intentional? Or just a miss, in case I can rectify this in upcoming PR for bulk pool.

For now, based upon above comments, I have included that in #45939

[bugraoz93]

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.

[shubhamraj-git]

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 #45816 . Let me know if you need to do this in other way.

Also, I did this since the Pool Bulk PR #45939 is ready, and this seems to be earliest solution, didn't brainstorm more, since you are already working on #45816 which will anyways refactor these all.

[bugraoz93]

For now, based upon above comments, I have included that in #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 #45816 . Let me know if you need to do this in other way.

Also, I did this since the Pool Bulk PR #45939 is ready, and this seems to be earliest solution, didn't brainstorm more, since you are already working on #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.

[pierrejeambrun]

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 😄)