GET media/config #1189

Half-Shot · 2018-05-03T12:49:58Z

Rationale

Often I'll upload things that are too large to the homeserver and then find that it only gets rejected once it has uploaded a sizable portion. This is intensely frustrating when I am on a mobile network or a slow connection.

This PR adds the endpoint for the homeserver to specify limits to uploads. Currently, this is only maximum size, but I would eventually expect this to be adapted to content types, per-user limits or something else.

Clients can then decide whether to attempt an upload.

GDoc https://docs.google.com/document/d/1fI4ZqQcyAyBEPMtS1MCZWpN84kEPdm9SDw6SVZsJvYY/edit?usp=sharing

Collection of PRs

Synapse Add GET media/v1/config synapse#3184
Matrix JS SDK Add getMediaLimits to client matrix-js-sdk#644
Matrix React SDK Check upload limits before trying to upload large files matrix-react-sdk#1876

turt2live · 2018-05-03T15:17:37Z

I'm completely failing to find the issue, but somewhere there was a suggestion to have a generic "server info" endpoint which would include this kind of information. The closest I found is https://github.com/matrix-org/matrix-doc/issues/500 however that doesn't describe the media repo portion.

For the synapse side: it really should be failing fast well before the upload finishes. I know riot-web does some weird things with media (at it's own detriment sometimes), but if the request has a Content-Length, surely synapse can use that value to try and fail fast. Clients should also be recommended to use chunked uploads or similar so the media repo can complain about file sizes.

Half-Shot · 2018-05-03T15:40:15Z

@turt2live That fails part of the issue, but I still think it's crap that then you're playing guessing games with what the maximum file size should be. Why not just declare it to the client.

I'm completely failing to find the issue, but somewhere there was a suggestion to have a generic "server info" endpoint which would include this kind of information. The closest I found is matrix-org/matrix-spec#68 however that doesn't describe the media repo portion.

I thought of it, but I felt like that would bloat quickly and since this is technically a module that you could actually replace with a seperate service, I wouldn't want this to get muddled into a generic server info endpoint. You could easily write your own media server and have it give it's own suggestions about file sizes.
The other intention was to make this endpoint eventually give other kinds of limits which you might want to be visible in the client before the user attempts an upload, for example, if I was writing a document sharing platform I might want to warn the user that we don't accept video/* files. That's kind of irrelevant right now, but I think that's my point made.

What I'm trying to go for is something like:

Half-Shot · 2018-05-03T15:56:57Z

More discussions in https://matrix.to/#/!PxMWbvomtRqGOTsaDK:half-shot.uk/$152536223672YbGdb:half-shot.uk.

Travis suggested that I should use different names for different size restrictions depending on the operation, so adjusting size to be upload_size

maxidorius · 2018-05-03T15:59:38Z

I'm not sure this is a smart idea since the recommended setup is to have a reverse proxy sitting in front of synapse, which also has its own value.

Also, AFAIK, you can fail-fast at the HTTP level to prevent large upload and the current issue is because of Riot, not synapse/reverse proxy?

Half-Shot · 2018-05-03T16:14:48Z

which also has its own value

I can't think of many reasons why you would do that, unless you wanted to give local services more/less limits in which case...you don't care about this API because it's a suggestion rather than enforcement.

Also, AFAIK, you can fail-fast at the HTTP level to prevent large upload and the current issue is because of Riot, not synapse/reverse proxy?

Yep, but that's only fixing one issue. It's still bad UX to fail at all if you can give the client advance warning and more importantly what those limits are. Reason: If my server has a limit of 1MB, and I want to upload an image of 10MB then I have to keep retrying at different sizes until it accepts it. By giving the client a set of limits, you can show in the UI what those limits are.

It also could very easily tie into per-user limits in the future which Travis and I were talking about.

Oh, and I don't remember it ever being the reccomended setup to have different max upload sizes on your reverse proxy and synapse.

maxidorius · 2018-05-03T16:23:46Z

200% agree on the UX and is why I brought up the reverse proxy. If the recommended setup tells you to put one, you can't ignore it as it can to equally bad UX like "you can upload up to 10MB. users attempts to upload 9MB, upload fails as file is too large". But obviously, it can be dealt with later, as this is only about the endpoint.

maxidorius · 2018-05-03T16:24:37Z

actually nevermind, I just realised this is for the spec, not for synapse. forget all my previous comments.

Half-Shot · 2018-05-03T16:26:20Z

:)

turt2live · 2018-05-03T16:30:50Z

to summarize some deliberation in other places (on my part):

This is fine (imo)
There's some desire to have per-user limits (ie: make authentication optional but recommended on this endpoint)
Per-room limits are a later thing and should be easily supported at that point

Half-Shot · 2018-05-03T16:44:48Z

There's some desire to have per-user limits

I'm feeling like for now we can leave this out until we've figured out exactly how you'd implement it. It's not going to be a major change to implement that at a later date

On the other hand I could specify it now, and synapse will only partially support the spec.

Per-room limits are a later thing and should be easily supported at that point

Oh and this will be under it's own /limits/room(s)/@abcdef:example.com later so it can be ignored for now.

turt2live · 2018-05-03T16:50:33Z

I'd certainly appreciate the suggestion for authentication be included in the spec, even if the synapse implementation doesn't care.

dbkr · 2018-05-04T12:33:20Z

api/client-server/content-repo.yaml

+            properties:
+              upload_size:
+                type: number
+                description: "The maximum size a upload can be in bytes."


dbkr · 2018-05-04T12:38:13Z

I'd strongly encourage going through the full spec-proposal process (ie. make a google doc), even for things like this that feels trivial, as they generally turn out to be more complex than you think. Eg. I have questions about the requirement for servers to allow anyone to query their upload limits without auth.

Half-Shot · 2018-05-04T14:18:57Z

Sure thing. Happy to do so, least I have a PoC to point to as well. I'll write one up.

Half-Shot · 2018-05-04T15:13:23Z

GDoc is here https://docs.google.com/document/d/1fI4ZqQcyAyBEPMtS1MCZWpN84kEPdm9SDw6SVZsJvYY/edit?usp=sharing

This is currently an unstable endpoint, but for the purpose of client validation it's been implemented here. Reference issue: matrix-org/matrix-spec-proposals#1189 This is based upon the behaviour described in the Google Doc proposal (see issue) as of writing.

turt2live · 2018-06-20T15:19:49Z

api/client-server/content-repo.yaml

+          schema:
+            type: object
+            properties:
+              upload_size:


proposal calls for m.upload.size btw (same with the example later on)

Half-Shot · 2018-06-21T08:37:57Z

@turt2live @anoadragon453 @uhoreg @ara4n and anyone else, could you give feedback on the PR so we can get it merged :)

anoadragon453 · 2018-06-21T10:40:05Z

Looks fine to me. Presumably #1232 can be closed when this is as well.

dbkr

Also can you rename this PR if it's no longer an initial draft please :)

dbkr · 2018-06-28T15:10:44Z

api/client-server/content-repo.yaml

+
+
+               If an accessToken is supplied, the configuration applied to the authenticated user is returned.
+               Otherwise it should give the configuration applied globally to the server.


This docs says, "Clients MUST request this endpoint with authorisation" so I don't think this matches the doc.

dbkr · 2018-06-28T15:11:54Z

api/client-server/content-repo.yaml

+  "/config":
+    get:
+      summary: Get the configuration for the media repository.
+               Clients SHOULD use this as a guide when uploading content.


This probably needs to clarify what it's talking about now, since this is now a generic /config API.

Edit: in fact this probably just needs to move to m.upload.size below

dbkr · 2018-06-28T15:24:37Z

api/client-server/content-repo.yaml

+               All values are intentionally left optional. Clients SHOULD follow
+               the advice given in the field description when the field is not available.
+
+               **NOTE:** Reverse proxies may apply their own configuration.


I see the intent here but probably only because I have the context of the normal nginx 2MB limit failure. I think this paragraph might be a bit confusing and left-field to others without this context. It could be nice to have a hint here, but I think it would have to be a little more specific and detailed. (As-is it's sort of a non-statement: the Matrix spec doesn't care what your front end proxy may or may not be doing, so long as what comes out of the sausage factory is compliant with the spec).

Also, strictly speaking, this probably ought to have been in the proposal doc if it's part of the proposal.

It's not part of the proposal because it's clearly out of scope, but leaving it out is going to also leave a lot of people confused. I think leaving a note to explain why it might fail on you is reasonable, though it could probably be worded better.

