shahar1 commented on code in PR #70534:
URL: https://github.com/apache/airflow/pull/70534#discussion_r3694926900
##########
providers/google/src/airflow/providers/google/firebase/operators/firestore.py:
##########
@@ -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()
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()
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
