shahar1 commented on PR #65499:
URL: https://github.com/apache/airflow/pull/65499#issuecomment-4381354452
**Overall: nearly there, two things to address before merge.**
---
**Bug: `BigQueryUpdateRoutineOperator` — `fields` uses the wrong namespace,
causing silent no-ops**
The `fields` parameter is documented and tested with Python SDK attribute
names (`"body"`, `"type_"`), but the merge loop in `update_routine` operates on
the **camelCase REST API repr** (`updates_repr`):
```python
for field in fields:
if field in updates_repr: # camelCase keys only
("definitionBody", not "body")
merged_repr[field] = updates_repr[field]
else:
merged_repr.pop(field, None) # also camelCase; Python names are
no-ops here
```
A user following the operator docstring example and passing
`fields=["body"]` will get a silent no-op: the key `"body"` isn't in
`updates_repr` (the correct key is `"definitionBody"`), so neither branch does
anything, and the routine is unchanged. The API call succeeds with no error.
The system test uses only `fields=["description"]`, which is identical in
both namespaces and therefore masks the issue. The operator unit test uses
`fields=["description", "body"]` — the `"body"` entry would silently do nothing
on a real hook.
Fix: the `fields` parameter should use camelCase REST names throughout
(`"definitionBody"`, `"routineType"`, etc.). Update the operator docstring, the
operator unit test, and the hook docstring to reflect this.
`_ROUTINE_WRITABLE_PROPERTIES` is an internal constant passed to
`client.update_routine()` and correctly uses Python attribute names — that part
is fine.
---
**Missing changelog entry**
[AGENTS.md](https://github.com/apache/airflow/blob/main/AGENTS.md) asks that
new user-visible provider changes be recorded directly in
`providers/google/docs/changelog.rst`. This PR adds 5 operators and a sensor —
no changelog entry is present.
---
**Minor (non-blocking)**
- `first, second = MagicMock(), MagicMock()` in
`TestBigQueryListRoutinesOperator.test_execute` lacks `spec=`. Project
guidelines require `spec`/`autospec` on all mocks.
---
*Drafted-by: GitHub Copilot (Claude Sonnet 4.6); 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]
