From c1f3f742fe03f8c9583a9b3aa316545c537cc41b Mon Sep 17 00:00:00 2001 From: Maurus Decimus <11444311+mdecimus@users.noreply.github.com> Date: Mon, 15 Jun 2026 21:28:27 +0200 Subject: [PATCH] Mailbox roles must be unique per archive (fixes #8) --- CHANGELOG.md | 13 ++ Cargo.lock | 2 +- Cargo.toml | 2 +- src/db/mod.rs | 1 + src/db/roles.rs | 90 ++++++++++ src/sync/export/tree.rs | 19 ++- src/sync/import_exchange_ews/folders.rs | 4 + src/sync/import_exchange_graph/folders.rs | 2 + src/sync/import_imap/coordinator.rs | 6 +- src/sync/import_jmap/mapping.rs | 6 +- tests/mock_sync.rs | 198 ++++++++++++++++++++++ 11 files changed, 336 insertions(+), 7 deletions(-) create mode 100644 src/db/roles.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cd7538..936843b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [1.0.3] - 2026-06-XX + +### Added + +### Changed + +### Fixed +- Mailbox roles must be unique per archive (#8). + +# Change Log + +All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). + ## [1.0.2] - 2026-06-11 ### Added diff --git a/Cargo.lock b/Cargo.lock index 49160a3..6fa54e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2939,7 +2939,7 @@ dependencies = [ [[package]] name = "vandelay" -version = "1.0.2" +version = "1.0.3" dependencies = [ "base64", "blake3", diff --git a/Cargo.toml b/Cargo.toml index a8c424a..545cc64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "vandelay" description = "JMAP account migration utility" -version = "1.0.2" +version = "1.0.3" authors = ["Stalwart Labs LLC "] license = "Apache-2.0 OR MIT" repository = "https://github.com/stalwartlabs/vandelay" diff --git a/src/db/mod.rs b/src/db/mod.rs index dd1293d..26c401a 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -14,6 +14,7 @@ pub mod imap_state; pub mod init; pub mod maildir_ids; pub mod managesieve_ids; +pub mod roles; pub mod sources; pub mod sync_state_jmap; pub mod takeout_ids; diff --git a/src/db/roles.rs b/src/db/roles.rs new file mode 100644 index 0000000..c9a48bc --- /dev/null +++ b/src/db/roles.rs @@ -0,0 +1,90 @@ +/* + * SPDX-FileCopyrightText: 2020 Stalwart Labs LLC + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + */ + +use rusqlite::{Connection, params}; + +pub fn unique_role( + conn: &Connection, + role: Option<&str>, + exclude_id: Option, +) -> rusqlite::Result> { + let Some(r) = role else { + return Ok(None); + }; + let taken: bool = conn.query_row( + "SELECT EXISTS(SELECT 1 FROM mailboxes WHERE role = ?1 AND (?2 IS NULL OR id != ?2))", + params![r, exclude_id], + |row| row.get(0), + )?; + Ok((!taken).then(|| r.to_owned())) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::init; + + fn mem() -> Connection { + let conn = Connection::open_in_memory().unwrap(); + init::apply_schema(&conn).unwrap(); + conn + } + + fn insert(conn: &Connection, name: &str, role: Option<&str>) -> i64 { + conn.execute( + "INSERT INTO mailboxes (name, parent_id, role, sort_order, is_subscribed) + VALUES (?1, NULL, ?2, 0, 1)", + params![name, role], + ) + .unwrap(); + conn.last_insert_rowid() + } + + #[test] + fn first_claimant_keeps_role_second_is_dropped() { + let conn = mem(); + let first = unique_role(&conn, Some("sent"), None).unwrap(); + assert_eq!(first.as_deref(), Some("sent")); + insert(&conn, "Sent", first.as_deref()); + + let second = unique_role(&conn, Some("sent"), None).unwrap(); + assert_eq!(second, None); + } + + #[test] + fn null_role_stays_null() { + let conn = mem(); + assert_eq!(unique_role(&conn, None, None).unwrap(), None); + } + + #[test] + fn distinct_roles_are_independent() { + let conn = mem(); + insert(&conn, "Sent", Some("sent")); + assert_eq!( + unique_role(&conn, Some("trash"), None).unwrap().as_deref(), + Some("trash") + ); + } + + #[test] + fn update_excludes_own_row() { + let conn = mem(); + let id = insert(&conn, "Sent", Some("sent")); + assert_eq!( + unique_role(&conn, Some("sent"), Some(id)).unwrap().as_deref(), + Some("sent") + ); + } + + #[test] + fn update_detects_other_holder() { + let conn = mem(); + insert(&conn, "Sent", Some("sent")); + let other = insert(&conn, "Envoyés", None); + assert_eq!(unique_role(&conn, Some("sent"), Some(other)).unwrap(), None); + } +} diff --git a/src/sync/export/tree.rs b/src/sync/export/tree.rs index 6b5390b..3b1b645 100644 --- a/src/sync/export/tree.rs +++ b/src/sync/export/tree.rs @@ -160,6 +160,8 @@ pub fn reconcile( .unwrap_or(0); let mut uploader = Uploader::new(net, &ctx.conn); + let mut taken_roles: HashSet = + targets.iter().filter_map(|t| t.role.clone()).collect(); let interleave = ty == ObjectType::FileNode; for d in 0..=max_depth { let level: Vec<&LocalNode> = to_create @@ -225,7 +227,22 @@ pub fn reconcile( continue; } match build_create(ctx, ty, n.local, maps, &mut uploader) { - Ok(obj) => { + Ok(mut obj) => { + if ty == ObjectType::Mailbox + && let Some(r) = n.role.as_deref() + { + if taken_roles.contains(r) { + if let Value::Object(m) = &mut obj { + m.remove("role"); + } + logger.warn(&format!( + "Mailbox local {}: role '{r}' already present on target, creating as a plain folder", + n.local + )); + } else { + taken_roles.insert(r.to_owned()); + } + } batch.push((format!("c{}", n.local), obj)); } Err(e) => { diff --git a/src/sync/import_exchange_ews/folders.rs b/src/sync/import_exchange_ews/folders.rs index 6a19720..1df194b 100644 --- a/src/sync/import_exchange_ews/folders.rs +++ b/src/sync/import_exchange_ews/folders.rs @@ -380,6 +380,8 @@ fn upsert_mailbox( .map_err(|e| Error::Partial(e.to_string()))?; let existing = ctx.local.get(&folder.folder.folder_id.id); let local_id = if let Some(row) = existing { + let role = crate::db::roles::unique_role(&tx, role, Some(row.local_id)) + .map_err(|e| Error::Partial(e.to_string()))?; tx.execute( "UPDATE mailboxes SET name = ?1, parent_id = ?2, role = ?3 WHERE id = ?4", params![folder.folder.display_name, parent_local, role, row.local_id], @@ -396,6 +398,8 @@ fn upsert_mailbox( ctx.counts.fetched += 1; row.local_id } else { + let role = crate::db::roles::unique_role(&tx, role, None) + .map_err(|e| Error::Partial(e.to_string()))?; tx.execute( "INSERT INTO mailboxes (name, parent_id, role, sort_order, is_subscribed) \ VALUES (?1, ?2, ?3, 0, 1)", diff --git a/src/sync/import_exchange_graph/folders.rs b/src/sync/import_exchange_graph/folders.rs index b9468b7..37a4b2d 100644 --- a/src/sync/import_exchange_graph/folders.rs +++ b/src/sync/import_exchange_graph/folders.rs @@ -91,6 +91,7 @@ pub fn reconcile_mail( server_ids.push(graph_id.to_owned()); let local_id = if let Some(id) = existing { + let role = crate::db::roles::unique_role(&tx, role, Some(id))?; tx.execute( "UPDATE mailboxes SET name = ?1, parent_id = ?2, role = ?3, is_subscribed = ?4 WHERE id = ?5", @@ -99,6 +100,7 @@ pub fn reconcile_mail( counts.fetched += 1; id } else { + let role = crate::db::roles::unique_role(&tx, role, None)?; tx.execute( "INSERT INTO mailboxes (name, parent_id, role, sort_order, is_subscribed) VALUES (?1, ?2, ?3, 0, ?4)", diff --git a/src/sync/import_imap/coordinator.rs b/src/sync/import_imap/coordinator.rs index 75d0b42..6159f0f 100644 --- a/src/sync/import_imap/coordinator.rs +++ b/src/sync/import_imap/coordinator.rs @@ -564,26 +564,28 @@ fn upsert_mailboxes( }; let existing = db::imap_ids::local_for_mailbox(&tx, source_id, &folder.name)?; let id = if let Some(id) = existing { + let role = db::roles::unique_role(&tx, folder.role, Some(id))?; tx.execute( "UPDATE mailboxes SET name = ?1, parent_id = ?2, role = ?3, is_subscribed = ?4 WHERE id = ?5", params![ folder.leaf, parent_local, - folder.role, + role, folder.subscribed as i64, id ], )?; id } else { + let role = db::roles::unique_role(&tx, folder.role, None)?; tx.execute( "INSERT INTO mailboxes (name, parent_id, role, sort_order, is_subscribed) VALUES (?1, ?2, ?3, 0, ?4)", params![ folder.leaf, parent_local, - folder.role, + role, folder.subscribed as i64 ], )?; diff --git a/src/sync/import_jmap/mapping.rs b/src/sync/import_jmap/mapping.rs index 9507fef..0727b97 100644 --- a/src/sync/import_jmap/mapping.rs +++ b/src/sync/import_jmap/mapping.rs @@ -113,13 +113,14 @@ pub fn insert_mailbox( resolver: &impl LocalResolver, ) -> Result { let parent = opt_parent(resolver, ObjectType::Mailbox, &wire.parent_id); + let role = crate::db::roles::unique_role(conn, wire.role.as_deref(), None)?; conn.execute( "INSERT INTO mailboxes (name, parent_id, role, sort_order, is_subscribed) VALUES (?1, ?2, ?3, ?4, ?5)", params![ wire.name, parent, - wire.role, + role, wire.sort_order, wire.is_subscribed as i64 ], @@ -401,6 +402,7 @@ pub fn update_mailbox( resolver: &impl LocalResolver, ) -> Result { let parent = opt_parent(resolver, ObjectType::Mailbox, &wire.parent_id); + let role = crate::db::roles::unique_role(conn, wire.role.as_deref(), Some(local_id))?; let n = conn.execute( "UPDATE mailboxes SET name = ?1, parent_id = ?2, role = ?3, sort_order = ?4, is_subscribed = ?5 @@ -409,7 +411,7 @@ pub fn update_mailbox( params![ wire.name, parent, - wire.role, + role, wire.sort_order, wire.is_subscribed as i64, local_id diff --git a/tests/mock_sync.rs b/tests/mock_sync.rs index 63bc2ed..d1c2916 100644 --- a/tests/mock_sync.rs +++ b/tests/mock_sync.rs @@ -2347,3 +2347,201 @@ fn first_run_cursor_is_captured_up_front_not_from_the_fetch() { ); let _ = std::fs::remove_file(&archive); } + +#[test] +fn export_duplicate_role_mailbox_created_as_plain_folder_keeping_subtree() { + let mut server = mockito::Server::new(); + let base = server.url(); + let api = "/jmap/api"; + let archive = tmp(); + { + let conn = db::init::open(&archive).unwrap(); + conn.execute( + "INSERT INTO mailboxes (id,name,parent_id,role) VALUES (1,'Inbox',NULL,'inbox')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO mailboxes (id,name,parent_id,role) VALUES (2,'Sent',NULL,'sent')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO mailboxes (id,name,parent_id,role) VALUES (3,'Éléments envoyés',NULL,'sent')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO mailboxes (id,name,parent_id,role) VALUES (4,'Brouillons locaux',3,NULL)", + [], + ) + .unwrap(); + } + + let _root = server.mock("GET", "/").with_status(404).create(); + let _wk = server + .mock("GET", "/.well-known/jmap") + .with_body(session_body_full(&base)) + .expect_at_least(1) + .create(); + let _mbterm = anchor_terminator(&mut server, api, "Mailbox"); + + let _mq = server + .mock("POST", api) + .match_body(Matcher::Regex("Mailbox/query".into())) + .with_body( + json!({"methodResponses":[["Mailbox/query", + {"accountId":"w","ids":["TI","TS"]},"q"]]}) + .to_string(), + ) + .expect_at_least(1) + .create(); + let _mg = server + .mock("POST", api) + .match_body(Matcher::Regex("Mailbox/get".into())) + .with_body( + json!({"methodResponses":[["Mailbox/get",{"accountId":"w","list":[ + {"id":"TI","name":"Inbox","role":"inbox","parentId":null,"myRights":{"mayDelete":true}}, + {"id":"TS","name":"Sent","role":"sent","parentId":null,"myRights":{"mayDelete":true}} + ],"notFound":[]},"g"]]}) + .to_string(), + ) + .expect_at_least(1) + .create(); + + let role_create = server + .mock("POST", api) + .match_body(Matcher::AllOf(vec![ + Matcher::Regex("Mailbox/set".into()), + Matcher::Regex("envoy".into()), + Matcher::Regex("\"role\"".into()), + ])) + .with_body( + json!({"methodResponses":[["Mailbox/set",{"accountId":"w", + "notCreated":{"c3":{"type":"invalidProperties","properties":["role"], + "description":"A mailbox with role 'sent' already exists."}}},"s"]]}) + .to_string(), + ) + .expect_at_most(1) + .create(); + let folder_create = server + .mock("POST", api) + .match_body(Matcher::AllOf(vec![ + Matcher::Regex("Mailbox/set".into()), + Matcher::Regex("envoy".into()), + ])) + .with_body( + json!({"methodResponses":[["Mailbox/set",{"accountId":"w", + "created":{"c3":{"id":"M3"}}},"s"]]}) + .to_string(), + ) + .expect(1) + .create(); + let child_create = server + .mock("POST", api) + .match_body(Matcher::AllOf(vec![ + Matcher::Regex("Mailbox/set".into()), + Matcher::Regex("Brouillons".into()), + ])) + .with_body( + json!({"methodResponses":[["Mailbox/set",{"accountId":"w", + "created":{"c4":{"id":"M4"}}},"s"]]}) + .to_string(), + ) + .expect(1) + .create(); + + let summary = sync::export::run( + common(&archive), + export_cfg_objects(&base, vec![ObjectType::Mailbox]), + ) + .expect("export"); + + role_create.assert(); + folder_create.assert(); + child_create.assert(); + + let counts = summary + .per_type + .iter() + .find(|(t, _)| *t == "Mailbox") + .map(|(_, c)| c.clone()) + .expect("mailbox counts"); + assert_eq!(counts.skipped, 2, "Inbox and Sent match existing role mailboxes"); + assert_eq!( + counts.created, 2, + "duplicate-role folder and its child are both created" + ); + assert_eq!(counts.failed, 0, "no cascade skip of the subtree"); + let _ = std::fs::remove_file(&archive); +} + +#[test] +fn import_jmap_duplicate_role_is_deduplicated_to_single_mailbox() { + let mut server = mockito::Server::new(); + let base = server.url(); + let api = "/jmap/api"; + let archive = tmp(); + + let _root = server.mock("GET", "/").with_status(404).create(); + let _wk = server + .mock("GET", "/.well-known/jmap") + .with_body(session_body_full(&base)) + .expect_at_least(1) + .create(); + let _term = anchor_terminator(&mut server, api, "Mailbox"); + + let _q = server + .mock("POST", api) + .match_body(Matcher::Regex("Mailbox/query".into())) + .with_body( + json!({"methodResponses":[["Mailbox/query", + {"accountId":"w","ids":["A","B","C"]},"q"]]}) + .to_string(), + ) + .expect(1) + .create(); + let _g = server + .mock("POST", api) + .match_body(Matcher::Regex("Mailbox/get".into())) + .with_body( + json!({"methodResponses":[["Mailbox/get",{"accountId":"w","state":"s1","list":[ + {"id":"A","name":"Sent","parentId":null,"role":"sent","sortOrder":0,"isSubscribed":true}, + {"id":"B","name":"Éléments envoyés","parentId":null,"role":"sent","sortOrder":0,"isSubscribed":true}, + {"id":"C","name":"Sent Items","parentId":null,"role":"sent","sortOrder":0,"isSubscribed":true} + ],"notFound":[]},"g"]]}) + .to_string(), + ) + .expect(1) + .create(); + + sync::import_jmap::run( + common(&archive), + import_cfg_objects(&base, vec![ObjectType::Mailbox]), + ) + .expect("import"); + + let conn = rusqlite::Connection::open(&archive).unwrap(); + let total: i64 = conn + .query_row("SELECT count(*) FROM mailboxes", [], |r| r.get(0)) + .unwrap(); + assert_eq!(total, 3, "all three folders are imported"); + let with_role: i64 = conn + .query_row( + "SELECT count(*) FROM mailboxes WHERE role = 'sent'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(with_role, 1, "exactly one mailbox keeps the sent role"); + let null_roles: i64 = conn + .query_row( + "SELECT count(*) FROM mailboxes WHERE role IS NULL", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(null_roles, 2, "the surplus duplicates become plain folders"); + drop(conn); + let _ = std::fs::remove_file(&archive); +}