Skip to content

Respect update_mask in bulk connection updates - #72486

Open
LostSnowfluff wants to merge 2 commits into
apache:mainfrom
LostSnowfluff:fix/connections-bulk-update-mask
Open

Respect update_mask in bulk connection updates#72486
LostSnowfluff wants to merge 2 commits into
apache:mainfrom
LostSnowfluff:fix/connections-bulk-update-mask

Conversation

@LostSnowfluff

Copy link
Copy Markdown

Description

The Connections bulk update action accepts update_mask and documents that only
the listed fields are applied. However, BulkConnectionService.handle_bulk_update
does not pass action.update_mask to update_orm_from_pydantic.

As a result, fields included in the request body but omitted from the update mask
are still updated. For example, a request with update_mask=["description"] also
changes conn_type when conn_type is present in the entity. This makes bulk
updates inconsistent with the single-connection PATCH behavior and can modify
fields that the caller did not intend to change.

Pass action.update_mask to the existing ORM update helper so bulk updates apply
the same field filtering as other update paths. Add a regression test covering a
request that provides both conn_type and description while masking only
description.

No documentation changes are needed because this restores the behavior already
described by the existing update_mask contract.

Was generative AI tooling used to co-author this PR?
  • Yes (OpenAI Codex)

Generated-by: OpenAI Codex following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions

@boring-cyborg

boring-cyborg Bot commented Sep 3, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@LostSnowfluff
LostSnowfluff marked this pull request as draft September 3, 2026 14:03
@LostSnowfluff
LostSnowfluff marked this pull request as ready for review September 3, 2026 14:10

@ColtenOuO ColtenOuO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction of this fix looks good, but other areas might need to be adjusted accordingly.

update_mask is shared across all entities, but an individual entity might not provide every field specified in the mask.

For example, if extra is included in the mask but omitted from one entity, this change passes the mask down to update_orm_from_pydantic, which ends up clearing that entity's existing extra via set_extra(None).

Could you test whether this bug holds true? If it does, I'd suggest fixing it and adding a regression test to prevent this from happening again.

@LostSnowfluff

Copy link
Copy Markdown
Author

The direction of this fix looks good, but other areas might need to be adjusted accordingly.

update_mask is shared across all entities, but an individual entity might not provide every field specified in the mask.

For example, if extra is included in the mask but omitted from one entity, this change passes the mask down to update_orm_from_pydantic, which ends up clearing that entity's existing extra via set_extra(None).

Could you test whether this bug holds true? If it does, I'd suggest fixing it and adding a regression test to prevent this from happening again.

Thanks for the thoughtful review. This is a very good catch.

I'll investigate how the shared update_mask interacts with fields omitted from individual entities, and verify whether it can clear existing values when passed to update_orm_from_pydantic.

If the issue does exist, I'll update the fix and add a regression test. Otherwise, I'll explain why this behavior does not take place. This may take some time, but I'll report back once I've completed the investigation.

Thanks again for your careful feedback.

@LostSnowfluff

Copy link
Copy Markdown
Author

I investigated this case and confirmed that the it is reproducible.

The fix now checks both the shared update_mask and the current entity’s provided fields before updating password or extra. This prevents omitted fields from being treated as None, while preserving the behavior of explicitly provided extra: null.

I also added regression tests for heterogeneous entity fields and explicit clearing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants