MarshalX / atproto

The AT Protocol (🦋 Bluesky) SDK for Python 🐍
https://atproto.blue
MIT License
289 stars 32 forks source link

get_blob errors on redirects #308

Closed kotx closed 6 months ago

kotx commented 6 months ago

I get this response when calling Client.get_blob:

atproto_client.exceptions.RequestException: Response(success=False, status_code=302, content=XrpcError(error='Redirecting', message='Redirecting to new blob location'), headers={'Date': 'Mon, 18 Mar 2024 04:21:28 GMT', 'Content-Type': 'application/json; charset=utf-8', 'Content-Length': '68', 'Connection': 'keep-alive', 'X-Powered-By': 'Express', 'Access-Control-Allow-Origin': '*', 'RateLimit-Limit': '3000', 'RateLimit-Remaining': '2972', 'RateLimit-Reset': '1710735984', 'RateLimit-Policy': '3000;w=300', 'location': 'https://hydnum.us-west.host.bsky.network/xrpc/com.atproto.sync.getBlob?cid=bafkreihh5vse3zoeob52vwxi3ow7hjqmdc4op5yydojdq5gs3t7fv235ue&did=did%3Aplc%3A6czns74qazqae4qcnmr36675', 'ETag': 'W/"44-1je7JKzDJZFd5iRtOI+IS+zlOOE"', 'Vary': 'Accept-Encoding'})

This is because _handle_response treats status codes outside the 200-299 range as errors: https://github.com/MarshalX/atproto/blob/56306ad25b1a6a042c67fdf1ae9f6037d28724dc/packages/atproto_client/request.py#L57-L66

We should probably use response.is_error instead, which returns True for 4xx-5xx status codes. Or for a more strict policy, response.is_success or response.is_redirect. But I'm not sure if is_redirect will return true after the client follows a redirect, so that might not be required.

We also need to set follow_redirects=True in httpx.Client.get or when constructing the httpx.Client.

But if you intended for follow_redirects=True to be passed in the **kwargs of get_blob, that would be fine too.

MarshalX commented 6 months ago

good catch! thank you! i didnt notice that httpx changed default behaviour for redirects following. we trust in server implementation, we trust to our client implementation. so i just enabled redirects which fixes the problem. wanna mention that redirect are still disabled in identity resolvers. let's keep it simple

MarshalX commented 6 months ago

fixed in v0.0.46