Skip to content

Validate storage transfer job body after template rendering - #70529

Open
mitre88 wants to merge 1 commit into
apache:mainfrom
mitre88:fix-storage-transfer-create-job
Open

Validate storage transfer job body after template rendering#70529
mitre88 wants to merge 1 commit into
apache:mainfrom
mitre88:fix-storage-transfer-create-job

Conversation

@mitre88

@mitre88 mitre88 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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

CloudDataTransferServiceCreateJobOperator lists body in template_fields but deep-copies and validates it in __init__. With a fully templated body (a Jinja expression or XComArg), the validator ran against the un-rendered expression, so TransferJobValidator's checks — the AWS-credential restriction and the single-data-source rule — were silently bypassed. The deep copy and validation now run at the start of execute(), against the rendered value, right before TransferJobPreprocessor mutates the body.

Added a test constructing the operator with a templated body that renders to a body embedding AWS credentials — with the previous implementation the credential check never fires and the job is created; now it raises before calling the hook. The class is removed from the exemption list and the validate-operators-init check passes locally.

Per the discussion in #70505 this is a genuine value read (validation of the rendered dict), not an argument-provision check.


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

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

The operator's body is a template field, but it was deep-copied and
validated in __init__, so a fully templated body skipped validation
entirely — the checks ran against the Jinja expression instead of the
rendered dict, silently bypassing the AWS-credential restriction. Part
of the burn-down tracked in apache#70296.

@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 in template_fields, so _validate_inputs() in __init__ was running TransferJobValidator against the un-rendered value; a templated body could never pass validation and a bad one broke Dag parsing instead of failing the task. Deferring both the deepcopy and the validation to execute fixes that, and removing the exemptions entry closes it out.

Good that the test asserts create_transfer_job.assert_not_called() — that shows it bails before touching the API.

One coverage gap inline.


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


def execute(self, context: Context) -> dict:
if isinstance(self.body, dict):
self.body = deepcopy(self.body)

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.

The deepcopy is load-bearing and its purpose isn't obvious from here — TransferJobPreprocessor.process_body() calls _inject_aws_credentials() and _reformat_schedule(), which mutate the body in place. Without the copy, the operator would write AWS credentials into the caller's own dict.

Two things worth doing:

  1. A one-line comment saying why the copy exists. The original had the copy sitting next to the assignment in __init__ where the reader could at least see it was defensive; now it's a bare reassignment in execute that looks removable. This is precisely the kind of line someone "simplifies" away later.
  2. A test for it. The new test covers the validation-raises path, but nothing asserts the behaviour the deepcopy is there for. Something like: build the operator with a dict containing an awsAccessKey, run a successful execute, and assert the dict you passed in is unchanged. Without that, the copy could be dropped in a future refactor and the whole suite would stay green while the operator started mutating user data.

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

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.

3 participants