From 15e1509fe37bf07e5a4a6bebdd40c648b66faaf4 Mon Sep 17 00:00:00 2001 From: Lennart <18233294+lennart-k@users.noreply.github.com> Date: Mon, 19 Jan 2026 14:48:21 +0100 Subject: [PATCH] sqlite_store: Add option to skip broken objects and add validation on start-up --- crates/caldav/src/calendar_object/methods.rs | 2 +- .../store_sqlite/src/addressbook_store/mod.rs | 61 ++++++++++-- crates/store_sqlite/src/calendar_store.rs | 94 ++++++++++++++++--- crates/store_sqlite/src/error.rs | 2 +- crates/store_sqlite/src/tests/mod.rs | 4 +- src/commands/mod.rs | 1 + src/config.rs | 2 + src/main.rs | 27 +++--- src/migration_0_12.rs | 76 --------------- 9 files changed, 156 insertions(+), 113 deletions(-) delete mode 100644 src/migration_0_12.rs diff --git a/crates/caldav/src/calendar_object/methods.rs b/crates/caldav/src/calendar_object/methods.rs index 3044aa7..873bc59 100644 --- a/crates/caldav/src/calendar_object/methods.rs +++ b/crates/caldav/src/calendar_object/methods.rs @@ -98,7 +98,7 @@ pub async fn put_event( Ok(object) => object, Err(err) => { warn!("invalid calendar data:\n{body}"); - warn!("{err:#?}"); + warn!("{err}"); return Err(Error::PreconditionFailed(Precondition::ValidCalendarData)); } }; diff --git a/crates/store_sqlite/src/addressbook_store/mod.rs b/crates/store_sqlite/src/addressbook_store/mod.rs index 4d85ff8..be71439 100644 --- a/crates/store_sqlite/src/addressbook_store/mod.rs +++ b/crates/store_sqlite/src/addressbook_store/mod.rs @@ -2,6 +2,7 @@ use super::ChangeOperation; use crate::BEGIN_IMMEDIATE; use async_trait::async_trait; use derive_more::derive::Constructor; +use ical::parser::ParserError; use rustical_ical::AddressObject; use rustical_store::{ Addressbook, AddressbookStore, CollectionMetadata, CollectionOperation, @@ -9,7 +10,7 @@ use rustical_store::{ }; use sqlx::{Acquire, Executor, Sqlite, SqlitePool, Transaction}; use tokio::sync::mpsc::Sender; -use tracing::{error_span, instrument, warn}; +use tracing::{error, error_span, instrument, warn}; pub mod birthday_calendar; @@ -18,6 +19,12 @@ struct AddressObjectRow { id: String, vcf: String, } +impl From for (String, Result) { + fn from(row: AddressObjectRow) -> Self { + let result = AddressObject::from_vcf(row.vcf); + (row.id, result) + } +} impl TryFrom for (String, AddressObject) { type Error = rustical_store::Error; @@ -31,6 +38,7 @@ impl TryFrom for (String, AddressObject) { pub struct SqliteAddressbookStore { db: SqlitePool, sender: Sender, + skip_broken: bool, } impl SqliteAddressbookStore { @@ -88,6 +96,36 @@ impl SqliteAddressbookStore { Ok(()) } + #[allow(clippy::missing_panics_doc)] + pub async fn validate_objects(&self, principal: &str) -> Result<(), Error> { + let mut success = true; + for addressbook in self.get_addressbooks(principal).await? { + for (object_id, res) in Self::_get_objects(&self.db, principal, &addressbook.id).await? + { + if let Err(err) = res { + warn!( + "Invalid address object found at {principal}/{addr_id}/{object_id}.vcf. Error: {err}", + addr_id = addressbook.id + ); + success = false; + } + } + } + if !success { + if self.skip_broken { + error!( + "Not all address objects are valid. Since data_store.sqlite.skip_broken=true they will be hidden. You are still advised to manually remove or repair the object. If you need help feel free to open up an issue on GitHub." + ); + } else { + error!( + "Not all address objects are valid. Since data_store.sqlite.skip_broken=false this causes a panic. Remove or repair the broken objects manually or set data_store.sqlite.skip_broken=false as a temporary solution to ignore the error. If you need help feel free to open up an issue on GitHub." + ); + panic!(); + } + } + Ok(()) + } + // Logs an operation to an address object async fn log_object_operation( tx: &mut Transaction<'_, Sqlite>, @@ -134,7 +172,7 @@ impl SqliteAddressbookStore { if let Err(err) = self.sender.try_send(CollectionOperation { topic, data }) { error_span!( "Error trying to send addressbook update notification:", - err = format!("{err:?}"), + err = format!("{err}"), ); } } @@ -353,8 +391,8 @@ impl SqliteAddressbookStore { executor: E, principal: &str, addressbook_id: &str, - ) -> Result, rustical_store::Error> { - sqlx::query_as!( + ) -> Result)>, Error> { + Ok(sqlx::query_as!( AddressObjectRow, "SELECT id, vcf FROM addressobjects WHERE principal = ? AND addressbook_id = ? AND deleted_at IS NULL", principal, @@ -363,8 +401,8 @@ impl SqliteAddressbookStore { .fetch_all(executor) .await.map_err(crate::Error::from)? .into_iter() - .map(std::convert::TryInto::try_into) - .collect() + .map(Into::into) + ) } async fn _get_object<'e, E: Executor<'e, Database = Sqlite>>( @@ -607,7 +645,16 @@ impl AddressbookStore for SqliteAddressbookStore { principal: &str, addressbook_id: &str, ) -> Result, rustical_store::Error> { - Self::_get_objects(&self.db, principal, addressbook_id).await + let objects = Self::_get_objects(&self.db, principal, addressbook_id).await?; + if self.skip_broken { + Ok(objects + .filter_map(|(id, res)| Some((id, res.ok()?))) + .collect()) + } else { + Ok(objects + .map(|(id, res)| res.map(|obj| (id, obj))) + .collect::, _>>()?) + } } #[instrument] diff --git a/crates/store_sqlite/src/calendar_store.rs b/crates/store_sqlite/src/calendar_store.rs index b06b085..7e84236 100644 --- a/crates/store_sqlite/src/calendar_store.rs +++ b/crates/store_sqlite/src/calendar_store.rs @@ -3,6 +3,7 @@ use crate::BEGIN_IMMEDIATE; use async_trait::async_trait; use chrono::TimeDelta; use derive_more::derive::Constructor; +use ical::parser::ParserError; use ical::types::CalDateTime; use regex::Regex; use rustical_ical::{CalendarObject, CalendarObjectType}; @@ -13,7 +14,7 @@ use rustical_store::{CollectionOperation, CollectionOperationInfo}; use sqlx::types::chrono::NaiveDateTime; use sqlx::{Acquire, Executor, Sqlite, SqlitePool, Transaction}; use tokio::sync::mpsc::Sender; -use tracing::{error_span, instrument, warn}; +use tracing::{error, error_span, instrument, warn}; #[derive(Debug, Clone)] struct CalendarObjectRow { @@ -22,6 +23,23 @@ struct CalendarObjectRow { uid: String, } +impl From for (String, Result) { + fn from(row: CalendarObjectRow) -> Self { + let result = CalendarObject::from_ics(row.ics).inspect(|object| { + if object.get_uid() != row.uid { + warn!( + "Calendar object {}.ics: UID={} and row uid={} do not match", + row.id, + object.get_uid(), + row.uid + ); + } + }); + + (row.id, result) + } +} + impl TryFrom for (String, CalendarObject) { type Error = rustical_store::Error; @@ -92,6 +110,7 @@ impl From for Calendar { pub struct SqliteCalendarStore { db: SqlitePool, sender: Sender, + skip_broken: bool, } impl SqliteCalendarStore { @@ -141,11 +160,40 @@ impl SqliteCalendarStore { if let Err(err) = self.sender.try_send(CollectionOperation { topic, data }) { error_span!( "Error trying to send calendar update notification:", - err = format!("{err:?}"), + err = format!("{err}"), ); } } + #[allow(clippy::missing_panics_doc)] + pub async fn validate_objects(&self, principal: &str) -> Result<(), Error> { + let mut success = true; + for calendar in self.get_calendars(principal).await? { + for (object_id, res) in Self::_get_objects(&self.db, principal, &calendar.id).await? { + if let Err(err) = res { + warn!( + "Invalid calendar object found at {principal}/{cal_id}/{object_id}.ics. Error: {err}", + cal_id = calendar.id + ); + success = false; + } + } + } + if !success { + if self.skip_broken { + error!( + "Not all calendar objects are valid. Since data_store.sqlite.skip_broken=true they will be hidden. You are still advised to manually remove or repair the object. If you need help feel free to open up an issue on GitHub." + ); + } else { + error!( + "Not all calendar objects are valid. Since data_store.sqlite.skip_broken=false this causes a panic. Remove or repair the broken objects manually or set data_store.sqlite.skip_broken=false as a temporary solution to ignore the error. If you need help feel free to open up an issue on GitHub." + ); + panic!(); + } + } + Ok(()) + } + /// In the past exports generated objects with invalid VERSION:4.0 /// This repair sets them to VERSION:2.0 #[allow(clippy::missing_panics_doc)] @@ -456,8 +504,8 @@ impl SqliteCalendarStore { executor: E, principal: &str, cal_id: &str, - ) -> Result, Error> { - sqlx::query_as!( + ) -> Result)>, Error> { + Ok(sqlx::query_as!( CalendarObjectRow, "SELECT id, uid, ics FROM calendarobjects WHERE principal = ? AND cal_id = ? AND deleted_at IS NULL", principal, @@ -466,8 +514,8 @@ impl SqliteCalendarStore { .fetch_all(executor) .await.map_err(crate::Error::from)? .into_iter() - .map(std::convert::TryInto::try_into) - .collect() + .map(Into::into) + ) } async fn _calendar_query<'e, E: Executor<'e, Database = Sqlite>>( @@ -475,14 +523,14 @@ impl SqliteCalendarStore { principal: &str, cal_id: &str, query: CalendarQuery, - ) -> Result, Error> { + ) -> Result)>, Error> { // We extend our query interval by one day in each direction since we really don't want to // miss any objects because of timezone differences // I've previously tried NaiveDate::MIN,MAX, but it seems like sqlite cannot handle these let start = query.time_start.map(|start| start - TimeDelta::days(1)); let end = query.time_end.map(|end| end + TimeDelta::days(1)); - sqlx::query_as!( + Ok(sqlx::query_as!( CalendarObjectRow, r"SELECT id, uid, ics FROM calendarobjects WHERE principal = ? AND cal_id = ? AND deleted_at IS NULL @@ -500,8 +548,7 @@ impl SqliteCalendarStore { .await .map_err(crate::Error::from)? .into_iter() - .map(std::convert::TryInto::try_into) - .collect() + .map(Into::into)) } async fn _get_object<'e, E: Executor<'e, Database = Sqlite>>( @@ -641,6 +688,7 @@ impl SqliteCalendarStore { principal: &str, cal_id: &str, synctoken: i64, + skip_broken: bool, ) -> Result<(Vec<(String, CalendarObject)>, Vec, i64), Error> { struct Row { object_id: String, @@ -670,6 +718,8 @@ impl SqliteCalendarStore { match Self::_get_object(&mut *conn, principal, cal_id, &object_id, false).await { Ok(object) => objects.push((object_id, object)), Err(rustical_store::Error::NotFound) => deleted_objects.push(object_id), + // Skip broken object + Err(rustical_store::Error::IcalError(_)) if skip_broken => (), Err(err) => return Err(err), } } @@ -820,7 +870,16 @@ impl CalendarStore for SqliteCalendarStore { cal_id: &str, query: CalendarQuery, ) -> Result, Error> { - Self::_calendar_query(&self.db, principal, cal_id, query).await + let objects = Self::_calendar_query(&self.db, principal, cal_id, query).await?; + if self.skip_broken { + Ok(objects + .filter_map(|(id, res)| Some((id, res.ok()?))) + .collect()) + } else { + Ok(objects + .map(|(id, res)| res.map(|obj| (id, obj))) + .collect::, _>>()?) + } } async fn calendar_metadata( @@ -851,7 +910,16 @@ impl CalendarStore for SqliteCalendarStore { principal: &str, cal_id: &str, ) -> Result, Error> { - Self::_get_objects(&self.db, principal, cal_id).await + let objects = Self::_get_objects(&self.db, principal, cal_id).await?; + if self.skip_broken { + Ok(objects + .filter_map(|(id, res)| Some((id, res.ok()?))) + .collect()) + } else { + Ok(objects + .map(|(id, res)| res.map(|obj| (id, obj))) + .collect::, _>>()?) + } } #[instrument] @@ -974,7 +1042,7 @@ impl CalendarStore for SqliteCalendarStore { cal_id: &str, synctoken: i64, ) -> Result<(Vec<(String, CalendarObject)>, Vec, i64), Error> { - Self::_sync_changes(&self.db, principal, cal_id, synctoken).await + Self::_sync_changes(&self.db, principal, cal_id, synctoken, self.skip_broken).await } fn is_read_only(&self, _cal_id: &str) -> bool { diff --git a/crates/store_sqlite/src/error.rs b/crates/store_sqlite/src/error.rs index 38e2af8..96e598a 100644 --- a/crates/store_sqlite/src/error.rs +++ b/crates/store_sqlite/src/error.rs @@ -18,7 +18,7 @@ impl From for Error { sqlx::Error::RowNotFound => Self::StoreError(rustical_store::Error::NotFound), sqlx::Error::Database(err) => { if err.is_unique_violation() { - warn!("{err:?}"); + warn!("{err}"); Self::StoreError(rustical_store::Error::AlreadyExists) } else { Self::SqlxError(sqlx::Error::Database(err)) diff --git a/crates/store_sqlite/src/tests/mod.rs b/crates/store_sqlite/src/tests/mod.rs index aee4f21..2f33ead 100644 --- a/crates/store_sqlite/src/tests/mod.rs +++ b/crates/store_sqlite/src/tests/mod.rs @@ -52,8 +52,8 @@ pub async fn test_store_context() -> TestStoreContext { let db = get_test_db().await; TestStoreContext { db: db.clone(), - addr_store: SqliteAddressbookStore::new(db.clone(), send_addr), - cal_store: SqliteCalendarStore::new(db.clone(), send_cal), + addr_store: SqliteAddressbookStore::new(db.clone(), send_addr, false), + cal_store: SqliteCalendarStore::new(db.clone(), send_cal, false), principal_store: SqlitePrincipalStore::new(db.clone()), sub_store: SqliteStore::new(db), } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 4f22346..7614232 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -18,6 +18,7 @@ pub fn cmd_gen_config(_args: GenConfigArgs) -> anyhow::Result<()> { data_store: DataStoreConfig::Sqlite(SqliteDataStoreConfig { db_url: "/var/lib/rustical/db.sqlite3".to_owned(), run_repairs: true, + skip_broken: true, }), tracing: TracingConfig::default(), frontend: FrontendConfig { diff --git a/src/config.rs b/src/config.rs index b559f8b..3a23f66 100644 --- a/src/config.rs +++ b/src/config.rs @@ -28,6 +28,8 @@ pub struct SqliteDataStoreConfig { pub db_url: String, #[serde(default = "default_true")] pub run_repairs: bool, + #[serde(default = "default_true")] + pub skip_broken: bool, } #[derive(Debug, Deserialize, Serialize)] diff --git a/src/main.rs b/src/main.rs index b29766a..ae7896d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -34,9 +34,6 @@ mod config; pub mod integration_tests; mod setup_tracing; -mod migration_0_12; -use migration_0_12::{validate_address_objects_0_12, validate_calendar_objects_0_12}; - #[derive(Parser, Debug)] #[command(author, version, about, long_about = None)] struct Args { @@ -73,13 +70,18 @@ async fn get_data_stores( DataStoreConfig::Sqlite(SqliteDataStoreConfig { db_url, run_repairs, + skip_broken, }) => { let db = create_db_pool(db_url, migrate).await?; // Channel to watch for changes (for DAV Push) let (send, recv) = tokio::sync::mpsc::channel(1000); - let addressbook_store = Arc::new(SqliteAddressbookStore::new(db.clone(), send.clone())); - let cal_store = Arc::new(SqliteCalendarStore::new(db.clone(), send)); + let addressbook_store = Arc::new(SqliteAddressbookStore::new( + db.clone(), + send.clone(), + *skip_broken, + )); + let cal_store = Arc::new(SqliteCalendarStore::new(db.clone(), send, *skip_broken)); if *run_repairs { info!("Running repair tasks"); addressbook_store.repair_orphans().await?; @@ -88,6 +90,13 @@ async fn get_data_stores( } let subscription_store = Arc::new(SqliteStore::new(db.clone())); let principal_store = Arc::new(SqlitePrincipalStore::new(db)); + + // Validate all calendar objects + for principal in principal_store.get_principals().await? { + cal_store.validate_objects(&principal.id).await?; + addressbook_store.validate_objects(&principal.id).await?; + } + ( addressbook_store, cal_store, @@ -125,14 +134,6 @@ async fn main() -> Result<()> { let (addr_store, cal_store, subscription_store, principal_store, update_recv) = get_data_stores(!args.no_migrations, &config.data_store).await?; - warn!( - "Validating calendar data against the next-version ical parser. -In the next major release these will be rejected and cause errors. -If any errors occur, please open an issue so they can be fixed before the next major release." - ); - validate_calendar_objects_0_12(principal_store.as_ref(), cal_store.as_ref()).await?; - validate_address_objects_0_12(principal_store.as_ref(), addr_store.as_ref()).await?; - let mut tasks = vec![]; if config.dav_push.enabled { diff --git a/src/migration_0_12.rs b/src/migration_0_12.rs deleted file mode 100644 index f661f8a..0000000 --- a/src/migration_0_12.rs +++ /dev/null @@ -1,76 +0,0 @@ -use ical::parser::{ical::IcalObjectParser, vcard::VcardParser}; -use rustical_store::{AddressbookStore, CalendarStore, auth::AuthenticationProvider}; -use tracing::{error, info}; - -pub async fn validate_calendar_objects_0_12( - principal_store: &impl AuthenticationProvider, - cal_store: &impl CalendarStore, -) -> Result<(), rustical_store::Error> { - let mut success = true; - for principal in principal_store.get_principals().await? { - for calendar in cal_store.get_calendars(&principal.id).await? { - for (object_id, object) in cal_store - .get_objects(&calendar.principal, &calendar.id) - .await? - { - if let Err(err) = - IcalObjectParser::from_slice(object.get_ics().as_bytes()).expect_one() - { - success = false; - error!( - "An error occured parsing a calendar object: principal={principal}, calendar={calendar}, object_id={object_id}: {err}", - principal = principal.id, - calendar = calendar.id, - ); - println!("{}", object.get_ics()); - } - } - } - } - if success { - info!("Your calendar data seems to be valid in the next major version."); - } else { - error!( - "Not all calendar objects will be successfully parsed in the next major version (v0.12). -This will not cause issues in this version, but please comment under the tracking issue on GitHub: -https://github.com/lennart-k/rustical/issues/165" - ); - } - Ok(()) -} - -pub async fn validate_address_objects_0_12( - principal_store: &impl AuthenticationProvider, - addr_store: &impl AddressbookStore, -) -> Result<(), rustical_store::Error> { - let mut success = true; - for principal in principal_store.get_principals().await? { - for addressbook in addr_store.get_addressbooks(&principal.id).await? { - for (object_id, object) in addr_store - .get_objects(&addressbook.principal, &addressbook.id) - .await? - { - if let Err(err) = VcardParser::from_slice(object.get_vcf().as_bytes()).expect_one() - { - success = false; - error!( - "An error occured parsing an address object: principal={principal}, addressbook={addressbook}, object_id={object_id}: {err}", - principal = principal.id, - addressbook = addressbook.id, - ); - println!("{}", object.get_vcf()); - } - } - } - } - if success { - info!("Your addressbook data seems to be valid in the next major version."); - } else { - error!( - "Not all address objects will be successfully parsed in the next major version (v0.12). -This will not cause issues in this version, but please comment under the tracking issue on GitHub: -https://github.com/lennart-k/rustical/issues/165" - ); - } - Ok(()) -}