Migrate direct message and tag state on room upgrade #4412

anoadragon453 · 2019-01-17T15:30:20Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file
Pull request includes a sign off

Sytest PR: matrix-org/sytest#547

anoadragon453 · 2019-01-17T15:44:11Z

~~According to the spec, is_direct should also be set in the createRoom event as well. That's still TODO.~~

Done.

codecov-io · 2019-01-17T15:46:51Z

Codecov Report

Merging #4412 into develop will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##           develop    #4412      +/-   ##
===========================================
+ Coverage    74.73%   74.75%   +0.02%     
===========================================
  Files          336      336              
  Lines        34122    34138      +16     
  Branches      5547     5552       +5     
===========================================
+ Hits         25500    25521      +21     
+ Misses        7048     7041       -7     
- Partials      1574     1576       +2

Signed-off-by: Andrew Morgan <andrew@amorgan.xyz>

…room_upgrade

anoadragon453 · 2019-01-22T13:46:00Z

So this seems to be failing due to a timeout on sending a make_leave request over federation.

Ctrl-F for 2019-01-22 13:27:05,040 in https://10830-22844864-gh.circle-artifacts.com/0/logs/server-0/homeserver.log to see the error. Possibly explains why the CI would fail while locally the tests run fine?

This PR does add logic to when someone joins a room. Not sure if that has to do with make_leave however.

richvdh

generally looks good, but a few suggestions for cleanups here.

synapse/handlers/room_member.py

richvdh · 2019-01-24T17:23:12Z

synapse/handlers/room_member.py

+                return
+            create_event = yield self.store.get_event(create_id)
+
+            if "predecessor" in create_event["content"]:


presumably we can use the method from the search PR for this

Ah, we can but it's inside SearchHandler:

synapse/synapse/handlers/search.py

Lines 41 to 56 in b1b6dba

def get_old_rooms_from_upgraded_room(self, room_id):

"""Retrieves room IDs of old rooms in the history of an upgraded room.

We do so by checking the m.room.create event of the room for a

`predecessor` key. If it exists, we add the room ID to our return

list and then check that room for a m.room.create event and so on

until we can no longer find any more previous rooms.

The full list of all found rooms in then returned.

Args:

room_id (str): id of the room to search through.

Returns:

Deferred[iterable[unicode]]: predecessor room ids

"""

Shall I move it to the store instead?

I was thinking specifically of get_room_predecessor, which is in the Store I think?

Oh, so it is. Will add, thanks.

richvdh · 2019-01-24T17:25:08Z

synapse/handlers/room_member.py