My 2cents: when I read that sentence, I understood that reverse proxy might alter the response to reflect their own limits, if needed. But it seems it's not the case, but rather "the info may not be authoritative"?

That was actually the original intention, but since it wasn't in the spec I guess it was a bit of a sneaky thing to ask for. "the info may not be authoritative" is probably a better thing to say. In most cases, you'd hope people configuring their nginx backends would set their limits up appropriately.

I appreciate you're trying to be very general here but I think you're losing the meaning, ie. it's not clear to me what "applying a configuration" is exactly. My suggestion would be something like:

Both clients and server administrators should be aware that proxies between the client and the server may affect the apparent behaviour of content repository APIs, for example, proxies may enforce a lower upload size limit than is advertised by the server on this endpoint. Clients should handle such situations gracefully.

Yeah, that's fine by me.

Thanks. :) Others: does this look OK?

turt2live · 2018-06-28T16:04:17Z

api/client-server/content-repo.yaml

+  "/config":
+    get:
+      summary: Get the configuration for the media repository.
+               Clients SHOULD use this as a guide when using media endpoints.


s/media/content repository/ please.

I only keep harassing you about this because the spec doesn't reference media in the text, and instead references "content".

Yeah, I thought about this when I wrote it but content repository seems wrong when all our endpoints start with /media. But heyho...

Half-Shot · 2018-06-28T17:00:21Z

@KitsuneRal @mujx want a look?

KitsuneRal · 2018-06-29T02:34:10Z

I'm generally happy with the PR (barring that "gracefully" sentence also discussed in #matrix-spec - I think it confuses more than clarifies so I'd rather just delete it and consider discrepancy between revproxies and homeservers as plain misconfiguration).
I have another concern though: now that we have /config (and anticipating more configuration points inside of it - both for content-repo/media server and for "genuine" homeserver, such TURN server config), I wonder how this will work given that media servers can be separate from homeservers. I can imagine that /config will simply be a union of media- and HS-configuration points if a homeserver also acts as a media server (and as long as we identify m.* configuration points as pertaining to either media servers or HSes but not both, I'm fine). @turt2live, you might want to chime in on that.

turt2live · 2018-06-29T02:39:36Z

The route is under /_matrix/media, so anything that applies to media would show up in this endpoint. More general configuration, such as MOTD and TURN would appear under a different not-yet-introduced endpoint.

KitsuneRal · 2018-06-29T02:42:21Z

Oh, right, I missed that the whole thing is still under media. Then I'd only remove the "gracefully" sentence but even that is not a showstopper for me (given that it's in a note and is on the verge of being non-normative anyway).

Half-Shot · 2018-06-29T08:56:44Z

I'm quite happy to remove that sentence, thanks for the input @KitsuneRal

dbkr · 2018-06-29T16:19:56Z

actually my approval comes with the caveat that you seem to have broken one of the scripts

Half-Shot · 2018-06-29T16:21:18Z

@dbkr Not me 👼 . One of the proposals is breaking the proposal py script.

turt2live · 2018-06-29T16:27:44Z

Looks like somehow the script is getting "documentation_url" instead of an array of issues from github?

turt2live · 2018-07-04T14:05:57Z

