Skip to content

Validate Cloud Function deploy body after template rendering - #70531

Merged
shahar1 merged 5 commits into
apache:mainfrom
mitre88:fix-cloud-function-deploy
Sep 23, 2026
Merged

shahar1 merged 5 commits into
apache:mainfrom
mitre88:fix-cloud-function-deploy

Conversation

@mitre88

@mitre88 mitre88 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Part of the template-field validation burn-down tracked in #70296.

CloudFunctionDeployFunctionOperator lists body, location, and api_version in template_fields, but __init__ validated the body, ran ZipPathPreprocessor.preprocess_body() (which both validates and mutates the body), and constructed GcpBodyFieldValidator pinned to the un-rendered api_version. A fully templated body crashed with AttributeError at parse time inside the preprocessor. All of this now runs at the start of execute() against the rendered values.

The missing-location/body truthiness checks and the zip-path exclusivity rules depend on rendered values (an expression rendering to an empty string must count as missing), so per the discussion in #70505 these are genuine value reads, not provision checks.

Tests: converted the four construction-time raise tests to execute-time, and added test_templated_body_deploys_after_rendering, which constructs the operator with a templated body (previously a parse-time crash) and deploys once the field holds the rendered value — it fails against the previous implementation. The class is removed from the exemption list and the validate-operators-init check passes locally.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Fable 5)

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

@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 — correct move. body is a template field, so GcpBodyFieldValidator in __init__ was validating the un-rendered value: a templated body could never pass validation, and a literal bad one broke Dag parsing instead of failing the task. Constructing the validator and the ZipPathPreprocessor in execute and running _validate_inputs() there is the right shape.

test_templated_body_deploys_after_rendering is a good addition — asserting create_new_function.assert_called_once() after a rendered body proves the happy path still works, not just that the failure path raises.

One robustness point inline about an attribute that no longer exists until execute runs.


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

Comment thread providers/google/src/airflow/providers/google/cloud/operators/functions.py Outdated
Comment thread providers/google/tests/unit/google/cloud/operators/test_functions.py Outdated
@shahar1
shahar1 force-pushed the fix-cloud-function-deploy branch from 3280b64 to 05b9690 Compare September 22, 2026 16:06
@shahar1
shahar1 force-pushed the fix-cloud-function-deploy branch from 05b9690 to 1a79c1e Compare September 22, 2026 20:12
mitre88 and others added 3 commits September 22, 2026 23:15
Rebased onto current main. Move body/location validation and zip
preprocessing into execute so Jinja-templated body is checked after render.
CloudFunctionDeployFunctionOperator only assigned self.zip_path_preprocessor
inside execute(), so between construction and the first execute() the
attribute didn't exist at all and any access raised AttributeError -
affecting subclasses, tests, and anything introspecting the operator
between parse and run. Initialize it to None in __init__, like the
sibling self._field_validator, and build the real ZipPathPreprocessor in
execute() as a local variable that both _validate_inputs() and
should_upload_function() use, so mypy can verify there is no Optional
dereference.
test_templated_body_deploys_after_rendering passed unmodified on
pre-PR main: with a templated string body and no zip_path, every
ZipPathPreprocessor check is an "x in self.body" membership test, and
`in` on a str is a substring test that just returns False, so the
old __init__-time preprocessing never raised and the test asserted
nothing that distinguished old from new behavior.

Replace it with two tests that fail on the pre-PR code and pass on
this PR:

- test_templated_body_with_zip_path_uploads_after_rendering: with a
  templated body and a zip_path, the pre-PR code preprocessed the
  un-rendered string at __init__ time, so upload_function was
  silently left False and the zip was never uploaded - a wrong
  result, not a crash. Confirmed by running this test against
  functions.py from before this PR (c85aff6^): it fails with
  "Expected 'upload_function_zip' to be called once. Called 0 times."

- test_templated_location_rendered_empty_raises: the pre-PR code
  checked `location` truthiness once, at __init__ time, against the
  truthy "{{ ... }}" string, and never re-checked it in execute, so
  an empty rendered location silently proceeded to deploy. Confirmed
  by running this test against the pre-PR code: it fails with
  "DID NOT RAISE AirflowException".

Both tests pass against this PR's functions.py, which validates
location and preprocesses the zip path after template rendering.
Moving GcpBodyFieldValidator construction into execute() was one of the
three render-order fixes here, but it was the only one left without a
test. sourceRepositoryUrl is the single api_version-gated spec in
CLOUD_FUNCTION_VALIDATION, so it is the only available lever to prove
the validator now sees the rendered value rather than the Jinja string.
@shahar1

shahar1 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Edit: few corrections I made after review (drafted by Claude Code & Codex).

  • body: The code expected a dictionary, but received text such as "{{ var.value.body }}". Checking whether that text contains sourceUploadUrl usually returns false, so the operator incorrectly decides it does not need to upload the zip. If the template’s variable name happens to contain a Cloud Functions field name, the same checks can instead produce a TypeError or a misleading conflict error. The original claim that this always caused an AttributeError was wrong.
    location: A template expression is a nonempty string, so it passes the original “is a location provided?” check—even if its eventual value is empty.
  • api_version: The validator receives the literal template expression instead of something like "v1beta2". It therefore skips checks that apply to that particular API version.
  • The test list is also stale. test_templated_body_deploys_after_rendering passed against
    unmodified main and was replaced by three tests that each fail without this change:
    test_templated_body_with_zip_path_uploads_after_rendering,
    test_templated_location_rendered_empty_raises and
    test_templated_api_version_validates_after_rendering — one per templated field.

@shahar1
shahar1 merged commit e5eda21 into apache:main Sep 23, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools area:providers provider:google Google (including GCP) related issues ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants