Add config option for setting homeserver's default room version #5223

anoadragon453 · 2019-05-21T15:39:12Z

Replaces DEFAULT_ROOM_VERSION constant with a method that first checks the config, then returns a hardcoded value if the option is not present.

That hardcoded value is now located in the new room.py config file.

richvdh · 2019-05-21T15:46:34Z

synapse/api/room_versions.py

+# the version we will give rooms which are created on this server
+# can be overridden by the config
+def get_default_room_version(config):
+    return KNOWN_ROOM_VERSIONS[config.default_room_version]


can we do this lookup once, when we first read the config?

richvdh · 2019-05-21T15:47:12Z

synapse/config/room.py

+from ._base import Config, ConfigError
+
+
+class RoomConfig(Config):


not sure if I'd bother with a separate config class... just chuck it in ServerConfig along with everything else.

codecov · 2019-05-21T15:54:33Z

Codecov Report

Merging #5223 into develop will increase coverage by <.01%.
The diff coverage is 84.61%.

@@             Coverage Diff             @@
##           develop    #5223      +/-   ##
===========================================
+ Coverage    62.49%   62.49%   +<.01%     
===========================================
  Files          338      338              
  Lines        35064    35067       +3     
  Branches      5738     5738              
===========================================
+ Hits         21913    21915       +2     
  Misses       11604    11604              
- Partials      1547     1548       +1

richvdh · 2019-05-22T08:50:50Z

synapse/config/server.py

@@ -88,6 +91,19 @@ def read_config(self, config):
            "restrict_public_rooms_to_local_users", False,
        )

+        self.default_room_version = config.get(


can you use a local variable for this rather than abuse the member field for a temporary var?

richvdh · 2019-05-22T08:51:33Z

synapse/config/server.py

+            "default_room_version", DEFAULT_ROOM_VERSION,
+        )
+
+        if self.default_room_version not in KNOWN_ROOM_VERSIONS:


people will forget to put quotes around the field in the yaml. can you convert to str before doing the lookup?

richvdh · 2019-05-22T08:53:17Z

synapse/config/server.py

+        #
+        # For example, for room version 1, default_room_version should be set
+        # to "1".
+        default_room_version: %(DEFAULT_ROOM_VERSION)s