@richvdh can I get your seal of approval on this? (you were one of the reviewers for the proposal, and it'd be good to get your feedback)

richvdh

lgtm otherwise

richvdh · 2018-07-06T08:40:23Z

api/client-server/content-repo.yaml

@@ -269,3 +269,41 @@ paths:
            "$ref": "definitions/error.yaml"
      tags:
        - Media
+  "/config":
+    get:
+      summary: Get the configuration for the content repository.


this isn't a summary! It's messing up the formatting in the swagger UI (https://matrix.org/docs/api/client-server/?url=https%3A%2F%2Fmatrix.org%2Fspeculator%2Fspec%2F1189%2Fapi-docs.json#!/Media/getConfig) : please see the notes at https://github.com/matrix-org/matrix-doc/blob/master/meta/documentation_style.rst#openapi.

oh dear, yes. good catch.

richvdh · 2018-07-06T09:32:38Z

api/client-server/content-repo.yaml

-                   All values are intentionally left optional. Clients SHOULD follow
-                   the advice given in the field description when the field is not available.
+      description: |-
+        Clients SHOULD use this as a guide when using content repository endpoints.


generally we start with a wordy description of the endpoint here. "This endpoint allows clients to retrieve information about the media repository."

[which makes it read better in the spec doc]

Initial draft of GET media/limits

2783820

Half-Shot force-pushed the hs/upload-limits branch from 0b2ad58 to 2783820 Compare May 3, 2018 12:54

This was referenced May 3, 2018

Add getMediaLimits to client matrix-org/matrix-js-sdk#644

Merged

Add GET media/v1/config matrix-org/synapse#3184

Merged

Half-Shot mentioned this pull request May 3, 2018

Check upload limits before trying to upload large files matrix-org/matrix-react-sdk#1876

Merged

size > upload_size

c9abf36

Add note about auth

de6fe1b

dbkr reviewed May 4, 2018

View reviewed changes

a upload > an upload

ffc8ee2

benparsons mentioned this pull request May 14, 2018

Media limits API #1232

Closed

/limits => /config

077cd04

turt2live reviewed Jun 20, 2018

View reviewed changes

upload_size => m.upload.size

6b9640b

dbkr requested changes Jun 28, 2018

View reviewed changes

Half-Shot changed the title ~~Initial draft of GET media/limits~~ GET media/config Jun 28, 2018

Hard limit on requiring accessToken. Rephrased some sections

091b2a6

turt2live reviewed Jun 28, 2018

View reviewed changes

Half-Shot added 2 commits June 28, 2018 17:07

media => content repository

1820df0

Better note for /config

21e957e

dbkr approved these changes Jun 28, 2018

View reviewed changes

+repository

41c18be

KitsuneRal approved these changes Jun 29, 2018

View reviewed changes

Remove "Clients should handle gracefully" sentence

a1309d6

dbkr approved these changes Jun 29, 2018

View reviewed changes

uhoreg approved these changes Jun 29, 2018

View reviewed changes

turt2live approved these changes Jul 4, 2018

View reviewed changes

richvdh approved these changes Jul 6, 2018

View reviewed changes

Half-Shot added 2 commits July 6, 2018 10:27

Split summary up into description and summary

d5ce87e

Better indent of description

2e6cc80

richvdh reviewed Jul 6, 2018

View reviewed changes

Wordy intro sentence for description

77f4ac5

maxidorius approved these changes Jul 6, 2018

View reviewed changes

Half-Shot merged commit ff3c379 into matrix-org:master Jul 6, 2018

richvdh mentioned this pull request Oct 15, 2018

Add info about the max upload limit when upload fails matrix-org/synapse#4037

Closed



		If an accessToken is supplied, the configuration applied to the authenticated user is returned.
		Otherwise it should give the configuration applied globally to the server.

GET media/config #1189

GET media/config #1189

Conversation

Half-Shot commented May 3, 2018 • edited Loading

Rationale

Collection of PRs

turt2live commented May 3, 2018

Half-Shot commented May 3, 2018 • edited Loading

Half-Shot commented May 3, 2018

maxidorius commented May 3, 2018

Half-Shot commented May 3, 2018 • edited Loading

maxidorius commented May 3, 2018

maxidorius commented May 3, 2018

Half-Shot commented May 3, 2018

turt2live commented May 3, 2018

Half-Shot commented May 3, 2018 • edited Loading

turt2live commented May 3, 2018

Choose a reason for hiding this comment

dbkr commented May 4, 2018 • edited Loading

Half-Shot commented May 4, 2018

Half-Shot commented May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Half-Shot commented Jun 21, 2018

anoadragon453 commented Jun 21, 2018

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Half-Shot commented Jun 28, 2018

KitsuneRal commented Jun 29, 2018

turt2live commented Jun 29, 2018

KitsuneRal commented Jun 29, 2018

Half-Shot commented Jun 29, 2018

dbkr commented Jun 29, 2018

Half-Shot commented Jun 29, 2018 • edited Loading

turt2live commented Jun 29, 2018

turt2live commented Jul 4, 2018

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Half-Shot commented May 3, 2018 •

edited

Loading

Half-Shot commented May 3, 2018 •

edited

Loading

Half-Shot commented May 3, 2018 •

edited

Loading

Half-Shot commented May 3, 2018 •

edited

Loading

dbkr commented May 4, 2018 •

edited

Loading

Half-Shot commented May 4, 2018 •

edited

Loading

Half-Shot commented Jun 29, 2018 •

edited

Loading