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

Add forgotten status to Room Details API #13503

Merged
merged 9 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13503.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add forgotten status to Room Details API.
5 changes: 4 additions & 1 deletion docs/admin_api/rooms.md
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ The following fields are possible in the JSON response body:
* `state_events` - Total number of state_events of a room. Complexity of the room.
* `room_type` - The type of the room taken from the room's creation event; for example "m.space" if the room is a space.
If the room does not define a type, the value will be `null`.
* `forgotten` - Whether all local users have
[forgotten](https://spec.matrix.org/latest/client-server-api/#leaving-rooms) the room.
Comment on lines +305 to +306
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should note that this was added in Synapse 1.66. I'll add something to this file now while preparing the release.


The API is:

Expand Down Expand Up @@ -330,7 +332,8 @@ A response body like the following is returned:
"guest_access": null,
"history_visibility": "shared",
"state_events": 93534,
"room_type": "m.space"
"room_type": "m.space",
"forgotten": false
}
```

Expand Down
1 change: 1 addition & 0 deletions synapse/rest/admin/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ async def on_GET(

members = await self.store.get_users_in_room(room_id)
ret["joined_local_devices"] = await self.store.count_devices_by_users(members)
ret["forgotten"] = await self.store.is_locally_forgotten_room(room_id)

return HTTPStatus.OK, ret

Expand Down
24 changes: 24 additions & 0 deletions synapse/storage/databases/main/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,30 @@ def _get_forgotten_rooms_for_user_txn(txn: LoggingTransaction) -> Set[str]:
"get_forgotten_rooms_for_user", _get_forgotten_rooms_for_user_txn
)

async def is_locally_forgotten_room(self, room_id: str) -> bool:
"""Returns whether all local users have forgotten this room_id.

Args:
room_id: The room ID to query.

Returns:
Whether the room is forgotten.
"""

sql = """
SELECT count(*) FROM local_current_membership
INNER JOIN room_memberships USING (room_id, event_id)
WHERE
room_id = ?
AND forgotten = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment: it might(?) be more performant to SELECT count(*) > 0 here. That gives the database a chance to stop the query as soon as it finds the first membership that hasn't been forgotten.

(There's also an index "room_memberships_user_room_forgotten" btree (user_id, room_id) WHERE forgotten = 1 but I'm not sure if there's an easy way to use that here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def _get_forgotten_rooms_for_user_txn(txn: LoggingTransaction) -> Set[str]:
# This is a slightly convoluted query that first looks up all rooms
# that the user has forgotten in the past, then rechecks that list
# to see if any have subsequently been updated. This is done so that
# we can use a partial index on `forgotten = 1` on the assumption
# that few users will actually forget many rooms.
#
# Note that a room is considered "forgotten" if *all* membership
# events for that user and room have the forgotten field set (as
# when a user forgets a room we update all rows for that user and
# room, not just the current one).
sql = """
SELECT room_id, (
SELECT count(*) FROM room_memberships
WHERE room_id = m.room_id AND user_id = m.user_id AND forgotten = 0
) AS count
FROM room_memberships AS m
WHERE user_id = ? AND forgotten = 1
GROUP BY room_id, user_id;
"""

This looks similar, but is very complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try to create a better sql query. I am feeling in a rabbit hole. It seems that Synapse is not possible to forget a room, at the moment. I think there is a bug in:

async def forget(self, user: UserID, room_id: str) -> None:
user_id = user.to_string()
member = await self._storage_controllers.state.get_current_state_event(
room_id=room_id, event_type=EventTypes.Member, state_key=user_id
)
membership = member.membership if member else None
if membership is not None and membership not in [
Membership.LEAVE,
Membership.BAN,
]:
raise SynapseError(400, "User %s in room %s" % (user_id, room_id))
if membership:
await self.store.forget(user_id, room_id)

There is a gap of unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proposal for an other query:

SELECT room_id, ( 
 SELECT count(*) > 0 FROM room_memberships 
 WHERE room_id = m.room_id AND forgotten = 0 
) AS count 
FROM room_memberships AS m 
WHERE room_id = ? AND forgotten = 1 
GROUP BY room_id;

Steps

  1. Query if the room is at least one time forgotten - uses partial index on forgotten = 1
  2. Query this rooms (result of no. 1) that there is no user which has not forgotten (or rejoin) this room

Results

  • count == 0 -> room is forgotten
  • count > 0 -> room is not fully forgotten but at least one time
  • no result / count is None -> room is not in list (unknown) or room is not at least one time forgotten

"""
Copy link
Member

Choose a reason for hiding this comment

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

The query plan looks sensible to me


rows = await self.db_pool.execute("is_forgotten_room", None, sql, room_id)

# `count(*)` returns always an integer
# If any rows still exist it means someone has not forgotten this room yet
return not rows[0][0]

async def get_rooms_user_has_been_in(self, user_id: str) -> Set[str]:
"""Get all rooms that the user has ever been in.

Expand Down
1 change: 1 addition & 0 deletions tests/rest/admin/test_room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1633,6 +1633,7 @@ def test_single_room(self) -> None:
self.assertIn("history_visibility", channel.json_body)
self.assertIn("state_events", channel.json_body)
self.assertIn("room_type", channel.json_body)
self.assertIn("forgotten", channel.json_body)
self.assertEqual(room_id_1, channel.json_body["room_id"])

def test_single_room_devices(self) -> None:
Expand Down
49 changes: 49 additions & 0 deletions tests/storage/test_roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,55 @@ def test__null_byte_in_display_name_properly_handled(self) -> None:
# Check that alice's display name is now None
self.assertEqual(row[0]["display_name"], None)

def test_room_is_locally_forgotten(self):
"""Test that when the last local user has forgotten a room it is known as forgotten."""
# join two local and one remote user
self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice)
self.inject_room_member(self.room, self.u_bob, Membership.JOIN)
self.inject_room_member(self.room, self.u_charlie.to_string(), Membership.JOIN)
self.assertFalse(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

# local users leave the room and the room is not forgotten
self.inject_room_member(self.room, self.u_alice, Membership.LEAVE)
self.inject_room_member(self.room, self.u_bob, Membership.LEAVE)
self.assertFalse(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

# first user forgets the room, room is not forgotten
self.get_success(self.store.forget(self.u_alice, self.room))
self.assertFalse(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

# second (last local) user forgets the room and the room is forgotten
self.get_success(self.store.forget(self.u_bob, self.room))
self.assertTrue(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

def test_join_locally_forgotten_room(self):
"""Tests if a user joins a forgotten room the room is not forgotten anymore."""
self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice)
self.assertFalse(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

# after leaving and forget the room, it is forgotten
self.inject_room_member(self.room, self.u_alice, Membership.LEAVE)
self.get_success(self.store.forget(self.u_alice, self.room))
self.assertTrue(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)

# after rejoin the room is not forgotten anymore
self.inject_room_member(self.room, self.u_alice, Membership.JOIN)
self.assertFalse(
self.get_success(self.store.is_locally_forgotten_room(self.room))
)


class CurrentStateMembershipUpdateTestCase(unittest.HomeserverTestCase):
def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
Expand Down