+                user_account_data = yield self.store.get_account_data_for_user(
+                    user_id,
+                )
+                room_tags = yield self.store.get_tags_for_room(


feels weird when this is up here and not used until later on

richvdh · 2019-01-24T17:26:43Z

synapse/handlers/room_member.py

+                            break
+
+                # Copy room tags if applicable
+                if room_tags:


think this condition is redundant? get_tags_for_room always returns a dict, even if it's empty.

richvdh · 2019-01-24T17:28:38Z

synapse/handlers/room_member.py

+                old_room_id = create_event["content"]["predecessor"]["room_id"]
+
+                # Retrieve room account data for predecessor room
+                user_account_data = yield self.store.get_account_data_for_user(


given you just want the first item in the tuple you can write:

user_account_data, _ = yield ...

which then saves the slightly cryptic [0] indexes later

Ohh, nice one. I love destructuring.

richvdh · 2019-01-24T17:30:42Z

synapse/handlers/room_member.py

+                )
+
+                # Copy direct message state if applicable
+                if user_account_data and "m.direct" in user_account_data[0]:


suggest you do direct_rooms = user_account_data.get("m.direct", {}) and you can drop these conditions.

HOWEVER it may be worth sanity-checking that nobody has decided to make m.direct a list or something else other than a dict, so if isinstance(direct_rooms, dict).

richvdh · 2019-01-24T17:31:32Z

synapse/handlers/room_member.py

+                # Copy room tags if applicable
+                if room_tags:
+                    # Copy each room tag to the new room
+                    for tag in room_tags.keys():


for tag, tag_content in room_tags.items():

Oof, I think I got lost in thinking how the tag data was structured to realize how clunky that code was.

Thanks for pointing it out.

…room_upgrade

richvdh

lgtm otherwise

richvdh · 2019-01-28T14:01:31Z

synapse/handlers/room_member.py

+            user_id (str)
+
+        Returns:
+            Deferred|None


ITYM Deferred[None] ?

Sure, it was just copied from another method. We seem to have several different ways of writing the same thing spread across different methods?

Deferred|None means: "either a Deferred (of unspecified value type), or None", which is different to what your method does

Ahh, I see, thank you.

…room_upgrade

anoadragon453 force-pushed the anoa/dm_room_upgrade branch from 1707fe3 to db9e7df Compare January 17, 2019 15:31

anoadragon453 mentioned this pull request Jan 18, 2019

Ensure room upgrades preserve DM and e2e flags #4391

Closed

anoadragon453 changed the title ~~Migrate direct message state on room upgrade~~ [WIP] Migrate direct message and tag state on room upgrade Jan 18, 2019

anoadragon453 force-pushed the anoa/dm_room_upgrade branch from d40d531 to 46718b1 Compare January 22, 2019 10:53

anoadragon453 added 8 commits January 22, 2019 11:00

Fix typo

75942af

Preserve DM status of a room on upgrade

4ff6d22

Signed-off-by: Andrew Morgan <andrew@amorgan.xyz>

Add changelog

1f18c7c

Prevent crash on user who doesn't have any direct rooms

887ca93

Migrating dm and room tags work for migrator

ea8903f

Fix typos

25d64a8

tags, m.direct copying over correctly

8c85f08

Join logic covers both room creator and arbitrary users

48951f4

anoadragon453 force-pushed the anoa/dm_room_upgrade branch from 2c5f453 to d9bcecf Compare January 22, 2019 11:00

anoadragon453 changed the title ~~[WIP] Migrate direct message and tag state on room upgrade~~ Migrate direct message and tag state on room upgrade Jan 22, 2019

anoadragon453 mentioned this pull request Jan 22, 2019

Add tests for transfer of tags and dm status on room upgrade matrix-org/sytest#547

Merged

Fix comments

8086a5c

anoadragon453 force-pushed the anoa/dm_room_upgrade branch from d9bcecf to 8086a5c Compare January 22, 2019 11:16

anoadragon453 added 2 commits January 22, 2019 12:13

Prevent duplicate room IDs in m.direct

c4875d8

Merge branch 'develop' of github.com:matrix-org/synapse into anoa/dm_…

117bc94

…room_upgrade

lint

766a172

anoadragon453 force-pushed the anoa/dm_room_upgrade branch from ab2f127 to 766a172 Compare January 22, 2019 13:51

anoadragon453 requested a review from a team January 22, 2019 13:58

richvdh suggested changes Jan 24, 2019

View reviewed changes

anoadragon453 added 4 commits January 25, 2019 11:09

Move tag and direct state copying into separate function

0862d35

Merge branch 'develop' of github.com:matrix-org/synapse into anoa/dm_…

821b65a

…room_upgrade

Move room_tag declaration to be closer to its use

6f3fda7

Remove unnecessary null check

516456b

anoadragon453 added 4 commits January 25, 2019 11:24

Destructure account data tuple before use

c4cdafa

Use python magic

8265995

Clean up direct_rooms access

da0d221

Fixes

9244a30

anoadragon453 requested review from a team and removed request for a team January 25, 2019 14:39

richvdh suggested changes Jan 28, 2019

View reviewed changes

anoadragon453 added 3 commits January 28, 2019 14:08

Reuse predecessor method

1ce4639

Merge branch 'develop' of github.com:matrix-org/synapse into anoa/dm_…

4026d55

…room_upgrade

Change return syntax in doc string

f0e96ab

anoadragon453 merged commit 88f4df8 into develop Jan 28, 2019

anoadragon453 deleted the anoa/dm_room_upgrade branch January 28, 2019 15:17

neilisfragile mentioned this pull request Feb 26, 2019

Synapse doesn't seem to transfer tags correctly on room upgrade #4703

Closed

ara4n mentioned this pull request Mar 1, 2022

Meta bug for edge cases that should be considered to make room upgrades transparent matrix-org/matrix-spec#456

Open

17 tasks

	def get_old_rooms_from_upgraded_room(self, room_id):
	"""Retrieves room IDs of old rooms in the history of an upgraded room.

	We do so by checking the m.room.create event of the room for a
	`predecessor` key. If it exists, we add the room ID to our return
	list and then check that room for a m.room.create event and so on
	until we can no longer find any more previous rooms.

	The full list of all found rooms in then returned.

	Args:
	room_id (str): id of the room to search through.

	Returns:
	Deferred[iterable[unicode]]: predecessor room ids
	"""

Uh oh!

Migrate direct message and tag state on room upgrade #4412

Migrate direct message and tag state on room upgrade #4412

Uh oh!

Conversation

anoadragon453 commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

anoadragon453 commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anoadragon453 commented Jan 22, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anoadragon453 commented Jan 17, 2019 •

edited

Loading

anoadragon453 commented Jan 17, 2019 •

edited

Loading

codecov-io commented Jan 17, 2019 •

edited

Loading