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

[WIP] MSC2370: Resolve URL API #2370

Draft
wants to merge 6 commits into
base: old_master
Choose a base branch
from

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Nov 29, 2019

This is a markdown-ification of #1270, with some clarifications and expansions added.

Rendered: https://github.com/uhoreg/matrix-doc/blob/resolve_url/proposals/2370-resolve-url.md

@uhoreg uhoreg changed the title MSC xxxx: Resolve URL API MSC 2370: Resolve URL API Nov 29, 2019

Request:
```
POST /_matrix/media/v1/resolve_url
Copy link
Member Author

Choose a reason for hiding this comment

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

Would this be better as a GET request (like GET /_matrix/media/r0/preview_url )?

Copy link
Member

Choose a reason for hiding this comment

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

Like GET /_matrix/media/v1/resolve_url?url=https://blabla.com? I was going to say it's annoying to have to URL-encode a URL, but seeing as /preview_url already requires this then it's probably fine.

Also why v1 whereas /preview_url is r0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also why v1 whereas /preview_url is r0?

Indeed. I copied this from the original doc, but the rest of /media uses r0, so this should too. Fixed.

As far as GET vs POST, I suppose it could be argued that since it causes something to be stored on the server, it should be a POST, whereas /preview_url does not cause anything to be stored on the server (beyond possible caching), so it's fine as a GET. I don't really have strong opinions either way, so if anyone does have opinions, please say so.

Copy link
Member

Choose a reason for hiding this comment

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

Annoyingly media is weird. It's still in the v1 version.

Copy link
Member Author

Choose a reason for hiding this comment

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

* If the resource cannot be downloaded because it has been blacklisted, it should
respond with an `M_FORBIDDEN` error and HTTP code 403.
* If the resource is larger than the homeserver's upload size limit, it should
respond with an `M_TOO_LARGE` error and HTTP code 400.
Copy link
Member Author

Choose a reason for hiding this comment

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

The original proposal listed this an HTTP code 413. However it seems like 413 is for when the request payload is too large, rather than for when a referenced resource is too large, so I think that just a generic 400 would be better here.

@uhoreg uhoreg added the proposal A matrix spec change proposal label Nov 29, 2019
* If the resource cannot be downloaded due to the URL returning another error,
the endpoint should respond with an `M_UNKNOWN` error and HTTP code 400.
* If the resource cannot be downloaded because it has been blacklisted, it should
respond with an `M_FORBIDDEN` error and HTTP code 403.
Copy link
Member Author

Choose a reason for hiding this comment

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

For reference, synapse returns M_UNKNOWN with HTTP code 403 on /preview_url, but using M_FORBIDDEN seems more useful.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, another MSC time? :)

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

I was dumb and didn't put my above comments in a review. So here's one to track my current thoughts on the doc.

@turt2live
Copy link
Member

@NotAFile please use a comment on the diff so others can reply more easily, even if it means putting the comment onto the title.

(This document is heavily based on
https://docs.google.com/document/d/1bbX1yxNETmMa-AxBGjIpb4lNoTuc3vjGXmbZWrNBlzM/edit)

Users sometimes want to upload images to Matrix rooms that they see on other
Copy link

@NotAFile NotAFile Dec 7, 2019

Choose a reason for hiding this comment

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

I don't quite understand why this is necessary. The URL preview API already returns images for links in messages, which riot displays, albeit smaller than full images:

image

The way some platforms like e.g. Telegram do this is to show message that contain image links with a preview that is very similar in size to posting a full image:

image

So the data is basically already there, Riot just needs to display image URL previews as big as images.

Doing this over the approach in this MSC has a few advantages in my opinion:

  1. No new API required. No additional Complexity is needed for servers and clients to implement.
  2. Saves storage Space. While the server still needs to proxy the image for privacy reasons, the server does not need to store the image forever. It can cache it short term and re-download it again later on demand.
  3. It requires no changes on the sending clients side, so it instantly works for everyone's messages. It also works with e.g. irc rooms, and in any message history predating this feature.
  4. It preserves the context that this is an image that's not created by the user, but downloaded from some URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

The semantics are different. If you post a URL, then you are dependent on that server hosting the image. Which means:

  • if the server goes down, then you no longer have access to the image
  • if the image on the server changes, then people will see a different image. This may or may not be what you want, depending on context.

Yes, pasting a URL and relying on URL preview may work in some cases, but may not work in others. For those cases where pasting a URL works, people are still free to use that.

Copy link

@NotAFile NotAFile Dec 9, 2019

Choose a reason for hiding this comment

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

Well, I think those objections are correct, but they exist identically for all URL previews, not just specifically sending images.

So, if those semantics are desired, why not still just reuse the URL preview API but query it before sending? URL previews obviously already support returning images.

So I understand why you'd want to get the image on the sender side now, but still not why we need a new, more limited version of an existing API for this functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you just use the URL preview API:

  • you don't know if the "preview" is exactly the same image, or a scaled/stripped version
  • you have to download and re-upload the image, rather than just have the server download the image and keep it
  • URL preview works that way for images, but what about audio or video files?

Copy link

Choose a reason for hiding this comment

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

URL preview works that way for images, but what about audio or video files?

So, my suggestion is to expand it to audio and video files, as Telegram does.

The reuploading point is valid though. Especially so for bridging.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, my suggestion is to expand it to audio and video files, as Telegram does.

"Previewing" a video file is semantically different from serving a copy of the file. A preview of a video file could legitimately be a single frame of the video, which would be different from what this MSC proposes. Similarly, a preview of a PDF would probably be a low-resolution image of the first page of the PDF. And, as I mentioned, a preview for an image file could be a scaled-down version of the image, rather than a copy of the original image.

Basically, this API is intended for a different use from the preview API.

@turt2live turt2live changed the title MSC2370: Resolve URL API [WIP] MSC2370: Resolve URL API Dec 7, 2019
proposals/2370-resolve-url.md Outdated Show resolved Hide resolved
Co-Authored-By: Travis Ralston <travpc@gmail.com>

Additionally, servers implementing this feature should advertise that they do
so by exposing a `org.matrix.resolve_url` flag in the `unstable_features` part
of the `/versions` response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative suggestion: Somehow make like mxc://proxy/<url here> things that will proxy that image, then. That way you can just url-encode the image / file you want an mxc link for and already have it. The media repo can handle something like caching, then, if wanted.

This would actually make bridging easier as you might end up with a lot of image URLs you want to display for just one image in an <img> tag, and that way you don't clutter your servers disk.

@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
@turt2live turt2live marked this pull request as draft April 8, 2021 23:36
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants