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.

Sign up for GitHub

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

Jump to bottom

GET media/config #1189

Merged
merged 18 commits into from
Jul 6, 2018
Merged

GET media/config #1189

Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2783820
Initial draft of GET media/limits
Half-Shot May 3, 2018
c9abf36
size > upload_size
Half-Shot May 3, 2018
de6fe1b
Add note about auth
Half-Shot May 3, 2018
ffc8ee2
a upload > an upload
Half-Shot May 8, 2018
077cd04
/limits => /config
Half-Shot Jun 20, 2018
6b9640b
upload_size => m.upload.size
Half-Shot Jun 20, 2018
6d0a56d
Suggestions by Travis tend to be sensible ones
Half-Shot Jun 22, 2018
ddc1523
Extending words and rate limiting
Half-Shot Jun 27, 2018
80935ea
Update content-repo.yaml
Half-Shot Jun 27, 2018
eeaf438
Speling is my weakniss
Half-Shot Jun 28, 2018
091b2a6
Hard limit on requiring accessToken. Rephrased some sections
Half-Shot Jun 28, 2018
1820df0
media => content repository
Half-Shot Jun 28, 2018
21e957e
Better note for /config
Half-Shot Jun 28, 2018
41c18be
+repository
Half-Shot Jun 28, 2018
a1309d6
Remove "Clients should handle gracefully" sentence
Half-Shot Jun 29, 2018
d5ce87e
Split summary up into description and summary
Half-Shot Jul 6, 2018
2e6cc80
Better indent of description
Half-Shot Jul 6, 2018
77f4ac5
Wordy intro sentence for description
Half-Shot Jul 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions api/client-server/content-repo.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,3 +269,41 @@ paths:
"$ref": "definitions/error.yaml"
tags:
- Media
"/config":
get:
summary: Get the configuration for the media repository.
Clients SHOULD use this as a guide when uploading content.
Copy link
Member

Choose a reason for hiding this comment

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

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. :) Others: does this look OK?



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.
Copy link
Member

Choose a reason for hiding this comment

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

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

operationId: getConfig
produces: ["application/json"]
security:
- accessToken: []
responses:
Copy link
Member

Choose a reason for hiding this comment

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

a 429 should be added to indicate rate limiting:

         429:
           description: This request was rate-limited.
           schema:
             "$ref": "definitions/error.yaml"

200:
description: The public content repository configuration for the matrix server.
schema:
type: object
properties:
m.upload.size:
type: number
description: |-
The maximum size an upload can be in bytes. If not listed or null,
the upload limit should be treated as unknown.
examples:
application/json: {
"m.upload.size": 50000000
}
429:
description: This request was rate-limited.
schema:
"$ref": "definitions/error.yaml"

tags:
- Media