Repository navigation
Validate Google operator templated parameters after rendering - #70534
Conversation
There was a problem hiding this comment.
AI Summary (outdated)
Click here
Thanks for picking this up — but I think the classification here needs revisiting, and the conclusion changes the shape of the PR.These are provision checks, and #70296 says they must stay in __init__
Your reply to kaxil cites #70505 as sanctioning the move, but #70505 is the PR that narrowed the rule in the opposite direction — to keep exactly these checks in the constructor. #70296's "False positives" section is explicit:
A check that only asks whether an argument was passed … belongs in
__init__and must not be moved. … Fix these by rewriting in place, not by moving.
with two reasons that both apply here: under render_template_as_native_obj=True a provided field renders to None, so the same check in execute() reports a supplied argument as missing; and raising at construction surfaces a static authoring mistake as a Dag import error rather than on a worker once per task instance and per retry.
Five already-merged burn-down PRs moved provision checks that should have stayed — reverting them is tracked in #70503. I'd rather not add three more.
What I think the right shape is
The bug you're describing is real — a templated audio / body that renders empty is never caught. But #70296 answers that case directly: "a provision check in __init__ guarantees nothing about the rendered value: code in execute() that needs the field set still needs its own guard." The documented model is S3DeleteObjectsOperator, where both copies stay — the __init__ copy catches the authoring mistake at parse time, the execute() copy catches the rendered value.
So: keep the constructor check, and add a guard in execute() — rather than relocating the single check.
Two things that make this more than a mechanical edit, worth thinking through:
if not self.bodyandif self.audio == ""are truthiness / equality tests. #70296 describes truthiness as asking a third question that matches neither provision nor value, so neither existing check is already in the sanctioned shape.- #70505 warns that rewriting a required-argument check to
is Noneloosens it.body,audio,config,input_data,voice,audio_config,target_bucket_nameandtarget_filenameare all required keyword arguments, soif body is Nonewould start acceptingbody={}, which the current check rejects. Please don't just swap the polarity.
On AirflowException → ValueError
No objection — ValueError is the right call for a constructor-time authoring error, and it avoids introducing a new Airflow-exception usage that the ongoing clean-up would only have to unwind later. Keep that part.
Scope note
None of these three classes appears in validate_operators_init_exemptions.txt — the hook never flagged them, since it can't see reads behind a _validate_inputs() call. So this is voluntary work outside the burn-down's tracked set, which is likely why the provision/value classification didn't get caught by the usual guardrails. Not a problem in itself, just worth knowing why there was no safety net.
Verified, and holds up regardless of the above
- All eight parameters really are
template_fields, so the underlying gap is real. - 13/13 tests pass on the branch; reverting only the three source files fails 7 of them, including both new tests. The coverage is honest.
generated/known_airflow_exceptions.txtdrops exactly the three entries; no leftover references; no:raises:docstrings to update.cloud/operators/translate_speech.pyalready raises insideexecute()— no adjacent gap.- No changelog entry needed; the google changelog is regenerated from
git log.
Separate follow-up, not for this PR
The _validate_inputs()-behind-a-helper blind spot isn't limited to these three: cloud/operators/bigtable.py:51 (BigtableValidationMixin, whose REQUIRED_ATTRIBUTES for BigtableCreateInstanceOperator are all template_fields) and cloud/operators/compute.py:70 have the same shape and are equally invisible to the hook. Worth teaching validate_operators_init.py to follow one level of helper call so the burn-down can actually see them.
This review was drafted by an AI-assisted tool and confirmed by an Apache Airflow maintainer. After you've addressed the points above and pushed an update, an Apache Airflow maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting
CloudSpeechToTextRecognizeSpeechOperator, CloudTextToSpeechSynthesizeOperator, and CloudFirestoreExportDatabaseOperator validate template fields inside _validate_inputs() called from __init__, so the checks ran against un-rendered Jinja expressions and an empty rendered value was never caught. The validation now runs at execute time, and the raises are narrowed from AirflowException to ValueError per the ongoing exception clean-up.
a7ed135 to
5fa86da
Compare
Three Google provider operators —
CloudSpeechToTextRecognizeSpeechOperator,CloudTextToSpeechSynthesizeOperator, andCloudFirestoreExportDatabaseOperator— validate their template fields (audio,config,input_data,voice,audio_config,target_bucket_name,target_filename,body) inside a_validate_inputs()helper called from__init__.Because the helper reads the fields through a method call, the
validate-operators-initcheck cannot see these reads (same blind spot as the AppFlow subclasses fixed in #70440), but the bug is the same one tracked in #70296: the emptiness checks run against un-rendered Jinja expressions, which are always non-empty, so a templated value rendering to an empty string is never caught. The_validate_inputs()call is moved to the start ofexecute()in all three operators.While relocating them, the
raise AirflowExceptionusages are narrowed toValueErrorper the ongoing exception clean-up (known_airflow_exceptions.txtentries for the three files drop to 0), following the pattern of #66279.Tests: converted the text-to-speech missing-argument test to the new exception type (it already exercised
execute), and added execute-time tests for the speech-to-text and Firestore operators that construct with a templated value and validate the rendered value — both fail against the previous implementation.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Fable 5) following the guidelines