Skip to content

Spaces: Fix removing a space child when the relationship is only 1-way. #5890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stefanceriu merged 2 commits into from
Nov 24, 2025
Merged

Spaces: Fix removing a space child when the relationship is only 1-way. #5890

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21c67b8
spaces: Fix an incorrect early return introduced at the last minute.
pixlwave Nov 24, 2025
be7699e
chore: Add forgotten tests for removing space child.
pixlwave Nov 24, 2025
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
357 changes: 239 additions & 118 deletions crates/matrix-sdk-ui/src/spaces/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,38 +276,34 @@ impl SpaceService {
let child_room =
self.client.get_room(&child_id).ok_or(Error::RoomNotFound(child_id.to_owned()))?;

if space_room
.get_state_event_static_for_key::<SpaceChildEventContent, _>(&child_id)
.await
.is_err()
if let Ok(Some(_)) =
space_room.get_state_event_static_for_key::<SpaceChildEventContent, _>(&child_id).await
{
// Redacting state is a "weird" thing to do, so send {} instead.
// https://github.com/matrix-org/matrix-spec/issues/2252
//
// Specifically, "The redaction of the state doesn't participate in state
// resolution so behaves quite differently from e.g. sending an empty form of
// that state events".
space_room
.send_state_event_raw("m.space.child", child_id.as_str(), serde_json::json!({}))
.await
.map_err(Error::UpdateRelationship)?;
} else {
warn!("A space child event wasn't found on the parent, ignoring.");
return Ok(());
}
// Redacting state is a "weird" thing to do, so send {} instead.
// https://github.com/matrix-org/matrix-spec/issues/2252
//
// Specifically, "The redaction of the state doesn't participate in state
// resolution so behaves quite differently from e.g. sending an empty form of
// that state events".
space_room
.send_state_event_raw("m.space.child", child_id.as_str(), serde_json::json!({}))
.await
.map_err(Error::UpdateRelationship)?;

if child_room
.get_state_event_static_for_key::<SpaceParentEventContent, _>(&space_id)
.await
.is_err()
if let Ok(Some(_)) =
child_room.get_state_event_static_for_key::<SpaceParentEventContent, _>(&space_id).await
{
// Same as the comment above.
child_room
.send_state_event_raw("m.space.parent", space_id.as_str(), serde_json::json!({}))
.await
.map_err(Error::UpdateRelationship)?;
} else {
warn!("A space parent event wasn't found on the child, ignoring.");
return Ok(());
}
// Same as the comment above.
child_room
.send_state_event_raw("m.space.parent", space_id.as_str(), serde_json::json!({}))
.await
.map_err(Error::UpdateRelationship)?;

Ok(())
}
Expand Down Expand Up @@ -477,48 +473,36 @@ mod tests {
let child_space_id_1 = room_id!("!child_space_1:example.org");
let child_space_id_2 = room_id!("!child_space_2:example.org");

server
.sync_room(
&client,
JoinedRoomBuilder::new(child_space_id_1)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_parent(parent_space_id.to_owned(), child_space_id_1.to_owned())
.sender(user_id),
),
)
.await;

