Skip to content
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

MSC3870: Async media upload extension: upload to URL #3870

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Fizzadar
Copy link
Contributor

@Fizzadar Fizzadar commented Aug 17, 2022

Rendered.

Extension that builds on MSC2246.

Signed off by Nick @ Beeper (@Fizzadar) [email protected].

@Fizzadar Fizzadar changed the title MSCXXXX: Media Async Upload URLs MSC3870: Media Async Upload URLs Aug 17, 2022
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Aug 17, 2022
@Fizzadar Fizzadar changed the title MSC3870: Media Async Upload URLs MSC3870: Async media upload extension: upload to URL Aug 17, 2022
{
"content_uri": "mxc://example.com/AQwafuaFswefuhsfAFAgsw",
"unused_expires_at": 1647257217083,
"upload_url": "https://cdn.example.com/media-repo/upload/XAPw4CtrzArk?signed=h4tGOHvCu"
Copy link
Member

Choose a reason for hiding this comment

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

What HTTP method does the client use to upload the file? POST? PUT? Or do you need another parameter to specify the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified in ba15fa9 (PUT, for now).

Copy link
Member

Choose a reason for hiding this comment

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

We can probably encode it into the field here, making it generic against providers.

Suggested change
"upload_url": "https://cdn.example.com/media-repo/upload/XAPw4CtrzArk?signed=h4tGOHvCu"
"upload_location": "PUT https://cdn.example.com/media-repo/upload/XAPw4CtrzArk?signed=h4tGOHvCu"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should use a separate field here just to be explicit, upload_method, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

proposals/3870-media-async-upload-url.md Outdated Show resolved Hide resolved
Comment on lines +22 to +24
The proposal modifies the MSC2246 create endpoint to optionally include a URL that a compatible
client may use to upload the content for the created MXID. This is considered optional and the
client may continue to upload via the new upload endpoint defined in MSC2246.
Copy link
Member

Choose a reason for hiding this comment

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

Clients should be required to use the URL when the server generates one, I think. The server is almost certainly trying to shed bandwidth load when it generates one of these URLs, and it's already gone through the effort to allocate a storage location for the client. That's a lot of wasted effort/cash if the client ignores the field.

For MSC purposes, making it required I think means:

  • New endpoint version for /create, to encourage clients to opt-in to the field's presence
  • Deprecating the old endpoint version, further pushing clients towards the new one
  • On PUT /upload, an error code to denote that uploads aren't welcome for that URI. This does not require a new endpoint version because the client already opted-in to using the new upload URL-supported endpoint earlier in the process.

Choose a reason for hiding this comment

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

Wouldn't clients still be able to avoid using the CDN by using the old synchronous upload endpoint?

If we're going through the effort to insist that clients use the upload_url, should we also start making plans tokill the ability to do synchrous uploads direct to the media server at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should leave this optional for now, and defer deprecation or blocking of direct upload to a later MSC. Allowing both in theory makes switching over to the upload URLs easier for client dev, and then we avoid all introducing additional complexity as part of this MSC.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't clients still be able to avoid using the CDN by using the old synchronous upload endpoint?

If we're going through the effort to insist that clients use the upload_url, should we also start making plans tokill the ability to do synchrous uploads direct to the media server at all?

The proposed plan deprecates the endpoint, making it eligible for removal in a later version of the spec.

I think we should leave this optional for now, and defer deprecation or blocking of direct upload to a later MSC. Allowing both in theory makes switching over to the upload URLs easier for client dev, and then we avoid all introducing additional complexity as part of this MSC.

Clients can still use deprecated endpoints, but without a reason to change there's not much motivation for them to implement this. I do feel quite strongly that if the server provides a URL, the client must use that URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed up fixes for the other things. I think deferring the new endpoint version and requiring clients use the URL provided is the right call - it'd be a quick follow up MSC, or might tie in with replacement plans for the old upload endpoints. Either way I don't see any real benefit to bundling it into this MSC.

client may use to upload the content for the created MXID. This is considered optional and the
client may continue to upload via the new upload endpoint defined in MSC2246.

`POST /_matrix/media/v3/create`
Copy link
Member

Choose a reason for hiding this comment

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

If keeping the optional behaviour:

Suggested change
`POST /_matrix/media/v3/create`
`POST /_matrix/media/v1/create`

If making it mandatory per https://github.com/matrix-org/matrix-spec-proposals/pull/3870/files#r1488692873 :

Suggested change
`POST /_matrix/media/v3/create`
`POST /_matrix/media/v2/create`

(the v3 endpoint has never existed in the spec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
"content_uri": "mxc://example.com/AQwafuaFswefuhsfAFAgsw",
"unused_expires_at": 1647257217083,
"upload_url": "https://cdn.example.com/media-repo/upload/XAPw4CtrzArk?signed=h4tGOHvCu"
Copy link
Member

Choose a reason for hiding this comment

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

I assume the URL expires at the same time as unused_expires_at - just need to document that in the MSC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fizzadar
Copy link
Contributor Author

Re: implementation: Beeper's custom HS/media-repo both already support this as well as all our clients. I don't recall if this is sufficient though?

@Fizzadar Fizzadar marked this pull request as ready for review August 10, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants