Skip to content

Validate Google operator templated parameters after rendering #70534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mitre88 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Validate Google operator templated parameters after rendering #70534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions generated/known_airflow_exceptions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,6 @@ providers/google/src/airflow/providers/google/cloud/operators/looker.py::1
providers/google/src/airflow/providers/google/cloud/operators/managed_kafka.py::15
providers/google/src/airflow/providers/google/cloud/operators/pubsub.py::1
providers/google/src/airflow/providers/google/cloud/operators/spanner.py::19
providers/google/src/airflow/providers/google/cloud/operators/speech_to_text.py::2
providers/google/src/airflow/providers/google/cloud/operators/text_to_speech.py::1
providers/google/src/airflow/providers/google/cloud/operators/translate.py::9
providers/google/src/airflow/providers/google/cloud/operators/translate_speech.py::2
providers/google/src/airflow/providers/google/cloud/operators/vertex_ai/batch_prediction_job.py::1
Expand Down Expand Up @@ -317,7 +315,6 @@ providers/google/src/airflow/providers/google/cloud/utils/credentials_provider.p
providers/google/src/airflow/providers/google/common/hooks/base_google.py::7
providers/google/src/airflow/providers/google/common/hooks/operation_helpers.py::2
providers/google/src/airflow/providers/google/firebase/hooks/firestore.py::1
providers/google/src/airflow/providers/google/firebase/operators/firestore.py::1
providers/google/src/airflow/providers/google/leveldb/hooks/leveldb.py::4
providers/google/src/airflow/providers/google/marketing_platform/hooks/campaign_manager.py::2
providers/google/src/airflow/providers/google/marketing_platform/hooks/search_ads.py::1
Expand Down
7 changes: 3 additions & 4 deletions providers/google/src/airflow/providers/google/cloud/operators/speech_to_text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from google.api_core.gapic_v1.method import DEFAULT, _MethodDefault
from google.protobuf.json_format import MessageToDict

