Request & follow redirects for /media/v3/download #16701
Conversation
| @@ -714,6 +720,26 @@ async def _send_request( | |||
| response.code, | |||
| response_phrase, | |||
| ) | |||
| elif ( | |||
| response.code in (307, 308) | |||
There was a problem hiding this comment.
I thought about wrapping the agent in a RedirectAgent, but that handles additional codes sadly.
e12d40a
to
d98b560
Compare
| _generate_uri: bool = True | ||
| """True to automatically generate the uri field based on the above information. | ||
| Set to False if manually configuring the URI. | ||
| """ |
There was a problem hiding this comment.
I couldn't figure out a better way to force attrs to do what I want. Using a default/factory works if you're either just creating the instance or evolving it and updating the URI, but not when evolving it and wanting to generate the URI again. I figured being explicit was best.
d98b560
to
8837c38
Compare
| def write_err(f: Failure) -> Failure: | ||
| f.trap(HttpResponseException) | ||
| output_stream.write(f.value.response) | ||
| return f |
There was a problem hiding this comment.
This one was new to me: https://docs.twisted.org/en/stable/api/twisted.python.failure.Failure.html#trap
TL;DR the trap call is a no-op if f contains an HTTPResponseException; otherwise the trap raises immediately so that the next errback can handle this Failure.
There was a problem hiding this comment.
I'd missed that this was in test code though. Why do we need to add this as an errback all of a sudden?
There was a problem hiding this comment.
We need to add it because we know call errback sometimes on the list of Deferreds so that we can resolve a request with an error instead of a response.
There was a problem hiding this comment.
Ahhh, I think I see: we never called errback on the mock until now!
| new_uri = urllib.parse.urljoin(request.uri, location) | ||
|
|
||
| return await self._send_request( | ||
| attr.evolve(request, uri=new_uri, generate_uri=False), |
There was a problem hiding this comment.
it's spelled _generate_uri in the struct definition. Is this some attrs way of declaring a private attribute that you can initialise via the constructor?
There was a problem hiding this comment.
Is this some attrs way of declaring a private attribute that you can initialise via the constructor?
Yes, pretty much. attrs strips the preceding underscores to let you have 'private' attributes that can be initialized.
There was a problem hiding this comment.
Co-authored-by: David Robertson <[email protected]>
|
Something like matrix-org/synapse-s3-storage-provider#107 might be useful if you're interested in supporting this for the Client-Server API. Implementing it in Synapse proper seems out of scope? |
This has two mostly unrelated changes to it:
/media/v3/downloadover federation (falling back to/media/r0/download)v3endpoint, addallow_redirectsfrom MSC3860I tested this by setting up a server locally and doing:
Which had the following logs:
You can see it attempts to fetch the media from Beeper's homeserver, gets a redirect to a CDN and then fetches it from there instead.
In order to accomplish this I reworked the fetching of downloaded media to be more similar to other federation requests. You can compare the solid to the dotted line. The main goal of this was to add the fallback from v3 -> r0.
flowchart A[MediaRepository._download_remote_file] B[FederationClient.download_media] C1[TransportLayerClient.download_media_r0] C2[TransportLayerClient.download_media_v3] D[MatrixFederationHttpClient.get_file] A --> D A -.-> B -.-> C2 -.-> D B -.-> C1 -.-> DFixes #15196, part of #15661.