Skip to content

Use is not None for GCSToS3Operator match_glob provision check - #71591

Open
DhanushAnegondi wants to merge 1 commit into
apache:mainfrom
DhanushAnegondi:use-is-not-none-for-match-glob-provision-check
Open

Use is not None for GCSToS3Operator match_glob provision check#71591
DhanushAnegondi wants to merge 1 commit into
apache:mainfrom
DhanushAnegondi:use-is-not-none-for-match-glob-provision-check

Conversation

@DhanushAnegondi

@DhanushAnegondi DhanushAnegondi commented Aug 14, 2026

Copy link
Copy Markdown

match_glob is in GCSToS3Operator.template_fields, so the constructor only ever sees the un-rendered Jinja expression, never the value the DAG author meant. The guard that rejects match_glob on an unsupported google provider tested it for truthiness:

if not self.__is_match_glob_supported and match_glob:

which is what validate-operators-init flags, and why the class is on the exemption list.

The check stays in __init__. It is a provision check, not a value check: it asks whether match_glob was supplied, combined with an environment capability the constructor can already answer for itself. It never inspects the value. Per the false-positives section of #70296 those belong in the constructor and get rewritten in place rather than moved — with render_template_as_native_obj=True a provided field can render to None, so the same check in execute() would report a supplied argument as missing; and raising at construction surfaces a static authoring mistake as a Dag import error rather than once per task instance and per retry.

Since #70505 narrowed the hook to sanction is None / is not None reads, the in-place rewrite passes and the exemption entry still goes.

What this changes

  • rewrites the guard to match_glob is not None
  • removes GCSToS3Operator from scripts/ci/prek/validate_operators_init_exemptions.txt — the hook fails on stale exemptions, so this has to land in the same commit as the fix
  • adds three test cases where nothing previously covered this error at all

Behavioural impact

One cell changes, on google providers older than 10.3.0:

match_glob before after
omitted (None) no raise no raise
"**/*.csv" raises raises
"" no raise raises

match_glob="" was supplied by the user and truthiness read it as absent. An empty glob is not a valid pattern, and on a provider below 10.3.0 it was never going to be honoured anyway — it would have been passed to a GCSHook.list() call that cannot accept it. This is the stricter direction that 05_pull_requests.rst sanctions for provision checks.

Relationship to #70723

Flagging this up front: #70723 is open against the same entry and reaches the same one-line fix. I worked this independently and only found that PR afterwards, so this is not a deliberate competing implementation — I'm raising it because the two diffs are close enough that a reviewer deserves to know rather than discover it.

Where they differ:

If maintainers prefer #70723 as the base — which is reasonable, it is older and more complete on the exception-type question — I am happy for this to be closed and will offer the missing test case there instead. I would rather that than have two near-identical PRs consuming review time.

Deliberately not changed

  • raise AirflowException — kept as-is, for the reason above.
  • The unreachable except ImportError. gcs_to_s3.py already does a module-level from airflow.providers.google.cloud.hooks.gcs import GCSHook, so the except ImportError around the function-level from airflow.providers.google import __version__ in __init__ cannot fire — the module import would have failed first. Real, but a separate concern.
  • The flatten_structure / keep_directory_structure warning. Reads two fields that are not template fields, so the rule does not apply and the hook does not flag it.

Verification

Run in a WSL checkout, from providers/amazon:

# baseline, before any change
uv run pytest tests/unit/amazon/aws/transfers/test_gcs_to_s3.py -q
# 30 passed

# the new tests against the UNFIXED source, to prove they discriminate
git checkout HEAD~1 -- providers/amazon/src/airflow/providers/amazon/aws/transfers/gcs_to_s3.py
uv run pytest tests/unit/amazon/aws/transfers/test_gcs_to_s3.py -q
# 2 failed, 31 passed - one failure per behavioural change:
#   [value_supplied]                 AirflowException raised, not ValueError  (exception type)
#   [empty_string_is_still_supplied] DID NOT RAISE ValueError                 (polarity)
git checkout HEAD -- providers/amazon/src/airflow/providers/amazon/aws/transfers/gcs_to_s3.py

# with the fix
uv run pytest tests/unit/amazon/aws/transfers/test_gcs_to_s3.py -q
# 33 passed
prek run --from-ref main
# validate-operators-init, ruff, ruff-format and codespell all pass on the changed files.
# Three unrelated hooks fail in my local environment only (breeze not on PATH, and
# AIRFLOW_HOME pointing at the checkout); no code hook fails.

No newsfragment: providers/amazon has no newsfragments/ directory, and providers/AGENTS.md says never to use them for providers.

related: #70296


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

Generated-by: Claude Code (Opus 5) following the guidelines

@boring-cyborg boring-cyborg Bot added area:dev-tools area:providers backport-to-v3-3-test Backport to v3-3-test provider:amazon AWS/Amazon - related issues labels Aug 14, 2026
@boring-cyborg

boring-cyborg Bot commented Aug 14, 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

@DhanushAnegondi
DhanushAnegondi force-pushed the use-is-not-none-for-match-glob-provision-check branch from a9f0132 to 71c4728 Compare August 14, 2026 22:16
match_glob is a template field, so its value is not available until
Jinja rendering happens on the worker, well after __init__ has run. The
guard that rejects match_glob on an unsupported google provider tested
it for truthiness, which the validate-operators-init prek hook flags.

The check only asks whether the argument was supplied; it never
inspects the value. Per the false-positive guidance on apache#70296 that
makes it a provision check, which is fixed in place rather than moved
to execute(). Moving it would break the check under
render_template_as_native_obj=True, where a supplied field can render
to None and so becomes indistinguishable from an omitted one, and would
defer a static authoring mistake from Dag parse time to every task
instance and retry.

Switching to an explicit is not None comparison changes one case:
match_glob="" is now rejected on a google provider older than 10.3.0,
where truthiness previously read the supplied empty string as absent.
An empty glob is not a valid pattern and was never honoured on those
versions.

Narrow the raised exception from AirflowException to ValueError, which
is what an invalid argument warrants, and drop the file from
generated/known_airflow_exceptions.txt accordingly. This follows the
merged apache#70359 precedent for S3DeleteObjectsOperator, which made the
same one-for-one change to the same four kinds of file.

Also remove the class from the prek exemption list, since the hook
fails on stale entries, and add tests covering supplied, empty-string
and omitted match_glob.

Related: apache#70296
@DhanushAnegondi
DhanushAnegondi force-pushed the use-is-not-none-for-match-glob-provision-check branch from 71c4728 to c1d5460 Compare August 14, 2026 22:30
@DhanushAnegondi
DhanushAnegondi marked this pull request as ready for review August 14, 2026 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant