-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
Fix: Regression due to httpx updates #823
Conversation
This was overdue for a cleanup. Thanks. Looks good once CI is happy. |
There are still a few bits that need to be cleaned up (CICD goes into an infinite loop now...) -- working on it. Perhaps this is a reintroduced bug in httpx, but probably still good to systematize our use of query params. Looks like they are working on it: encode/httpx#3433 |
content = handle_error( | ||
self.context.http_client.get( | ||
self.item["links"]["block"], | ||
url_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that in current httpx any query params here are ignored in params
is passed.
In [6]: httpx.get("http://example.com?a=1&b=3", params={'a': 2, 'c': 3}).request.url
Out[6]: URL('http://example.com?a=2&c=3')
In [7]: httpx.get("http://example.com?a=1&b=3").request.url
Out[7]: URL('http://example.com?a=1&b=3')
Updates of
httpx
to version 0.28.0 break the existing pagination handling code (and likely other functionality too) where a url is constructed using query parameters that are passed as both, explicitly as part of the url and as kwargs to thehttpx.client
. This patch fixes this by refactoring the query parameters only as kwargs.Furthermore,
httpx.AsyncClient
no longer acceptsapp
as a kwarg; instead, the application needs to be wrapped into anASGITransport
object explicitly before being passed it toAsyncClient
.This behavior has been observed when running a CI/CD pipeline in the main Bluesky repo. After the fix, the previously failing tests have passed (in a local environment).
Checklist
Add the ticket number which this PR closes to the comment section