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

Open
wants to merge 6 commits into
base: master
from
Open

Conversation

@uhoreg
Copy link
Member

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

This comment has been minimized.

Copy link
@uhoreg

uhoreg Nov 29, 2019

Author Member

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

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Dec 3, 2019

Member

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?

This comment has been minimized.

Copy link
@uhoreg

uhoreg Dec 4, 2019

Author Member

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.

This comment has been minimized.

Copy link
@turt2live

turt2live Dec 4, 2019

Member

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

This comment has been minimized.

* 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.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Nov 29, 2019

Author Member

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 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.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Nov 29, 2019

Author Member

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

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Dec 3, 2019

Member

Indeed, another MSC time? :)

@turt2live turt2live changed the title MSC 2370: Resolve URL API MSC2370: Resolve URL API Nov 29, 2019
@turt2live turt2live self-requested a review Nov 29, 2019
Copy link
Member

anoadragon453 left a comment

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

This comment has been minimized.

Copy link
Member

turt2live commented Dec 7, 2019

@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

This comment has been minimized.

Copy link
@NotAFile

NotAFile Dec 7, 2019

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.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Dec 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@NotAFile

NotAFile Dec 9, 2019

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.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Dec 9, 2019

Author Member

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?
@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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.