Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

On upgrade room only send canonical alias once. #7547

Merged
merged 3 commits into from
May 22, 2020

Conversation

erikjohnston
Copy link
Member

Instead of doing a complicated dance of deleting and moving aliases one
by one, which sends a canonical alias update into the old room for each
one, lets do it all in one go.

This also changes the function to move all local alias events to the new
room, however that happens later on anyway.

Instead of doing a complicated dance of deleting and moving aliases one
by one, which sends a canonical alias update into the old room for each
one, lets do it all in one go.

This also changes the function to move *all* local alias events to the new
room, however that happens later on anyway.
@erikjohnston erikjohnston force-pushed the erikj/move_alias_efficiently branch from cf49e61 to 25f3f41 Compare May 21, 2020 09:46
@erikjohnston erikjohnston requested a review from a team May 21, 2020 09:46
@anoadragon453 anoadragon453 self-requested a review May 21, 2020 10:54
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine from a python and room upgrade perspective, but I'd like to get someone more familiar with the new alias semantics to review as well.

synapse/handlers/room.py Outdated Show resolved Hide resolved
synapse/handlers/room.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from a team May 21, 2020 12:09
@erikjohnston
Copy link
Member Author

erikjohnston commented May 21, 2020

Looks fine from a python and room upgrade perspective, but I'd like to get someone more familiar with the new alias semantics to review as well.

I believe this is keeping existing behaviour, we have some pretty detailed sytests for this. Note that the delete_association function updated the old canonical alias event before

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot more sense on the second go around. Just had a quick question.

new_canonical_alias_content = {}

canonical = canonical_alias_event.content.get("alias")
if canonical and self.hs.is_mine_id(canonical):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not able to move canonical aliases if they don't belong to us? It's kind of annoying if the person upgrading a room (with canonical alias #project:matrix.org for visibility) has to move it via their matrix.org account.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we check the canonical alias event to make sure that all the aliases listed do actually point to the room. We can't a) move aliases that don't belong to the server, nor b) set the canonical alias if its still pointing to the old room (and I'm not sure we'd want to).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes of course. So in this case the room will be upgraded, and the alias will be left behind until someone from the server that owns it, moves it.

That actually does sound intuitive.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm then!

@erikjohnston erikjohnston merged commit 710d958 into develop May 22, 2020
@erikjohnston erikjohnston deleted the erikj/move_alias_efficiently branch May 22, 2020 10:41
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
Instead of doing a complicated dance of deleting and moving aliases one
by one, which sends a canonical alias update into the old room for each
one, lets do it all in one go.

This also changes the function to move *all* local alias events to the new
room, however that happens later on anyway.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants