Skip to content

add validation for variables import JSON structure#65047

Closed
rjgoyln wants to merge 2 commits intoapache:mainfrom
rjgoyln:fix/variable-import-type-validation
Closed

add validation for variables import JSON structure#65047
rjgoyln wants to merge 2 commits intoapache:mainfrom
rjgoyln:fix/variable-import-type-validation

Conversation

@rjgoyln
Copy link
Copy Markdown
Contributor

@rjgoyln rjgoyln commented Apr 11, 2026

Summary

This PR improves validation and error handling for the variables import command.

Previously, the implementation assumed that the input JSON was always a dictionary,
which could lead to unexpected AttributeError when processing invalid inputs.
This PR introduces explicit validation to ensure safer and more predictable behavior.


Changes

  • Validate that the top-level JSON structure is a dictionary
  • Enforce presence of "value" field for nested variable objects
  • Add unit tests to cover invalid input scenarios

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

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

Copy link
Copy Markdown
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

We were aiming to add validation to the entire stack of airflowctl. Adding high-level field validation here #65092 (issue attached) for generic cases and handling with more granular exceptions for all, what do you think?

Copy link
Copy Markdown
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Maybe just keeping the JSON part to see if it is valid JSON, or not, but that was already handled above.

...
    if not os.path.exists(args.file):
        rich.print(f"[red]Missing variable file: {args.file}")
        sys.exit(1)
    with open(args.file) as var_file:
        try:
            var_json = json.load(var_file)
        except json.JSONDecodeError:
            rich.print(f"[red]Invalid variable file: {args.file}")
            sys.exit(1)
...

I would say let's merge the generic solution for everything and close this. Many thanks for the PR! This could cause confusion, double validation of fields, etc.
Or if you think we can somehow combine, open to any ideas :)

@rjgoyln
Copy link
Copy Markdown
Contributor Author

rjgoyln commented Apr 12, 2026

Maybe just keeping the JSON part to see if it is valid JSON, or not, but that was already handled above.

...
    if not os.path.exists(args.file):
        rich.print(f"[red]Missing variable file: {args.file}")
        sys.exit(1)
    with open(args.file) as var_file:
        try:
            var_json = json.load(var_file)
        except json.JSONDecodeError:
            rich.print(f"[red]Invalid variable file: {args.file}")
            sys.exit(1)
...

I would say let's merge the generic solution for everything and close this. Many thanks for the PR! This could cause confusion, double validation of fields, etc. Or if you think we can somehow combine, open to any ideas :)

Thanks for the feedback! I agree that avoiding double validation and keeping the logic unified is better for the codebase. I’ll close this PR.

@rjgoyln rjgoyln closed this Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants