yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
MIT License
158 stars 21 forks source link

Underlying HTTPX clients are shared among several GitHub instances #79

Closed frankie567 closed 6 months ago

frankie567 commented 6 months ago

I discovered a strange behavior while testing out version v0.11.0a2. It seems that if you create several GitHub client in the same process/thread, they reuse the underlying HTTPX client, including the auth flow.

It's a problem if you need to have several clients with different kind of authentications running in your process.

Reproducible example

from githubkit import GitHub, TokenAuthStrategy, UnauthAuthStrategy

def multiple_clients() -> None:
    github_unauth = GitHub(UnauthAuthStrategy())
    response_unauth = github_unauth.rest.rate_limit.get()
    authorization_unauth = response_unauth.headers.get("authorization")
    print(authorization_unauth)  # None, as expected

    github_token = GitHub(TokenAuthStrategy("MY_TOKEN"))
    response_token = github_token.rest.rate_limit.get()
    authorization_token = response_token.headers.get("authorization")
    print(authorization_token)  # Also None, but should be `token MY TOKEN`

if __name__ == "__main__":
    multiple_clients()

Additional context

I suspect it's because ContextVar is used to retain the HTTPX client:

https://github.com/yanyongyu/githubkit/blob/8a381cba793b1f0a3d4ea0aee816336f40f7331f/githubkit/core.py#L147-L152


However, what I can't explain is that the same code works correctly in version 0.10.7, while ContextVar were already there. Maybe a side effect of https://github.com/yanyongyu/githubkit/commit/d005552425a75e2312fec5ba77f446fed1063d29?

yanyongyu commented 6 months ago

I can reproduce. I will look into this problem.

yanyongyu commented 6 months ago

The problem also happens when http_cache=False, and the clients used to request api are not the same.

It seems the TokenAuthStrategy auth flow is not called.

yanyongyu commented 6 months ago

it is caused by namespace caching in https://github.com/yanyongyu/githubkit/blob/8a381cba793b1f0a3d4ea0aee816336f40f7331f/githubkit/versions/rest.py#L54-L55

The cache is used without considering the github instance 😂

frankie567 commented 6 months ago

Oh, that's a nasty one! Glad you were able to track it down 😉

yanyongyu commented 6 months ago

It seems the code you provide has some small mistakes. The Authorization header is in the response.raw_request.headers not response.headers.

frankie567 commented 6 months ago

It seems the code you provide has some small mistakes. The Authorization header is in the response.raw_request.headers not response.headers.

Oh yeah, of course. But you got the idea 😅 Glad you were able to solve it, thanks 🙏

yanyongyu commented 6 months ago

I have just released 0.11.0a3, with this issue fixed. 😄