Skip to content

SyncClient: update session with transport's headers #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shtrom wants to merge 3 commits into
base: main
Choose a base branch
from
+51 −5
Open

SyncClient: update session with transport's headers #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a97d868
test_client: test that REST requests have the correct headers
shtrom Aug 25, 2026
9f64417
SyncClient: update session with transport's headers
shtrom Aug 25, 2026
3292352
SyncClient: retain existing session headers
shtrom Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/simple_github/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ def _get_requests_session(self) -> Session:
session = self._get_gql_session()
assert isinstance(session.transport, RequestsHTTPTransport)
assert session.transport.session

# mozilla-releng/simple-github#202: work around graphql-python/gql#613.
if session.transport.headers:
session.transport.session.headers.update(session.transport.headers)

Comment on lines +145 to +148

@Eijebong Eijebong Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got 2 issues.

It's not in the right place. It should be in _get_gql_session or doing GQL -> REST -> GQL with the same object ends up with the second gql call behaving different.

And secondly, this overrides all headers instead of updating them. So we lose default headers (notably Accept-Encoding and User-Agent).

Something like this test shows both well:

def test_headers(responses, sync_client):
    defaults = set(requests.Session().headers)
    print("Default:", defaults)
    responses.post(GITHUB_GRAPHQL_ENDPOINT, status=200, json={"data": {"foo": "bar"}})
    responses.get(f"{GITHUB_API_ENDPOINT}/octocat", status=200, json={"answer": 42})

    sync_client.execute("query { foo }")
    before = dict(responses.calls[-1].request.headers)
    print("Before:", before)

    sync_client.get("/octocat")
    sync_client.execute("query { foo }")
    after = dict(responses.calls[-1].request.headers)
    print("After:", after)

    session = sync_client._get_requests_session()
    assert defaults <= set(session.headers)
    assert before == after
    assert session.headers["Accept"] == "application/vnd.github+json"
    assert session.headers["Authorization"] == f"Bearer {sync_client.auth._token}"

You get:

Default: {'Accept-Encoding', 'User-Agent', 'Accept', 'Connection'}
Before: {'User-Agent': 'python-requests/2.34.2', 'Accept-Encoding': 'gzip, deflate, br, zstd', 'Accept': 'application/vnd.github+json', 'Connection': 'keep-alive', 'Authorization': 'Bearer abc', 'Content-Length': '24', 'Content-Type': 'application/json'}
After: {'Accept': 'application/vnd.github+json', 'Authorization': 'Bearer abc', 'Content-Length': '24', 'Content-Type': 'application/json'}

I'm 95% sure the UA missing gets saved by urllib3 later on and that it'll work anyway (github requires a UA) but it's a very weird behavior and I don't want to have to debug something 6 months from now because the UA changes depending on request order

@shtrom shtrom Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And secondly, this overrides all headers instead of updating them. So we lose default headers (notably Accept-Encoding and User-Agent).

Yep, a dict update would be better.

It's not in the right place. It should be in _get_gql_session

I started moving this, but I have misgivings: _get_requests_session is the method that specifically reaches deep into the session to get the transport.session, which happens to not be exactly as we'd expect (which is not contractually guaranteed by gql graphql-python/gql#613 (comment)). I think it makes more sense to be fixing the session in _get_requests_session rather than in _get_gql_session (note that they are note the same “session”), though perhaps it's not desirable to morph it for everything, and instead we should copy it before updating the headers.

@shtrom shtrom Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I just found this part of a test https://github.com/shtrom/simple-github/blob/9f64417ca1f20aed1861cc58083652a07cf96ef4/test/test_client.py#L117-L120 that seems to have the same intent as the ones I added... but it doesn't check the headers of the returned session, and instead check the transport (that I suggest we update from).

Perhaps this test can be updated instead to test the session?

OTOH, I found this test when I started deepcopying the session, which broke the equality assertions here.

I'm not sure what the best approach would be here:

  1. don't deepcopy.
  2. remove equality assertions, and ...
  3. ... potentially cache the returned session.

In favour of 1, I'm not even sure we can deepcopy the requests.Session safely.

WDYT?

@shtrom shtrom Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, if we update without a deepcopy, we might as well do it ASAP, so in _get_gql_session as you suggest. But we're essentially fiddling with gql's (semi) internals.

return session.transport.session

def _get_retry_session(self) -> Session:
Expand Down
51 changes: 46 additions & 5 deletions test/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import pytest
import pytest_asyncio
import requests
from aiohttp import ClientResponseError
from gql import Client as GqlClient
from gql.client import ReconnectingAsyncClientSession, SyncClientSession
Expand Down Expand Up @@ -168,21 +169,25 @@ async def test_async_client_rest(aioresponses, async_client):
resp = await client.get("/octocat")
result = await resp.json()
assert result == {"answer": 42}
assert_correct_aiorequest_headers(aioresponses, url, "GET")

aioresponses.post(url, status=200, payload={"answer": 42})
resp = await client.post("/octocat", data={"foo": "bar"})
result = await resp.json()
assert result == {"answer": 42}
assert_correct_aiorequest_headers(aioresponses, url, "POST")

aioresponses.put(url, status=200, payload={"answer": 42})
resp = await client.put("/octocat", data={"foo": "bar"})
result = await resp.json()
assert result == {"answer": 42}
assert_correct_aiorequest_headers(aioresponses, url, "PUT")

aioresponses.patch(url, status=200, payload={"answer": 42})
resp = await client.patch("/octocat", data={"foo": "bar"})
result = await resp.json()
assert result == {"answer": 42}
assert_correct_aiorequest_headers(aioresponses, url, "PATCH")

aioresponses.delete(url, status=200)
await client.delete("/octocat")
Expand All @@ -198,13 +203,26 @@ async def test_async_client_rest(aioresponses, async_client):
# internal aiohttp-retry tracking data
trace_request_ctx=mock.ANY,
)
assert_correct_aiorequest_headers(aioresponses, url, "DELETE")

aioresponses.get(url, status=401)
with pytest.raises(ClientResponseError):
resp = await client.get("/octocat")
resp.raise_for_status()


def assert_correct_aiorequest_headers(aioresponses, url: str, method: str = "GET"):
aioresponses.assert_called_with(
url,
method=method,
args_to_match=["headers"],
headers={
"Accept": "application/vnd.github+json",
"Authorization": "Bearer abc",
},
)


@pytest.mark.asyncio
async def test_async_client_retries_on_5xx(aioresponses, async_client):
client = async_client
Expand All @@ -222,39 +240,62 @@ def test_sync_client_rest(responses, sync_client):
client = sync_client
url = f"{GITHUB_API_ENDPOINT}/octocat"

responses.get(url, status=200, json={"answer": 42})
get_mock = responses.get(url, status=200, json={"answer": 42})
resp = client.get("/octocat")
result = resp.json()
assert result == {"answer": 42}
assert_correct_request_headers(get_mock.calls[0].request)

responses.post(url, status=200, json={"answer": 42})
post_mock = responses.post(url, status=200, json={"answer": 42})
resp = client.post("/octocat", data={"foo": "bar"})
result = resp.json()
assert result == {"answer": 42}
assert_correct_request_headers(post_mock.calls[0].request)

responses.put(url, status=200, json={"answer": 42})
put_mock = responses.put(url, status=200, json={"answer": 42})
resp = client.put("/octocat", data={"foo": "bar"})
result = resp.json()
assert result == {"answer": 42}
assert_correct_request_headers(put_mock.calls[0].request)

responses.patch(url, status=200, json={"answer": 42})
patch_mock = responses.patch(url, status=200, json={"answer": 42})
resp = client.patch("/octocat", data={"foo": "bar"})
result = resp.json()
assert result == {"answer": 42}
assert_correct_request_headers(patch_mock.calls[0].request)

responses.delete(url, status=200)
delete_mock = responses.delete(url, status=200)
client.delete("/octocat")
resp = responses.calls[-1].response
assert resp.url == url
assert resp.request.method == "DELETE"
assert resp.status_code == 200
assert_correct_request_headers(delete_mock.calls[0].request)

responses.get(url, status=401)
with pytest.raises(HTTPError):
resp = client.get("/octocat")
resp.raise_for_status()


def assert_correct_request_headers(request: requests.Request):
# We want to retain the original request headers.
default_request_headers = requests.Session().headers
for hdr, val in default_request_headers.items():
if hdr.lower() in ["accept", "authorization"]:
continue
assert (
request.headers[hdr] == val
), "Incorrectly inherited header from the original requests session"

assert (
request.headers["accept"] == "application/vnd.github+json"
), "Incorrect Accept in request to GitHub REST API"
assert (
request.headers["authorization"] == "Bearer abc"
), "Incorrect Authorization in request to GitHub REST API"


def test_sync_client_retries_on_5xx(responses, sync_client):
client = sync_client
url = f"{GITHUB_API_ENDPOINT}/octocat"
Expand Down