update m.direct list for InviteAutoAccepter module #11578

chagai95 · 2021-12-14T19:20:26Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Signed-off-by: Chagai Friedlander me@chagai.website
Code style is correct
(run the linters)

Sorry for not running the linters, will try tomorrow.

…napse into update-m-direct-list

chagai95 · 2021-12-15T18:41:03Z

I did not manage to run the linters because I built using docker and I could not figure out how to run the script from inside the container, but I think the tests are enough, so I can mark that as checked if the tests go through? Just uncheck if this is a wrong assumption...

chagai95 · 2021-12-15T18:41:47Z

synapse/handlers/room_member.py

@@ -503,6 +503,50 @@ async def update_membership(

        return result

+    async def update_m_direct(


I'm not sure about async for the function I added, does it need this?

Yes since it contains await in the body — perfectly correct

chagai95 · 2021-12-15T18:42:23Z

synapse/module_api/__init__.py

+        is_direct = content.get("is_direct", None)
+        logger.debug("InviteAutoAccepter: is_direct is %s", is_direct)
+        if is_direct:
+            await self._hs.get_room_member_handler().update_m_direct(


I'm not sure about await, does it need to wait?

Seems reasonable to me :)

chagai95 · 2021-12-15T18:43:13Z

synapse/module_api/__init__.py

+        # Update the m direct list
+        is_direct = content.get("is_direct", None)
+        logger.debug("InviteAutoAccepter: is_direct is %s", is_direct)
+        if is_direct:


I notices a log where the sender and target are the same, is this somehow because there is also an event for the user who created the room, should I exclude such cases?
if target != sender and is_direct

The auto-accept invite module works by automatically accepting the invites (sorry to sound tautological) — in that case, the receiver of the invite sends a 'join' membership event for their own user so the sender and target are the same. This is expected behaviour and it would not be able to work otherwise.

Note that I don't think these changes will do anything as of right now because the module does not specify a content dictionary and content.get("is_direct", None) will therefore give you None every time.

Ah yes sorry I probably forgot to add the content part being passed to this pull request. so I guess I do need that check.

reivilibre

Thanks for contributing! I hope the comments above are helpful?

reivilibre · 2021-12-16T16:25:42Z

synapse/handlers/room_member.py

@@ -503,6 +503,50 @@ async def update_membership(

        return result

+    async def update_m_direct(


Yes since it contains await in the body — perfectly correct

reivilibre · 2021-12-16T16:26:34Z

synapse/module_api/__init__.py

+        is_direct = content.get("is_direct", None)
+        logger.debug("InviteAutoAccepter: is_direct is %s", is_direct)
+        if is_direct:
+            await self._hs.get_room_member_handler().update_m_direct(


Seems reasonable to me :)

reivilibre · 2021-12-16T16:33:40Z

synapse/module_api/__init__.py

+        # Update the m direct list
+        is_direct = content.get("is_direct", None)
+        logger.debug("InviteAutoAccepter: is_direct is %s", is_direct)
+        if is_direct:


The auto-accept invite module works by automatically accepting the invites (sorry to sound tautological) — in that case, the receiver of the invite sends a 'join' membership event for their own user so the sender and target are the same. This is expected behaviour and it would not be able to work otherwise.

Note that I don't think these changes will do anything as of right now because the module does not specify a content dictionary and content.get("is_direct", None) will therefore give you None every time.

reivilibre · 2021-12-16T17:47:28Z

synapse/module_api/__init__.py

@@ -880,6 +880,16 @@ async def update_room_membership(
            content=content,
        )

+        # Update the m direct list
+        is_direct = content.get("is_direct", None)


Is is_direct being in the content of an event something you've seen elsewhere? Looking at join events for my DMs I can't see an example of this.
Oh however I do see it in the invite event's content.
The usual way this seems to work (according to the spec) is that the client adds the room to m.direct after accepting the invite (which contains is_direct) (it could prompt the user first or ignore it, but Element doesn't do either of those).

After asking around, I think it would be better to expose ~~a separate function for setting whether the room is in m.direct for a given user, and have the auto-accept module call into that (since it knows if an invite has is_direct set).~~
EDIT: separate functions for reading and writing account data (basically module-friendly versions of get_global_account_data_by_type_for_user and add_account_data_for_user ?). Worth bearing in mind the scope (since account data could be per-room or global; suspect we only care about the global one here).

I'm not aware of the per room one and what it's used for (or could be used for) so I'm guessing the global one is what we care about.

Can I ask why it's better to separate this into two separate functions? 🙈

I think there are a few reasons why a separate function arguably makes sense.

Clients have to make a separate request for setting a room as direct or not, so it makes some sense to reflect that design in the module API. Exposing account data is a bit more general than just the m.direct setting and may well have other uses in the future (for example, tags other than m.direct, lists of ignored users, and so on).
The two operations are also somewhat distinct; joining a room modifies room state, but setting it as direct modifies the user's account state. The current design doesn't leave a way for the status to be changed after the room is joined (and it also has no meaning if the target membership is anything other than 'join').

benparsons · 2021-12-21T10:08:27Z

@reivilibre thanks for picking this up - can you tell me what the next steps are? Is there an option to merge and iterate on the requirement?

reivilibre · 2021-12-21T16:21:04Z

@reivilibre thanks for picking this up - can you tell me what the next steps are? Is there an option to merge and iterate on the requirement?

I don't think the code will work as intended as it currently stands, since it assumes that join events will contain is_direct (this is not the case — invite events contain that).
It's also difficult to merge and change later since this becomes public API the moment it goes into a release, which then causes backward compatibility problems if we try to change it.

The next steps seem to be:

take most of the actual logic from this PR to a new PR in the auto accept module
write a PR against Synapse that exposes two methods to get and update globally-scoped account data for a given user. It's likely fine to just pass through something similar to the existing functions on the data store; the main thing that's needed is that modules can access it and that it's documented as public API rather than having modules reach into the depths of Synapse's code (which we can and most likely will break when shuffling code around).

I'll be away until after Christmas but if there are any questions I'll be happy to address them when I'm here.

chagai95 · 2021-12-22T21:20:45Z

It actually works exactly as intended but I completely understand your concern, from how I understood the spec it could also be implemented this way but it's not how it should be done.

I did add the content to the membership=join member event and used (abused) that to check if the m.direct list should be updated or not, isn't that more like a feature than a bug? 😉

There is some discussion here that is relevant I think 🙃

What I'm trying to say is that the correct solution for this it seems to be is to implement canonical DMs and that'll probably break everything anyway? It seems to me like adding a new API call from synapse for account data might be a little bit of an overkill for our use case (but nevertheless pretty cool to have as a feature in general), I was also under the impression that's not something that should be easy for system admins to tamper with because it's a personal user setting? but maybe I understood that wrong?

reivilibre · 2021-12-29T15:17:35Z

I did add the content to the membership=join member event and used (abused) that to check if the m.direct list should be updated or not, isn't that more like a feature than a bug? wink

My reading of the spec is that is_direct only has a meaning for invites — as such, putting it in a join event for this one use case is a bit of a hack (and makes things very confusing, especially because it doesn't behave in the same way as a client sending that event would).

The existing client workflow is to send a plain join, then read-modify-write the account data. It seems only reasonable to expect a module to do the same.

If you were going to place a flag in the event content, then realistically it should be a vendor-specific-prefixed name like org.matrix.synapse.is_direct since is_direct has no current meaning in the spec in that context (but might do in the future!). However I'm mostly saying this to illustrate the issue I have with it: it doesn't feel right at all.

It seems to me like adding a new API call from synapse for account data might be a little bit of an overkill for our use case (but nevertheless pretty cool to have as a feature in general)

Being a bit extreme, you could say this to some degree about many API calls that Synapse exposes, however I think it's sensible to prefer module logic to exist in modules and for the interfaces to be generic.
Nowhere (except for room upgrades :/) in Synapse currently cares about what rooms are m.direct.
On the other hand, passing through the existing functions for changing account data seems fairly easy to do and gives the same functionality as the client endpoints do.

N.B. I wouldn't object to leaving out the room-scoped account data functions for another time; no need to go and add everything right away — it's fine to keep it simple.

I was also under the impression that's not something that should be easy for system admins to tamper with because it's a personal user setting?

This is not a system admin, this is a module. You could make the same point about sending messages or indeed automatically accepting invites on behalf of a user: that's a personal user choice. So I think if it's fine to accept an invite on behalf of a user, then it's fine to also amend the account data to set the direct flag on it.

What I'm trying to say is that the correct solution for this it seems to be is to implement canonical DMs and that'll probably break everything anyway?

Maybe, but I don't think Canonical DMs are part of the spec and may never be (they've been floating around for years I think..). I think we'll need to cross that bridge if/when we come to it.
I'm not up to date on Canonical DMs but it seems to me that the client API would break in the same way.

Hopefully that addresses your concerns?

chagai95 · 2021-12-29T23:25:15Z

Yes, very detailed explanation, thanks for that! will hopefully get around soon to adding another function for that, otherwise if anyone else wants to take over and do what needs to be done, that'll be great.

chagai95 · 2022-01-05T15:49:06Z

I exposed another method to update the m direct list and created a PR on the auto_accept_invite module as well using that exposed function. I also realized I understood yet another thing I was doing wrong (sender/target) and did that hopefully the correct way this time.

reivilibre · 2022-01-10T14:46:39Z

synapse/module_api/__init__.py

@@ -889,6 +889,40 @@ async def update_room_membership(

        return event

+    async def update_m_direct(


Sorry, I believe I have been misunderstood here.

What I was trying to say is that update_m_direct should live in the synapse-auto-accept-invite module.

Instead, I would suggest adding add_account_data_for_user (which is just a thin wrapper around add_account_data_for_user) and get_account_data_for_user (a thin wrapper again) to the Module API.

Yep but as a first step I added just updating the list. I guess the logic for that is also better in the module, could we keep that also for a next step?

The Module API is supposed to be a stable API. So I don't think we should add things to it if we're already planning to get rid of them soon, as it would mean we would need to maintain an unnecessary layer of backwards compatibility when we do the switch. So since we already have an idea of what the long-term solution should look like (and it's not a complex one) I think we should just do it.
As a nitpick, I think add_account_data_for_user should instead be named set_account_data_for_user - it makes it clearer that the method can be used for both creating new account data and updating existing one (sadly this makes it a bit inconsistent with the method from the account data handler, but I think it's best to make things clearer to module developers).

As a nitpick, I think add_account_data_for_user should instead be named set_account_data_for_user

No objections here with that; it does reflect that it will overwrite account data so sounds right.

anoadragon453 · 2022-03-30T13:50:53Z

synapse/handlers/room_member.py

+        target: str,
+        room_id: str,
+    ) -> None:
+        """Update a user's m_direct list.


Note that this PR should ensure that there is a way of allowing the Module API to ignore updates from itself via the proposed on_account_data_change callback (#12327). Perhaps by adding an argument to that method to inform modules that it was a module vs. a user that updated the account data.

Perhaps by adding an argument to that method to inform modules that it was a module vs. a user that updated the account data.

If we want to do that, then we should do it before #12327 merges. I guess we can add an argument to the account data handler's methods to differentiate changes done by the server vs changes done by an user (I'm not feeling comfortable about adding a flag about whether the change is done by a module specifically).

Though it's worth noting that we have other similar callbacks, such as on_new_event, which don't have such an argument and it's never been an issue, so I'm wondering whether we actually need that.

dklimpel · 2022-04-06T19:52:55Z

IMO this is replaced by #12391

reivilibre · 2022-04-07T11:01:12Z

Superseded by #12391 indeed — thanks though!

update m.direct list for InviteAutoAccepter module

896e67b

chagai95 requested a review from a team as a code owner December 14, 2021 19:20

chagai95 and others added 5 commits December 14, 2021 20:24

adding file to change log

ebaf4d1

forgot a period 🤦‍♂️

3d9ff23

put the room id in the right list

c057768

Merge branch 'update-m-direct-list' of https://github.com/chagai95/sy…

15b3424

…napse into update-m-direct-list

fix for lint

0c20d53

chagai95 commented Dec 15, 2021

View reviewed changes

reivilibre self-assigned this Dec 16, 2021

reivilibre suggested changes Dec 16, 2021

View reviewed changes

richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jan 4, 2022

exposing another method to update m direct list

4ab0858

False is nicer than None

e0012a5

reivilibre self-requested a review January 7, 2022 17:29

reivilibre removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jan 10, 2022

reivilibre suggested changes Jan 10, 2022

View reviewed changes

erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Feb 23, 2022

anoadragon453 reviewed Mar 30, 2022

View reviewed changes

reivilibre closed this Apr 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update m.direct list for InviteAutoAccepter module #11578

update m.direct list for InviteAutoAccepter module #11578

chagai95 commented Dec 14, 2021 •

edited

Loading

chagai95 commented Dec 15, 2021 •

edited

Loading

chagai95 Dec 15, 2021

reivilibre Dec 16, 2021

chagai95 Dec 15, 2021 •

edited

Loading

reivilibre Dec 16, 2021

chagai95 Dec 15, 2021 •

edited

Loading

reivilibre Dec 16, 2021

chagai95 Dec 18, 2021

reivilibre left a comment

reivilibre Dec 16, 2021

reivilibre Dec 16, 2021

reivilibre Dec 16, 2021

reivilibre Dec 16, 2021 •

edited

Loading

chagai95 Dec 18, 2021

chagai95 Dec 18, 2021

reivilibre Dec 20, 2021

benparsons commented Dec 21, 2021

reivilibre commented Dec 21, 2021

chagai95 commented Dec 22, 2021 •

edited

Loading

reivilibre commented Dec 29, 2021

chagai95 commented Dec 29, 2021

chagai95 commented Jan 5, 2022 •

edited

Loading

reivilibre Jan 10, 2022

chagai95 Jan 10, 2022

babolivier Jan 11, 2022

reivilibre Jan 21, 2022

anoadragon453 Mar 30, 2022

babolivier Mar 30, 2022

dklimpel commented Apr 6, 2022

reivilibre commented Apr 7, 2022

		@@ -503,6 +503,50 @@ async def update_membership(

		return result

		async def update_m_direct(

		@@ -889,6 +889,40 @@ async def update_room_membership(

		return event

		async def update_m_direct(

update m.direct list for InviteAutoAccepter module #11578

update m.direct list for InviteAutoAccepter module #11578

Conversation

chagai95 commented Dec 14, 2021 • edited Loading

Pull Request Checklist

chagai95 commented Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chagai95 Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chagai95 Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre 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

reivilibre Dec 16, 2021 • edited Loading

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

benparsons commented Dec 21, 2021

reivilibre commented Dec 21, 2021

chagai95 commented Dec 22, 2021 • edited Loading

reivilibre commented Dec 29, 2021

chagai95 commented Dec 29, 2021

chagai95 commented Jan 5, 2022 • edited Loading

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

dklimpel commented Apr 6, 2022

reivilibre commented Apr 7, 2022

chagai95 commented Dec 14, 2021 •

edited

Loading

chagai95 commented Dec 15, 2021 •

edited

Loading

chagai95 Dec 15, 2021 •

edited

Loading

chagai95 Dec 15, 2021 •

edited

Loading

reivilibre Dec 16, 2021 •

edited

Loading

chagai95 commented Dec 22, 2021 •

edited

Loading

chagai95 commented Jan 5, 2022 •

edited

Loading