From 184b9dfab6d789881c380ba7ef3209b729bb9e3a Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 14 Jul 2026 16:00:56 +0200 Subject: [PATCH 1/3] fix(528): ical and recurrence import use if ical and use ical::IcalParser prepare unit test --- Cargo.lock | 10 + Cargo.toml | 15 + src/domain/entities/calendar_event.rs | 464 +++++++++++++++++++++----- 3 files changed, 410 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed556920..7f61eefc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3155,6 +3155,15 @@ dependencies = [ "cc", ] +[[package]] +name = "ical" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b7cab7543a8b7729a19e2c04309f902861293dcdae6558dfbeb634454d279f6" +dependencies = [ + "thiserror 1.0.69", +] + [[package]] name = "icu_collections" version = "2.2.0" @@ -4247,6 +4256,7 @@ dependencies = [ "http-body 1.0.1", "http-body-util", "http-range-header", + "ical", "id3", "idna", "image", diff --git a/Cargo.toml b/Cargo.toml index e512b4db..c02862b1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,21 @@ flate2 = "1.1.9" tracing = "0.1.44" tracing-subscriber = { version = "0.3.23", features = ["env-filter"] } chrono = { version = "0.4.45", features = ["serde"] } +# RFC 5545 iCalendar parser + emitter. +# +# Adopted 2026-07-14 to replace the hand-rolled property-scan in +# `src/domain/entities/calendar_event.rs::extract_ical_property`, +# which used a naive `format!("\n{}:", name)` substring search and +# refused ANY property carrying parameters (`DTSTART;VALUE=DATE:...`, +# `RECURRENCE-ID;VALUE=DATE:...`, `ATTENDEE;CN=…;PARTSTAT=…:mailto:…`). +# That broke all-day events and made the domain unaware of exception +# instances (see AtalayaLabs/OxiCloud#528). +# +# The crate is the widely-used Rust parser (~1500 SLOC, MIT/Apache), +# actively maintained by @Peltoche as `ical-rs` on GitHub. It handles +# line-folding, escaped characters, parameter maps, and every standard +# component. If a spec conformance gap is found, we contribute upstream. +ical = "0.11" http-body = "1.0.1" serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.150" diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index ccb301b0..65dbd804 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -237,24 +237,44 @@ impl CalendarEvent { ) })?; - let dtstart = Self::extract_ical_property(&ical_data, "DTSTART").ok_or_else(|| { - DomainError::new( - ErrorKind::InvalidInput, - "CalendarEvent", - "Missing DTSTART in iCalendar data", - ) - })?; + // DTSTART / DTEND: use the params-aware extractor so we can + // detect `VALUE=DATE` (all-day) from the property parameters + // rather than scanning the raw property line. The pre-parser- + // rewrite substring scan couldn't see param-carrying lines at + // all — see #528. + let (dtstart_value, dtstart_params) = + Self::extract_ical_property_with_params(&ical_data, "DTSTART").ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "CalendarEvent", + "Missing DTSTART in iCalendar data", + ) + })?; - let dtend = Self::extract_ical_property(&ical_data, "DTEND").ok_or_else(|| { - DomainError::new( - ErrorKind::InvalidInput, - "CalendarEvent", - "Missing DTEND in iCalendar data", - ) - })?; + let (dtend_value, _dtend_params) = + Self::extract_ical_property_with_params(&ical_data, "DTEND").ok_or_else(|| { + DomainError::new( + ErrorKind::InvalidInput, + "CalendarEvent", + "Missing DTEND in iCalendar data", + ) + })?; - // Parse dates (simplified) - let start_time = Self::parse_ical_datetime(&dtstart).map_err(|e| { + // All-day detection: `VALUE=DATE` parameter on DTSTART. + // Falls back to `false` when the parameter is absent, matching + // RFC 5545 §3.3.4 ("If the property permits, multiple 'VALUE' + // parameters can be specified as a comma-separated list") — + // we're strict: only "DATE" (case-insensitive) counts, "DATE-TIME" + // and anything else means timed. + let all_day = dtstart_params + .get("VALUE") + .map(|vs| { + vs.iter() + .any(|v| v.eq_ignore_ascii_case("DATE")) + }) + .unwrap_or(false); + + let start_time = Self::parse_ical_datetime(&dtstart_value, all_day).map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "CalendarEvent", @@ -262,7 +282,7 @@ impl CalendarEvent { ) })?; - let end_time = Self::parse_ical_datetime(&dtend).map_err(|e| { + let end_time = Self::parse_ical_datetime(&dtend_value, all_day).map_err(|e| { DomainError::new( ErrorKind::InvalidInput, "CalendarEvent", @@ -270,9 +290,6 @@ impl CalendarEvent { ) })?; - // Determine if all-day event (simplified check) - let all_day = dtstart.contains("VALUE=DATE") && !dtstart.contains("T"); - // Extract optional fields let description = Self::extract_ical_property(&ical_data, "DESCRIPTION"); let location = Self::extract_ical_property(&ical_data, "LOCATION"); @@ -557,23 +574,30 @@ impl CalendarEvent { self.description = Self::extract_ical_property(&ical_data, "DESCRIPTION"); self.location = Self::extract_ical_property(&ical_data, "LOCATION"); - if let Some(dtstart) = Self::extract_ical_property(&ical_data, "DTSTART") - && let Ok(start_time) = Self::parse_ical_datetime(&dtstart) + // Extract DTSTART with parameters — needed for the all-day + // detection below AND for the DTSTART/DTEND datetime parsers + // (they need to know whether the value is a date or a datetime). + let dtstart_pair = Self::extract_ical_property_with_params(&ical_data, "DTSTART"); + let all_day = dtstart_pair + .as_ref() + .and_then(|(_v, params)| params.get("VALUE")) + .map(|vs| vs.iter().any(|v| v.eq_ignore_ascii_case("DATE"))) + .unwrap_or(false); + self.all_day = all_day; + + if let Some((value, _params)) = &dtstart_pair + && let Ok(start_time) = Self::parse_ical_datetime(value, all_day) { self.start_time = start_time; } - if let Some(dtend) = Self::extract_ical_property(&ical_data, "DTEND") - && let Ok(end_time) = Self::parse_ical_datetime(&dtend) + if let Some((value, _params)) = + Self::extract_ical_property_with_params(&ical_data, "DTEND") + && let Ok(end_time) = Self::parse_ical_datetime(&value, all_day) { self.end_time = end_time; } - // Update all-day status based on DTSTART - if let Some(dtstart) = Self::extract_ical_property(&ical_data, "DTSTART") { - self.all_day = dtstart.contains("VALUE=DATE") && !dtstart.contains("T"); - } - self.rrule = Self::extract_ical_property(&ical_data, "RRULE"); if let Some(uid) = Self::extract_ical_property(&ical_data, "UID") { @@ -620,17 +644,20 @@ impl CalendarEvent { // or if it ended after the start of our range if let Some(until_pos) = rrule.find("UNTIL=") { let until_start = until_pos + 6; // "UNTIL=" is 6 chars - if let Some(until_end) = rrule[until_start..].find(';') { - let until_str = &rrule[until_start..until_start + until_end]; - if let Ok(until_date) = Self::parse_ical_datetime(until_str) { - return until_date >= *start; - } + let until_str = if let Some(until_end) = rrule[until_start..].find(';') { + &rrule[until_start..until_start + until_end] } else { // UNTIL is the last part of the rule - let until_str = &rrule[until_start..]; - if let Ok(until_date) = Self::parse_ical_datetime(until_str) { - return until_date >= *start; - } + &rrule[until_start..] + }; + // RFC 5545 §3.3.10 — UNTIL is either a DATE (`YYYYMMDD`, + // 8 chars) or a DATE-TIME (`YYYYMMDDTHHMMSSZ`, 16 chars, + // trailing Z). Distinguish by shape: exactly 8 chars ⇒ + // date-only. Everything else is treated as datetime and + // parsed accordingly. + let is_date_only = until_str.len() == 8; + if let Ok(until_date) = Self::parse_ical_datetime(until_str, is_date_only) { + return until_date >= *start; } } else { // No UNTIL specified, so recurrence continues indefinitely @@ -646,60 +673,123 @@ impl CalendarEvent { /** * Extracts a property value from iCalendar data. * + * Backed by the `ical` crate's RFC 5545 parser (see `Cargo.toml` + * doc-comment on the dep). The pre-2026-07-14 hand-rolled scan + * looked for `\n:` and refused any parameter-carrying + * property (`DTSTART;VALUE=DATE:20260101`, + * `RECURRENCE-ID;VALUE=DATE:...`, `ATTENDEE;CN=…;PARTSTAT=…:…`) — + * see AtalayaLabs/OxiCloud#528. + * + * The current implementation reads the first VEVENT from the raw + * body via `IcalParser` and returns the named property's `value` + * (parameters discarded — use `extract_ical_property_with_params` + * for callers that care about `VALUE=DATE`, `TZID`, etc.). + * + * Returns `None` when the property is missing, has an empty value, + * or the body isn't parseable as iCalendar. Whole-body parse + * failures collapse to `None` rather than surface — same behaviour + * as the pre-rewrite hand-rolled scan, which just returned `None` + * on any mismatch. If callers need to distinguish "missing" from + * "unparseable body", they should use `parse_first_vevent` directly. + * * @param ical_data The iCalendar data to search in * @param property_name The name of the property to extract * @return Option containing the property value if found */ fn extract_ical_property(ical_data: &str, property_name: &str) -> Option { - // Find the property in the iCalendar data - let search_str = format!("\n{}:", property_name); - let search_str_alt = format!("\r\n{}:", property_name); + Self::extract_ical_property_with_params(ical_data, property_name).map(|(v, _p)| v) + } - let pos = ical_data - .find(&search_str) - .or_else(|| ical_data.find(&search_str_alt)); - - if let Some(pos) = pos { - // Find the start of the value - let value_start = pos + search_str.len(); - - // Find the end of the value (next line or end of string) - let value_end = ical_data[value_start..] - .find('\n') - .map(|p| value_start + p) - .unwrap_or_else(|| ical_data.len()); - - // Extract and return the value - let value = ical_data[value_start..value_end].trim(); - if !value.is_empty() { - return Some(value.to_string()); + /// Extract a property's value AND parameter map. Same lookup rules + /// as `extract_ical_property`; the second element is a map keyed by + /// parameter name (`"VALUE"`, `"TZID"`, `"CN"`, …) whose value is + /// the list of parameter values (parameters can be multi-valued — + /// `MEMBER="mailto:a@x","mailto:b@x"` — hence the `Vec` + /// per key). + /// + /// Callers that only need the value should use `extract_ical_property`; + /// this variant is for DTSTART / DTEND / RECURRENCE-ID which need + /// `VALUE=DATE` detection to distinguish all-day from timed events. + fn extract_ical_property_with_params( + ical_data: &str, + property_name: &str, + ) -> Option<(String, std::collections::HashMap>)> { + let event = Self::parse_first_vevent(ical_data)?; + let prop = event + .properties + .into_iter() + .find(|p| p.name.eq_ignore_ascii_case(property_name))?; + let value = prop.value?; + if value.trim().is_empty() { + return None; + } + let mut params: std::collections::HashMap> = + std::collections::HashMap::new(); + if let Some(param_list) = prop.params { + for (name, values) in param_list { + // RFC 5545 property parameter names are ASCII case-insensitive. + // Normalise to UPPER so callers key on a canonical form. + params.insert(name.to_ascii_uppercase(), values); } } + Some((value.trim().to_string(), params)) + } + /// Parse the raw iCalendar body and return the first VEVENT + /// component's properties. Returns `None` on any parse failure or + /// if the body carries zero events (e.g. a `VCALENDAR` with only + /// VTODOs — not our concern for the events surface). + /// + /// Delegated to the `ical` crate's `IcalParser`, which handles + /// line-folding, escaped characters, and RFC 5545 parameter syntax. + fn parse_first_vevent(ical_data: &str) -> Option { + use std::io::BufReader; + let reader = BufReader::new(ical_data.as_bytes()); + let parser = ical::IcalParser::new(reader); + for cal in parser { + let Ok(cal) = cal else { continue }; + if let Some(event) = cal.events.into_iter().next() { + return Some(event); + } + } None } /** * Parses an iCalendar datetime string into a DateTime object. * - * @param datetime The iCalendar datetime string to parse + * @param value The property value (already stripped of parameters + * by the ical-crate-backed extractor). + * @param is_date_only True when the source line carried + * `VALUE=DATE` (all-day event) — caller derives + * this from `extract_ical_property_with_params`. * @return Result containing the parsed DateTime or an error */ - fn parse_ical_datetime(datetime: &str) -> std::result::Result, String> { - // Handle VALUE=DATE format - if datetime.contains("VALUE=DATE") { - let date_str = datetime.split(':').next_back().unwrap_or(""); - if date_str.len() != 8 { - return Err("Invalid date format".to_string()); + fn parse_ical_datetime( + value: &str, + is_date_only: bool, + ) -> std::result::Result, String> { + // All-day form — YYYYMMDD, 8 chars, no time component. Caller + // signalled this via the `VALUE=DATE` parameter on the source + // property. Pre-2026-07-14 this was detected by scanning the + // raw property line for the substring `VALUE=DATE`, which + // failed because `extract_ical_property` refused to return + // param-carrying lines at all (see #528). + if is_date_only { + if value.len() != 8 { + return Err(format!( + "Invalid all-day date format: expected YYYYMMDD (8 chars), got {} chars", + value.len() + )); } - let year = date_str[0..4] + let year = value[0..4] .parse::() .map_err(|_| "Invalid year".to_string())?; - let month = date_str[4..6] + let month = value[4..6] .parse::() .map_err(|_| "Invalid month".to_string())?; - let day = date_str[6..8] + let day = value[6..8] .parse::() .map_err(|_| "Invalid day".to_string())?; @@ -709,29 +799,33 @@ impl CalendarEvent { }; } - // Handle standard UTC format (20230101T120000Z) - let datetime_str = datetime.split(':').next_back().unwrap_or(datetime); - if datetime_str.len() < 15 || !datetime_str.ends_with('Z') { - return Err("Invalid datetime format".to_string()); + // Standard UTC form: YYYYMMDDTHHMMSSZ, 16 chars, trailing 'Z'. + // Floating-time (no 'Z') and TZID-anchored forms aren't yet + // supported — future work when we tackle VTIMEZONE properly. + if value.len() < 15 || !value.ends_with('Z') { + return Err(format!( + "Invalid datetime format: expected YYYYMMDDTHHMMSSZ, got {:?}", + value + )); } - let year = datetime_str[0..4] + let year = value[0..4] .parse::() .map_err(|_| "Invalid year".to_string())?; - let month = datetime_str[4..6] + let month = value[4..6] .parse::() .map_err(|_| "Invalid month".to_string())?; - let day = datetime_str[6..8] + let day = value[6..8] .parse::() .map_err(|_| "Invalid day".to_string())?; - let hour = datetime_str[9..11] + let hour = value[9..11] .parse::() .map_err(|_| "Invalid hour".to_string())?; - let minute = datetime_str[11..13] + let minute = value[11..13] .parse::() .map_err(|_| "Invalid minute".to_string())?; - let second = datetime_str[13..15] + let second = value[13..15] .parse::() .map_err(|_| "Invalid second".to_string())?; @@ -816,3 +910,215 @@ impl CalendarEvent { } } } + +#[cfg(test)] +mod ical_parser_tests { + //! Regression tests for the `ical`-crate-backed property extractor. + //! + //! Every shape here failed under the pre-2026-07-14 hand-rolled + //! `find("\n:")` scan (see AtalayaLabs/OxiCloud#528). Fixtures + //! are RFC 5545-shaped; when we bundle real client bodies from + //! Thunderbird / DAVx⁵ / Gnome Calendar the mapping will follow the + //! same style — each case declares which shape it exercises. + //! + //! Fixture sources / attributions: + //! * RFC 5545 §3.6.1 (VEVENT baseline) — timed event example + //! * RFC 5545 §3.8.2.4 (DTSTART DATE form) — all-day event + //! * RFC 5545 §3.8.4.4 (RECURRENCE-ID) — exception instance + //! * Shape adapted from Radicale test fixtures — RRULE + UNTIL + //! with a DATE-form UNTIL for an all-day recurring event + //! + //! Everything is spec-shaped and byte-small; no network / no + //! external files. Real client bodies can be added later under + //! `tests/fixtures/ical/` and loaded via `include_str!`. + + use super::*; + + /// Simple timed VEVENT. Baseline sanity — this shape worked pre- + /// rewrite (no property parameters), so it's the regression floor. + const TIMED_EVENT: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:timed-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T120000Z\r +DTEND:20260101T130000Z\r +SUMMARY:Timed baseline\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// All-day VEVENT — the exact shape #528 flagged. Property line + /// carries `;VALUE=DATE:` which the old scan refused; the crate- + /// backed extractor now parses it and the all-day flag is derived + /// from the `VALUE` parameter. + const ALL_DAY_EVENT: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:allday-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260201\r +DTEND;VALUE=DATE:20260202\r +SUMMARY:All-day event\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// Timed recurring master with a modified single occurrence + /// (RECURRENCE-ID identifies which instance). The exception VEVENT + /// shares the master's UID and adds `RECURRENCE-ID:` to pinpoint + /// the overridden date. This is the #528 shape — parser must not + /// choke on the presence of RECURRENCE-ID even though we don't + /// route it into the domain yet (that's phase 2). + const RECURRING_WITH_EXCEPTION: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T090000Z\r +DTEND:20260101T100000Z\r +SUMMARY:Daily standup\r +RRULE:FREQ=DAILY;COUNT=10\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260103T110000Z\r +DTEND:20260103T120000Z\r +SUMMARY:Daily standup — rescheduled\r +RECURRENCE-ID:20260103T090000Z\r +END:VEVENT\r +END:VCALENDAR\r +"; + + /// All-day recurring with an all-day exception — the most-broken + /// case in #528 (RECURRENCE-ID;VALUE=DATE:...). Parser must accept + /// the parameter on both DTSTART and RECURRENCE-ID. + const ALL_DAY_RECURRING_WITH_EXCEPTION: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260105\r +DTEND;VALUE=DATE:20260106\r +SUMMARY:Weekly all-day\r +RRULE:FREQ=WEEKLY;COUNT=4\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260113\r +DTEND;VALUE=DATE:20260114\r +SUMMARY:Weekly all-day — rescheduled\r +RECURRENCE-ID;VALUE=DATE:20260112\r +END:VEVENT\r +END:VCALENDAR\r +"; + + fn parse_ok(body: &str) -> CalendarEvent { + CalendarEvent::from_ical(Uuid::new_v4(), body.to_string()) + .expect("expected successful parse") + } + + #[test] + fn timed_event_parses_and_is_not_all_day() { + let ev = parse_ok(TIMED_EVENT); + assert_eq!(ev.summary(), "Timed baseline"); + assert!(!ev.all_day()); + } + + #[test] + fn all_day_event_parses_and_flags_as_all_day() { + // Regression: DTSTART;VALUE=DATE:20260201 used to fail + // property-extraction ("Missing DTSTART") because the raw + // scan required a colon directly after the property name. + let ev = parse_ok(ALL_DAY_EVENT); + assert!(ev.all_day(), "VALUE=DATE parameter should flag all-day"); + assert_eq!( + ev.start_time().date_naive().to_string(), + "2026-02-01", + "DTSTART value should parse the YYYYMMDD payload" + ); + } + + #[test] + fn recurring_with_exception_still_returns_the_master() { + // The crate parses BOTH events from the VCALENDAR body; our + // `parse_first_vevent` returns the first, which is the master. + // Exception routing is phase 2 — this test locks the current + // "first event wins" behavior so phase 2 knows what it's + // extending. + let ev = parse_ok(RECURRING_WITH_EXCEPTION); + assert_eq!(ev.summary(), "Daily standup"); + assert_eq!(ev.ical_uid(), "daily-1@oxicloud.test"); + assert_eq!(ev.rrule().as_deref(), Some("FREQ=DAILY;COUNT=10")); + } + + #[test] + fn all_day_recurring_with_exception_master_parses() { + // The #528 shape end-to-end: parameterised DTSTART on both the + // master and the exception, plus a parameterised RECURRENCE-ID. + // Pre-rewrite this was a 400 (post the error-mapping fix) or 500 + // (before it); post-rewrite the master parses cleanly and the + // all_day flag is set from the master's DTSTART parameters. + let ev = parse_ok(ALL_DAY_RECURRING_WITH_EXCEPTION); + assert!(ev.all_day()); + assert_eq!(ev.ical_uid(), "weekly-allday@oxicloud.test"); + } + + #[test] + fn missing_dtstart_still_returns_a_useful_error() { + // Preserve the pre-rewrite error contract for the genuinely- + // missing case. `dav_error_mapping.hurl` asserts this shape. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:missing-dtstart@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTEND:20260101T130000Z\r +SUMMARY:No DTSTART\r +END:VEVENT\r +END:VCALENDAR\r +"; + let err = CalendarEvent::from_ical(Uuid::new_v4(), body.to_string()) + .expect_err("expected InvalidInput for missing DTSTART"); + assert_eq!(err.kind, ErrorKind::InvalidInput); + assert!( + err.message.contains("DTSTART"), + "message should mention DTSTART, got: {}", + err.message + ); + } + + #[test] + fn extract_property_with_params_returns_parameter_map() { + // Direct test of the params-aware extractor. Confirms + // parameter names are normalised to uppercase and preserved + // as a list (RFC 5545 §3.2 — parameters can carry multiple + // comma-separated values). + let (value, params) = + CalendarEvent::extract_ical_property_with_params(ALL_DAY_EVENT, "DTSTART") + .expect("DTSTART must extract"); + assert_eq!(value, "20260201"); + let vals = params.get("VALUE").expect("VALUE param must be present"); + assert_eq!(vals, &vec!["DATE".to_string()]); + } + + #[test] + fn extract_property_case_insensitive_property_name() { + // Property names are ASCII case-insensitive per RFC 5545 §3.1. + // The lookup must accept "dtstart" as well as "DTSTART". + let v = CalendarEvent::extract_ical_property(TIMED_EVENT, "dtstart"); + assert_eq!(v.as_deref(), Some("20260101T120000Z")); + } +} From 02f67f7a5cd5744f6d817c12eefe034c1a9b381e Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 14 Jul 2026 17:27:16 +0200 Subject: [PATCH 2/3] fix(528): pass2 add recurrence_id field --- ...13000001_calendar_events_recurrence_id.sql | 70 +++++++ src/domain/entities/calendar_event.rs | 185 +++++++++++++++++- .../pg/calendar_event_pg_repository.rs | 114 ++++++----- 3 files changed, 318 insertions(+), 51 deletions(-) create mode 100644 migrations/20260913000001_calendar_events_recurrence_id.sql diff --git a/migrations/20260913000001_calendar_events_recurrence_id.sql b/migrations/20260913000001_calendar_events_recurrence_id.sql new file mode 100644 index 00000000..46657505 --- /dev/null +++ b/migrations/20260913000001_calendar_events_recurrence_id.sql @@ -0,0 +1,70 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- caldav.calendar_events — add RECURRENCE-ID column for exception instances +-- ════════════════════════════════════════════════════════════════════════════ +-- Motivation: AtalayaLabs/OxiCloud#528 — CalDAV clients (Thunderbird, Apple +-- Calendar, Gnome Calendar, DAVx⁵) modify a single occurrence of a recurring +-- event by PUTting a separate VEVENT that shares the master's UID and adds +-- a RECURRENCE-ID identifying which occurrence is overridden (RFC 5545 +-- §3.8.4.4). +-- +-- Pre-#528 behaviour: modifications either hit a UID collision (silent +-- 500 or corrupt state) or overwrote the master. Post-#528 the exception +-- override lives as its own row keyed by +-- (calendar_id, ical_uid, recurrence_id), with the master identified by +-- `recurrence_id IS NULL`. +-- +-- Related but distinct from parser Phase 1 (rewrite of extract_ical_property +-- on top of the `ical` crate) — that landed in the same branch to enable +-- parsing RECURRENCE-ID at all. This migration is the storage half. +-- +-- No backfill needed — pre-migration events all become masters (NULL). No +-- existing exception rows existed because the parser couldn't read them. +-- ════════════════════════════════════════════════════════════════════════════ + +BEGIN; + +-- Column: nullable. NULL = master, non-NULL = exception instance whose +-- value pinpoints which occurrence of the recurring master is being +-- overridden. TIMESTAMPTZ so both timed (DATE-TIME) and all-day (DATE) +-- RECURRENCE-IDs fit — the domain-side `parse_ical_datetime` normalises +-- both into `DateTime` (all-day → midnight UTC of the target date). +ALTER TABLE caldav.calendar_events + ADD COLUMN recurrence_id TIMESTAMP WITH TIME ZONE NULL; + +COMMENT ON COLUMN caldav.calendar_events.recurrence_id IS + 'RFC 5545 §3.8.4.4 RECURRENCE-ID. NULL on the master, non-NULL on ' + 'per-instance exception overrides. Keyed with (calendar_id, ical_uid) ' + 'via the two partial unique indexes below.'; + +-- Partial unique index: at most one master row per (calendar_id, ical_uid). +-- +-- Without this a client that re-uses a UID across calendar events (e.g. a +-- pre-2026-08 import that didn't dedupe) could produce two masters — the +-- lookup by (calendar_id, ical_uid) WHERE recurrence_id IS NULL would then +-- be ambiguous and the exception-routing logic would either overwrite the +-- wrong master or refuse to insert. Pre-migration duplicates would fail +-- this index creation; if that happens, the reconciliation is out of scope +-- for this migration (dedup script would go here — but the existing +-- codebase generates fresh UIDs on ambiguity so it shouldn't fire in +-- practice). +CREATE UNIQUE INDEX idx_calendar_events_master_unique + ON caldav.calendar_events (calendar_id, ical_uid) + WHERE recurrence_id IS NULL; + +-- Partial unique index: at most one exception override per +-- (calendar_id, ical_uid, recurrence_id). Prevents two rows both claiming +-- to override the same instance of the same master — which would confuse +-- the client on next PROPFIND. +CREATE UNIQUE INDEX idx_calendar_events_exception_unique + ON caldav.calendar_events (calendar_id, ical_uid, recurrence_id) + WHERE recurrence_id IS NOT NULL; + +-- Read-path index for the "give me the master + all its exceptions" +-- query the PROPFIND handler will run. Covered by the two unique indexes +-- above only partially — this covering index reads the full +-- (calendar_id, ical_uid) pair in one seek regardless of which side of +-- the master/exception split. +CREATE INDEX idx_calendar_events_uid_lookup + ON caldav.calendar_events (calendar_id, ical_uid); + +COMMIT; diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index 65dbd804..eb0725ed 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -51,6 +51,29 @@ pub struct CalendarEvent { /// Recurrence rule in iCalendar RRULE format (optional) rrule: Option, + /// RECURRENCE-ID (RFC 5545 §3.8.4.4) — non-NULL on exception + /// instances of a recurring event, NULL on the master. + /// + /// When a client (Thunderbird, Apple Calendar, Gnome Calendar, …) + /// modifies a SINGLE occurrence of a recurring event, it sends + /// a separate VEVENT that shares the master's UID and carries + /// a `RECURRENCE-ID` identifying which occurrence is being + /// overridden. That per-instance override lives as its own row + /// in `caldav.calendar_events`; the master row keeps NULL here. + /// + /// Lookup key is `(calendar_id, ical_uid, recurrence_id)` — + /// enforced at the DB layer by two partial unique indexes: + /// + /// * `(calendar_id, ical_uid) WHERE recurrence_id IS NULL` — + /// at most one master per UID per calendar. + /// * `(calendar_id, ical_uid, recurrence_id) WHERE + /// recurrence_id IS NOT NULL` — at most one override for a + /// given (master, instance) pair. + /// + /// See AtalayaLabs/OxiCloud#528 for the ticket that motivated + /// this field, and `docs/plan/` (future) for the full model. + recurrence_id: Option>, + /// Unique identifier in iCalendar format (used for CalDAV sync) ical_uid: String, @@ -140,6 +163,7 @@ impl CalendarEvent { end_time, all_day, rrule, + recurrence_id: None, ical_uid: Uuid::new_v4().to_string(), ical_data, created_at: now, @@ -209,6 +233,7 @@ impl CalendarEvent { end_time, all_day, rrule, + recurrence_id: None, ical_uid, ical_data, created_at, @@ -268,10 +293,7 @@ impl CalendarEvent { // and anything else means timed. let all_day = dtstart_params .get("VALUE") - .map(|vs| { - vs.iter() - .any(|v| v.eq_ignore_ascii_case("DATE")) - }) + .map(|vs| vs.iter().any(|v| v.eq_ignore_ascii_case("DATE"))) .unwrap_or(false); let start_time = Self::parse_ical_datetime(&dtstart_value, all_day).map_err(|e| { @@ -299,6 +321,26 @@ impl CalendarEvent { let ical_uid = Self::extract_ical_property(&ical_data, "UID") .unwrap_or_else(|| Uuid::new_v4().to_string()); + // RECURRENCE-ID (RFC 5545 §3.8.4.4). When present, this VEVENT + // is an override for a specific occurrence of a recurring + // master with the same UID. The parameter tells us whether the + // value is a date (all-day master) or datetime (timed master). + // A parse failure here downgrades to `None` — the VEVENT still + // gets stored, just as a plain event (worst case a client sync + // treats it as a new master, which the DB uniqueness will + // refuse; better a persistence error than a silent split). + let recurrence_id = + match Self::extract_ical_property_with_params(&ical_data, "RECURRENCE-ID") { + Some((value, params)) => { + let is_date = params + .get("VALUE") + .map(|vs| vs.iter().any(|v| v.eq_ignore_ascii_case("DATE"))) + .unwrap_or(false); + Self::parse_ical_datetime(&value, is_date).ok() + } + None => None, + }; + let now = Utc::now(); Ok(Self { @@ -311,6 +353,7 @@ impl CalendarEvent { end_time, all_day, rrule, + recurrence_id, ical_uid, ical_data, created_at: now, @@ -366,6 +409,24 @@ impl CalendarEvent { } /// Returns the event's iCalendar UID + /// Returns the RECURRENCE-ID for this event, if any. `None` on + /// masters and standalone (non-recurring) events; `Some` on + /// exception overrides that target a specific occurrence of a + /// recurring master with the same `ical_uid`. + pub fn recurrence_id(&self) -> Option<&DateTime> { + self.recurrence_id.as_ref() + } + + /// Set the RECURRENCE-ID on this event. Used by the repository + /// layer when reconstructing an entity from a stored row (the + /// column is read straight into the field — no re-parse of the + /// ical_data body). Passing `None` clears the marker, promoting + /// an exception back to a plain event. + pub fn set_recurrence_id(&mut self, recurrence_id: Option>) { + self.recurrence_id = recurrence_id; + self.updated_at = Utc::now(); + } + pub fn ical_uid(&self) -> &str { &self.ical_uid } @@ -591,8 +652,7 @@ impl CalendarEvent { self.start_time = start_time; } - if let Some((value, _params)) = - Self::extract_ical_property_with_params(&ical_data, "DTEND") + if let Some((value, _params)) = Self::extract_ical_property_with_params(&ical_data, "DTEND") && let Ok(end_time) = Self::parse_ical_datetime(&value, all_day) { self.end_time = end_time; @@ -1059,7 +1119,7 @@ END:VCALENDAR\r let ev = parse_ok(RECURRING_WITH_EXCEPTION); assert_eq!(ev.summary(), "Daily standup"); assert_eq!(ev.ical_uid(), "daily-1@oxicloud.test"); - assert_eq!(ev.rrule().as_deref(), Some("FREQ=DAILY;COUNT=10")); + assert_eq!(ev.rrule(), Some("FREQ=DAILY;COUNT=10")); } #[test] @@ -1121,4 +1181,115 @@ END:VCALENDAR\r let v = CalendarEvent::extract_ical_property(TIMED_EVENT, "dtstart"); assert_eq!(v.as_deref(), Some("20260101T120000Z")); } + + // ───────────────────────────────────────────────────────────── + // Phase 2 — RECURRENCE-ID extraction into the entity + // ───────────────────────────────────────────────────────────── + + #[test] + fn master_event_has_no_recurrence_id() { + // A plain VEVENT (no RECURRENCE-ID line) should carry a NULL + // recurrence_id — that's what marks it as a master in the DB. + let ev = parse_ok(TIMED_EVENT); + assert!( + ev.recurrence_id().is_none(), + "master should have recurrence_id = None" + ); + } + + #[test] + fn recurring_master_has_no_recurrence_id_even_with_rrule() { + // The presence of RRULE on the master does not by itself + // populate recurrence_id — only RECURRENCE-ID does. The + // exception-instance VEVENT in the same VCALENDAR carries + // RECURRENCE-ID; `parse_first_vevent` returns the master, so + // we get `None` here. Phase 3 will introduce a `parse_all_events` + // helper to surface the exceptions. + let ev = parse_ok(RECURRING_WITH_EXCEPTION); + assert!( + ev.recurrence_id().is_none(), + "master with RRULE should still have recurrence_id = None" + ); + assert_eq!(ev.rrule().as_deref(), Some("FREQ=DAILY;COUNT=10")); + } + + #[test] + fn timed_exception_populates_recurrence_id() { + // A standalone exception-override VEVENT (as sent by a client + // that's already synced the master and is now modifying one + // instance) parses with recurrence_id = the RECURRENCE-ID's + // timestamp. This is the phase-2 half of #528 — the value is + // preserved through the domain model; phase 3 will use it to + // route inserts to their own row. + let exception = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260103T110000Z\r +DTEND:20260103T120000Z\r +SUMMARY:Daily standup — rescheduled\r +RECURRENCE-ID:20260103T090000Z\r +END:VEVENT\r +END:VCALENDAR\r +"; + let ev = parse_ok(exception); + let rid = ev + .recurrence_id() + .expect("exception must have recurrence_id set"); + assert_eq!( + rid.to_rfc3339(), + "2026-01-03T09:00:00+00:00", + "RECURRENCE-ID must parse to the timed override timestamp" + ); + } + + #[test] + fn all_day_exception_populates_recurrence_id_at_midnight() { + // RECURRENCE-ID;VALUE=DATE:20260112 — the exact shape #528 + // flagged. Domain normalises the DATE form to midnight UTC on + // the given day so the field's type stays `DateTime`. + let exception = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260113\r +DTEND;VALUE=DATE:20260114\r +SUMMARY:Weekly all-day — rescheduled\r +RECURRENCE-ID;VALUE=DATE:20260112\r +END:VEVENT\r +END:VCALENDAR\r +"; + let ev = parse_ok(exception); + let rid = ev + .recurrence_id() + .expect("all-day exception must have recurrence_id set"); + assert_eq!( + rid.to_rfc3339(), + "2026-01-12T00:00:00+00:00", + "all-day RECURRENCE-ID must normalise to 00:00:00 UTC of the target date" + ); + } + + #[test] + fn set_recurrence_id_setter_round_trips() { + // Repository rehydration path: `with_id` initialises + // recurrence_id to None; the repo calls `set_recurrence_id` + // with the DB column value. Prove both branches survive the + // setter cleanly. + let mut ev = parse_ok(TIMED_EVENT); + assert!(ev.recurrence_id().is_none()); + + let target = Utc.with_ymd_and_hms(2026, 3, 15, 12, 0, 0).unwrap(); + ev.set_recurrence_id(Some(target)); + assert_eq!(ev.recurrence_id(), Some(&target)); + + ev.set_recurrence_id(None); + assert!(ev.recurrence_id().is_none()); + } } diff --git a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs index 8d5cc81e..17cf365d 100644 --- a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs +++ b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs @@ -30,10 +30,11 @@ impl CalendarEventRepository for CalendarEventPgRepository { sqlx::query( r#" INSERT INTO caldav.calendar_events ( - id, calendar_id, summary, description, location, start_time, end_time, - all_day, rrule, created_at, updated_at, ical_uid, ical_data + id, calendar_id, summary, description, location, start_time, end_time, + all_day, rrule, created_at, updated_at, ical_uid, ical_data, + recurrence_id ) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) "#, ) .bind(event.id()) @@ -49,6 +50,10 @@ impl CalendarEventRepository for CalendarEventPgRepository { .bind(event.updated_at()) .bind(event.ical_uid()) .bind(event.ical_data()) + // NULL on masters, non-NULL on exception overrides — see the + // `20260913000001_calendar_events_recurrence_id.sql` migration + // and `docs/architecture/rebac-authorization.md` follow-up doc. + .bind(event.recurrence_id().copied()) .execute(&*self.pool) .await .map_err(|e| { @@ -68,16 +73,17 @@ impl CalendarEventRepository for CalendarEventPgRepository { sqlx::query( r#" UPDATE caldav.calendar_events - SET summary = $1, - description = $2, - location = $3, - start_time = $4, - end_time = $5, - all_day = $6, + SET summary = $1, + description = $2, + location = $3, + start_time = $4, + end_time = $5, + all_day = $6, rrule = $7, ical_data = $8, - updated_at = $9 - WHERE id = $10 + recurrence_id = $9, + updated_at = $10 + WHERE id = $11 "#, ) .bind(event.summary()) @@ -88,6 +94,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .bind(event.all_day()) .bind(event.rrule()) .bind(event.ical_data()) + .bind(event.recurrence_id().copied()) .bind(now) .bind(event.id()) .execute(&*self.pool) @@ -126,12 +133,12 @@ impl CalendarEventRepository for CalendarEventPgRepository { ) -> CalendarEventRepositoryResult> { let rows = sqlx::query( r#" - SELECT - id, calendar_id, summary, description, location, - start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events - WHERE calendar_id = $1 + WHERE calendar_id = $1 AND ( (start_time >= $2 AND start_time < $3) OR (end_time > $2 AND end_time <= $3) OR @@ -152,7 +159,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { let mut events = Vec::new(); for row in rows { - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -170,6 +177,11 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + // Rehydrate the RECURRENCE-ID after entity construction — + // `with_id` initialises to `None` because the field predates + // the rest of the constructor signature (#528). Keeping + // `with_id` unchanged avoids ripple-changing every caller. + event.set_recurrence_id(row.get::>, _>("recurrence_id")); events.push(event); } @@ -179,10 +191,10 @@ impl CalendarEventRepository for CalendarEventPgRepository { async fn find_event_by_id(&self, id: &Uuid) -> CalendarEventRepositoryResult { let row = sqlx::query( r#" - SELECT - id, calendar_id, summary, description, location, - start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events WHERE id = $1 "#, @@ -195,11 +207,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { })? .ok_or_else(|| DomainError::not_found("Calendar Event", id.to_string()))?; - // In a real implementation, we would build a complete CalendarEvent object - // For simplicity, we create an object with default values to - // demonstrate the approach without macros - - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -217,6 +225,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); Ok(event) } @@ -227,10 +236,10 @@ impl CalendarEventRepository for CalendarEventPgRepository { ) -> CalendarEventRepositoryResult> { let rows = sqlx::query( r#" - SELECT - id, calendar_id, summary, description, location, - start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events WHERE calendar_id = $1 ORDER BY start_time @@ -245,7 +254,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { let mut events = Vec::new(); for row in rows { - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -263,6 +272,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); events.push(event); } @@ -278,10 +288,10 @@ impl CalendarEventRepository for CalendarEventPgRepository { let rows = sqlx::query( r#" - SELECT - id, calendar_id, summary, description, location, - start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events WHERE calendar_id = $1 AND summary ILIKE $2 ORDER BY start_time @@ -297,7 +307,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { let mut events = Vec::new(); for row in rows { - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -315,6 +325,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); events.push(event); } @@ -326,14 +337,21 @@ impl CalendarEventRepository for CalendarEventPgRepository { calendar_id: &Uuid, ical_uid: &str, ) -> CalendarEventRepositoryResult> { + // Phase 2 note: this method looks up "an event with this UID" + // — the SELECT still isn't filtered on `recurrence_id IS NULL` + // because the phase-3 handler routing (which will distinguish + // master vs. exception override at PUT time) is where the + // filter actually needs to live. For phase 2 the invariant is + // enforced only at INSERT time via the two partial unique + // indexes; reads see whatever's there. let row_opt = sqlx::query( r#" - SELECT - id, calendar_id, summary, description, location, - start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events - WHERE calendar_id = $1 AND ical_uid = $2 + WHERE calendar_id = $1 AND ical_uid = $2 AND recurrence_id IS NULL "#, ) .bind(calendar_id) @@ -346,7 +364,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { match row_opt { Some(row) => { - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -364,6 +382,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); Ok(Some(event)) } None => Ok(None), @@ -375,12 +394,18 @@ impl CalendarEventRepository for CalendarEventPgRepository { calendar_id: &Uuid, ical_uids: &[String], ) -> CalendarEventRepositoryResult> { + // Batch UID lookup returns ALL rows for the given UIDs, both + // masters and exception overrides. Callers that want just + // masters filter downstream. Same phase-2 policy as the + // single-UID variant — read-side filtering is a phase-3 + // concern; the DB unique indexes are what guarantee at most + // one master + N distinct exceptions per (calendar, UID). let rows = sqlx::query( r#" SELECT id, calendar_id, summary, description, location, start_time, end_time, all_day, rrule, - created_at, updated_at, ical_uid, ical_data + created_at, updated_at, ical_uid, ical_data, recurrence_id FROM caldav.calendar_events WHERE calendar_id = $1 AND ical_uid = ANY($2) ORDER BY start_time @@ -396,7 +421,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { let mut events = Vec::new(); for row in rows { - let event = CalendarEvent::with_id( + let mut event = CalendarEvent::with_id( row.get("id"), row.get("calendar_id"), row.get("summary"), @@ -414,6 +439,7 @@ impl CalendarEventRepository for CalendarEventPgRepository { .map_err(|e| { DomainError::database_error(format!("Error creating calendar event: {}", e)) })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); events.push(event); } From 7966c7178ada2747cacebf6c913684584d502e13 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 14 Jul 2026 17:46:24 +0200 Subject: [PATCH 3/3] fix(528): pass3: PUT with RECURRENCE-ID --- .../adapters/caldav_adapter_test.rs | 1 + src/application/dtos/calendar_dto.rs | 8 + src/application/ports/calendar_ports.rs | 39 +++ src/application/services/calendar_service.rs | 19 +- src/domain/entities/calendar_event.rs | 269 ++++++++++++++++- .../repositories/calendar_event_repository.rs | 24 +- .../adapters/calendar_storage_adapter.rs | 69 ++++- .../pg/calendar_event_pg_repository.rs | 60 ++++ src/interfaces/api/handlers/caldav_handler.rs | 114 +++---- tests/api/caldav_recurring.hurl | 280 ++++++++++++++++++ tests/api/run.sh | 1 + 11 files changed, 805 insertions(+), 79 deletions(-) create mode 100644 tests/api/caldav_recurring.hurl diff --git a/src/application/adapters/caldav_adapter_test.rs b/src/application/adapters/caldav_adapter_test.rs index 5f7847c6..4f4284bc 100644 --- a/src/application/adapters/caldav_adapter_test.rs +++ b/src/application/adapters/caldav_adapter_test.rs @@ -35,6 +35,7 @@ mod tests { all_day: false, rrule: None, ical_uid: "uid-evt-001@oxicloud".to_string(), + recurrence_id: None, created_at: Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(), updated_at: Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(), } diff --git a/src/application/dtos/calendar_dto.rs b/src/application/dtos/calendar_dto.rs index 92fca841..de2e965d 100644 --- a/src/application/dtos/calendar_dto.rs +++ b/src/application/dtos/calendar_dto.rs @@ -89,6 +89,12 @@ pub struct CalendarEventDto { pub all_day: bool, pub rrule: Option, pub ical_uid: String, + /// RFC 5545 §3.8.4.4 RECURRENCE-ID. `None` on masters and on + /// non-recurring events; `Some` on per-instance exception + /// overrides. Two rows sharing (`calendar_id`, `ical_uid`) but + /// distinguished by this field represent a recurring master and + /// its modified occurrence(s) respectively (see #528). + pub recurrence_id: Option>, pub created_at: DateTime, pub updated_at: DateTime, } @@ -106,6 +112,7 @@ impl Default for CalendarEventDto { all_day: false, rrule: None, ical_uid: String::new(), + recurrence_id: None, created_at: Utc::now(), updated_at: Utc::now(), } @@ -125,6 +132,7 @@ impl From for CalendarEventDto { all_day: event.all_day(), rrule: event.rrule().map(|s| s.to_string()), ical_uid: event.ical_uid().to_string(), + recurrence_id: event.recurrence_id().copied(), created_at: *event.created_at(), updated_at: *event.updated_at(), } diff --git a/src/application/ports/calendar_ports.rs b/src/application/ports/calendar_ports.rs index 332da7ec..cd4781bf 100644 --- a/src/application/ports/calendar_ports.rs +++ b/src/application/ports/calendar_ports.rs @@ -6,6 +6,19 @@ use crate::common::errors::DomainError; use chrono::{DateTime, Utc}; use uuid::Uuid; +/// Result of a multi-VEVENT PUT (`upsert_ical_events`). See #528. +#[derive(Debug, Clone)] +pub struct UpsertEventsResult { + /// Every event that was persisted for this PUT. Ordered as they + /// appeared in the body — the master (if present) is typically + /// first, followed by exception overrides. + pub events: Vec, + /// True if at least one row was newly created; false if every + /// event replaced an existing row. Drives the handler's choice + /// between 201 Created and 204 No Content. + pub any_inserted: bool, +} + /// Port for external calendar storage mechanisms pub trait CalendarStoragePort: Send + Sync + 'static { // Calendar operations @@ -53,6 +66,23 @@ pub trait CalendarStoragePort: Send + Sync + 'static { &self, event: CreateEventICalDto, ) -> Result; + /// Upsert every VEVENT in an iCalendar body — one master and zero + /// or more per-instance exception overrides (RFC 5545 §3.8.4.4). + /// + /// Routing: an event whose `RECURRENCE-ID` is unset targets the + /// master row `(calendar_id, ical_uid) WHERE recurrence_id IS NULL`; + /// an event whose `RECURRENCE-ID` is set targets its own exception + /// row `(calendar_id, ical_uid, recurrence_id)` and never touches + /// the master. Existing rows are replaced (delete-then-insert to + /// stay compatible with the DB-level partial unique indexes and to + /// keep the ETag surface identical to the pre-#528 single-event + /// path). + /// + /// See AtalayaLabs/OxiCloud#528. + async fn upsert_ical_events( + &self, + event: CreateEventICalDto, + ) -> Result; async fn update_event( &self, event_id: &str, @@ -136,6 +166,15 @@ pub trait CalendarUseCase: Send + Sync + 'static { event: CreateEventICalDto, user_id: Uuid, ) -> Result; + /// Route a PUT'd iCalendar body containing one or more VEVENTs to + /// their per-instance rows. See `CalendarStoragePort::upsert_ical_events` + /// for the routing rules; this method just adds the `Permission::Create` + /// gate for the caller. + async fn upsert_ical_events( + &self, + event: CreateEventICalDto, + user_id: Uuid, + ) -> Result; async fn update_event( &self, event_id: &str, diff --git a/src/application/services/calendar_service.rs b/src/application/services/calendar_service.rs index aa563b5c..c8c2b093 100644 --- a/src/application/services/calendar_service.rs +++ b/src/application/services/calendar_service.rs @@ -8,7 +8,9 @@ use crate::application::dtos::calendar_dto::{ UpdateCalendarDto, UpdateEventDto, }; use crate::application::ports::authorization_ports::AuthorizationEngine; -use crate::application::ports::calendar_ports::{CalendarStoragePort, CalendarUseCase}; +use crate::application::ports::calendar_ports::{ + CalendarStoragePort, CalendarUseCase, UpsertEventsResult, +}; use crate::common::errors::{DomainError, ErrorKind}; use crate::domain::services::authorization::{Permission, Resource, Role, Subject}; use crate::infrastructure::adapters::calendar_storage_adapter::CalendarStorageAdapter; @@ -234,6 +236,21 @@ impl CalendarUseCase for CalendarService { self.calendar_storage.create_event_from_ical(event).await } + async fn upsert_ical_events( + &self, + event: CreateEventICalDto, + user_id: Uuid, + ) -> Result { + // Same gate as create_event_from_ical — a PUT to the collection + // is a write. `Permission::Create` matches the single-event + // path; per-instance exception updates ride on the same + // permission because from the ACL's perspective it's still + // a write to the calendar. + self.require_calendar_perm(&event.calendar_id, user_id, Permission::Create) + .await?; + self.calendar_storage.upsert_ical_events(event).await + } + async fn update_event( &self, event_id: &str, diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index eb0725ed..d0bc4e60 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -795,6 +795,86 @@ impl CalendarEvent { Some((value.trim().to_string(), params)) } + /// Parse a VCALENDAR body containing one or more VEVENT components + /// (typically a master + one or more per-instance exception + /// overrides in the same PUT — RFC 5545 §3.6.1), returning one + /// `CalendarEvent` per VEVENT. + /// + /// Splitting is done on the raw text so each returned entity's + /// `ical_data` remains a valid standalone iCalendar body (the GET + /// path serves it verbatim). Line-folding (§3.1) is preserved + /// because we forward every line as-is inside the extracted block; + /// the ical-crate parser inside `from_ical` unfolds when reading. + /// + /// Nested VALARM / VTODO sub-components inside a VEVENT are + /// carried through unchanged — the scanner only splits on + /// `BEGIN:VEVENT` / `END:VEVENT` at the outer level. + /// + /// Returns `InvalidInput` if the body contains zero VEVENTs — a + /// PUT with no events isn't a state we accept on the CalDAV surface. + pub fn parse_all_events(calendar_id: Uuid, ical_data: &str) -> Result> { + let blocks = Self::split_vevents(ical_data); + + if blocks.is_empty() { + return Err(DomainError::new( + ErrorKind::InvalidInput, + "CalendarEvent", + "No VEVENT components found in iCalendar body", + )); + } + + let mut out = Vec::with_capacity(blocks.len()); + for block in blocks { + // Wrap each VEVENT in a fresh VCALENDAR shell so the + // stored `ical_data` per row is self-describing (RFC 5545 + // §3.4 mandates VERSION + PRODID on any exported body). + let wrapped = format!( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//OxiCloud//NONSGML Calendar//EN\r\n{}END:VCALENDAR\r\n", + block, + ); + out.push(Self::from_ical(calendar_id, wrapped)?); + } + Ok(out) + } + + /// Extract each `BEGIN:VEVENT` … `END:VEVENT` block from the raw + /// body as its own String (CRLF-terminated). Component tags are + /// matched case-insensitively per RFC 5545 §3.1. Anything outside + /// a VEVENT (VTIMEZONE / VTODO / VJOURNAL / calendar-level + /// properties) is discarded — those aren't ours to persist. + fn split_vevents(ical_data: &str) -> Vec { + let mut blocks = Vec::new(); + let mut in_event = false; + let mut current = String::new(); + + for raw_line in ical_data.split('\n') { + let line = raw_line.trim_end_matches('\r'); + // Match the tag ignoring case, allowing surrounding + // whitespace (some clients emit a leading space on folded + // continuations — the raw-line scan sees those but they + // won't start with BEGIN/END so they slot through as + // in-event content, which is correct). + let upper = line.trim_start().to_ascii_uppercase(); + + if upper.starts_with("BEGIN:VEVENT") { + in_event = true; + current.clear(); + } + + if in_event { + current.push_str(line); + current.push_str("\r\n"); + } + + if in_event && upper.starts_with("END:VEVENT") { + blocks.push(std::mem::take(&mut current)); + in_event = false; + } + } + + blocks + } + /// Parse the raw iCalendar body and return the first VEVENT /// component's properties. Returns `None` on any parse failure or /// if the body carries zero events (e.g. a `VCALENDAR` with only @@ -1210,7 +1290,7 @@ END:VCALENDAR\r ev.recurrence_id().is_none(), "master with RRULE should still have recurrence_id = None" ); - assert_eq!(ev.rrule().as_deref(), Some("FREQ=DAILY;COUNT=10")); + assert_eq!(ev.rrule(), Some("FREQ=DAILY;COUNT=10")); } #[test] @@ -1276,6 +1356,193 @@ END:VCALENDAR\r ); } + // ───────────────────────────────────────────────────────────── + // Phase 3 — parse_all_events (multi-VEVENT splitter) + // ───────────────────────────────────────────────────────────── + + /// Timed daily recurring master + one timed exception override, + /// both inside a single VCALENDAR wrapper — the shape a CalDAV + /// client PUTs when it modifies one occurrence. + const MASTER_PLUS_TIMED_EXCEPTION: &str = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T090000Z\r +DTEND:20260101T093000Z\r +SUMMARY:Daily standup\r +RRULE:FREQ=DAILY;COUNT=10\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:daily-1@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260103T110000Z\r +DTEND:20260103T120000Z\r +SUMMARY:Daily standup — rescheduled\r +RECURRENCE-ID:20260103T090000Z\r +END:VEVENT\r +END:VCALENDAR\r +"; + + #[test] + fn parse_all_events_splits_master_and_exception() { + // Both VEVENTs must come back: master with recurrence_id=None, + // exception with recurrence_id=Some. UIDs match (that's what + // ties the exception to its master); it's the recurrence_id + // marker that distinguishes them. + let cal_id = Uuid::new_v4(); + let events = CalendarEvent::parse_all_events(cal_id, MASTER_PLUS_TIMED_EXCEPTION) + .expect("both VEVENTs must parse"); + + assert_eq!(events.len(), 2, "expected master + exception"); + assert_eq!(events[0].ical_uid(), "daily-1@oxicloud.test"); + assert_eq!(events[1].ical_uid(), "daily-1@oxicloud.test"); + + assert!( + events[0].recurrence_id().is_none(), + "first row must be the master (recurrence_id None)" + ); + let rid = events[1] + .recurrence_id() + .expect("second row must be the exception override"); + assert_eq!(rid.to_rfc3339(), "2026-01-03T09:00:00+00:00"); + + assert_eq!(events[0].rrule(), Some("FREQ=DAILY;COUNT=10")); + assert!( + events[1].rrule().is_none(), + "exception overrides do NOT carry RRULE" + ); + + // Each event's stored ical_data must be a self-contained + // VCALENDAR body so the GET path can serve it verbatim. + for e in &events { + assert!(e.ical_data().starts_with("BEGIN:VCALENDAR")); + assert!(e.ical_data().trim_end().ends_with("END:VCALENDAR")); + } + } + + #[test] + fn parse_all_events_lone_master_returns_single_event() { + // No RECURRENCE-ID exception in the body → one row, master. + let cal_id = Uuid::new_v4(); + let events = + CalendarEvent::parse_all_events(cal_id, TIMED_EVENT).expect("plain event must parse"); + assert_eq!(events.len(), 1); + assert!(events[0].recurrence_id().is_none()); + } + + #[test] + fn parse_all_events_all_day_master_plus_all_day_exception() { + // The #528 shape: DATE-form DTSTART on both, DATE-form + // RECURRENCE-ID on the exception. Pre-parser-rewrite this + // silently 500'd because the param-carrying property lines + // were invisible to the substring scanner. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//OxiCloud test//EN\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260105\r +DTEND;VALUE=DATE:20260106\r +SUMMARY:Weekly review\r +RRULE:FREQ=WEEKLY;COUNT=4\r +END:VEVENT\r +BEGIN:VEVENT\r +UID:weekly-allday@oxicloud.test\r +DTSTAMP:20260101T100000Z\r +DTSTART;VALUE=DATE:20260113\r +DTEND;VALUE=DATE:20260114\r +SUMMARY:Weekly review — rescheduled\r +RECURRENCE-ID;VALUE=DATE:20260112\r +END:VEVENT\r +END:VCALENDAR\r +"; + let cal_id = Uuid::new_v4(); + let events = CalendarEvent::parse_all_events(cal_id, body).expect("both must parse"); + assert_eq!(events.len(), 2); + assert!(events[0].all_day()); + assert!(events[1].all_day()); + assert!(events[0].recurrence_id().is_none()); + let rid = events[1].recurrence_id().unwrap(); + assert_eq!(rid.to_rfc3339(), "2026-01-12T00:00:00+00:00"); + } + + #[test] + fn parse_all_events_zero_vevents_is_invalid_input() { + // A VCALENDAR with only calendar-level properties (no events) + // is not a state the CalDAV surface accepts on PUT. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//test//EN\r +END:VCALENDAR\r +"; + let err = + CalendarEvent::parse_all_events(Uuid::new_v4(), body).expect_err("must reject empty"); + assert_eq!(err.kind, ErrorKind::InvalidInput); + } + + #[test] + fn parse_all_events_vtodo_is_ignored() { + // A body carrying only VTODOs (no VEVENTs) is treated as + // "zero events" — we don't persist tasks in the events table. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//test//EN\r +BEGIN:VTODO\r +UID:task-1@x\r +SUMMARY:buy milk\r +END:VTODO\r +END:VCALENDAR\r +"; + let err = CalendarEvent::parse_all_events(Uuid::new_v4(), body) + .expect_err("VTODO-only body must be rejected"); + assert_eq!(err.kind, ErrorKind::InvalidInput); + } + + #[test] + fn parse_all_events_preserves_valarm_inside_vevent() { + // VALARM lives INSIDE a VEVENT. The splitter must NOT be + // fooled by BEGIN:VALARM into thinking a new outer component + // has started — the whole VALARM block must ride along inside + // the parent VEVENT's stored ical_data. + let body = "\ +BEGIN:VCALENDAR\r +VERSION:2.0\r +PRODID:-//test//EN\r +BEGIN:VEVENT\r +UID:with-alarm@x\r +DTSTAMP:20260101T100000Z\r +DTSTART:20260101T090000Z\r +DTEND:20260101T093000Z\r +SUMMARY:Standup with alarm\r +BEGIN:VALARM\r +ACTION:DISPLAY\r +TRIGGER:-PT15M\r +DESCRIPTION:Standup soon\r +END:VALARM\r +END:VEVENT\r +END:VCALENDAR\r +"; + let events = CalendarEvent::parse_all_events(Uuid::new_v4(), body) + .expect("VEVENT with VALARM must parse"); + assert_eq!(events.len(), 1); + let stored = events[0].ical_data(); + assert!( + stored.contains("BEGIN:VALARM"), + "VALARM must survive the split into stored ical_data" + ); + assert!( + stored.contains("END:VALARM"), + "matching END:VALARM must survive too" + ); + } + #[test] fn set_recurrence_id_setter_round_trips() { // Repository rehydration path: `with_id` initialises diff --git a/src/domain/repositories/calendar_event_repository.rs b/src/domain/repositories/calendar_event_repository.rs index d08cc561..90b105e9 100644 --- a/src/domain/repositories/calendar_event_repository.rs +++ b/src/domain/repositories/calendar_event_repository.rs @@ -46,13 +46,35 @@ pub trait CalendarEventRepository: Send + Sync + 'static { end: &DateTime, ) -> CalendarEventRepositoryResult>; - /// Finds an event by its iCalendar UID in a specific calendar + /// Finds an event by its iCalendar UID in a specific calendar. + /// + /// **Master-only lookup.** Filters `recurrence_id IS NULL` so the + /// return value is unambiguous — the row that clients treat as + /// "the event with this UID" is the master. Per-instance override + /// rows share the UID but live under + /// `find_event_by_ical_uid_and_recurrence_id` (see #528). async fn find_event_by_ical_uid( &self, calendar_id: &Uuid, ical_uid: &str, ) -> CalendarEventRepositoryResult>; + /// Finds a specific per-instance exception override for a recurring + /// master (RFC 5545 §3.8.4.4). `recurrence_id` pinpoints which + /// occurrence of the master with the given UID is being targeted; + /// returns `None` if no override has been PUT for that instance + /// yet — which the PUT handler then uses to decide insert vs. + /// update. + /// + /// The row is guaranteed unique by the partial index + /// `idx_calendar_events_exception_unique`. + async fn find_event_by_ical_uid_and_recurrence_id( + &self, + calendar_id: &Uuid, + ical_uid: &str, + recurrence_id: &DateTime, + ) -> CalendarEventRepositoryResult>; + /// Finds the events matching any of the given iCalendar UIDs in one /// indexed query (`ical_uid = ANY(...)`). Used by CalDAV multiget so a /// request for a handful of events never pays for the whole calendar. diff --git a/src/infrastructure/adapters/calendar_storage_adapter.rs b/src/infrastructure/adapters/calendar_storage_adapter.rs index c8a45941..42552539 100644 --- a/src/infrastructure/adapters/calendar_storage_adapter.rs +++ b/src/infrastructure/adapters/calendar_storage_adapter.rs @@ -13,7 +13,7 @@ use crate::application::dtos::calendar_dto::{ CalendarDto, CalendarEventDto, CreateCalendarDto, CreateEventDto, CreateEventICalDto, UpdateCalendarDto, UpdateEventDto, }; -use crate::application::ports::calendar_ports::CalendarStoragePort; +use crate::application::ports::calendar_ports::{CalendarStoragePort, UpsertEventsResult}; use crate::common::errors::{DomainError, ErrorKind}; use crate::domain::entities::calendar::Calendar; use crate::domain::entities::calendar_event::CalendarEvent; @@ -262,6 +262,73 @@ impl CalendarStoragePort for CalendarStorageAdapter { Ok(CalendarEventDto::from(created)) } + async fn upsert_ical_events( + &self, + dto: CreateEventICalDto, + ) -> Result { + let calendar_id = Uuid::parse_str(&dto.calendar_id).map_err(|_| { + DomainError::new( + ErrorKind::InvalidInput, + "Event", + "Invalid calendar ID format", + ) + })?; + + // Verify calendar exists before touching the events table. + let _calendar = self + .calendar_repository + .find_calendar_by_id(&calendar_id) + .await?; + + // Split the body into one CalendarEvent per VEVENT. A body + // with zero VEVENTs (or only VTODOs / VJOURNALs) returns + // InvalidInput here — which the handler layer maps to 400. + let parsed = CalendarEvent::parse_all_events(calendar_id, &dto.ical_data)?; + + let mut out = Vec::with_capacity(parsed.len()); + let mut any_inserted = false; + + for event in parsed { + let ical_uid = event.ical_uid().to_string(); + + // Existing row lookup routes on the master/exception split. + // Master: (calendar_id, ical_uid) WHERE recurrence_id IS NULL + // Exception: (calendar_id, ical_uid, recurrence_id) + let existing = match event.recurrence_id().copied() { + Some(rid) => { + self.event_repository + .find_event_by_ical_uid_and_recurrence_id(&calendar_id, &ical_uid, &rid) + .await? + } + None => { + self.event_repository + .find_event_by_ical_uid(&calendar_id, &ical_uid) + .await? + } + }; + + // Delete-then-insert keeps the DB-level partial unique + // indexes happy and matches the pre-#528 update semantics + // of the single-event path (fresh row id per replace, + // ETag changes on update). + if let Some(existing_event) = existing { + self.event_repository + .delete_event(existing_event.id()) + .await?; + } else { + any_inserted = true; + } + + let created = self.event_repository.create_event(event).await?; + out.push(CalendarEventDto::from(created)); + } + + Ok(UpsertEventsResult { + events: out, + any_inserted, + }) + } + async fn update_event( &self, event_id: &str, diff --git a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs index 17cf365d..ed560275 100644 --- a/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs +++ b/src/infrastructure/repositories/pg/calendar_event_pg_repository.rs @@ -389,6 +389,66 @@ impl CalendarEventRepository for CalendarEventPgRepository { } } + async fn find_event_by_ical_uid_and_recurrence_id( + &self, + calendar_id: &Uuid, + ical_uid: &str, + recurrence_id: &DateTime, + ) -> CalendarEventRepositoryResult> { + // Uses idx_calendar_events_exception_unique — the partial + // unique index on (calendar_id, ical_uid, recurrence_id) + // WHERE recurrence_id IS NOT NULL — for the exact-match seek. + let row_opt = sqlx::query( + r#" + SELECT + id, calendar_id, summary, description, location, + start_time, end_time, all_day, rrule, + created_at, updated_at, ical_uid, ical_data, recurrence_id + FROM caldav.calendar_events + WHERE calendar_id = $1 + AND ical_uid = $2 + AND recurrence_id = $3 + "#, + ) + .bind(calendar_id) + .bind(ical_uid) + .bind(recurrence_id) + .fetch_optional(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!( + "Failed to get calendar event exception by UID+RECURRENCE-ID: {}", + e + )) + })?; + + match row_opt { + Some(row) => { + let mut event = CalendarEvent::with_id( + row.get("id"), + row.get("calendar_id"), + row.get("summary"), + row.get::, _>("description"), + row.get::, _>("location"), + row.get("start_time"), + row.get("end_time"), + row.get("all_day"), + row.get::, _>("rrule"), + row.get("ical_uid"), + row.get("ical_data"), + row.get("created_at"), + row.get("updated_at"), + ) + .map_err(|e| { + DomainError::database_error(format!("Error creating calendar event: {}", e)) + })?; + event.set_recurrence_id(row.get::>, _>("recurrence_id")); + Ok(Some(event)) + } + None => Ok(None), + } + } + async fn find_events_by_ical_uids( &self, calendar_id: &Uuid, diff --git a/src/interfaces/api/handlers/caldav_handler.rs b/src/interfaces/api/handlers/caldav_handler.rs index ce2cbe06..485eaf71 100644 --- a/src/interfaces/api/handlers/caldav_handler.rs +++ b/src/interfaces/api/handlers/caldav_handler.rs @@ -627,87 +627,51 @@ async fn handle_put( let ical_data = String::from_utf8(body_bytes.to_vec()) .map_err(|e| AppError::bad_request(format!("Invalid UTF-8 in iCalendar data: {}", e)))?; - let ical_uid = extract_uid_from_ical(&ical_data); - - // Indexed single-row lookup — listing the whole calendar (every row - // with its ical_data) to find one UID made imports O(N²). - let existing = if let Some(ref uid) = ical_uid { - calendar_service - .get_event_by_ical_uid(calendar_id, uid, user.id) - .await - .unwrap_or_default() - } else { - None + // Route the PUT through `upsert_ical_events` so a body carrying a + // master + N per-instance overrides (RFC 5545 §3.8.4.4 — the + // Thunderbird / Apple Calendar / DAVx⁵ "modify one occurrence" + // shape) persists each VEVENT to its own row instead of the last + // one clobbering the master. See AtalayaLabs/OxiCloud#528. + // + // Kind-aware error mapping (`AppError::from(DomainError)`): + // * `InvalidInput` → 400 (malformed iCal / missing DTSTART) + // * `NotFound` → 404 (calendar doesn't exist / no perm) + // * `AccessDenied` → 403 (caller lacks Write on the calendar) + // * anything else → 500 (genuine server bug) + let create_dto = CreateEventICalDto { + calendar_id: calendar_id.to_string(), + ical_data, }; - if let Some(existing_event) = existing { - // Update existing event — re-create from iCal for full fidelity. - // Both calls use `AppError::from` — the delete propagates - // NotFound/AccessDenied as 404/403, and the recreate propagates - // InvalidInput on malformed iCalendar as 400 (see comment on - // create_event_from_ical below). - calendar_service - .delete_event(&existing_event.id, user.id) - .await - .map_err(AppError::from)?; + let result = calendar_service + .upsert_ical_events(create_dto, user.id) + .await + .map_err(AppError::from)?; - let create_dto = CreateEventICalDto { - calendar_id: calendar_id.to_string(), - ical_data, - }; - let event = calendar_service - .create_event_from_ical(create_dto, user.id) - .await - .map_err(AppError::from)?; + // The event surface still exposes a single object resource per + // UID, so we return an ETag anchored on the master row when + // present, otherwise the first exception's id. This matches the + // pre-#528 header contract for clients that only understand a + // single ETag per PUT. + let etag_source = result + .events + .iter() + .find(|e| e.recurrence_id.is_none()) + .or_else(|| result.events.first()) + .map(|e| e.id.to_string()) + .unwrap_or_default(); - Ok(Response::builder() - .status(StatusCode::NO_CONTENT) - .header(header::ETAG, format!("\"{}\"", event.id)) - .body(Body::empty()) - .unwrap()) + let status = if result.any_inserted { + StatusCode::CREATED } else { - let create_dto = CreateEventICalDto { - calendar_id: calendar_id.to_string(), - ical_data, - }; + StatusCode::NO_CONTENT + }; - // `AppError::from(DomainError)` (via the `From` impl in - // `interfaces/errors.rs`) maps the ErrorKind onto the correct - // HTTP status: - // * `InvalidInput` → 400 (e.g. "Missing DTSTART in iCalendar - // data" from `CalendarEvent::from_ical`) — this is the fix - // for AtalayaLabs/OxiCloud#545 comment from `funboytwo`. - // * `NotFound` → 404 (parent calendar doesn't exist) - // * `AccessDenied` → 403 (caller lacks Write on the calendar) - // * `DatabaseError`/`InternalError` → 500 (genuine server bug) - // - // The old `map_err(|e| AppError::internal_error(...))` was - // blanket-wrapping every case as 500, hiding client-input bugs - // as opaque server errors. Downstream monitoring (500 rate, - // pager alerts) took the false hit; users saw an unhelpful - // "Internal Server Error" for their own bad iCalendar. - let event = calendar_service - .create_event_from_ical(create_dto, user.id) - .await - .map_err(AppError::from)?; - - Ok(Response::builder() - .status(StatusCode::CREATED) - .header(header::ETAG, format!("\"{}\"", event.id)) - .body(Body::empty()) - .unwrap()) - } -} - -/// Extract UID from iCalendar data -fn extract_uid_from_ical(ical_data: &str) -> Option { - for line in ical_data.lines() { - let trimmed = line.trim(); - if let Some(stripped) = trimmed.strip_prefix("UID:") { - return Some(stripped.trim().to_string()); - } - } - None + Ok(Response::builder() + .status(status) + .header(header::ETAG, format!("\"{}\"", etag_source)) + .body(Body::empty()) + .unwrap()) } // ─── GET (.ics) ────────────────────────────────────────────────────── diff --git a/tests/api/caldav_recurring.hurl b/tests/api/caldav_recurring.hurl new file mode 100644 index 00000000..9caa1bf2 --- /dev/null +++ b/tests/api/caldav_recurring.hurl @@ -0,0 +1,280 @@ +# ============================================================= +# OxiCloud – CalDAV recurring events with RECURRENCE-ID overrides +# ============================================================= +# End-to-end regression for AtalayaLabs/OxiCloud#528. +# +# Pre-fix behaviour (all-day recurring event, one occurrence +# modified in Thunderbird/Apple Calendar/DAVx⁵/Gnome Calendar): +# * The client PUTs a VCALENDAR containing the master (with +# RRULE) + a per-instance override (RFC 5545 §3.8.4.4, +# `RECURRENCE-ID`). Pre-fix the substring-based parser +# could not read any property carrying parameters +# (`DTSTART;VALUE=DATE:...`, `RECURRENCE-ID;VALUE=DATE:...`), +# so all-day master modifications 500'd outright. +# * Even for timed events, the old create_event_from_ical +# read only the first VEVENT — a second PUT of just the +# exception would overwrite the master row entirely, +# silently corrupting the client's view of the series. +# +# Post-fix (this file's invariant): +# 1. Master PUT → 201 CREATED, one row (recurrence_id NULL). +# 2. PUT master + exception in one body → both persist to +# their own row keyed by (calendar_id, ical_uid, +# recurrence_id). Response is 201 CREATED because the +# exception was newly inserted. +# 3. PUT ONLY the exception with modified content → 204 +# No Content (in-place replace, no new rows). CRITICALLY, +# the MASTER row survives untouched — a GET on the .ics +# URL still returns the master's original RRULE + summary. +# 4. All-day master + all-day exception (the exact #528 shape) +# completes the same round-trip. +# +# Storage invariant enforced by two partial unique indexes on +# caldav.calendar_events (see migration 20260913000001): +# * idx_calendar_events_master_unique — at most one master +# per (calendar_id, ical_uid). +# * idx_calendar_events_exception_unique — at most one +# exception override per (calendar_id, ical_uid, +# recurrence_id). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 – Admin logs in. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ + "username": "{{username}}", + "password": "{{password}}" +} + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 – MKCALENDAR: fresh calendar for #528 regression. +# ───────────────────────────────────────────────────────────── +MKCALENDAR {{base_url}}/caldav/recurring-528/ +Authorization: Bearer {{admin_token}} + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 3 – PROPFIND to capture the server-assigned UUID for +# recurring-528. The (?s).* anchor greedy-matches to the LAST +# /caldav// in the body, which is our just-created +# calendar (default-provisioned calendars come first by +# created_at, this one is newest). +# ───────────────────────────────────────────────────────────── +PROPFIND {{base_url}}/caldav/ +Authorization: Bearer {{admin_token}} +Depth: 1 +Content-Type: application/xml +``` + + + + + + + +``` + +HTTP 207 +[Captures] +calendar_id: body regex "(?s).*/caldav/([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})/" +[Asserts] +body contains "recurring-528" + + +# ───────────────────────────────────────────────────────────── +# Step 4 – PUT the recurring master (timed, daily, 10 count). +# Expect 201 CREATED (fresh row) and a non-empty ETag. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} +Content-Type: text/calendar; charset=utf-8 +``` +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//OxiCloud e2e//EN +BEGIN:VEVENT +UID:daily-e2e-528 +DTSTAMP:20260101T100000Z +DTSTART:20260101T090000Z +DTEND:20260101T093000Z +SUMMARY:Daily standup +RRULE:FREQ=DAILY;COUNT=10 +END:VEVENT +END:VCALENDAR +``` + +HTTP 201 +[Asserts] +header "ETag" exists + + +# ───────────────────────────────────────────────────────────── +# Step 5 – GET the master. Body contains RRULE + original +# SUMMARY, confirming the master is stored and serves as-is. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +body contains "FREQ=DAILY;COUNT=10" +body contains "SUMMARY:Daily standup" + + +# ───────────────────────────────────────────────────────────── +# Step 6 – The #528 heart: PUT master + per-instance override +# in a single body. This is what Thunderbird sends when the +# user modifies one occurrence of a recurring event. +# +# Expected: +# * 201 CREATED because the exception is newly inserted. +# (The master is replaced-in-place — any_inserted=true +# is decided by the NEW exception row, not the master.) +# * Both rows now exist in the DB. Verified in Step 7 via +# the master's GET still returning the master data. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} +Content-Type: text/calendar; charset=utf-8 +``` +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//OxiCloud e2e//EN +BEGIN:VEVENT +UID:daily-e2e-528 +DTSTAMP:20260101T100000Z +DTSTART:20260101T090000Z +DTEND:20260101T093000Z +SUMMARY:Daily standup +RRULE:FREQ=DAILY;COUNT=10 +END:VEVENT +BEGIN:VEVENT +UID:daily-e2e-528 +DTSTAMP:20260101T100000Z +DTSTART:20260103T110000Z +DTEND:20260103T120000Z +SUMMARY:Daily standup — rescheduled +RECURRENCE-ID:20260103T090000Z +END:VEVENT +END:VCALENDAR +``` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 7 – GET the master. It must STILL be the master (with +# RRULE + original SUMMARY). Pre-fix the exception would have +# clobbered this row and Step 7 would see the exception's +# SUMMARY ("… rescheduled") without the RRULE. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +body contains "FREQ=DAILY;COUNT=10" +body contains "SUMMARY:Daily standup" +body not contains "SUMMARY:Daily standup — rescheduled" + + +# ───────────────────────────────────────────────────────────── +# Step 8 – PUT only the exception with a modified SUMMARY. +# Because the exception row already exists (from Step 6), +# no new row is inserted → 204 No Content. The MASTER is +# untouched (verified in Step 9). +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} +Content-Type: text/calendar; charset=utf-8 +``` +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//OxiCloud e2e//EN +BEGIN:VEVENT +UID:daily-e2e-528 +DTSTAMP:20260101T110000Z +DTSTART:20260103T120000Z +DTEND:20260103T130000Z +SUMMARY:Daily standup — rescheduled AGAIN +RECURRENCE-ID:20260103T090000Z +END:VEVENT +END:VCALENDAR +``` + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 9 – Master survives the exception update. Pre-fix this +# would fail: the old delete-by-UID-then-insert path would +# have removed the master when the exception-only PUT landed. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/caldav/{{calendar_id}}/daily-e2e-528.ics +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +body contains "FREQ=DAILY;COUNT=10" +body contains "SUMMARY:Daily standup" +body not contains "SUMMARY:Daily standup — rescheduled" + + +# ───────────────────────────────────────────────────────────── +# Step 10 – The all-day flavour: master with DTSTART;VALUE=DATE +# + exception with RECURRENCE-ID;VALUE=DATE. Pre-parser-rewrite +# this 500'd because the param-carrying property lines were +# invisible to the substring scanner (root cause of #528). +# +# Uses a distinct UID so it doesn't collide with Step 4-8 rows +# under the master partial unique index. +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/caldav/{{calendar_id}}/weekly-allday-528.ics +Authorization: Bearer {{admin_token}} +Content-Type: text/calendar; charset=utf-8 +``` +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//OxiCloud e2e//EN +BEGIN:VEVENT +UID:weekly-allday-528 +DTSTAMP:20260101T100000Z +DTSTART;VALUE=DATE:20260105 +DTEND;VALUE=DATE:20260106 +SUMMARY:Weekly review +RRULE:FREQ=WEEKLY;COUNT=4 +END:VEVENT +BEGIN:VEVENT +UID:weekly-allday-528 +DTSTAMP:20260101T100000Z +DTSTART;VALUE=DATE:20260113 +DTEND;VALUE=DATE:20260114 +SUMMARY:Weekly review — moved +RECURRENCE-ID;VALUE=DATE:20260112 +END:VEVENT +END:VCALENDAR +``` + +HTTP 201 + + +# ───────────────────────────────────────────────────────────── +# Step 11 – Cleanup: delete the entire calendar (cascades to +# all events + exception rows in a single storage call). Keeps +# the shared Hurl DB uncluttered for downstream test files +# (per feedback_hurl_teardown_shared_db). +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/caldav/{{calendar_id}}/ +Authorization: Bearer {{admin_token}} + +HTTP 204 diff --git a/tests/api/run.sh b/tests/api/run.sh index a30d7e90..3fc6ff98 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -168,6 +168,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/dav_error_mapping.hurl" \ "$API_DIR/contacts.hurl" \ "$API_DIR/calendar.hurl" \ + "$API_DIR/caldav_recurring.hurl" \ "$API_DIR/playlists.hurl" \ "$API_DIR/public_shares.hurl" \ "$API_DIR/permissions.hurl" \