server
.sync_room(
&client,
JoinedRoomBuilder::new(child_space_id_2)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_parent(parent_space_id.to_owned(), child_space_id_2.to_owned())
.sender(user_id),
),
)
.await;
server
.sync_room(
&client,
JoinedRoomBuilder::new(parent_space_id)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_child(parent_space_id.to_owned(), child_space_id_1.to_owned())
.sender(user_id),
)
.add_state_event(
factory
.space_child(parent_space_id.to_owned(), child_space_id_2.to_owned())
.sender(user_id),
),
)
.await;
add_room_with_relationships(
child_space_id_1,
vec![parent_space_id],
vec![],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(
child_space_id_2,
vec![parent_space_id],
vec![],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(
parent_space_id,
vec![],
vec![child_space_id_1, child_space_id_2],
&client,
&server,
&factory,
user_id,
)
.await;

// Only the parent space is returned
assert_eq!(
Expand Down Expand Up @@ -745,57 +729,36 @@ mod tests {
let unknown_parent_space_id = room_id!("!unknown_parent_space:example.org");
let child_space_id = room_id!("!child_space:example.org");

server
.sync_room(
&client,
JoinedRoomBuilder::new(child_space_id)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_parent(parent_space_id_1.to_owned(), child_space_id.to_owned())
.sender(user_id),
)
.add_state_event(
factory
.space_parent(parent_space_id_2.to_owned(), child_space_id.to_owned())
.sender(user_id),
)
.add_state_event(
factory
.space_parent(
unknown_parent_space_id.to_owned(),
child_space_id.to_owned(),
)
.sender(user_id),
),
)
.await;

server
.sync_room(
&client,
JoinedRoomBuilder::new(parent_space_id_1)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_child(parent_space_id_1.to_owned(), child_space_id.to_owned())
.sender(user_id),
),
)
.await;

server
.sync_room(
&client,
JoinedRoomBuilder::new(parent_space_id_2)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type())
.add_state_event(
factory
.space_child(parent_space_id_2.to_owned(), child_space_id.to_owned())
.sender(user_id),
),
)
.await;
add_room_with_relationships(
child_space_id,
vec![parent_space_id_1, parent_space_id_2, unknown_parent_space_id],
vec![],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(
parent_space_id_1,
vec![],
vec![parent_space_id_1],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(
parent_space_id_2,
vec![],
vec![parent_space_id_1],
&client,
&server,
&factory,
user_id,
)
.await;

let space_service = SpaceService::new(client.clone());

Expand Down Expand Up @@ -924,6 +887,137 @@ mod tests {
assert!(result.is_ok());
}

#[async_test]
async fn test_remove_child_from_space() {
// Given a space and child room where the user is admin of both.
let server = MatrixMockServer::new().await;
let client = server.client_builder().build().await;
let user_id = client.user_id().unwrap();
let factory = EventFactory::new();

server.mock_room_state_encryption().plain().mount().await;

let space_child_event_id = event_id!("$1");
let space_parent_event_id = event_id!("$2");
server.mock_set_space_child().ok(space_child_event_id.to_owned()).expect(1).mount().await;
server.mock_set_space_parent().ok(space_parent_event_id.to_owned()).expect(1).mount().await;

let parent_id = room_id!("!parent_space:example.org");
let child_id = room_id!("!child_space:example.org");

add_room_with_relationships(
parent_id,
vec![],
vec![child_id],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(
child_id,
vec![parent_id],
vec![],
&client,
&server,
&factory,
user_id,
)
.await;

let space_service = SpaceService::new(client.clone());

// When removing the child from the space.
let result =
space_service.remove_child_from_space(child_id.to_owned(), parent_id.to_owned()).await;

// Then both space child and parent events are removed successfully.
assert!(result.is_ok());
}

#[async_test]
async fn test_remove_child_from_space_without_parent_event() {
// Given a space with a child where the m.space.parent event wasn't set.
let server = MatrixMockServer::new().await;
let client = server.client_builder().build().await;
let user_id = client.user_id().unwrap();
let factory = EventFactory::new();

server.mock_room_state_encryption().plain().mount().await;

let space_child_event_id = event_id!("$1");
server.mock_set_space_child().ok(space_child_event_id.to_owned()).expect(1).mount().await;
server.mock_set_space_parent().unauthorized().expect(0).mount().await;

let parent_id = room_id!("!parent_space:example.org");
let child_id = room_id!("!child_space:example.org");

add_room_with_relationships(
parent_id,
vec![],
vec![child_id],
&client,
&server,
&factory,
user_id,
)
.await;
add_room_with_relationships(child_id, vec![], vec![], &client, &server, &factory, user_id)
.await;

let space_service = SpaceService::new(client.clone());

// When removing the child from the space.
let result =
space_service.remove_child_from_space(child_id.to_owned(), parent_id.to_owned()).await;

// Then the child event is removed successfully and the parent event removal is
// not attempted.
assert!(result.is_ok());
}

#[async_test]
async fn test_remove_child_from_space_without_child_event() {
// Given a space with a child where the space's m.space.child event wasn't set.
let server = MatrixMockServer::new().await;
let client = server.client_builder().build().await;
let user_id = client.user_id().unwrap();
let factory = EventFactory::new();

server.mock_room_state_encryption().plain().mount().await;

let space_parent_event_id = event_id!("$2");
server.mock_set_space_child().unauthorized().expect(0).mount().await;
server.mock_set_space_parent().ok(space_parent_event_id.to_owned()).expect(1).mount().await;

let parent_id = room_id!("!parent_space:example.org");
let child_id = room_id!("!child_space:example.org");

add_room_with_relationships(parent_id, vec![], vec![], &client, &server, &factory, user_id)
.await;
add_room_with_relationships(
child_id,
vec![parent_id],
vec![],
&client,
&server,
&factory,
user_id,
)
.await;

let space_service = SpaceService::new(client.clone());

// When removing the child from the space.
let result =
space_service.remove_child_from_space(child_id.to_owned(), parent_id.to_owned()).await;

// Then the parent event is removed successfully and the child event removal is
// not attempted.
assert!(result.is_ok());
}

async fn add_space_rooms_with(
rooms: Vec<(&RoomId, Option<&str>)>,
client: &Client,
Expand All @@ -948,6 +1042,33 @@ mod tests {
}
}

async fn add_room_with_relationships(
room_id: &RoomId,
parents: Vec<&RoomId>,
children: Vec<&RoomId>,
client: &Client,
server: &MatrixMockServer,
factory: &EventFactory,
user_id: &UserId,
) {
let mut builder = JoinedRoomBuilder::new(room_id)
.add_state_event(factory.create(user_id, RoomVersionId::V1).with_space_type());

for parent_id in parents {
builder = builder.add_state_event(
factory.space_parent(parent_id.to_owned(), room_id.to_owned()).sender(user_id),
);
}

for child_id in children {
builder = builder.add_state_event(
factory.space_child(room_id.to_owned(), child_id.to_owned()).sender(user_id),
);
}

server.sync_room(client, builder).await;
}

async fn add_rooms_with_power_level(
rooms: Vec<(&RoomId, i32)>,
client: &Client,
Expand Down
Loading