Add GET media/v1/config by Half-Shot · Pull Request #3184 · matrix-org/synapse

Half-Shot · 2018-05-03T12:57:39Z

Part of matrix-org/matrix-spec-proposals#1189.
Exposes the /media/r0/config endpoint

matrixbot · 2018-05-03T12:57:41Z

Can one of the admins verify this patch?

matrixbot · 2018-05-03T12:57:41Z

Can one of the admins verify this patch?

erikjohnston · 2018-05-04T15:28:13Z

synapse/rest/media/v1/limits_resource.py

+
+
+class MediaLimitsResource(Resource):
+    # isLeaf = True


Why is this commented out?

Oh. I had no idea what it was for and forgot to ask. So errm, what is it for?

https://stackoverflow.com/a/42799031

erikjohnston · 2018-05-04T15:29:30Z

synapse/rest/media/v1/limits_resource.py

+        Resource.__init__(self)
+        self.limits_dict = {}
+        config = hs.get_config()
+        self.limits_dict["upload_size"] = config.max_upload_size


I'd write this as:

def __init__(self, hs): Resource.__init__(self) config = hs.get_config() self.limits_dict = { "upload_size": config.max_upload_size, }

krombel · 2018-05-08T16:22:22Z

synapse/rest/media/v1/limits_resource.py

