Skip to content

Don't delete devices marked as dehydrated devices #4268

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
merged 2 commits into from
Mar 19, 2025
+10 −2
Merged

Don't delete devices marked as dehydrated devices #4268

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20e2e24
don't delete devices marked as dehydrated devices
uhoreg Mar 14, 2025
9b8a824
change filter condition so that Some(false) does not signify dehydrated
reivilibre Mar 19, 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
12 changes: 10 additions & 2 deletions crates/matrix-synapse/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ struct SynapseDeviceListResponse {
#[derive(Serialize, Deserialize)]
struct SynapseDevice {
device_id: String,

#[serde(default, skip_serializing_if = "Option::is_none")]
dehydrated: Option<bool>,
}

#[derive(Serialize)]
Expand Down Expand Up @@ -316,6 +319,7 @@ impl HomeserverConnection for SynapseConnection {
.post(&format!("_synapse/admin/v2/users/{mxid}/devices"))
.json(&SynapseDevice {
device_id: device_id.to_owned(),
dehydrated: None,
})
.send_traced()
.await
Expand Down Expand Up @@ -410,8 +414,12 @@ impl HomeserverConnection for SynapseConnection {
.await
.context("Failed to parse response while querying devices from Synapse")?;

let existing_devices: HashSet<String> =
body.devices.into_iter().map(|d| d.device_id).collect();
let existing_devices: HashSet<String> = body
.devices
.into_iter()
.filter(|d| d.dehydrated.is_none())
Copy link
Contributor

Choose a reason for hiding this comment

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

we could also alternatively consider hiding dehydrated devices from the admin API, but in principle I think it does make sense to show them there, so happy with this

Copy link
Contributor

Choose a reason for hiding this comment

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

that said, shouldn't we have d.dehydrated != Some(true) here, or make dehydrated: bool with a default of false? (probably in favour of the latter since that's the simplest and the semantics match)

Copy link
Contributor

Choose a reason for hiding this comment

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

though the struct is serialised again, so Option<bool> makes sense unless we split the usage — I'll change the filter condition, push, merge

.map(|d| d.device_id)
.collect();

// First, delete all the devices that are not needed anymore
let to_delete = existing_devices.difference(&devices).cloned().collect();
Expand Down
Loading