dabla commented on code in PR #69904:
URL: https://github.com/apache/airflow/pull/69904#discussion_r4400759575
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -230,6 +230,7 @@ def __init__(
data: dict[str, Any] | str | None = None,
headers: dict[str, str] | None = None,
extra_options: dict[str, Any] | None = None,
+ response_error_codes_allowlist: list[str] | tuple[str, ...] | None =
None,
Review Comment:
**[warning]** The class docstring still describes the old behaviour and does
not document the new parameter.
Line 213 (not in the diff) says the trigger "fires when the request to a URL
returns a non-404 status code", and the `:param` list stops at `poke_interval`.
Now that the retried codes are configurable, say so and add `:param
response_error_codes_allowlist:` with the same wording as the sensor (default
`["404"]` when `None`, empty list to allowlist nothing).
Nit on the same line: the sensor passes a tuple and the triggerer passes a
list after deserialisation, which `Sequence[str] | None` covers without
spelling both out.
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -238,6 +239,7 @@ def __init__(
self.data = data
self.headers = headers
self.extra_options = extra_options or {}
+ self.response_error_codes_allowlist =
tuple(response_error_codes_allowlist or ("404",))
Review Comment:
**[blocker]** An empty allowlist turns back into `("404",)` in the trigger,
so `[]` no longer makes 404 fail after deferral.
`HttpSensor` documents `response_error_codes_allowlist=[]` as the way to
make `404 Not Found` raise ([sensor
docstring](https://github.com/apache/airflow/blob/63742f231601883959077bb983545f68e18a5dce/providers/http/src/airflow/providers/http/sensors/http.py#L69-L71))
and normalises on `is None` for that reason
([`__init__`](https://github.com/apache/airflow/blob/63742f231601883959077bb983545f68e18a5dce/providers/http/src/airflow/providers/http/sensors/http.py#L112-L114)).
The sensor then hands the trigger `()`, and `tuple(() or ("404",))` is
`("404",)` again. Probed on this head:
```
sensor response_error_codes_allowlist=[] -> () '404:Not
Found'.startswith(...) -> False
trigger (built by execute()) -> ('404',) '404:Not
Found'.startswith(...) -> True
```
So the sync poke raises on 404 while the deferred trigger keeps sleeping on
it: the same class of divergence this PR fixes for 503. Mirror the sensor's
normalisation:
```python
self.response_error_codes_allowlist = (
("404",) if response_error_codes_allowlist is None else
tuple(response_error_codes_allowlist)
)
```
`None` still means "old default" for triggers serialised before this change
(their kwargs have no such key), so the backward-compatible path is kept. A
test with `[]` that asserts a `404:Not Found` error propagates out of `run()`
would pin this.
##########
providers/http/src/airflow/providers/http/triggers/http.py:
##########
@@ -272,8 +275,10 @@ async def run(self) -> AsyncIterator[TriggerEvent]:
yield TriggerEvent(True)
return
except AirflowException as exc:
- if str(exc).startswith("404"):
+ if str(exc).startswith(self.response_error_codes_allowlist):
await asyncio.sleep(self.poke_interval)
+ continue
+ raise
Review Comment:
**[warning]** The new `raise` branch has no test.
This is the second behaviour change of the PR: a non-allowlisted error used
to be swallowed and retried in a tight loop, and now ends the trigger, which
fails the task the same way the sync `poke()` does. That is the right parity,
but nothing covers it: `test_run_retries_allowlisted_errors` only exercises the
sleep branch. Add a case where `hook.run` raises
`HttpErrorException("403:Forbidden")` and assert `run()` raises it
(`pytest.raises`), so a later refactor cannot bring the silent loop back.
##########
providers/http/tests/unit/http/triggers/test_http.py:
##########
@@ -207,9 +208,36 @@ def test_serialization(self, sensor_trigger):
"headers": TEST_HEADERS,
"data": TEST_DATA,
"extra_options": TEST_EXTRA_OPTIONS,
+ "response_error_codes_allowlist": ("404",),
"poke_interval": 5.0,
}
+ def test_serialization_with_response_error_codes_allowlist(self):
+ trigger = HttpSensorTrigger(
+ endpoint=TEST_ENDPOINT,
+ response_error_codes_allowlist=("404", "503"),
+ )
+
+ _, kwargs = trigger.serialize()
+
+ assert kwargs["response_error_codes_allowlist"] == ("404", "503")
+
+ @pytest.mark.asyncio
+ @mock.patch(HTTP_PATH.format("asyncio.sleep"))
+ @mock.patch(HTTP_PATH.format("HttpAsyncHook"))
+ async def test_run_retries_allowlisted_errors(self, mock_hook, mock_sleep):
+ trigger = HttpSensorTrigger(
+ endpoint=TEST_ENDPOINT,
+ response_error_codes_allowlist=["404", "503"],
+ )
+ mock_hook.return_value.run.side_effect =
[AirflowException("503:Service Unavailable"), None]
Review Comment:
**[blocker]** This test fails on the PR head: the patched hook is a plain
`MagicMock`, so the second `run()` returns `None` and `await None` raises.
Run on `63742f2` with the provider's test deps:
```
providers/http/src/airflow/providers/http/triggers/http.py:268: in run
await hook.run(
E TypeError: object NoneType can't be used in 'await' expression
FAILED
test_http.py::TestHttpSensorTrigger::test_run_retries_allowlisted_errors
```
The first call "works" only because the `MagicMock` raises synchronously
before anything is awaited. Make `run` an `AsyncMock` so both calls go through
`await`:
```python
mock_hook.return_value.run = mock.AsyncMock(
side_effect=[HttpErrorException("503:Service Unavailable"), None]
)
```
Two related points for the same test:
- Raise what the hook raises. `HttpAsyncHook.run` wraps
`ClientResponseError` into `HttpErrorException(f"{e.status}:{e.message}")`
([hook](https://github.com/apache/airflow/blob/main/providers/http/src/airflow/providers/http/hooks/http.py#L879-L880));
a bare `AirflowException` passes the `except` only because
`HttpErrorException` subclasses it. `main` already imports `HttpErrorException`
in this file's import block, which is also where the branch now conflicts with
`main` (see summary).
- The hook mock is spec-less; `mock.patch(HTTP_PATH.format("HttpAsyncHook"),
autospec=True)` would have caught the non-awaitable `run` at once, and the
repo's AGENTS.md asks for a spec on new mocks.
--
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]
