From 4499054228e870a1b869a886c98d7217e8a70b4a Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 17:27:12 -0500 Subject: [PATCH 1/2] feat(calendar): expand RRULE recurrence in ListEvents and filter by calendar_id Wire the previously-dead nuncio_cal::RecurrenceEngine into CalendarGrpcService::list_events: non-recurring events pass through unchanged, while recurring masters are expanded into real occurrences for the requested window via a new store method (list_recurring_calendar_events, no time filter since a master's own timestamps can precede the window while its occurrences fall inside it) and a pure expand_events_for_window merge helper that keeps recurring rows sourced only from expansion, never double-counting a master that also overlaps the window in the raw query. Fix expand_occurrences to stop trusting an empty match as "return the raw master": that fabricated an out-of-window result whenever a window fell entirely outside a bounded (UNTIL/COUNT) series, and could hang scanning an unbounded series for a match that would never come. skip_while/take_while over the RRuleSet's monotonic iterator bounds the walk and returns a genuinely empty result instead. Also apply the calendar_id filter list_events accepted but ignored, closing the gap between it and the mock backend's own filtering. Refs #186 --- crates/nuncio-cal/src/rrule.rs | 50 ++++++- crates/nuncio-store/src/db.rs | 91 ++++++++++++ crates/nunciod/src/grpc.rs | 250 +++++++++++++++++++++++++++++++-- 3 files changed, 376 insertions(+), 15 deletions(-) diff --git a/crates/nuncio-cal/src/rrule.rs b/crates/nuncio-cal/src/rrule.rs index 2fb763b..7148521 100644 --- a/crates/nuncio-cal/src/rrule.rs +++ b/crates/nuncio-cal/src/rrule.rs @@ -47,9 +47,15 @@ impl RecurrenceEngine { let duration = event.end_time - event.start_time; + // `RRuleSet`'s iterator yields occurrences in strictly increasing date order, so once + // an occurrence passes `window_end_dt` every later one will too -- `take_while` lets + // us stop walking the (possibly unbounded, no-UNTIL/COUNT) series there instead of + // scanning forever for a match that will never come once the series has moved past + // the window. `skip_while` similarly avoids materializing pre-window occurrences. let results = rrule_set .into_iter() - .filter(|dt| *dt >= window_start_dt && *dt <= window_end_dt) + .skip_while(|dt| *dt < window_start_dt) + .take_while(|dt| *dt <= window_end_dt) .take(Self::MAX_EXPANSION_COUNT as usize); let mut occurrences = Vec::new(); @@ -62,12 +68,11 @@ impl RecurrenceEngine { occurrences.push(instance); } - if occurrences.is_empty() { - // Return original if no occurrences fall strictly inside window - Ok(vec![event.clone()]) - } else { - Ok(occurrences) - } + // No fallback to the raw master here: an empty result genuinely means no occurrence + // of this recurring event falls inside the requested window (e.g. the window is past + // an `UNTIL`/`COUNT` bound), and returning the master's own out-of-window timestamps + // instead would fabricate a result the caller never asked for. + Ok(occurrences) } } @@ -110,6 +115,37 @@ mod tests { assert!(results[0].id.contains("evt-weekly_occ_")); } + #[test] + fn recurring_event_with_no_occurrences_in_window_returns_empty() { + let event = sample_recurring_event(); + // Window is entirely before the series' own DTSTART -- no occurrence can fall + // inside it, so this must return no events at all, not the out-of-window master. + let results = RecurrenceEngine::expand_occurrences(&event, 1_600_000_000, 1_600_003_600) + .expect("expansion succeeds"); + assert!(results.is_empty()); + } + + #[test] + fn until_bound_stops_occurrences_past_the_bound() { + let mut event = sample_recurring_event(); + // Weekly series ends after its third occurrence (2024-01-15). + event.rrule = Some("FREQ=WEEKLY;INTERVAL=1;UNTIL=20240115T000000Z".to_string()); + + // Querying a window entirely after the UNTIL bound must produce zero occurrences. + let past_bound = RecurrenceEngine::expand_occurrences(&event, 1_707_350_400, 1_709_251_200) + .expect("expansion succeeds"); + assert!(past_bound.is_empty()); + + // Querying a window spanning the bound only returns occurrences up to and + // including it, never past it. + let spanning_bound = RecurrenceEngine::expand_occurrences(&event, 1704067200, 1706745600) + .expect("expansion succeeds"); + assert_eq!(spanning_bound.len(), 3); + for occurrence in &spanning_bound { + assert!(occurrence.start_time <= 1_705_276_800); // 2024-01-15T00:00:00Z + } + } + #[test] fn invalid_rrule_returns_parse_error() { let mut event = sample_recurring_event(); diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 79066c2..dc1759a 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -995,6 +995,58 @@ impl DatabaseEngine { .collect()) } + /// Query every persisted recurring calendar event (non-null, non-empty `rrule`) for + /// `account_id`, with **no** time-window filter -- a recurring master's own stored + /// `start_time`/`end_time` can validly sit long before (or after) any given query + /// window while its expanded occurrences still fall inside it, so callers that need + /// to materialize occurrences for a window must expand every one of the account's + /// recurring masters rather than pre-filtering by the master row's own timestamps. + #[allow(clippy::type_complexity)] + pub async fn list_recurring_calendar_events( + &self, + account_id: &str, + ) -> Result, DatabaseError> { + let rows: Vec<( + String, + String, + String, + String, + i64, + i64, + Option, + Option, + )> = sqlx::query_as( + r#" + SELECT id, account_id, calendar_id, summary, start_time, end_time, rrule, location + FROM calendar_events + WHERE account_id = ? AND rrule IS NOT NULL AND rrule != '' + ORDER BY start_time ASC + "#, + ) + .bind(account_id) + .fetch_all(&self.pool) + .await + .map_err(DatabaseError::Query)?; + + Ok(rows + .into_iter() + .map( + |(id, account_id, calendar_id, summary, start_time, end_time, rrule, location)| { + nuncio_core::model::CalendarEvent { + id, + account_id, + calendar_id, + summary, + start_time, + end_time, + rrule, + location, + } + }, + ) + .collect()) + } + /// Retrieve a single calendar event by ID. #[allow(clippy::type_complexity)] pub async fn get_calendar_event( @@ -1981,6 +2033,45 @@ mod tests { assert_eq!(hits[0].id, "evt-db-100"); } + /// `list_recurring_calendar_events` returns only rows with a non-empty `rrule`, for the + /// requested account only, and applies no time-window filter at all -- a master whose own + /// stored `start_time` is far outside any given query window must still come back, since + /// its expanded occurrences may fall inside that window. + #[tokio::test] + async fn list_recurring_calendar_events_filters_by_account_and_rrule_presence() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let mut recurring = sample_calendar_event("evt-recurring", "acct-1"); + recurring.rrule = Some("FREQ=WEEKLY;INTERVAL=1".to_string()); + recurring.start_time = 1_000; // far outside any window used below + recurring.end_time = 4_600; + engine.save_calendar_event(&recurring).await.unwrap(); + + let non_recurring = sample_calendar_event("evt-single", "acct-1"); + engine.save_calendar_event(&non_recurring).await.unwrap(); + + let mut other_account_recurring = sample_calendar_event("evt-other-acct", "acct-2"); + other_account_recurring.rrule = Some("FREQ=DAILY".to_string()); + engine + .save_calendar_event(&other_account_recurring) + .await + .unwrap(); + + let recurring_for_acct1 = engine + .list_recurring_calendar_events("acct-1") + .await + .expect("list recurring calendar events succeeds"); + assert_eq!(recurring_for_acct1.len(), 1); + assert_eq!(recurring_for_acct1[0].id, "evt-recurring"); + + let recurring_for_acct2 = engine + .list_recurring_calendar_events("acct-2") + .await + .expect("list recurring calendar events succeeds"); + assert_eq!(recurring_for_acct2.len(), 1); + assert_eq!(recurring_for_acct2[0].id, "evt-other-acct"); + } + /// Re-saving an event with the same ID (an update, not a fresh insert) replaces both the /// row and its FTS entry rather than duplicating either. #[tokio::test] diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index a8a2654..9d1297a 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -439,6 +439,48 @@ fn map_calendar_event_to_proto(event: nuncio_core::model::CalendarEvent) -> Cale } } +/// Combines an account's in-window events with its recurring masters into the final event +/// list for a `ListEvents` query window, expanding each recurring master into its real +/// occurrences via [`nuncio_cal::RecurrenceEngine::expand_occurrences`] rather than trusting +/// the master row's own stored `start_time`/`end_time` to represent every future instance. +/// +/// `recurring` events are never sourced from `all_in_window` for the final result -- only +/// `e.rrule.is_none()` rows from `all_in_window` are kept verbatim -- so a recurring master +/// that also happens to overlap the window in `all_in_window` is never emitted a second time +/// alongside its expanded occurrences. +/// +/// A recurring event whose `rrule` fails to parse degrades to the raw stored master (mirroring +/// [`RecurrenceEngine::expand_occurrences`]'s own no-occurrences-found fallback) with a warning +/// logged, rather than failing the whole query for one malformed rule among many. +fn expand_events_for_window( + all_in_window: Vec, + recurring: Vec, + start_window: i64, + end_window: i64, +) -> Vec { + let mut events: Vec = all_in_window + .into_iter() + .filter(|e| e.rrule.is_none()) + .collect(); + + for event in recurring { + match nuncio_cal::RecurrenceEngine::expand_occurrences(&event, start_window, end_window) { + Ok(occurrences) => events.extend(occurrences), + Err(e) => { + tracing::warn!( + "Calendar: failed to expand rrule for event '{}', falling back to raw \ + stored event: {}", + event.id, + e + ); + events.push(event); + } + } + } + + events +} + /// Test-only injection point for the daemon's calendar sync backend, /// mirroring [`MailEngineOverrides`]'s shape. /// @@ -517,18 +559,25 @@ impl Calendar for CalendarGrpcService { if req.account_id.is_empty() { return Err(Status::invalid_argument("account_id is required")); } - // `DatabaseEngine::list_calendar_events` filters by account_id and - // time window only -- there is no per-calendar-collection filter in - // the store today, so `req.calendar_id` is not applied here. This - // never fabricates data; it genuinely returns every persisted event - // for the account in the requested window, same as `req.calendar_id` - // being absent from the query would. - let events = self + let all_in_window = self .db .list_calendar_events(&req.account_id, req.start_window, req.end_window) .await - .map_err(|e| Status::internal(format!("failed to list calendar events: {e}")))? + .map_err(|e| Status::internal(format!("failed to list calendar events: {e}")))?; + + let recurring = self + .db + .list_recurring_calendar_events(&req.account_id) + .await + .map_err(|e| Status::internal(format!("failed to list recurring events: {e}")))?; + + let mut events = + expand_events_for_window(all_in_window, recurring, req.start_window, req.end_window); + + events.retain(|e| req.calendar_id.is_empty() || e.calendar_id == req.calendar_id); + + let events = events .into_iter() .map(map_calendar_event_to_proto) .collect(); @@ -3705,6 +3754,65 @@ mod tests { assert_eq!(response.events[0].id, "evt-in-window"); } + /// `ListEvents` filters by `calendar_id` when it's non-empty: an event that overlaps the + /// window but belongs to a different calendar collection must not appear in the response. + #[tokio::test] + async fn list_events_excludes_events_from_a_different_calendar_id() { + use nuncio_proto::v1::calendar_client::CalendarClient; + use nuncio_proto::v1::ListEventsRequest; + + let (db, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + db.save_calendar_event(&sample_calendar_event( + "evt-work-cal", + "acct-cal-1", + "cal-work", + 1_700_000_000, + 1_700_003_600, + )) + .await + .expect("save work-calendar event"); + db.save_calendar_event(&sample_calendar_event( + "evt-personal-cal", + "acct-cal-1", + "cal-personal", + 1_700_000_000, + 1_700_003_600, + )) + .await + .expect("save personal-calendar event"); + + let event_bus = Arc::new(EventBus::new()); + let secrets = Arc::new(SecretManager::mock()); + let filter_engine = Arc::new(FilterEngine::new(Vec::new()).expect("empty rule set")); + let (addr, _handle) = spawn_test_server_with( + event_bus, + Arc::new(db), + filter_engine, + secrets, + "correct-token", + ) + .await; + + let mut client = CalendarClient::connect(format!("http://{addr}")) + .await + .expect("client connects"); + let response = client + .list_events(authed_bearer_request(ListEventsRequest { + account_id: "acct-cal-1".to_string(), + calendar_id: "cal-work".to_string(), + start_window: 1_699_999_000, + end_window: 1_700_004_000, + })) + .await + .expect("list_events succeeds") + .into_inner(); + + assert_eq!(response.events.len(), 1); + assert_eq!(response.events[0].id, "evt-work-cal"); + } + #[tokio::test] async fn list_events_rejects_empty_account_id() { use nuncio_proto::v1::calendar_client::CalendarClient; @@ -3908,4 +4016,130 @@ mod tests { "Summary for evt-sync-injected-1" ); } + + // ---- expand_events_for_window (pure, no DB/gRPC) ---- + + fn recurring_event( + id: &str, + rrule: &str, + start: i64, + end: i64, + ) -> nuncio_core::model::CalendarEvent { + let mut event = sample_calendar_event(id, "acct-cal-1", "cal-work", start, end); + event.rrule = Some(rrule.to_string()); + event + } + + /// A weekly event's later occurrence (well past the stored master's own start/end) is + /// returned when the query window covers only that later occurrence -- proving the merge + /// helper materializes real occurrences rather than trusting the master row's own window + /// membership. + #[test] + fn expand_events_for_window_finds_later_occurrence_of_a_recurring_event() { + let master = recurring_event( + "evt-weekly", + "FREQ=WEEKLY;INTERVAL=1", + 1_704_067_200, // 2024-01-01T00:00:00Z + 1_704_070_800, + ); + + // Query a window 4 weeks later, which does not overlap the master's own + // start/end at all. + let start_window = 1_706_400_000; // 2024-01-28 + let end_window = 1_706_500_000; + + let result = expand_events_for_window(Vec::new(), vec![master], start_window, end_window); + + assert_eq!(result.len(), 1); + assert!(result[0].start_time >= start_window && result[0].start_time <= end_window); + assert!(result[0].id.starts_with("evt-weekly_occ_")); + } + + /// A non-recurring event passed through `all_in_window` is returned unchanged. + #[test] + fn expand_events_for_window_passes_non_recurring_events_through_unchanged() { + let event = sample_calendar_event( + "evt-single", + "acct-cal-1", + "cal-work", + 1_700_000_000, + 1_700_003_600, + ); + + let result = expand_events_for_window( + vec![event.clone()], + Vec::new(), + 1_699_999_000, + 1_700_004_000, + ); + + assert_eq!(result, vec![event]); + } + + /// A recurring master whose own stored start/end also overlaps the query window must + /// never appear twice -- once as the raw row from `all_in_window` and again as an + /// expanded occurrence. + #[test] + fn expand_events_for_window_never_double_counts_a_recurring_master() { + let master = recurring_event( + "evt-weekly", + "FREQ=WEEKLY;INTERVAL=1", + 1_704_067_200, + 1_704_070_800, + ); + + // The master's own row also overlaps the window (as `list_calendar_events` + // would genuinely return it), simulating the exact double-count risk. + let result = expand_events_for_window( + vec![master.clone()], + vec![master], + 1_704_067_200, + 1_704_070_800, + ); + + assert_eq!(result.len(), 1); + assert!(result[0].id.starts_with("evt-weekly_occ_")); + } + + /// A `COUNT`/`UNTIL`-bounded recurrence produces no events for a window entirely past + /// its bound. + #[test] + fn expand_events_for_window_honors_until_bound() { + let master = recurring_event( + "evt-bounded", + "FREQ=WEEKLY;INTERVAL=1;UNTIL=20240115T000000Z", + 1_704_067_200, + 1_704_070_800, + ); + + let result = expand_events_for_window( + Vec::new(), + vec![master], + 1_709_251_200, // 2024-03-01, well past the UNTIL bound + 1_709_337_600, + ); + + assert!(result.is_empty()); + } + + /// An event with an unparseable `rrule` degrades to the raw stored master rather than + /// vanishing or failing the whole merge. + #[test] + fn expand_events_for_window_degrades_unparseable_rrule_to_raw_master() { + let master = recurring_event( + "evt-bad-rrule", + "NOT_A_VALID_RRULE", + 1_700_000_000, + 1_700_003_600, + ); + + let result = expand_events_for_window( + Vec::new(), + vec![master.clone()], + 1_699_999_000, + 1_700_004_000, + ); + + assert_eq!(result, vec![master]); + } } -- 2.54.0 From 84ac82b29d375c3559343ad85da71e08be44476a Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 17:33:52 -0500 Subject: [PATCH 2/2] docs(nunciod): reword stale comment referencing removed rrule fallback --- crates/nunciod/src/grpc.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index 9d1297a..a33c0f7 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -449,8 +449,7 @@ fn map_calendar_event_to_proto(event: nuncio_core::model::CalendarEvent) -> Cale /// that also happens to overlap the window in `all_in_window` is never emitted a second time /// alongside its expanded occurrences. /// -/// A recurring event whose `rrule` fails to parse degrades to the raw stored master (mirroring -/// [`RecurrenceEngine::expand_occurrences`]'s own no-occurrences-found fallback) with a warning +/// A recurring event whose `rrule` fails to parse degrades to the raw stored master (a genuine persisted row -- just unexpanded, not fabricated data) with a warning /// logged, rather than failing the whole query for one malformed rule among many. fn expand_events_for_window( all_in_window: Vec, -- 2.54.0