Move experimental features MSC3026, MSC3881, and MSC3967 to per-user flags by H-Shay · Pull Request #15345 · matrix-org/synapse

H-Shay · 2023-03-28T22:11:09Z

This is a follow up to #15344 (and is based on that branch) which actually implements moving the features from the experimental config to per-user flags.

…eys) from config to per-user flag

…flag

synapse/push/bulk_push_rule_evaluator.py

clokep

Looks reasonable overall. The main thing that scares me a bit is that we don't have a single spot we're checking logic. Let me know if you want to discuss!

changelog.d/15345.feature

synapse/handlers/presence.py

synapse/rest/client/sync.py

clokep · 2023-05-08T14:34:00Z

synapse/rest/client/versions.py

                    # Supports the busy presence state described in MSC3026.
-                    "org.matrix.msc3026.busy_presence": self.config.experimental.msc3026_enabled,
+                    "org.matrix.msc3026.busy_presence": True,


I don't think this is really accurate -- don't we need to check this based on which user is requesting /versions?

For example, a client might use this to enable UI allowing users to say they're busy. The user would then receive an unexpected error when attempting to use that feature.

(Similar feedback to below for MSC3881)

Hmm I think you're right - it would be True or False based on either the config or whether the feature is enabled in the DB, but how would that be checked here per user? Is it possible to extract the username from the GET request to /versions (my suspicion is no?)

Nevermind I figured it out.

Well I thought I figured it out but since /versions doesn't require authentication there's no access token to extract the user id from. Is there a way to figure out the user id from a request that doesn't require authentication? If not, what's the best way to handle what /versions should return here if we can't determine the user (and thus cannot check if it is enabled per user)?

The only thing I can think of is to make it optionally accept authentication and ask the clients to pretty please change their request code so it sends auth for /versions if they want to use unstable features... :/

Hmm, I wonder if we should start putting unstable features in /capabilities API (which is authed) instead?

Hmm, I wonder if we should start putting unstable features in /capabilities API (which is authed) instead?

I think at some point I was told to prefer /versions over /capabilities, but after some searching I can't seem to find a reference to this.

I can't seem to find a reference to this.

It's in the spec: https://spec.matrix.org/v1.6/client-server-api/#capabilities-negotiation
The relevant bit is:

This system should not be used to advertise unstable or experimental features - this is better done by the /versions endpoint.

Given this it seems like Oliver's suggestion is the way to go? Any other suggestions I should consider?

Given this it seems like Oliver's suggestion is the way to go? Any other suggestions I should consider?

I'd probably double check if clients would be willing to do this first?

MSC4026 was written for this, it has now passed FCP.

synapse/storage/databases/main/experimental_features.py

tests/handlers/test_presence.py

…atrix-org/synapse into shay/experimental_flags_part_2

H-Shay · 2023-05-10T16:36:52Z

synapse/rest/admin/experimental_features.py

    from synapse.server import HomeServer


-class ExperimentalFeature(str, Enum):


This was moved to the experimental features database class to avoid a circular import that occurred when using this type in the presence handler.

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

H-Shay requested a review from a team as a code owner March 28, 2023 22:11

H-Shay marked this pull request as draft March 28, 2023 22:20

H-Shay removed the request for review from a team March 28, 2023 22:20

H-Shay self-assigned this Apr 20, 2023

Base automatically changed from shay/experimental_flags to develop April 28, 2023 18:33

H-Shay force-pushed the shay/experimental_flags_part_2 branch 3 times, most recently from d8e9080 to 19e26c5 Compare May 2, 2023 03:14

H-Shay added 12 commits May 1, 2023 21:01

move experimental feature msc3026 (busy presence) to per-user flag

fea933f

move msc3967 (Do not require UIA when first uploading cross signing k…

0d61d3d

…eys) from config to per-user flag

move experimental feature msc3881 (remotely toggle push) to per-user …

1739ce6

…flag

move experimental feature msc2654 (unread counts) to per-user flag

4aea2de

newsfragment

4291c66

forgot to lint

d3cc11d

add experimental features store to worker store

ca3e15b

re-add parameters to test

51769a9

add a db function to tell if just one feature is enabled

f9e7a0a

update tests to use enum

842eb40

fall back to default config setting if not enabled in table

e5f33c5

stupid github web editor

15dd372

H-Shay force-pushed the shay/experimental_flags_part_2 branch from 19e26c5 to 15dd372 Compare May 2, 2023 04:06

clokep reviewed May 2, 2023

View reviewed changes

synapse/push/bulk_push_rule_evaluator.py Outdated Show resolved Hide resolved

change how config is checked

e53a8a5

H-Shay changed the title ~~Move experimental features MSC3026, MSC2654, MSC3881, and MSC3967 to per-user flags~~ Move experimental features MSC3026, MSC3881, and MSC3967 to per-user flags May 2, 2023

remove support for per-user msc2654

e8c571b

H-Shay force-pushed the shay/experimental_flags_part_2 branch from 73cf3c3 to e8c571b Compare May 2, 2023 18:51

H-Shay marked this pull request as ready for review May 2, 2023 19:14

H-Shay mentioned this pull request May 2, 2023

Remove references to supporting per-user flag for msc2654 #15522

Merged

H-Shay requested a review from a team May 5, 2023 14:58

clokep suggested changes May 8, 2023

View reviewed changes

H-Shay added 7 commits May 10, 2023 08:53

move ExperimentalFeature definition to avoid circular import

aea7cbd

consolidate logic checking config and db to one place

e156b84

test activation both by config and admin api

7568c72

remove changes to sync

9155b82

Merge branch 'develop' into shay/experimental_flags_part_2

2cb4182

small fix

a767f1c

Merge branch 'shay/experimental_flags_part_2' of https://github.com/m…

b99dab3

…atrix-org/synapse into shay/experimental_flags_part_2

H-Shay commented May 10, 2023

View reviewed changes

Update changelog.d/15345.feature

86ec834

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

wrjlewis unassigned H-Shay Sep 7, 2023

matrixbot mentioned this pull request Dec 21, 2023

Move experimental features MSC3026, MSC3881, and MSC3967 to per-user flags element-hq/synapse#15345

Open

		from synapse.server import HomeServer


		class ExperimentalFeature(str, Enum):

Uh oh!

Conversation

H-Shay commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

reivilibre May 9, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

H-Shay commented Mar 28, 2023 •

edited

Loading

reivilibre May 9, 2023 •

edited

Loading