Skip to content

Refactor validate_key to raise ValueError instead of AirflowException - #68890

Merged
potiuk merged 7 commits into
apache:mainfrom
23tae:refactor-validate-key-exception
Aug 30, 2026
Merged

potiuk merged 7 commits into
apache:mainfrom
23tae:refactor-validate-key-exception

Conversation

@23tae

@23tae 23tae commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Why

The AirflowException is too broad for simple input validation errors. Since validate_key strictly checks the format and length of a string argument, using a standard ValueError provides a more specific error type for input validation failures.

What

  • Replaced AirflowException with ValueError inside the validate_key utility function.
  • Updated the corresponding parametrized test cases in test_validate_key to expect the new exception.
  • Updated downstream test cases (e.g., Kubernetes tests) to expect ValueError.

Error Output Example

Before:

airflow.sdk.exceptions.AirflowException: The key 'invalid..key' must not contain consecutive dots ('..') to prevent path traversal

After:

ValueError: The key 'invalid..key' must not contain consecutive dots ('..') to prevent path traversal

Verification

  • Static Checks (Prek): Passed
  • Unit Tests (Breeze): Passed

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jun 25, 2026
@potiuk

potiuk commented Jul 30, 2026

Copy link
Copy Markdown
Member

Needs conflict resolving

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — this is the right direction and better justified than the diff alone suggests. The Task SDK's own validate_key in airflow/sdk/definitions/_internal/node.py already raises ValueError for both the length and regex cases, so this brings airflow.utils.helpers.validate_key into line with a migration that's already happened on the other copy rather than making a fresh judgement call. Decrementing the known_airflow_exceptions.txt count from 4 to 1 alongside is good hygiene.

One thing to flag on blast radius, since it's wider than "a helper in utils": besides airflow connections add/import, this function is what KubernetesPodOperator uses for pod-name validation (providers/cncf/kubernetes/.../operators/pod.py:1517, validate_key(name, max_length=220)). So a Dag that wraps KPO execution in except AirflowException and relies on catching an invalid pod name will stop catching it — ValueError isn't a subclass of AirflowException.

That's a user-visible behaviour change in airflow-core, so I think it wants a newsfragment (airflow-core/newsfragments/68890.significant.rst or .misc.rst) noting that validate_key now raises ValueError and naming the KPO pod-name path specifically, so anyone with that except clause finds it. Happy to approve on that basis — please add one before merge.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Comment thread airflow-core/src/airflow/utils/helpers.py Outdated
@potiuk
potiuk merged commit f2db94d into apache:main Aug 30, 2026
155 checks passed
imrichardwu pushed a commit to imrichardwu/airflow that referenced this pull request Sep 11, 2026
…apache#68890)

* Replace AirflowException with ValueError in validate_key

* Update Kubernetes tests and known exceptions for refactor

* Fix backwards compatibility in Kubernetes tests for validate_key

* Align validate_key error message with Task SDK

* Add newsfragment for KubernetesPodOperator behavior change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants