shahar1 commented on code in PR #74930:
URL: https://github.com/apache/airflow/pull/74930#discussion_r4306237946
##########
providers/google/tests/unit/google/cloud/utils/test_field_validator.py:
##########
@@ -313,3 +315,37 @@ def test_validate_should_validate_group_of_specs(self):
validator = GcpBodyFieldValidator(specification, "v1")
validator.validate(body)
+
+ def
test_validate_should_not_warn_for_field_matching_current_api_version(self,
caplog):
+ # A field declared for the same api_version the validator runs with is
a valid, known
+ # field. It must not be reported as "not specified in the validation
specification".
+ specification = [
+ dict(name="name", allow_empty=False),
+ dict(name="versioned_field", optional=True, api_version="v1"),
+ ]
+ body = {"name": "value", "versioned_field": "present"}
+
+ validator = GcpBodyFieldValidator(specification, "v1")
+ with caplog.at_level(logging.WARNING):
+ validator.validate(body)
+
+ assert "versioned_field" not in caplog.text
Review Comment:
**minor** (Testing): both new tests assert on `caplog.text` (here and line
351). AGENTS.md → Testing Standards:
> Do not assert on raw log text (`caplog.text` for example), these are
legacy string matching APIs planned for removal. Structured assertions via
`caplog` (which resolves to `cap_structlog` under structlog) are fine and
preferred: `"event name" in caplog` or `{"event": ..., "field": ...} in caplog`.
Patching the validator's logger avoids string matching and also works in the
Airflow 2.11 compat run, where `caplog` falls back to pytest's stock fixture
and the structured `in caplog` form is not available:
```python
with mock.patch.object(validator.log, "warning") as mock_warning:
validator.validate(body)
mock_warning.assert_not_called()
```
##########
providers/google/src/airflow/providers/google/cloud/utils/field_validator.py:
##########
@@ -437,15 +437,19 @@ def validate(self, body_to_validate: dict) -> None:
all_field_names = {
spec["name"]
for spec in self._validation_specs
- if spec.get("type") != "union" and spec.get("api_version") !=
self._api_version
+ if spec.get("type") != "union"
+ and (not spec.get("api_version") or spec.get("api_version") ==
self._api_version)
Review Comment:
**minor** (Testing): this flip also changes the opposite case. A body field
whose spec `api_version` *differs* from the validator's was previously in
`all_field_names` (no warning) and now triggers the "is in the body, but is not
specified" warning. Real example: `CLOUD_FUNCTION_VALIDATION` with the default
`api_version="v1"` and a body containing `sourceRepositoryUrl` (tagged
`v1beta2`) now logs this warning. I think the new behaviour is the right one,
but it is untested. AGENTS.md → Testing Standards:
> Target exactly 100% coverage of what the PR changes — no more, no less.
Every changed or added behaviour must have a test; every test must fail without
the PR's change.
One parametrized test covers both directions, and could replace the two new
tests (`Use @pytest.mark.parametrize for multiple similar inputs`). I checked
locally that both cases below pass on this branch and fail on `main`:
```python
@pytest.mark.parametrize(
("field_api_version", "should_warn"),
[
pytest.param("v1", False, id="matching-api-version"),
pytest.param("v1beta2", True, id="other-api-version"),
],
)
def test_validate_warns_only_for_fields_of_other_api_version(self,
field_api_version, should_warn):
specification = [
dict(name="name", allow_empty=False),
dict(name="versioned_field", optional=True,
api_version=field_api_version),
]
validator = GcpBodyFieldValidator(specification, "v1")
with mock.patch.object(validator.log, "warning") as mock_warning:
validator.validate({"name": "value", "versioned_field": "present"})
assert mock_warning.called is should_warn
```
(The union-nested variant can be a second set of params with the field
inside an `type="union"` spec.)
--
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]