+        Resource.__init__(self)
+        config = hs.get_config()
+        self.limits_dict = {
+            "upload_size": config.max_upload_size,


You can shorten it by s/config/hs.get_config()/ as you do not store config somewhere.
But that I think that is personal preference...

Eh, the reason for this was to leave it open for more configuration options in the future. Personally I'd be fine with your suggestion if I was only planning to use it once.

But uh, I could go either way with this.

turt2live · 2018-06-20T16:34:52Z

synapse/rest/media/v1/config_resource.py

+        Resource.__init__(self)
+        config = hs.get_config()
+        self.limits_dict = {
+            "upload_size": config.max_upload_size,


m.upload.size

turt2live · 2018-06-20T16:35:42Z

synapse/rest/media/v1/media_repository.py

            self.putChild("preview_url", PreviewUrlResource(
                hs, media_repo, media_repo.media_storage,
            ))
+        self.putChild("limits", MediaLimitsResource(hs))


this is config now

Half-Shot · 2018-07-06T13:28:08Z

Is this a smidge more merge-able now matrix-org/matrix-spec-proposals#1189 is a thing?

concerns addressed

erikjohnston · 2018-07-18T12:11:39Z

synapse/rest/media/v1/config_resource.py

+
+    def render_GET(self, request):
+        respond_with_json(request, 200, self.limits_dict, send_cors=True)
+        return NOT_DONE_YET


Presumably we shouldn't be returning NOT_DONE_YET if we have actually finished the request.

@hawkowl how is this meant to work in twisted?

I'm pretty sure this is correct. Your options for returning from a render_* method are:

return a bytes, which twisted will then render as the body and call request.finish()

return NOT_DONE_YET, which skips the above two.

the implication is that if you have rendered the body and called finish() yourself, you should call NOT_DONE_YET.

Note that this is exactly what happens with a whole bunch of our handlers that look like they are asynchronous, but actually complete synchronously.

HOWEVER. There is a problem here, in that wrap_json_request_handler does important things (1: adds exception handling, though that may be moot here; 2: adds logging). So, can haz wrap_json_request_handler please?

erikjohnston · 2018-07-18T12:12:20Z

Other than weirdness about NOT_DONE_YET, which hopefully hawkowl can give a pointer on, its fine by me.

richvdh · 2018-07-27T08:03:03Z

synapse/rest/media/v1/config_resource.py

+            "m.upload.size": config.max_upload_size,
+        }
+
+    @wrap_json_request_handler


hum; does this actually work?

the docstring on wrap_json_request_handler says:

The handler method must have a signature of "handle_foo(self, request)",
where "self" must have a "clock" attribute (and "request" must be a
SynapseRequest).

I don't think the first is true. You might need a self.clock = hs.get_clock() in the constructor.

richvdh · 2018-07-27T08:13:24Z

retest this please

Half-Shot · 2018-07-27T10:25:04Z

Looks like a 500 on the endpoint. I'll fix.

richvdh · 2018-07-27T11:09:01Z

changelog.d/3184.feature

@@ -0,0 +1 @@
+Add /_media/r0/config


I guess this won't mean much to the average reader of the synapse changelog. You probably need to say it's a new endpoint for clients. A link to the proposal might help.

richvdh · 2018-07-27T11:11:32Z

synapse/rest/media/v1/config_resource.py

+
+    @wrap_json_request_handler
+    def _async_render_GET(self, request):
+        return respond_with_json(request, 200, self.limits_dict)


needs auth?

richvdh · 2018-08-15T18:40:01Z

@matrixbot retest this please

Half-Shot · 2018-08-16T00:27:18Z

@matrixbot retest this please

Half-Shot · 2018-08-16T00:30:46Z

Tests are now passing for this endpoint so I don't know why the other tests are upset.

richvdh · 2018-08-16T12:27:46Z

@matrixbot retest this please

richvdh · 2018-08-16T12:28:00Z

(they were failing due to outdated sytest)

richvdh · 2018-08-16T12:52:43Z

retest this please

SRSLY

Half-Shot · 2018-08-16T13:24:09Z

Thanks!

@vojeroen

Features -------- - Add support for the SNI extension to federation TLS connections. Thanks to @vojeroen! ([\#3439](#3439)) - Add /_media/r0/config ([\#3184](#3184)) - speed up /members API and add `at` and `membership` params as per MSC1227 ([\#3568](#3568)) - implement `summary` block in /sync response as per MSC688 ([\#3574](#3574)) - Add lazy-loading support to /messages as per MSC1227 ([\#3589](#3589)) - Add ability to limit number of monthly active users on the server ([\#3633](#3633)) - Support more federation endpoints on workers ([\#3653](#3653)) - Basic support for room versioning ([\#3654](#3654)) - Ability to disable client/server Synapse via conf toggle ([\#3655](#3655)) - Ability to whitelist specific threepids against monthly active user limiting ([\#3662](#3662)) - Add some metrics for the appservice and federation event sending loops ([\#3664](#3664)) - Where server is disabled, block ability for locked out users to read new messages ([\#3670](#3670)) - set admin uri via config, to be used in error messages where the user should contact the administrator ([\#3687](#3687)) - Synapse's presence functionality can now be disabled with the "use_presence" configuration option. ([\#3694](#3694)) - For resource limit blocked users, prevent writing into rooms ([\#3708](#3708)) Bugfixes -------- - Fix occasional glitches in the synapse_event_persisted_position metric ([\#3658](#3658)) - Fix bug on deleting 3pid when using identity servers that don't support unbind API ([\#3661](#3661)) - Make the tests pass on Twisted < 18.7.0 ([\#3676](#3676)) - Don’t ship recaptcha_ajax.js, use it directly from Google ([\#3677](#3677)) - Fixes test_reap_monthly_active_users so it passes under postgres ([\#3681](#3681)) - Fix mau blocking calulation bug on login ([\#3689](#3689)) - Fix missing yield in synapse.storage.monthly_active_users.initialise_reserved_users ([\#3692](#3692)) - Improve HTTP request logging to include all requests ([\#3700](#3700)) - Avoid timing out requests while we are streaming back the response ([\#3701](#3701)) - Support more federation endpoints on workers ([\#3705](#3705), [\#3713](#3713)) - Fix "Starting db txn 'get_all_updated_receipts' from sentinel context" warning ([\#3710](#3710)) - Fix bug where `state_cache` cache factor ignored environment variables ([\#3719](#3719)) - Fix bug in v0.33.3rc1 which caused infinite loops and OOMs ([\#3723](#3723)) - Fix bug introduced in v0.33.3rc1 which made the ToS give a 500 error ([\#3732](#3732)) Deprecations and Removals ------------------------- - The Shared-Secret registration method of the legacy v1/register REST endpoint has been removed. For a replacement, please see [the admin/register API documentation](https://github.com/matrix-org/synapse/blob/master/docs/admin_api/register_api.rst). ([\#3703](#3703)) Internal Changes ---------------- - The test suite now can run under PostgreSQL. ([\#3423](#3423)) - Refactor HTTP replication endpoints to reduce code duplication ([\#3632](#3632)) - Tests now correctly execute on Python 3. ([\#3647](#3647)) - Sytests can now be run inside a Docker container. ([\#3660](#3660)) - Port over enough to Python 3 to allow the sytests to start. ([\#3668](#3668)) - Update docker base image from alpine 3.7 to 3.8. ([\#3669](#3669)) - Rename synapse.util.async to synapse.util.async_helpers to mitigate async becoming a keyword on Python 3.7. ([\#3678](#3678)) - Synapse's tests are now formatted with the black autoformatter. ([\#3679](#3679)) - Implemented a new testing base class to reduce test boilerplate. ([\#3684](#3684)) - Rename MAU prometheus metrics ([\#3690](#3690)) - add new error type ResourceLimit ([\#3707](#3707)) - Logcontexts for replication command handlers ([\#3709](#3709)) - Update admin register API documentation to reference a real user ID. ([\#3712](#3712))

Add GET media/v1/limits

53385e8

This was referenced May 3, 2018

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

Merged

GET media/config matrix-org/matrix-spec-proposals#1189

Merged

size > upload_size as per spec feedback

3976352

Half-Shot force-pushed the hs/upload-limits branch from 4a70fcb to 3976352 Compare May 3, 2018 16:30

erikjohnston reviewed May 4, 2018

View reviewed changes

Half-Shot added 2 commits May 8, 2018 15:30

Make MediaLimitsResource Leafy

f8b06fc

Restructure __init__

fee0450

krombel reviewed May 8, 2018

View reviewed changes

Half-Shot added 2 commits June 20, 2018 17:25

Update and rename limits_resource.py to config_resource.py

c06b1cb

media limits => config

b1d8777

turt2live suggested changes Jun 20, 2018

View reviewed changes

turt2live previously requested changes Jun 20, 2018

View reviewed changes

Half-Shot added 2 commits June 21, 2018 11:05

upload_size => m.upload.size

1d53129

/limits => /config

0aa14e4

Half-Shot changed the title ~~Add GET media/v1/limits~~ Add GET media/v1/config Jun 22, 2018

Half-Shot mentioned this pull request Jun 23, 2018

Add tests for /_matrix/media/r0/config matrix-org/sytest#456

Closed

Half-Shot assigned erikjohnston Jun 23, 2018

Half-Shot requested a review from a team July 9, 2018 09:12

Half-Shot added 3 commits July 9, 2018 13:31

Merge branch 'develop' into hs/upload-limits

a7f4ebb

Add import for MediaConfigResource

87fe4f2

Create 3184.feature

48251dc

erikjohnston reviewed Jul 18, 2018

View reviewed changes

erikjohnston assigned hawkowl and unassigned erikjohnston Jul 18, 2018

richvdh assigned Half-Shot and unassigned hawkowl Jul 18, 2018

wrapped

3f520b2

richvdh mentioned this pull request Jul 27, 2018

Test hs/upload limits matrix-org/sytest#466

Merged

richvdh reviewed Jul 27, 2018

View reviewed changes

Half-Shot added 2 commits July 27, 2018 12:01

wrap_json_request_handlering

8cc46ed

linting

bf13c18

richvdh reviewed Jul 27, 2018

View reviewed changes

Half-Shot added 3 commits July 27, 2018 12:17

Auth media/r0/config

bf30fd8

Merge remote-tracking branch 'upstream/develop' into hs/upload-limits

46afacc

isorted this mess

0f6e583

richvdh requested a review from a team August 15, 2018 18:40

Merge remote-tracking branch 'upstream/develop' into hs/upload-limits

b3e005d

richvdh merged commit c151b32 into matrix-org:develop Aug 16, 2018

richvdh mentioned this pull request Oct 15, 2018

Add info about the max upload limit when upload fails #4037

Closed

Uh oh!

Conversation

Half-Shot commented May 3, 2018 • edited by richvdh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matrixbot commented May 3, 2018

Uh oh!

matrixbot commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krombel May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Half-Shot commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Jul 27, 2018

Uh oh!

Half-Shot commented Jul 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Aug 15, 2018

Uh oh!

Half-Shot commented Aug 16, 2018

Uh oh!

Half-Shot commented Aug 16, 2018

Uh oh!

richvdh commented Aug 16, 2018

Uh oh!

richvdh commented Aug 16, 2018

Uh oh!

richvdh commented Aug 16, 2018

Uh oh!

Half-Shot commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Half-Shot commented May 3, 2018 •

edited by richvdh

Loading

krombel May 8, 2018 •

edited

Loading