Normalize BigQuery DTS sensor expected statuses after rendering - #70528
Normalize BigQuery DTS sensor expected statuses after rendering#70528mitre88 wants to merge 1 commit into
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Thanks — correct, and this one is the clearest case of the pattern. expected_statuses is in template_fields, so normalising it in __init__ ran against the raw "{{ ... }}" string: _normalize_state_list would try TransferState["{{ VAR.VALUE.EXPECTED_STATUS }}"] and blow up at parse time before the value ever existed. Deferring to poke is the only way this can work.
A side benefit worth noting: self.expected_statuses now holds the rendered value rather than a set[TransferState], which makes the rendered-template view in the UI show what the user actually wrote instead of an enum set.
One observation inline about what a bad value now looks like.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| message = f"Transfer {self.run_id} did not succeed" | ||
| raise AirflowException(message) | ||
| return run.state in self.expected_statuses | ||
| return run.state in self._normalize_state_list(self.expected_statuses) |
There was a problem hiding this comment.
Two small consequences of moving the normalisation here, neither blocking:
Error quality. _normalize_state_list does TransferState[state.upper()], so a typo'd status now surfaces as a bare KeyError: 'SUCEEDED' from inside the sensor on every poke, rather than at parse time where the traceback at least pointed at the Dag file. That was survivable when it failed fast at parse; as a runtime failure it's worth wrapping in a ValueError naming the offending value and the valid ones. Arguably out of scope here, but this PR is what makes it user-visible.
Repeated work. Normalisation now runs on every poke rather than once. It's cheap and a sensor is I/O-bound, so this is genuinely negligible — mentioning it only so it's a conscious choice rather than an oversight. If you'd rather avoid it, normalising once into a cached attribute on first poke would do it.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Rebased onto current main. Validate expected_statuses after template rendering so templated values are checked at runtime.
8637f8f to
dc4e9b5
Compare
Part of the template-field validation burn-down tracked in #70296.
BigQueryDataTransferServiceTransferRunSensorlistsexpected_statusesintemplate_fieldsbut converts it toTransferStatemembers in__init__via_normalize_state_list, so passing a Jinja expression raisedKeyErrorat parse time. The sensor now stores the raw value and normalizes it inpoke, after rendering.Added a test constructing the sensor with a templated
expected_statusesthat passes once the field holds the rendered value — it fails against the previous implementation. The class is removed from the exemption list and thevalidate-operators-initcheck passes locally.Per the discussion in #70505 this is a genuine value transformation (not an argument-provision check), so it belongs at execute time.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Fable 5) following the guidelines