dabla commented on code in PR #72398:
URL: https://github.com/apache/airflow/pull/72398#discussion_r4307214566
##########
providers/http/src/airflow/providers/http/hooks/http.py:
##########
@@ -687,6 +711,35 @@ async def request_func() -> ClientResponse:
raise NotImplementedError # should not reach this, but makes mypy
happy
+ async def _send(
+ self,
+ url: str,
+ *,
+ data: dict[str, Any] | str | None,
+ json: dict[str, Any] | str | None,
+ headers: dict[str, Any],
+ extra_options: dict[str, Any],
+ ) -> ClientResponse:
+ extra_options = dict(extra_options)
+ connection_header_names = set(self.config.connection_headers)
Review Comment:
**[warning]** A header the caller passes is dropped on a cross-origin hop
when the Extra has a header of the same name. The sync hook keeps it.
`config.connection_headers` holds every Extra key. `run()` then lays the
caller's `headers=` over the Extra values in `merged_headers`, but the names
stay in the set, so the middleware pops the caller's value as well. To check, I
ran this head against two local aiohttp servers on different ports. The Extra
was `{"X-API-Key": "s", "Accept": "application/json"}` and the call was
`run(endpoint=..., headers={"Accept": "text/csv", "X-Caller": "c"})`. The
redirect target got `X-Caller` but no `Accept`.
#72165 handles this the other way on purpose. `get_conn` removes caller and
`default_headers` names from the tracked set, and
`test_caller_header_overriding_connection_header_is_kept` covers it, because
the caller controls the request either way. If the two PRs land together as
planned, the same `run(headers=...)` call would behave differently on
`HttpHook` and `HttpAsyncHook`.
Suggestion: compute the names in `run()`, where the caller's headers are
still separate, and pass them down instead of reading
`self.config.connection_headers` in `_send`:
```python
caller_names = {str(name).lower() for name in headers or {}}
connection_header_names = {
name for name in self.config.connection_headers if name.lower() not in
caller_names
}
```
A case like #72165's parametrized test (`Accept` / `accept` / `ACCEPT`)
would keep the two hooks aligned.
##########
providers/http/docs/changelog.rst:
##########
@@ -27,6 +27,14 @@
Changelog
---------
+.. warning::
Review Comment:
**[nit]** The note reads as if it covers every HTTP hook, but it describes
only `HttpAsyncHook`.
"Extra headers from the HTTP Connection are sent on the first request and on
redirects that stay on the same origin" is true for `HttpHook` only once #72165
is merged. The last sentence ("follow aiohttp's origin rules") then applies to
the async hook alone. Same for the paragraph in `connections/http.rst`. #72165
adds no note of its own, which I raised on that PR. One wording that names both
hooks would hold whichever merges first, e.g. "`HttpHook` and `HttpAsyncHook`
do not forward Extra headers when a redirect leaves the origin of the first
request …".
It would also help to give the two ways out for anyone who relied on the old
behaviour (raised on #70000 for the same change): point the connection at the
final host, or set `allow_redirects=False` and make the second request yourself.
--
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]