blank (or rather, #-only) line missing here

suggest commenting this out by default, because (a) that's the convention (b) it means that we can change the default for people who don't deliberately override it.

richvdh · 2019-05-22T08:53:50Z

synapse/config/server.py

+        # Known room versions are listed here:
+        # https://matrix.org/docs/spec/#complete-list-of-room-versions
+        #
+        # For example, for room version 1, default_room_version should be set


Never hurts to be too user-friendly.

richvdh · 2019-05-22T08:55:00Z

synapse/api/room_versions.py

+
+# the version we will give rooms which are created on this server.
+# can be overridden by the config
+def get_default_room_version(config):


this function is a bit pointless

old

synapse/util/dictutils.py

erikjohnston · 2019-05-23T12:50:18Z

synapse/config/server.py

@@ -582,7 +611,7 @@ def default_config(self, server_name, data_dir_path, **kwargs):
        # Defaults to 'true'.
        #
        #allow_per_room_profiles: false
-        """ % locals()
+        """ % merge_dicts(locals(), globals())


I'm not a fan of this tbh, as globals() can get quite cluttered, and it could easily clobber locals. Can we not just set default_room_version = DEFAULT_ROOM_VERSION or whatever? Or even just have #default_room_version: "<some_room_version>"?

merge_dicts returns a new dict rather than appending to locals().

Can we not just set default_room_version = DEFAULT_ROOM_VERSION or whatever?

Yeah we can just do this I suppose.

merge_dicts returns a new dict rather than appending to locals().

Sure, but the result would produce the value from globals() rather than locals(), which feels surprising. In general I think this has crossed the threshold of "too magic magic".

old

erikjohnston

LGTM, once you delete the new dictutils.py

@dnaf

Synapse 1.0.0rc1 (2019-06-07) ============================= Features -------- - Synapse now more efficiently collates room statistics. ([\#4338](#4338), [\#5260](#5260), [\#5324](#5324)) - Add experimental support for relations (aka reactions and edits). ([\#5220](#5220)) - Ability to configure default room version. ([\#5223](#5223), [\#5249](#5249)) - Allow configuring a range for the account validity startup job. ([\#5276](#5276)) - CAS login will now hit the r0 API, not the deprecated v1 one. ([\#5286](#5286)) - Validate federation server TLS certificates by default (implements [MSC1711](https://github.com/matrix-org/matrix-doc/blob/master/proposals/1711-x509-for-federation.md)). ([\#5359](#5359)) - Update /_matrix/client/versions to reference support for r0.5.0. ([\#5360](#5360)) - Add a script to generate new signing-key files. ([\#5361](#5361)) - Update upgrade and installation guides ahead of 1.0. ([\#5371](#5371)) - Replace the `perspectives` configuration section with `trusted_key_servers`, and make validating the signatures on responses optional (since TLS will do this job for us). ([\#5374](#5374)) - Add ability to perform password reset via email without trusting the identity server. ([\#5377](#5377)) - Set default room version to v4. ([\#5379](#5379)) Bugfixes -------- - Fixes client-server API not sending "m.heroes" to lazy-load /sync requests when a rooms name or its canonical alias are empty. Thanks to @dnaf for this work! ([\#5089](#5089)) - Prevent federation device list updates breaking when processing multiple updates at once. ([\#5156](#5156)) - Fix worker registration bug caused by ClientReaderSlavedStore being unable to see get_profileinfo. ([\#5200](#5200)) - Fix race when backfilling in rooms with worker mode. ([\#5221](#5221)) - Fix appservice timestamp massaging. ([\#5233](#5233)) - Ensure that server_keys fetched via a notary server are correctly signed. ([\#5251](#5251)) - Show the correct error when logging out and access token is missing. ([\#5256](#5256)) - Fix error code when there is an invalid parameter on /_matrix/client/r0/publicRooms ([\#5257](#5257)) - Fix error when downloading thumbnail with missing width/height parameter. ([\#5258](#5258)) - Fix schema update for account validity. ([\#5268](#5268)) - Fix bug where we leaked extremities when we soft failed events, leading to performance degradation. ([\#5274](#5274), [\#5278](#5278), [\#5291](#5291)) - Fix "db txn 'update_presence' from sentinel context" log messages. ([\#5275](#5275)) - Fix dropped logcontexts during high outbound traffic. ([\#5277](#5277)) - Fix a bug where it is not possible to get events in the federation format with the request `GET /_matrix/client/r0/rooms/{roomId}/messages`. ([\#5293](#5293)) - Fix performance problems with the rooms stats background update. ([\#5294](#5294)) - Fix noisy 'no key for server' logs. ([\#5300](#5300)) - Fix bug where a notary server would sometimes forget old keys. ([\#5307](#5307)) - Prevent users from setting huge displaynames and avatar URLs. ([\#5309](#5309)) - Fix handling of failures when processing incoming events where calling `/event_auth` on remote server fails. ([\#5317](#5317)) - Ensure that we have an up-to-date copy of the signing key when validating incoming federation requests. ([\#5321](#5321)) - Fix various problems which made the signing-key notary server time out for some requests. ([\#5333](#5333)) - Fix bug which would make certain operations (such as room joins) block for 20 minutes while attemoting to fetch verification keys. ([\#5334](#5334)) - Fix a bug where we could rapidly mark a server as unreachable even though it was only down for a few minutes. ([\#5335](#5335), [\#5340](#5340)) - Fix a bug where account validity renewal emails could only be sent when email notifs were enabled. ([\#5341](#5341)) - Fix failure when fetching batches of events during backfill, etc. ([\#5342](#5342)) - Add a new room version where the timestamps on events are checked against the validity periods on signing keys. ([\#5348](#5348), [\#5354](#5354)) - Fix room stats and presence background updates to correctly handle missing events. ([\#5352](#5352)) - Include left members in room summaries' heroes. ([\#5355](#5355)) - Fix `federation_custom_ca_list` configuration option. ([\#5362](#5362)) - Fix missing logcontext warnings on shutdown. ([\#5369](#5369)) Improved Documentation ---------------------- - Fix docs on resetting the user directory. ([\#5282](#5282)) - Fix notes about ACME in the MSC1711 faq. ([\#5357](#5357)) Internal Changes ---------------- - Synapse will now serve the experimental "room complexity" API endpoint. ([\#5216](#5216)) - The base classes for the v1 and v2_alpha REST APIs have been unified. ([\#5226](#5226), [\#5328](#5328)) - Simplifications and comments in do_auth. ([\#5227](#5227)) - Remove urllib3 pin as requests 2.22.0 has been released supporting urllib3 1.25.2. ([\#5230](#5230)) - Preparatory work for key-validity features. ([\#5232](#5232), [\#5234](#5234), [\#5235](#5235), [\#5236](#5236), [\#5237](#5237), [\#5244](#5244), [\#5250](#5250), [\#5296](#5296), [\#5299](#5299), [\#5343](#5343), [\#5347](#5347), [\#5356](#5356)) - Specify the type of reCAPTCHA key to use. ([\#5283](#5283)) - Improve sample config for monthly active user blocking. ([\#5284](#5284)) - Remove spurious debug from MatrixFederationHttpClient.get_json. ([\#5287](#5287)) - Improve logging for logcontext leaks. ([\#5288](#5288)) - Clarify that the admin change password API logs the user out. ([\#5303](#5303)) - New installs will now use the v54 full schema, rather than the full schema v14 and applying incremental updates to v54. ([\#5320](#5320)) - Improve docstrings on MatrixFederationClient. ([\#5332](#5332)) - Clean up FederationClient.get_events for clarity. ([\#5344](#5344)) - Various improvements to debug logging. ([\#5353](#5353)) - Don't run CI build checks until sample config check has passed. ([\#5370](#5370)) - Automatically retry buildkite builds (max twice) when an agent is lost. ([\#5380](#5380))

anoadragon453 added 4 commits May 21, 2019 16:38

Add config option for setting homeserver's default room version

407cf07

Add changelog

92c2f73

lint

7efcd3e

lint2

d345b22

richvdh reviewed May 21, 2019

View reviewed changes

Single lookup. Remove separate RoomConfig

78e8e58

anoadragon453 added 3 commits May 21, 2019 17:08

lint and clean

b0daa72

sample config

52f926b

typo

bd6bb52

anoadragon453 requested a review from a team May 21, 2019 16:51

anoadragon453 added 4 commits May 21, 2019 19:57

Single, simple point for default room version change

add1375

Punctuation

4d31165

Fix config file

d56c059

lint

94677c9

richvdh previously requested changes May 22, 2019

View reviewed changes

anoadragon453 added 2 commits May 22, 2019 10:03

Address review comments

a080a3a

Add files

e783ddd

anoadragon453 requested a review from richvdh May 22, 2019 09:32

anoadragon453 commented May 22, 2019

View reviewed changes

synapse/util/dictutils.py Outdated Show resolved Hide resolved

anoadragon453 requested a review from a team May 23, 2019 10:54

erikjohnston previously requested changes May 23, 2019

View reviewed changes

less magic

687501a

anoadragon453 requested a review from erikjohnston May 23, 2019 13:03

erikjohnston approved these changes May 23, 2019

View reviewed changes

bye bye dictutils

a807f7f

anoadragon453 merged commit 6368150 into develop May 23, 2019

anoadragon453 deleted the anoa/default_room_ver_config branch May 23, 2019 14:00

richvdh mentioned this pull request May 24, 2019

Add missing blank line in config #5249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config option for setting homeserver's default room version #5223

Add config option for setting homeserver's default room version #5223

anoadragon453 commented May 21, 2019

richvdh May 21, 2019

richvdh May 21, 2019

codecov bot commented May 21, 2019 •

edited

Loading

richvdh May 22, 2019

richvdh May 22, 2019

richvdh May 22, 2019

richvdh May 22, 2019

richvdh May 22, 2019

anoadragon453 May 22, 2019

richvdh May 22, 2019

erikjohnston May 23, 2019

anoadragon453 May 23, 2019

erikjohnston May 23, 2019

erikjohnston left a comment

		from ._base import Config, ConfigError


		class RoomConfig(Config):

Add config option for setting homeserver's default room version #5223

Add config option for setting homeserver's default room version #5223

Conversation

anoadragon453 commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 21, 2019 • edited Loading

Codecov Report

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

erikjohnston left a comment

Choose a reason for hiding this comment

codecov bot commented May 21, 2019 •

edited

Loading