from airflow.providers.common.compat.sdk import AirflowException
from airflow.providers.google.cloud.hooks.speech_to_text import CloudSpeechToTextHook, RecognitionAudio
from airflow.providers.google.cloud.operators.cloud_base import GoogleCloudBaseOperator
from airflow.providers.google.common.hooks.base_google import PROVIDE_PROJECT_ID
Expand Down Expand Up @@ -99,17 +98,17 @@ def __init__(
self.gcp_conn_id = gcp_conn_id
self.retry = retry
self.timeout = timeout
self._validate_inputs()
self.impersonation_chain = impersonation_chain
super().__init__(**kwargs)

def _validate_inputs(self) -> None:
if self.audio == "":
raise AirflowException("The required parameter 'audio' is empty")
raise ValueError("The required parameter 'audio' is empty")
if self.config == "":
raise AirflowException("The required parameter 'config' is empty")
raise ValueError("The required parameter 'config' is empty")

def execute(self, context: Context):
self._validate_inputs()
hook = CloudSpeechToTextHook(
gcp_conn_id=self.gcp_conn_id,
impersonation_chain=self.impersonation_chain,
Expand Down
5 changes: 2 additions & 3 deletions providers/google/src/airflow/providers/google/cloud/operators/text_to_speech.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

from google.api_core.gapic_v1.method import DEFAULT, _MethodDefault

from airflow.providers.common.compat.sdk import AirflowException
from airflow.providers.google.cloud.hooks.gcs import GCSHook
from airflow.providers.google.cloud.hooks.text_to_speech import CloudTextToSpeechHook
from airflow.providers.google.cloud.operators.cloud_base import GoogleCloudBaseOperator
Expand Down Expand Up @@ -112,7 +111,6 @@ def __init__(
self.gcp_conn_id = gcp_conn_id
self.retry = retry
self.timeout = timeout
self._validate_inputs()
self.impersonation_chain = impersonation_chain
super().__init__(**kwargs)

Expand All @@ -125,9 +123,10 @@ def _validate_inputs(self) -> None:
"target_filename",
]:
if getattr(self, parameter) == "":
raise AirflowException(f"The required parameter '{parameter}' is empty")
raise ValueError(f"The required parameter '{parameter}' is empty")

def execute(self, context: Context) -> None:
self._validate_inputs()
hook = CloudTextToSpeechHook(
gcp_conn_id=self.gcp_conn_id,
impersonation_chain=self.impersonation_chain,
Expand Down
5 changes: 2 additions & 3 deletions providers/google/src/airflow/providers/google/firebase/operators/firestore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from collections.abc import Sequence
from typing import TYPE_CHECKING

from airflow.providers.common.compat.sdk import AirflowException
from airflow.providers.google.common.hooks.base_google import PROVIDE_PROJECT_ID
from airflow.providers.google.firebase.hooks.firestore import CloudFirestoreHook
from airflow.providers.google.version_compat import BaseOperator
Expand Down Expand Up @@ -78,14 +77,14 @@ def __init__(
self.project_id = project_id
self.gcp_conn_id = gcp_conn_id
self.api_version = api_version
self._validate_inputs()

@kaxil kaxil Jul 30, 2026

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.

Why this change?

@mitre88 mitre88 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kaxil Because body is a template field, so if not self.body in __init__ reads the unrendered value: a Jinja template string like "{{ ti.xcom_pull(...) }}" is always truthy at construction time, so the check can never catch a body that renders empty — and per the template-field validation burn-down (#70296), value checks on templated parameters belong after rendering. Moving the call to execute() validates the rendered value. (An argument-provision check would stay in __init__ per #70505, but not self.body is a truthiness/value check, which is exactly the kind that must move.)

Same rationale for the speech_to_text/text_to_speech changes in this PR.

Drafted-by: Claude Code (Fable 5) (no human review before posting)

self.impersonation_chain = impersonation_chain

def _validate_inputs(self) -> None:
if not self.body:
raise AirflowException("The required parameter 'body' is missing")
raise ValueError("The required parameter 'body' is missing")

def execute(self, context: Context):
self._validate_inputs()

@shahar1 shahar1 Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the move I'd like to reconsider — and it's the same question kaxil raised above.

if not self.body asks whether a meaningful body was supplied, which #70296 classes as a provision check, and provision checks are the documented exception that must stay in __init__:

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.

The two stated reasons both bite here: with render_template_as_native_obj=True a provided field can render to None, so this check in execute() will report a supplied argument as missing; and moving it turns a static authoring mistake into a per-task-instance, per-retry runtime failure instead of a Dag import error.

#70505 — which you cite above — is the PR that narrowed the hook to permit this check in the constructor, not to require moving it. Five merged PRs already made this move and are queued for revert in #70503.

The rendered-value gap you're fixing is real, though. #70296's answer is to have both: "a provision check in __init__ guarantees nothing about the rendered value: code in execute() that needs the field set still needs its own guard." S3DeleteObjectsOperator is the worked example — both copies stay. So I'd keep the __init__ check and add this one, rather than relocate it.

One trap if you do rewrite the constructor copy: #70505 warns that turning a required-argument check into is None loosens it. body is required, so if body is None would start accepting body={}, which today's check rejects.

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

hook = CloudFirestoreHook(
gcp_conn_id=self.gcp_conn_id,
api_version=self.api_version,
Expand Down
16 changes: 16 additions & 0 deletions providers/google/tests/unit/google/cloud/operators/test_speech_to_text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ def test_recognize_speech_green_path(self, mock_hook):
config=CONFIG, audio=AUDIO, retry=DEFAULT, timeout=None
)

@patch("airflow.providers.google.cloud.operators.speech_to_text.CloudSpeechToTextHook")
def test_empty_audio_fails_at_execute_time(self, mock_hook):
op = CloudSpeechToTextRecognizeSpeechOperator(
project_id=PROJECT_ID,
gcp_conn_id=GCP_CONN_ID,
audio="{{ var.value.audio }}",
config=CONFIG,
task_id="id",
)
# Template rendering replaces the Jinja expression with the resolved value before execute.
op.audio = ""

with pytest.raises(ValueError, match="The required parameter 'audio' is empty"):
op.execute(context={"task_instance": Mock()})
mock_hook.assert_not_called()

@patch("airflow.providers.google.cloud.operators.speech_to_text.CloudSpeechToTextHook")
def test_missing_config(self, mock_hook):
mock_hook.return_value.recognize_speech.return_value = True
Expand Down
3 changes: 1 addition & 2 deletions providers/google/tests/unit/google/cloud/operators/test_text_to_speech.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import pytest
from google.api_core.gapic_v1.method import DEFAULT

from airflow.providers.common.compat.sdk import AirflowException
from airflow.providers.google.cloud.operators.text_to_speech import CloudTextToSpeechSynthesizeOperator

PROJECT_ID = "project-id"
Expand Down Expand Up @@ -98,7 +97,7 @@ def test_missing_arguments(
):
mocked_context = Mock()

with pytest.raises(AirflowException) as ctx:
with pytest.raises(ValueError, match="is empty") as ctx:
CloudTextToSpeechSynthesizeOperator(
project_id="project-id",
input_data=input_data,
Expand Down
17 changes: 17 additions & 0 deletions providers/google/tests/unit/google/firebase/operators/test_firestore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

from unittest import mock

import pytest

from airflow.providers.google.firebase.operators.firestore import CloudFirestoreExportDatabaseOperator

TEST_OUTPUT_URI_PREFIX: str = "gs://example-bucket/path"
Expand All @@ -42,3 +44,18 @@ def test_execute(self, mock_firestore_hook):
mock_firestore_hook.return_value.export_documents.assert_called_once_with(
body=EXPORT_DOCUMENT_BODY, database_id="(default)", project_id=TEST_PROJECT_ID
)

@mock.patch("airflow.providers.google.firebase.operators.firestore.CloudFirestoreHook")
def test_empty_body_fails_at_execute_time(self, mock_firestore_hook):
op = CloudFirestoreExportDatabaseOperator(
task_id="test-task",
body="{{ var.value.export_body }}",
gcp_conn_id="google_cloud_default",
project_id=TEST_PROJECT_ID,
)
# Template rendering replaces the Jinja expression with the resolved value before execute.
op.body = None

with pytest.raises(ValueError, match="The required parameter 'body' is missing"):
op.execute(mock.MagicMock())
mock_firestore_hook.return_value.export_documents.assert_not_called()
Loading