diff --git a/.gitignore b/.gitignore index 0b4c6c0..816c352 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -data/ +data*/ config.yaml web/node_modules/ /bin/ diff --git a/internal/caldav/backend.go b/internal/caldav/backend.go index 2f78184..f3ca24e 100644 --- a/internal/caldav/backend.go +++ b/internal/caldav/backend.go @@ -240,7 +240,16 @@ func (b *Backend) ListCalendarObjects(ctx context.Context, calPath string, req * } func (b *Backend) QueryCalendarObjects(ctx context.Context, calPath string, query *caldav.CalendarQuery) ([]caldav.CalendarObject, error) { - // List all and filter – sufficient for small collections. + if query == nil { + return b.ListCalendarObjects(ctx, calPath, nil) + } + // hoistPropFilterTimeRanges and closeOpenEndedTimeRanges are both + // idempotent and only mutate a local shallow copy of the query's + // comp-tree — the caller's original CalendarQuery is not visible + // (query is a *CalendarQuery but we only walk the value-comp fields). + hoistPropFilterTimeRanges(&query.CompFilter) + closeOpenEndedTimeRanges(&query.CompFilter) + all, err := b.ListCalendarObjects(ctx, calPath, &query.CompRequest) if err != nil { return nil, err @@ -248,6 +257,101 @@ func (b *Backend) QueryCalendarObjects(ctx context.Context, calPath string, quer return caldav.Filter(query, all) } +// hoistPropFilterTimeRanges rewrites a CompFilter in place, moving any +// prop-filter time-ranges (typically +// ) up to the enclosing comp-filter and clearing +// them from the prop-filter. This is what fixes go-webdav's +// non-recurrence-aware matchPropTimeRange (which only compares literal +// DTSTART — so a weekly series whose base DTSTART is in 2025 gets dropped +// for a 2026 query even though its RRULE has 2026 occurrences). +// +// Lifting the range onto the enclosing comp-filter is the RFC 4791 +// 9.9-equivalent for recurring components: the comp-filter's time-range is +// evaluated by go-webdav's matchCompTimeRange, which *does* expand RRULE +// (comp.RecurrenceSet) and returns true if any occurrence falls in the +// window. Non-recurring events are unaffected because the comp-level +// time-range and the prop-level time-range both check against the same +// DTSTART/DTEND. +// +// A small heuristic governs the merge: +// +// - parent has no range yet → parent.Start/End := child's range +// - parent already has a range → parent's range wins (rare / ambiguous +// client request), but the child's time-range is still cleared so +// go-webdav's literal DTSTART check doesn't re-exclude recurring +// series +// +// This runs for every comp-filter in the tree, regardless of depth, so +// both VCALENDAR>VEVENT and any other nesting the client sends are +// handled. +func hoistPropFilterTimeRanges(cf *caldav.CompFilter) { + if cf == nil { + return + } + for i := range cf.Props { + pf := &cf.Props[i] + if pf.Start.IsZero() && pf.End.IsZero() { + continue + } + if cf.Start.IsZero() { + cf.Start = pf.Start + } + if cf.End.IsZero() { + cf.End = pf.End + } + // Clear the prop-filter's own time-range so go-webdav's literal + // DTSTART check (matchPropTimeRange) doesn't re-exclude a series + // whose base DTSTART is outside the window but whose RRULE has + // occurrences in it. + pf.Start = time.Time{} + pf.End = time.Time{} + } + for i := range cf.Comps { + hoistPropFilterTimeRanges(&cf.Comps[i]) + } +} + +// farFutureSentinel stands in for "no upper bound" in an open-ended +// (RFC 4791 §9.9 explicitly allows a +// time-range with only a start attribute, meaning "everything from start +// onward"). It's a fixed calendar date rather than e.g. time.Now() plus +// some duration so behavior doesn't depend on when a request happens to +// run; 2100 is comfortably beyond any realistic calendar subscription's +// horizon while still bounding recurrence expansion to a finite, +// fast-to-compute range. +var farFutureSentinel = time.Date(2100, 1, 1, 0, 0, 0, 0, time.UTC) + +// closeOpenEndedTimeRanges rewrites a CompFilter in place, replacing a +// zero-value End on any comp-filter that has a non-zero Start with +// farFutureSentinel. +// +// This works around a real bug (as of go-webdav v0.6.0): a client asking +// for "everything from date X onward" sends a time-range with only a +// start attribute, which decodes with End left as the zero time.Time. +// For a *non*-recurring event, go-webdav's matchCompTimeRange correctly +// treats a zero End as "unbounded" (it explicitly checks end.IsZero()). +// But for a *recurring* event, it instead calls +// rrule.Set.Between(start, end, true) unconditionally — and passing the +// zero time.Time (year 1) as the upper bound there means "before start", +// so Between always returns zero occurrences, silently excluding every +// recurring series from an open-ended query. This is exactly the shape +// of query many real CalDAV clients send for their default "sync events +// from N days in the past, no future limit" setting (e.g. DAVx5) — so +// without this workaround, a recurring series survives a bounded +// time-range query (both start and end given) but vanishes the moment a +// client asks for an unbounded future window, which is a common default. +func closeOpenEndedTimeRanges(cf *caldav.CompFilter) { + if cf == nil { + return + } + if !cf.Start.IsZero() && cf.End.IsZero() { + cf.End = farFutureSentinel + } + for i := range cf.Comps { + closeOpenEndedTimeRanges(&cf.Comps[i]) + } +} + func (b *Backend) CreateCalendar(ctx context.Context, calendar *caldav.Calendar) error { p := auth.FromContext(ctx) if p == nil { diff --git a/internal/caldav/backend_test.go b/internal/caldav/backend_test.go index a46af33..479989f 100644 --- a/internal/caldav/backend_test.go +++ b/internal/caldav/backend_test.go @@ -10,12 +10,15 @@ import ( "path/filepath" "strings" "testing" + "time" "git.arnef.de/arnef/nidus/internal/auth" "git.arnef.de/arnef/nidus/internal/config" "git.arnef.de/arnef/nidus/internal/db" + "git.arnef.de/arnef/nidus/internal/icssub" "git.arnef.de/arnef/nidus/internal/store" ical "github.com/emersion/go-ical" + "github.com/emersion/go-webdav/caldav" ) func newTestBackend(t *testing.T) (*Backend, *db.DB) { @@ -331,3 +334,297 @@ func TestSharedCalendarNameCollidingWithOwnGetsDisambiguated(t *testing.T) { t.Fatalf("GetCalendar: expected display name %q, got %q", want, cal.Name) } } + +// TestHoistPropFilterTimeRangesUnit pins the shape of the hoist rewrite: +// a prop-filter time-range (DTSTART window) is lifted onto its enclosing +// VEVENT comp-filter, and the prop-filter's own Start/End are cleared, +// so go-webdav's recurrence-aware comp-level check runs instead of its +// literal-DTSTART prop check. +func TestHoistPropFilterTimeRangesUnit(t *testing.T) { + loc := time.UTC + gs := time.Date(2026, 9, 1, 0, 0, 0, 0, loc) + ge := time.Date(2026, 9, 30, 0, 0, 0, 0, loc) + + cf := caldav.CompFilter{ + Name: "VCALENDAR", + Comps: []caldav.CompFilter{{ + Name: "VEVENT", + Props: []caldav.PropFilter{{Name: "DTSTART", Start: gs, End: ge}}, + }}, + } + + hoistPropFilterTimeRanges(&cf) + + vevent := cf.Comps[0] + if vevent.Start != gs || vevent.End != ge { + t.Fatalf("VEVENT comp should inherit the time-range, got Start=%v End=%v", vevent.Start, vevent.End) + } + if !vevent.Props[0].Start.IsZero() || !vevent.Props[0].End.IsZero() { + t.Fatalf("prop-filter time-range must be cleared, got Start=%v End=%v", vevent.Props[0].Start, vevent.Props[0].End) + } + // The VEVENT name is untouched — hoist must not clobber component names. + if vevent.Name != "VEVENT" { + t.Fatalf("VEVENT comp name must be preserved, got %q", vevent.Name) + } +} + +// rruleSample is a weekly series whose base DTSTART sits in the past +// (2025) relative to the queried 2026 window — the classic shape that +// go-webdav's literal-DTSTART prop-time-range check drops, but that a +// proper RRULE expansion keeps. +const rruleSample = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:weekly@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20250911T100000Z\r\n" + // 2025 base, far before the 2026-09 window + "DTEND:20250911T103000Z\r\n" + + "RRULE:FREQ=WEEKLY;UNTIL=20270826T080000Z;INTERVAL=3;BYDAY=TH;WKST=SU\r\n" + + "SUMMARY:Frontend Weekly\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +// TestQueryCalendarObjectsPropFilterTimeRangeRecurring is the regression +// test for the "CalDAV still misses Frontend Weekly / Abstimmung / +// Prd-Deployment" report. DAVx5 and Thunderbird issue calendar-query +// filters as +// *inside* a VEVENT comp-filter. go-webdav's matchPropTimeRange only +// compares the literal DTSTART (2025), so without the hoist the 2026-09 +// query returns an empty result for a weekly series whose base date is +// in 2025. The hoist lifts the time-range onto the comp-filter, where +// go-webdav's recurrence-aware matchCompTimeRange (RecurrenceSet) keeps +// the series. +func TestQueryCalendarObjectsPropFilterTimeRangeRecurring(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(rruleSample)) + })) + defer srv.Close() + + dir := t.TempDir() + st, err := store.NewStore(filepath.Join(dir, "data")) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + dbase, err := db.Open(filepath.Join(dir, "test.db")) + if err != nil { + t.Fatalf("Open db: %v", err) + } + defer dbase.Close() + if err := dbase.CreateUser("alice", "pw", "", ""); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := dbase.CreateICSSubscription("alice", "holidays", srv.URL, ""); err != nil { + t.Fatalf("CreateICSSubscription: %v", err) + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + b := NewBackend(&config.Config{}, st, dbase, logger, icssub.NewCache(time.Hour)) + ctx := auth.NewContext(context.Background(), &auth.Principal{Username: "alice"}) + + loc := time.UTC + gs := time.Date(2026, 9, 1, 0, 0, 0, 0, loc) + ge := time.Date(2026, 9, 30, 0, 0, 0, 0, loc) + + // The DAVx5/Thunderbird form: a prop-filter time-range on DTSTART + // nested inside a VEVENT comp-filter under VCALENDAR. + q := &caldav.CalendarQuery{ + CompFilter: caldav.CompFilter{ + Name: "VCALENDAR", + Comps: []caldav.CompFilter{{ + Name: "VEVENT", + Props: []caldav.PropFilter{{Name: "DTSTART", Start: gs, End: ge}}, + }}, + }, + } + objs, err := b.QueryCalendarObjects(ctx, "/cal/home/holidays/", q) + if err != nil { + t.Fatalf("QueryCalendarObjects: %v", err) + } + if len(objs) == 0 { + t.Fatal("QueryCalendarObjects with a DAVx5-shape prop-filter time-range " + + "returned 0 objects for a weekly series whose RRULE has 2026-09 " + + "occurrences, but whose base DTSTART is 2025 — this is the " + + "regression the hoist fix exists to prevent") + } + found := false + for _, co := range objs { + for _, ev := range co.Data.Events() { + if p := ev.Props.Get(ical.PropSummary); p != nil && p.Value == "Frontend Weekly" { + found = true + } + } + } + if !found { + t.Fatal("expected 'Frontend Weekly' in the query result") + } +} + +// TestReportPropFilterTimeRangeRecurringE2E drives the *full* HTTP stack a +// real CalDAV client uses: NewHandler → go-webdav caldav.Handler → XML decode +// of the // body → +// QueryCalendarObjects (hoist) → caldav.Filter. The in-process tests above +// hand-build the CompFilter struct and therefore skip the XML-decode step, +// so this is the only test that can catch a bug in how the client's actual +// request gets parsed. DAVx5/Thunderbird send exactly this shape. +func TestReportPropFilterTimeRangeRecurringE2E(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/calendar; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(rruleSample)) + })) + defer srv.Close() + + dir := t.TempDir() + st, err := store.NewStore(filepath.Join(dir, "data")) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + dbase, err := db.Open(filepath.Join(dir, "test.db")) + if err != nil { + t.Fatalf("db.Open: %v", err) + } + defer dbase.Close() + if err := dbase.CreateUser("alice", "pw", "", ""); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := dbase.CreateICSSubscription("alice", "holidays", srv.URL, ""); err != nil { + t.Fatalf("CreateICSSubscription: %v", err) + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + cfg := &config.Config{} + cfg.Auth.Realm = "test" + + handlers := auth.NewMiddleware(cfg, dbase, logger) + httpd := httptest.NewServer(handlers.Wrap(NewHandler(cfg, st, dbase, logger, icssub.NewCache(time.Hour)))) + defer httpd.Close() + + reportXML := ` + + + + + + + + + + + +` + + req, err := http.NewRequest("REPORT", httpd.URL+"/cal/home/holidays/", strings.NewReader(reportXML)) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + req.SetBasicAuth("alice", "pw") + req.Header.Set("Content-Type", "application/xml; charset=utf-8") + req.Header.Set("Depth", "1") + + resp, err := httpd.Client().Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + + if resp.StatusCode != http.StatusMultiStatus { + t.Fatalf("REPORT status = %d, want 207; body:\n%s", resp.StatusCode, body) + } + if !strings.Contains(string(body), "Frontend Weekly") { + t.Fatalf("REPORT response did not include the recurring 'Frontend Weekly' series. This is the exact DAVx5/Thunderbird request shape; body:\n%s", body) + } +} + +// TestCloseOpenEndedTimeRangesUnit exercises closeOpenEndedTimeRanges +// directly: a comp-filter with only Start set (no End) must get End +// filled in with farFutureSentinel, and a comp-filter with neither set +// must be left alone (it has no time-range at all, so there's nothing to +// "close"). +func TestCloseOpenEndedTimeRangesUnit(t *testing.T) { + gs := time.Date(2026, 6, 8, 0, 0, 0, 0, time.UTC) + cf := caldav.CompFilter{ + Name: "VCALENDAR", + Comps: []caldav.CompFilter{ + {Name: "VEVENT", Start: gs}, // open-ended: no End + {Name: "VTODO"}, // no time-range at all + }, + } + + closeOpenEndedTimeRanges(&cf) + + if cf.Comps[0].End != farFutureSentinel { + t.Fatalf("open-ended VEVENT comp-filter should get End=farFutureSentinel, got %v", cf.Comps[0].End) + } + if !cf.Comps[1].Start.IsZero() || !cf.Comps[1].End.IsZero() { + t.Fatalf("VTODO comp-filter without any time-range must be left untouched, got Start=%v End=%v", + cf.Comps[1].Start, cf.Comps[1].End) + } +} + +// TestQueryCalendarObjectsOpenEndedTimeRangeRecurring is the regression +// test for a real bug found while investigating the "missing recurring +// events" report: a client asking for "everything from date X onward" +// (RFC 4791 §9.9 explicitly permits a time-range with only a start +// attribute) — which is exactly what a client's default "sync events +// from N days in the past, no future limit" setting produces — decodes +// with End left as the zero time.Time. go-webdav's matchCompTimeRange +// then calls rrule.Set.Between(start, zero-time, true) for any recurring +// event, which always returns zero occurrences (the "end" bound is year +// 1, before "start"), so *every* recurring series silently vanishes from +// an open-ended query, even though the exact same series matches fine +// when the client happens to also send an explicit (bounded) end date. +func TestQueryCalendarObjectsOpenEndedTimeRangeRecurring(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(rruleSample)) + })) + defer srv.Close() + + dir := t.TempDir() + st, err := store.NewStore(filepath.Join(dir, "data")) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + dbase, err := db.Open(filepath.Join(dir, "test.db")) + if err != nil { + t.Fatalf("Open db: %v", err) + } + defer dbase.Close() + if err := dbase.CreateUser("alice", "pw", "", ""); err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := dbase.CreateICSSubscription("alice", "holidays", srv.URL, ""); err != nil { + t.Fatalf("CreateICSSubscription: %v", err) + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + b := NewBackend(&config.Config{}, st, dbase, logger, icssub.NewCache(time.Hour)) + ctx := auth.NewContext(context.Background(), &auth.Principal{Username: "alice"}) + + // "Sync from 90 days ago, no future limit": only Start is set. + gs := time.Date(2026, 6, 8, 0, 0, 0, 0, time.UTC) + q := &caldav.CalendarQuery{ + CompFilter: caldav.CompFilter{ + Name: "VCALENDAR", + Comps: []caldav.CompFilter{{ + Name: "VEVENT", + Start: gs, // no End: open-ended + }}, + }, + } + objs, err := b.QueryCalendarObjects(ctx, "/cal/home/holidays/", q) + if err != nil { + t.Fatalf("QueryCalendarObjects: %v", err) + } + found := false + for _, co := range objs { + for _, ev := range co.Data.Events() { + if p := ev.Props.Get(ical.PropSummary); p != nil && p.Value == "Frontend Weekly" { + found = true + } + } + } + if !found { + t.Fatal("expected 'Frontend Weekly' to match an open-ended (start-only) time-range query") + } +} diff --git a/internal/caldav/ics.go b/internal/caldav/ics.go index daa5293..d3bbd53 100644 --- a/internal/caldav/ics.go +++ b/internal/caldav/ics.go @@ -30,22 +30,14 @@ func (b *Backend) icsSubscriptionCalendarMeta(owner string, sub db.ICSSubscripti } } -// icsObjectUID returns the UID a fetched VEVENT should be addressed by: -// its own UID property if it has one, otherwise a placeholder derived from -// the object ID (so the event still has a *unique* UID in the returned -// VCALENDAR). -func icsObjectUID(ev ical.Event, fallback string) string { - if p := ev.Props.Get(ical.PropUID); p != nil && p.Value != "" { - return p.Value - } - return fallback -} - // icsSubscriptionCalendarObjects fetches sub's remote calendar (via -// b.icsCache) and returns one caldav.CalendarObject per VEVENT. The -// object path is derived from icssub.EventID(ev) so the same event keeps -// the same path across fetches, even if its position in the document -// changes. +// b.icsCache) and returns one caldav.CalendarObject per Series — the +// source feed's own group of VEVENTs sharing a UID (a recurring base plus +// its explicit per-occurrence instances). Exposing the whole group as a +// single object is the correct, non-lossy shape: a CalDAV client keeps the +// entire series (RRULE + overrides) instead of the base and its instances +// arriving as competing objects. The object path is derived from +// series.ID(), stable across fetches of the same feed. func (b *Backend) listICSSubscriptionCalendarObjects(localName string, sub db.ICSSubscription) ([]caldav.CalendarObject, error) { cal, err := b.icsCache.Get(sub.URL) if err != nil { @@ -53,8 +45,11 @@ func (b *Backend) listICSSubscriptionCalendarObjects(localName string, sub db.IC } var objs []caldav.CalendarObject - for _, ev := range cal.Events() { - obj, err := b.encodeICSObject(localName, ev) + for _, series := range icssub.GroupSeries(cal) { + if series.Base == nil && len(series.Instances) == 0 { + continue + } + obj, err := b.encodeICSSeries(localName, series) if err != nil { continue } @@ -64,51 +59,37 @@ func (b *Backend) listICSSubscriptionCalendarObjects(localName string, sub db.IC } // icsSubscriptionCalendarObject fetches sub's remote calendar and returns -// the single VEVENT whose derived object ID matches objID. +// the Series whose stable ID matches objID. func (b *Backend) icsSubscriptionCalendarObject(localName, objID string, sub db.ICSSubscription) (*caldav.CalendarObject, error) { cal, err := b.icsCache.Get(sub.URL) if err != nil { return nil, webdav.NewHTTPError(http.StatusNotFound, fmt.Errorf("fetching ics subscription: %w", err)) } - for _, ev := range cal.Events() { - if icssub.EventID(ev) != objID { + for _, series := range icssub.GroupSeries(cal) { + if series.Base == nil && len(series.Instances) == 0 { continue } - return b.encodeICSObject(localName, ev) + if series.ID() != objID { + continue + } + return b.encodeICSSeries(localName, series) } return nil, webdav.NewHTTPError(http.StatusNotFound, fmt.Errorf("ics subscription event not found")) } -// encodeICSObject wraps a single fetched VEVENT ev into its own -// caldav.CalendarObject, encoding it as a standalone one-event calendar -// the same way every other calendar object in this backend is represented. -func (b *Backend) encodeICSObject(localName string, ev ical.Event) (*caldav.CalendarObject, error) { - objID := icssub.EventID(ev) - if objID == "" { - // Not addressable (no DTSTART) — skip. - return nil, fmt.Errorf("event has no DTSTART; not addressable") - } - uid := icsObjectUID(ev, objID) - - event := ical.NewEvent() - event.Props = ev.Props - if event.Props.Get(ical.PropUID) == nil { - event.Props.SetText(ical.PropUID, uid) - } - if event.Props.Get(ical.PropDateTimeStamp) == nil { - event.Props.SetDateTime(ical.PropDateTimeStamp, time.Now().UTC()) - } - - out := ical.NewCalendar() - out.Props.SetText(ical.PropVersion, "2.0") - out.Props.SetText(ical.PropProductID, "-//nidus//ics-subscription//EN") - out.Children = append(out.Children, event.Component) +// encodeICSSeries wraps a Series (base VEVENT + explicit instances, all the +// feed's events that share a UID) into its own caldav.CalendarObject, +// encoded as a multi-VEVENT VCALENDAR the same way the source subscription +// is published. +func (b *Backend) encodeICSSeries(localName string, series *icssub.Series) (*caldav.CalendarObject, error) { + out := series.Calendar() var buf strings.Builder if err := ical.NewEncoder(&buf).Encode(out); err != nil { return nil, fmt.Errorf("encoding ics subscription event: %w", err) } data := []byte(buf.String()) + objID := series.ID() return &caldav.CalendarObject{ Path: calObjectPath(localName, objID), diff --git a/internal/caldav/ics_shared_test.go b/internal/caldav/ics_shared_test.go index 4f26aa6..42ec059 100644 --- a/internal/caldav/ics_shared_test.go +++ b/internal/caldav/ics_shared_test.go @@ -49,6 +49,23 @@ func mustCalEvent(t *testing.T, raw string) ical.Event { return evs[0] } +// icssubSeriesID returns the stable ID the CalDAV backend advertises for the +// (single) series in raw. All tests here use single-series feeds, so this is +// just GroupSeries(raw)[0].ID(). +func icssubSeriesID(t *testing.T, raw string) string { + t.Helper() + _ = t + cal, err := ical.NewDecoder(strings.NewReader(raw)).Decode() + if err != nil { + panic("icssubSeriesID: " + err.Error()) + } + series := icssub.GroupSeries(cal) + if len(series) == 0 { + panic("icssubSeriesID: no series") + } + return series[0].ID() +} + // TestICSSubscriptionListAndGetObject verifies the full CalDAV read path // for an ICS subscription: ListCalendarObjects returns one synthetic // stand-alone object whose Path is derived from icssub.EventID (not from @@ -92,16 +109,12 @@ func TestICSSubscriptionListAndGetObject(t *testing.T) { t.Fatalf("expected 1 object, got %d", len(objs)) } - // The Path must be derived from the event's DTSTART/DTEND/SUMMARY — - // i.e. icssub.EventID(ev), not from the event's position in the feed. - ev := mustCalEvent(t, icsSample) - wantID := icssub.EventID(ev) - if wantID == "" { - t.Fatal("EventID must be non-empty for a valid event") - } + // The Path must be derived from the event's stable identity (its UID) — + // i.e. the series ID — not from the event's position in the feed. + wantID := icssubSeriesID(t, icsSample) wantPath := calObjectPath("holidays", wantID) if objs[0].Path != wantPath { - t.Fatalf("Path = %q, want %q (icssub.EventID-based, not position-based)", objs[0].Path, wantPath) + t.Fatalf("Path = %q, want %q (series-ID-based, not position-based)", objs[0].Path, wantPath) } // GetCalendarObject by the same Path should return an object that has @@ -260,7 +273,7 @@ func TestICSSubscriptionEventIDMatchesEventInCalDav(t *testing.T) { t.Fatalf("list: objs=%d err=%v", len(objs), err) } gotPath := objs[0].Path - wantID := icssub.EventID(mustCalEvent(t, icsSample)) + wantID := icssubSeriesID(t, icsSample) wantPath := calObjectPath("holidays", wantID) if gotPath != wantPath { t.Fatalf("CalDAV object path %q does not match web-side EventID-derived %q", gotPath, wantPath) diff --git a/internal/icalfix/icalfix.go b/internal/icalfix/icalfix.go index 1920e2e..334ace0 100644 --- a/internal/icalfix/icalfix.go +++ b/internal/icalfix/icalfix.go @@ -27,8 +27,20 @@ import "regexp" var tzidParamRe = regexp.MustCompile(`TZID=("?)([^";:\r\n]+)("?)`) // tzidLineRe matches a standalone "TZID:" property line, as found -// inside a VTIMEZONE component. -var tzidLineRe = regexp.MustCompile(`(?m)^TZID:([^\r\n]+)$`) +// inside a VTIMEZONE component. The trailing "(\r?)" capture group is +// required, not cosmetic: Go's RE2 engine only matches "$" in multi-line +// mode immediately before a literal "\n", not before "\r\n" — so without +// consuming (and re-emitting) an optional trailing "\r" explicitly, this +// regex silently fails to match on any CRLF-terminated feed (which is +// every Outlook/Exchange-published ICS feed in practice), leaving the +// VTIMEZONE's own TZID line un-normalized even though the "TZID=..." +// parameter form (tzidParamRe, used on DTSTART/DTEND/etc.) still matches +// fine. The practical effect was a VTIMEZONE whose declared TZID +// ("W. Europe Standard Time") never matched the now-normalized +// "TZID=Europe/Berlin" parameters referencing it elsewhere in the same +// object, so any code trying to find "the VTIMEZONE for Europe/Berlin" +// (see internal/icssub.Series.referencedTimezones) would never find one. +var tzidLineRe = regexp.MustCompile(`(?m)^TZID:([^\r\n]+)(\r?)$`) // NormalizeTimeZones rewrites any recognized Windows timezone identifier // in data to its IANA equivalent, leaving everything else (including @@ -51,7 +63,12 @@ func NormalizeTimeZones(data []byte) []byte { if !ok { return m } - return []byte("TZID:" + iana) + // sub[2] is the optional trailing "\r" the regex consumed as part + // of the match (see the doc comment above) — it must be re-emitted + // here, or a CRLF-terminated line loses its "\r" and every + // subsequent line in the file ends up misaligned relative to the + // original byte offsets a caller might have recorded. + return []byte("TZID:" + iana + string(sub[2])) }) return data } diff --git a/internal/icalfix/icalfix_test.go b/internal/icalfix/icalfix_test.go index abe120a..f858a0e 100644 --- a/internal/icalfix/icalfix_test.go +++ b/internal/icalfix/icalfix_test.go @@ -69,3 +69,36 @@ func TestNormalizeTimeZonesLeavesUnknownAndIANAZonesAlone(t *testing.T) { t.Fatalf("expected data to be unchanged, got:\n%s", got) } } + +// TestNormalizeTimeZonesFixesVTIMEZONECRLFLine reproduces a real bug: the +// standalone "TZID:" property line inside a VTIMEZONE component +// (as opposed to a "TZID=" parameter on DTSTART/DTEND/etc.) was +// never being normalized on real-world feeds, because every +// Outlook/Exchange-published ICS uses CRLF line endings and Go's RE2 "$" +// anchor in multi-line mode only matches immediately before a literal +// "\n" — not before "\r\n". So on a line like +// "TZID:W. Europe Standard Time\r\n", the old +// `^TZID:([^\r\n]+)$` pattern never matched at all, leaving the +// VTIMEZONE's own declared TZID un-normalized while every "TZID=..." +// parameter elsewhere in the same object WAS normalized (that regex has +// no such issue) — so nothing referencing the VTIMEZONE by name could +// ever find it again. See internal/icssub.Series.referencedTimezones, +// which depends on exactly this match to embed the right VTIMEZONE in a +// synthetic per-series VCALENDAR. +func TestNormalizeTimeZonesFixesVTIMEZONECRLFLine(t *testing.T) { + const raw = "BEGIN:VCALENDAR\r\n" + + "BEGIN:VTIMEZONE\r\n" + + "TZID:W. Europe Standard Time\r\n" + + "END:VTIMEZONE\r\n" + + "END:VCALENDAR\r\n" + const want = "BEGIN:VCALENDAR\r\n" + + "BEGIN:VTIMEZONE\r\n" + + "TZID:Europe/Berlin\r\n" + + "END:VTIMEZONE\r\n" + + "END:VCALENDAR\r\n" + + got := string(NormalizeTimeZones([]byte(raw))) + if got != want { + t.Fatalf("CRLF-terminated VTIMEZONE TZID line was not normalized:\ngot: %q\nwant: %q", got, want) + } +} diff --git a/internal/icssub/icssub.go b/internal/icssub/icssub.go index 9039cae..7a0adf0 100644 --- a/internal/icssub/icssub.go +++ b/internal/icssub/icssub.go @@ -220,6 +220,243 @@ func normalizeURL(u string) string { return u } +// Series is one addressable calendar entity in a fetched ICS/calendar +// subscription feed: all VEVENTs that share the same UID, or — for feeds +// that omit UIDs — that hash to the same EventID, grouped together. +// +// This matches how Outlook / Exchange publish recurring events ("fully +// expanded" ICS): one base VEVENT carrying the RRULE, plus one bare +// instance VEVENT per explicit occurrence (per-instance edits, +// one-off additions, "Canceled:" overrides) — all under the SAME UID. +// Treating the base and each instance as separate events is what produces +// the two visible bugs, so everything on the subscription path +// (web grid, CalDAV objects, detail page) is keyed off the Series, not off +// individual VEVENTs: +// +// - the web month/week grid paints ONE entry per (series, day), so a day +// covered by both the RRULE base and an explicit instance is not +// rendered twice; +// - the CalDAV backend advertises ONE calendar object per series, whose +// VCALENDAR holds the base + all instances — the shape the source feed +// uses — so clients keep the whole series instead of dropping it. +// +// A series has at least one of Base or a non-empty Instances; Key is +// stable across fetches and is the only thing callers address it by. +type Series struct { + Key string + Base *ical.Event // the VEVENT carrying the RRULE, if any + Instances []ical.Event // the explicit per-occurrence VEVENTs + + // tzs holds every VTIMEZONE component from the source feed (shared + // across all series parsed from the same feed). Calendar() embeds + // whichever of these are actually referenced by this series' VEVENTs, + // so the synthetic per-series VCALENDAR stays RFC 5545-compliant. See + // Calendar's doc comment for why this matters. + tzs []*ical.Component +} + +// ID returns a stable filesystem-name/URL-path identifier for the series +// (32 hex chars + ".ics"), derived from the series Key. Two different +// series never collide; the same series keeps the same ID across +// refetches of the same feed. +func (s *Series) ID() string { + sum := sha256.Sum256([]byte("series:" + s.Key)) + return hex.EncodeToString(sum[:16]) + ".ics" +} + +// GroupSeries folds all VEVENTs of cal into Series, one per UID (falling +// back to EventID for feeds without UIDs). Within a series the VEVENT that +// carries an RRULE becomes Base; every other VEVENT is an Instance. The +// returned slice preserves the feed's first-seen order of each series. +func GroupSeries(cal *ical.Calendar) []*Series { + if cal == nil { + return nil + } + + // Collect every VTIMEZONE the feed defines, so each series can embed + // whichever ones its own VEVENTs actually reference (see Calendar). + var tzs []*ical.Component + for _, child := range cal.Children { + if child.Name == ical.CompTimezone { + tzs = append(tzs, child) + } + } + + byKey := make(map[string]*Series) + var order []string + for _, ev := range cal.Events() { + var key string + if uid := ev.Props.Get(ical.PropUID); uid != nil && uid.Value != "" { + key = "uid=" + uid.Value + } else if id := EventID(ev); id != "" { + key = "eid=" + id + } else { + continue // not addressable + } + s := byKey[key] + if s == nil { + s = &Series{Key: key, tzs: tzs} + byKey[key] = s + order = append(order, key) + } + if ev.Props.Get(ical.PropRecurrenceRule) != nil { + if s.Base == nil { + s.Base = &ev + } + } else { + s.Instances = append(s.Instances, ev) + } + } + out := make([]*Series, 0, len(order)) + for _, key := range order { + out = append(out, byKey[key]) + } + return out +} + +// Anchor returns the VEVENT that best represents the series for display of +// a single occurrence: the recurring Base when present, otherwise the +// earliest Instance. It is nil for an empty series. +func (s *Series) Anchor() *ical.Event { + if s.Base != nil { + return s.Base + } + best := -1 + for i := range s.Instances { + if best == -1 || dtStartEarlier(s.Instances[i], s.Instances[best]) { + best = i + } + } + if best == -1 { + return nil + } + return &s.Instances[best] +} + +// Calendar returns a fresh, self-contained ical.Calendar holding every +// VEVENT of the series (base first, then instances in feed order), i.e. the +// source feed's own group of events — suitable to encode as one CalDAV +// calendar object and for the subscription detail page. +// +// It also embeds every VTIMEZONE component (copied from the source feed) +// that's actually referenced by one of the series' own VEVENTs (via a +// TZID parameter on DTSTART/DTEND/RECURRENCE-ID/EXDATE/RDATE). Per RFC +// 5545 §3.6.5, a TZID that isn't "UTC" or a bare offset MUST have a +// matching VTIMEZONE definition in the same iCalendar object. Without it, +// strict parsers (notably ical4j, which DAVx5 is built on) can fail to +// resolve the timezone for recurrence-rule expansion and silently drop +// the whole VEVENT — which is exactly what made recurring events +// (e.g. a weekly meeting) vanish from CalDAV clients even though the +// event listing/query logic itself was correct: each series used to be +// encoded as a bare VEVENT-only VCALENDAR with no VTIMEZONE at all. +func (s *Series) Calendar() *ical.Calendar { + out := ical.NewCalendar() + out.Props.SetText(ical.PropVersion, "2.0") + out.Props.SetText(ical.PropProductID, "-//nidus//ics-subscription//EN") + + for _, tz := range s.referencedTimezones() { + out.Children = append(out.Children, tz) + } + if s.Base != nil { + out.Children = append(out.Children, s.Base.Component) + } + for i := range s.Instances { + out.Children = append(out.Children, s.Instances[i].Component) + } + return out +} + +// referencedTimezones returns the subset of s.tzs whose TZID is +// referenced by any property of s.Base or s.Instances, preserving s.tzs' +// original order and including each matched VTIMEZONE at most once. +func (s *Series) referencedTimezones() []*ical.Component { + if len(s.tzs) == 0 { + return nil + } + + needed := make(map[string]bool) + note := func(ev *ical.Event) { + if ev == nil { + return + } + for _, props := range ev.Props { + for _, p := range props { + if tzid := p.Params.Get(ical.PropTimezoneID); tzid != "" { + needed[tzid] = true + } + } + } + } + note(s.Base) + for i := range s.Instances { + note(&s.Instances[i]) + } + if len(needed) == 0 { + return nil + } + + var out []*ical.Component + for _, tz := range s.tzs { + tzidProp := tz.Props.Get(ical.PropTimezoneID) + if tzidProp == nil || !needed[tzidProp.Value] { + continue + } + out = append(out, tz) + } + return out +} + +// OccurrencesIn returns, for every calendar day (formatted with layout +// "2006-01-02", in loc) in [gridStart, gridEnd] that the series occupies, +// the single VEVENT representing the series on that day. The recurring +// base (RRULE expansion) supplies the day's event, but an explicit +// instance on the same day takes precedence (Exchange's "override" model: +// this is how per-instance edits and "Canceled:" entries replace the +// series occurrence for that day). At most one event per day is ever +// returned, so callers can paint exactly one grid cell per day. +func (s *Series) OccurrencesIn(gridStart, gridEnd time.Time, loc *time.Location) map[string]ical.Event { + const layout = "2006-01-02" + out := make(map[string]ical.Event) + note := func(t0 time.Time, ev ical.Event) { + out[t0.In(loc).Format(layout)] = ev + } + + if s.Base != nil { + if rset, err := s.Base.RecurrenceSet(loc); err == nil && rset != nil { + for _, occ := range rset.Between(gridStart, gridEnd, true) { + note(occ, *s.Base) + } + } + } + for i := range s.Instances { + dtp := s.Instances[i].Props.Get(ical.PropDateTimeStart) + if dtp == nil { + continue + } + t0, err := dtp.DateTime(loc) + if err != nil || t0.Before(gridStart) || t0.After(gridEnd) { + continue + } + note(t0, s.Instances[i]) + } + return out +} + +// dtStartEarlier reports whether a's DTSTART value sorts before b's +// (string compare of the raw property value is sufficient for a +// deterministic tie-break used by Anchor). +func dtStartEarlier(a, b ical.Event) bool { + av := a.Props.Get(ical.PropDateTimeStart) + bv := b.Props.Get(ical.PropDateTimeStart) + switch { + case av == nil: + return false + case bv == nil: + return true + } + return av.Value < bv.Value +} + // EventID returns a stable, short identifier for a single ical.Event, // suitable for use as a filesystem object name or URL path segment. It is // the first 32 hex chars (128 bits) of diff --git a/internal/icssub/icssub_test.go b/internal/icssub/icssub_test.go index 995569b..d521d03 100644 --- a/internal/icssub/icssub_test.go +++ b/internal/icssub/icssub_test.go @@ -197,3 +197,273 @@ func TestEventIDWorksWithoutUID(t *testing.T) { t.Fatal("EventID should be non-empty even without UID") } } + +// outlookStyleSeries mimics the canonical Outlook/Exchange shape: one UID +// carrying a base VEVENT (with RRULE) plus bare per-occurrence instance +// VEVENTs (the "fully expanded" ICS model). GroupSeries must fold all of +// them into ONE Series (base + 2 instances), not two separate entities. +const outlookStyleSeries = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:series@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260306T103000Z\r\n" + + "DTEND:20260306T113000Z\r\n" + + "RRULE:FREQ=WEEKLY;UNTIL=20260903T083000Z;INTERVAL=3;BYDAY=FR;WKST=SU\r\n" + + "SUMMARY:#smarttouch Planning\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:series@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260320T103000Z\r\n" + + "DTEND:20260320T113000Z\r\n" + + "SUMMARY:#smarttouch Planning\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:series@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260403T103000Z\r\n" + + "DTEND:20260403T123000Z\r\n" + + "SUMMARY:#smarttouch Planning (extended)\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +func TestGroupSeriesFoldsOutlookSeriesShape(t *testing.T) { + cal, err := ical.NewDecoder(strings.NewReader(outlookStyleSeries)).Decode() + if err != nil { + t.Fatal(err) + } + if got := len(cal.Events()); got != 3 { + t.Fatalf("fixture must have 3 VEVENTs, got %d", got) + } + series := GroupSeries(cal) + if len(series) != 1 { + t.Fatalf("GroupSeries: expected 1 series, got %d", len(series)) + } + s := series[0] + if s.Base == nil { + t.Fatal("the base (RRULE) VEVENT must be the series Base") + } + if s.Base.Props.Get(ical.PropUID).Value != "series@nidus.test" { + t.Fatalf("wrong base UID: %q", s.Base.Props.Get(ical.PropUID).Value) + } + if len(s.Instances) != 2 { + t.Fatalf("expected 2 explicit instances, got %d", len(s.Instances)) + } + if s.ID() == "" { + t.Fatal("series ID must be non-empty") + } + // Anchor must be the base. + if got := s.Anchor().Props.Get(ical.PropUID).Value; got != "series@nidus.test" { + t.Fatalf("Anchor must be the base, got UID %q", got) + } + // Calendar() must contain all three VEVENTs. + if got := s.Calendar().Events(); len(got) != 3 { + t.Fatalf("Calendar() must contain 3 events, got %d", len(got)) + } +} + +// tzAwareSeries mimics a real Outlook/Exchange feed (already run through +// icalfix.NormalizeTimeZones, hence "Europe/Berlin" rather than the raw +// Windows zone name): a VCALENDAR with one VTIMEZONE the events actually +// reference, plus a second, unrelated VTIMEZONE no event references. +const tzAwareSeries = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VTIMEZONE\r\n" + + "TZID:Europe/Berlin\r\n" + + "END:VTIMEZONE\r\n" + + "BEGIN:VTIMEZONE\r\n" + + "TZID:America/New_York\r\n" + + "END:VTIMEZONE\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:tz-series@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART;TZID=Europe/Berlin:20260910T100000\r\n" + + "DTEND;TZID=Europe/Berlin:20260910T110000\r\n" + + "RRULE:FREQ=WEEKLY;INTERVAL=1;BYDAY=WE\r\n" + + "SUMMARY:Frontend Weekly\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +// TestSeriesCalendarEmbedsReferencedTimezone is the regression test for +// the bug report "Frontend Weekly / Abstimmung Next-Release Package / +// Prd-Deployment don't show up in the CalDAV client": Series.Calendar() +// used to build a VTIMEZONE-less VCALENDAR for every ICS-subscription +// series, even though its VEVENTs' DTSTART/DTEND still carried a +// TZID=Europe/Berlin parameter — an RFC 5545 §3.6.5 violation that made +// strict clients (notably ical4j, which DAVx5 is built on) fail to +// resolve the timezone for recurrence-rule expansion, silently dropping +// the whole (recurring) VEVENT from sync. The fix must embed exactly the +// VTIMEZONE(s) the series' own VEVENTs reference — no more, no less. +func TestSeriesCalendarEmbedsReferencedTimezone(t *testing.T) { + cal, err := ical.NewDecoder(strings.NewReader(tzAwareSeries)).Decode() + if err != nil { + t.Fatal(err) + } + series := GroupSeries(cal) + if len(series) != 1 { + t.Fatalf("expected 1 series, got %d", len(series)) + } + out := series[0].Calendar() + + var tzids []string + for _, child := range out.Children { + if child.Name == ical.CompTimezone { + tzids = append(tzids, child.Props.Get(ical.PropTimezoneID).Value) + } + } + if len(tzids) != 1 || tzids[0] != "Europe/Berlin" { + t.Fatalf("expected exactly the referenced VTIMEZONE (Europe/Berlin) to be embedded, got %v", tzids) + } + + // Re-encoding and re-decoding must round-trip: the VEVENT's DTSTART + // TZID must resolve to a VTIMEZONE actually present in the same + // object (this is what a strict client like ical4j checks). + if got := len(out.Events()); got != 1 { + t.Fatalf("expected 1 VEVENT in the series calendar, got %d", got) + } +} + +// twoDistinctSeries are two completely unrelated events (different UIDs). +// GroupSeries must NOT fold them together. +const twoDistinctSeries = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:a@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260809T090000Z\r\n" + + "DTEND:20260809T100000Z\r\n" + + "SUMMARY:meeting A\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:b@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260809T110000Z\r\n" + + "DTEND:20260809T120000Z\r\n" + + "SUMMARY:meeting B\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +func TestGroupSeriesKeepsDistinctSeries(t *testing.T) { + cal, err := ical.NewDecoder(strings.NewReader(twoDistinctSeries)).Decode() + if err != nil { + t.Fatal(err) + } + if got := len(GroupSeries(cal)); got != 2 { + t.Fatalf("two distinct UIDs must produce two series, got %d", got) + } +} + +// seriesOnlyInstances is the shape of a set of one-off events on one UID +// with no RRULE. The earliest instance must anchor the series. +const seriesOnlyInstances = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:g@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260810T090000Z\r\n" + + "DTEND:20260810T100000Z\r\n" + + "SUMMARY:later\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:g@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260809T090000Z\r\n" + + "DTEND:20260809T100000Z\r\n" + + "SUMMARY:earlier\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +func TestGroupSeriesAnchorWithoutBase(t *testing.T) { + cal, err := ical.NewDecoder(strings.NewReader(seriesOnlyInstances)).Decode() + if err != nil { + t.Fatal(err) + } + series := GroupSeries(cal) + if len(series) != 1 { + t.Fatalf("expected 1 series, got %d", len(series)) + } + s := series[0] + if s.Base != nil { + t.Fatalf("no event carries RRULE, so Base must be nil, got %v", s.Base) + } + if len(s.Instances) != 2 { + t.Fatalf("expected 2 instances, got %d", len(s.Instances)) + } + anchor := s.Anchor() + if anchor == nil { + t.Fatal("Anchor must be non-nil for a non-empty series") + } + sp := anchor.Props.Get(ical.PropSummary) + if sp == nil || sp.Value != "earlier" { + t.Fatalf("Anchor should be the earliest instance, got %v", sp) + } +} + +// TestSeriesOccurrencesInReturnsOneDayPerDay verifies the crux of the +// web-grid fix: on a day the RRULE base covers AND an explicit instance +// covers, OccurrencesIn returns exactly one event for that day (the +// instance, which "wins" per the Exchange override model), and does not +// double-paint. +func TestSeriesOccurrencesInReturnsOneDayPerDay(t *testing.T) { + cal, err := ical.NewDecoder(strings.NewReader(outlookStyleSeries)).Decode() + if err != nil { + t.Fatal(err) + } + series := GroupSeries(cal) + if len(series) != 1 { + t.Fatalf("expected 1 series, got %d", len(series)) + } + s := series[0] + + loc := time.UTC + // Window that contains 2026-03-20 (which is both an RRULE date and a + // separate explicit instance in the fixture). + gs := time.Date(2026, 3, 1, 0, 0, 0, 0, loc) + ge := time.Date(2026, 4, 10, 0, 0, 0, 0, loc) + + occ := s.OccurrencesIn(gs, ge, loc) + if len(occ) < 3 { + t.Fatalf("expected at least 3 days of occurrences, got %d: %v", len(occ), keysOf(occ)) + } + // The explicit instance on 2026-03-20 (title "#smarttouch Planning") + // must be the representative event for that day (the base also covers + // that day via RRULE — but the instance wins). + evOnMar20, ok := occ["2026-03-20"] + if !ok { + t.Fatalf("2026-03-20 is in the window AND is an explicit instance — must be present, got %v", keysOf(occ)) + } + if p := evOnMar20.Props.Get(ical.PropSummary); p == nil || p.Value != "#smarttouch Planning" { + t.Fatalf("instance on 2026-03-20 should win over the base; got %v", p) + } + // 2026-04-03 has an explicit instance with a different title — should + // be that title (not the base title) for that day. + evOnApr3, ok := occ["2026-04-03"] + if !ok { + t.Fatalf("2026-04-03 must be present, got %v", keysOf(occ)) + } + if p := evOnApr3.Props.Get(ical.PropSummary); p == nil || p.Value != "#smarttouch Planning (extended)" { + t.Fatalf("instance on 2026-04-03 should have the overridden title; got %v", p) + } + // 2026-03-06 is the RRULE base's DTSTART and NOT explicitly overridden — + // the base should be the representative for that day. + evOnBase, ok := occ["2026-03-06"] + if !ok { + t.Fatalf("2026-03-06 (base DTSTART) must be present, got %v", keysOf(occ)) + } + if p := evOnBase.Props.Get(ical.PropSummary); p == nil || p.Value != "#smarttouch Planning" { + t.Fatalf("base should represent 2026-03-06; got %v", p) + } +} + +func keysOf(m map[string]ical.Event) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} diff --git a/internal/web/calendar.go b/internal/web/calendar.go index b5898fc..ff4ae9b 100644 --- a/internal/web/calendar.go +++ b/internal/web/calendar.go @@ -296,9 +296,9 @@ func parseWeekStart(r *http.Request) time.Time { // buildMonthView loads every event from every calendar visible to // username (own + shared), then places each occurrence's days onto a // 6-week grid covering the requested month (plus enough leading/trailing -// days of neighboring months to fill full weeks). Recurring events -// (RRULE) are not expanded — only an event's own DTSTART/DTEND span is -// considered. +// days of neighboring months to fill full weeks). Recurring events (RRULE) +// are expanded: every occurrence falling in the grid's window is placed +// (see eventOccurrenceDays), not just the event's base DTSTART/DTEND. func (s *Server) buildMonthView(username string, year int, month time.Month) (templates.MonthViewData, error) { loc := time.Local first := time.Date(year, month, 1, 0, 0, 0, 0, loc) @@ -473,33 +473,24 @@ func (s *Server) collectCalendarEvents(username string, gridStart, gridEnd time. if err != nil { continue } - form, err := eventFormFromICS(id, data) + ev, err := firstEventFromICS(data) if err != nil { s.logger.Warn("decoding calendar object", "calendar", entry.Name, "id", id, "error", err) continue } - startDay, endDay, err := eventDayRange(form, loc) + daysSet := eventOccurrenceDays(ev, gridStart, gridEnd, loc, dayIndex) + if len(daysSet) == 0 { + continue + } + form, err := eventFormFromComponent(id, ev) if err != nil { continue } - if endDay.Before(gridStart) || startDay.After(gridEnd) { - continue + timeText := "" + if !form.AllDay { + timeText = form.StartTime } - if startDay.Before(gridStart) { - startDay = gridStart - } - if endDay.After(gridEnd) { - endDay = gridEnd - } - for d, n := startDay, 0; !d.After(endDay) && n < totalDays; d, n = d.AddDate(0, 0, 1), n+1 { - idx, ok := dayIndex[d.Format(dateLayout)] - if !ok { - continue - } - timeText := "" - if !form.AllDay { - timeText = form.StartTime - } + for idx := range daysSet { days[idx].Events = append(days[idx].Events, templates.EventSummary{ ID: id, CalRef: entry.Ref, @@ -529,25 +520,76 @@ func mondayOf(t time.Time) time.Time { return time.Date(t.Year(), t.Month(), t.Day(), 0, 0, 0, 0, t.Location()).AddDate(0, 0, -offset) } -// eventDayRange returns the inclusive [start, end] calendar-day span an -// event occupies, in loc, for placing it on the month grid. -func eventDayRange(form templates.EventFormData, loc *time.Location) (start, end time.Time, err error) { - start, err = time.ParseInLocation(dateLayout, form.StartDate, loc) +// eventOccurrenceDays returns the set of grid day indices (into +// dayIndex) that event ev occupies within [gridStart, gridEnd]. A plain +// event occupies its DTSTART..DTEND day span. A recurring event (RRULE) +// occupies the day span of each occurrence whose expansion falls in the +// window, so e.g. a weekly meeting is painted on every in-grid weekly date +// rather than only its original date. EXDATE/RDATE are honored via +// go-ical's RecurrenceSet. Only indices for days actually in the grid are +// returned; occurrences entirely outside the window paint nothing. +func eventOccurrenceDays(ev ical.Event, gridStart, gridEnd time.Time, loc *time.Location, dayIndex map[string]int) map[int]bool { + out := make(map[int]bool) + addDay := func(day time.Time) { + day = day.In(loc) + if idx, ok := dayIndex[day.Format(dateLayout)]; ok { + out[idx] = true + } + } + + startProp := ev.Props.Get(ical.PropDateTimeStart) + if startProp == nil { + return nil + } + baseStart, err := startProp.DateTime(loc) if err != nil { - return time.Time{}, time.Time{}, err + return nil } - endDate := form.EndDate - if endDate == "" { - endDate = form.StartDate + + // Per-occurrence duration: the DTSTART..DTEND span of the base event + // (all-day events with only DTEND=DTSTART+1 give a 1-day span; timed + // events give their hour span). Recurrence preserves the duration. + dur := time.Duration(0) + if endProp := ev.Props.Get(ical.PropDateTimeEnd); endProp != nil { + if baseEnd, err := endProp.DateTime(loc); err == nil { + dur = baseEnd.Sub(baseStart) + } } - end, err = time.ParseInLocation(dateLayout, endDate, loc) + + spread := func(start time.Time) { + end := start.Add(dur) + for d := start; !d.After(end); d = d.AddDate(0, 0, 1) { + addDay(d) + if len(out) == len(dayIndex) { + return + } + } + } + + if rset, rerr := ev.RecurrenceSet(loc); rerr == nil && rset != nil { + for _, occ := range rset.Between(gridStart, gridEnd, true) { + spread(occ) + } + } else { + spread(baseStart) + } + return out +} + +// firstEventFromICS decodes data (a single-VEVENT calendar object) and +// returns its first VEVENT, for grid placement (which needs the raw +// ical.Event so recurring events can be expanded, see +// eventOccurrenceDays). +func firstEventFromICS(data []byte) (ical.Event, error) { + calendar, err := ical.NewDecoder(bytes.NewReader(icalfix.NormalizeTimeZones(data))).Decode() if err != nil { - return time.Time{}, time.Time{}, err + return ical.Event{}, err } - if end.Before(start) { - end = start + events := calendar.Events() + if len(events) == 0 { + return ical.Event{}, fmt.Errorf("no VEVENT in calendar object") } - return start, end, nil + return events[0], nil } func (s *Server) handleEventNew(w http.ResponseWriter, r *http.Request) { @@ -712,8 +754,9 @@ func (s *Server) eventForDisplay(username, ref, id string) (color string, form t } // icsEventForDisplay resolves one of username's ICS subscriptions named -// ref[len(!):] and looks up the event whose icssub.EventID hashes to id. -// Returns the subscription's display color and a form with Writable=false. +// ref[len(!):] and looks up the Series whose icssub ID matches id, then +// returns the subscription's display color and a form (Writable=false) +// summarizing the series' anchor event (base, or earliest instance). func (s *Server) icsEventForDisplay(username, ref, id string) (color string, form templates.EventFormData, err error) { name := strings.TrimPrefix(ref, icsRefPrefix) sub, err := s.dbase.GetICSSubscription(username, name) @@ -724,11 +767,15 @@ func (s *Server) icsEventForDisplay(username, ref, id string) (color string, for if err != nil { return "", form, err } - for _, ev := range cal.Events() { - if icssub.EventID(ev) != id { + for _, series := range icssub.GroupSeries(cal) { + if series.Base == nil && len(series.Instances) == 0 { continue } - form, err = eventFormFromComponent(id, ev) + if series.ID() != id { + continue + } + anchor := series.Anchor() + form, err = eventFormFromComponent(id, *anchor) if err != nil { return "", form, err } @@ -1304,44 +1351,46 @@ func (s *Server) addBirthdayEvents(username string, entry calendarEntry, gridSta // addICSEvents fetches entry's remote ICS/webcal calendar (via s.icsCache, // which uses stale-while-revalidate so a month-view render never blocks on -// network I/O) and places each VEVENT's occurrence onto the month grid, -// the same way a stored calendar object would be. Each event's ID is the -// stable icssub.EventID hash, which routes through the read-only detail -// page (eventForDisplay → icsEventForDisplay). +// network I/O) and places each Series' day-occurrences onto the grid. +// +// A "Series" is one UID group in the feed: the base (RRULE-carrying) +// VEVENT plus any explicit per-occurrence instances. OccurrencesIn +// collapses them to one event per (series, day), so a day the base RRULE +// covers AND an explicit instance covers is painted once, with the +// instance winning (Exchange's "override" model — this is how "Canceled:" +// entries replace the base occurrence for that day). func (s *Server) addICSEvents(entry calendarEntry, gridStart, gridEnd time.Time, loc *time.Location, dayIndex map[string]int, days []templates.MonthDay) error { cal, err := s.icsCache.Get(entry.ICSURL) if err != nil { return err } - const totalDays = 42 - for _, ev := range cal.Events() { - id := icssub.EventID(ev) - if id == "" { + for _, series := range icssub.GroupSeries(cal) { + if series.Base == nil && len(series.Instances) == 0 { continue } - form, err := eventFormFromComponent(id, ev) + id := series.ID() + occByDay := series.OccurrencesIn(gridStart, gridEnd, loc) + if len(occByDay) == 0 { + continue + } + anchor := series.Anchor() + baseForm, err := eventFormFromComponent(id, *anchor) if err != nil { continue } - startDay, endDay, err := eventDayRange(form, loc) - if err != nil { - continue - } - if endDay.Before(gridStart) || startDay.After(gridEnd) { - continue - } - if startDay.Before(gridStart) { - startDay = gridStart - } - if endDay.After(gridEnd) { - endDay = gridEnd - } - for d, n := startDay, 0; !d.After(endDay) && n < totalDays; d, n = d.AddDate(0, 0, 1), n+1 { - idx, ok := dayIndex[d.Format(dateLayout)] + for dayStr, ev := range occByDay { + idx, ok := dayIndex[dayStr] if !ok { continue } + // Per-day data: the day's own summary (an explicit instance + // may override the base's — "Canceled: X" or a per-instance + // title change). Fall back to the base summary otherwise. + form := baseForm + if ps := ev.Props.Get(ical.PropSummary); ps != nil && ps.Value != "" { + form.Summary = ps.Value + } timeText := "" if !form.AllDay { timeText = form.StartTime diff --git a/internal/web/ics_detail_test.go b/internal/web/ics_detail_test.go index d575e51..61c7b96 100644 --- a/internal/web/ics_detail_test.go +++ b/internal/web/ics_detail_test.go @@ -74,9 +74,9 @@ func TestICSDetailRouteRendersReadonlyEvent(t *testing.T) { handler := s.Handler(emptyStaticFS{}) cookie := loginAs(t, handler, "alice", "password") - id := icssub.EventID(mustParseICS(t, icsDetailSample)) + id := seriesID(t, icsDetailSample) if id == "" { - t.Fatal("EventID must be non-empty") + t.Fatal("series ID must be non-empty") } req := httptest.NewRequest(http.MethodGet, "/calendar/"+url.PathEscape("!holidays")+"/"+id, nil) @@ -147,7 +147,7 @@ func TestMonthGridLinksICSEventToDetailPage(t *testing.T) { if !strings.Contains(body, "ICS detail event") { t.Fatalf("month grid should list ICS events, got:\n%s", body) } - id := icssub.EventID(mustParseICS(t, icsDetailSample)) + id := seriesID(t, icsDetailSample) // The grid renders the detail link with a "/web/calendar/!/" // shape (see eventLinkURL in calendar.templ). The "!" in the ref is // not %-escaped by templ.URL — we observed un-escaped output in the @@ -161,6 +161,102 @@ func TestMonthGridLinksICSEventToDetailPage(t *testing.T) { } } +// TestEventOccurrenceDaysExpandsRRule verifies that a recurring event +// (RRULE) is placed on every in-window occurrence day, not just its base +// date — the core of the "recurring series events missing" fix. +func TestEventOccurrenceDaysExpandsRRule(t *testing.T) { + const raw = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:rec1@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260803T090000Z\r\n" + // a Monday + "DTEND:20260803T100000Z\r\n" + + "RRULE:FREQ=WEEKLY;COUNT=4\r\n" + + "SUMMARY:weekly meeting\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + + ev := mustParseICS(t, raw) + loc := time.UTC + + // Window covering the whole of August 2026 (the 4 weekly occurrences: + // Aug 3, 10, 17, 24 are all within range). + gridStart := time.Date(2026, 8, 1, 0, 0, 0, 0, loc) + gridEnd := time.Date(2026, 8, 31, 0, 0, 0, 0, loc) + + dayIndex := make(map[string]int) + for d, i := gridStart, 0; !d.After(gridEnd); d, i = d.AddDate(0, 0, 1), i+1 { + dayIndex[d.Format(dateLayout)] = i + } + + got := eventOccurrenceDays(ev, gridStart, gridEnd, loc, dayIndex) + + wantDates := []string{"2026-08-03", "2026-08-10", "2026-08-17", "2026-08-24"} + for _, ws := range wantDates { + if _, ok := got[dayIndex[ws]]; !ok { + t.Errorf("expected recurring occurrence on %s, got days %v", ws, got) + } + } + if len(got) != 4 { + t.Errorf("expected exactly 4 in-window occurrence days, got %d (%v)", len(got), got) + } +} + +// TestEventOccurrenceDaysSingleEvent verifies a non-recurring event occupies +// only its own day(s) (no spurious expansion). +func TestEventOccurrenceDaysSingleEvent(t *testing.T) { + const raw = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//nidus//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:single1@nidus.test\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260805T090000Z\r\n" + + "DTEND:20260805T100000Z\r\n" + + "SUMMARY:one off\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + + ev := mustParseICS(t, raw) + loc := time.UTC + gridStart := time.Date(2026, 8, 1, 0, 0, 0, 0, loc) + gridEnd := time.Date(2026, 8, 31, 0, 0, 0, 0, loc) + + dayIndex := make(map[string]int) + for d, i := gridStart, 0; !d.After(gridEnd); d, i = d.AddDate(0, 0, 1), i+1 { + dayIndex[d.Format(dateLayout)] = i + } + + got := eventOccurrenceDays(ev, gridStart, gridEnd, loc, dayIndex) + if len(got) != 1 { + t.Fatalf("expected exactly 1 occurrence day, got %d (%v)", len(got), got) + } + if _, ok := got[dayIndex["2026-08-05"]]; !ok { + t.Errorf("expected event on 2026-08-05, got days %v", got) + } +} + +// seriesID returns the stable ID that addICSEvents/icsEventForDisplay +// advertise for the (single) series in raw — GroupSeries(raw)[0].ID(). +// ICS-subscription grid cells and detail links are keyed by the series, +// not by individual VEVENTs, so this is what the href/ID in assertions +// must match. +func seriesID(t *testing.T, raw string) string { + t.Helper() + _ = t + cal, err := ical.NewDecoder(strings.NewReader(raw)).Decode() + if err != nil { + t.Fatalf("seriesID: ical decode: %v", err) + } + series := icssub.GroupSeries(cal) + if len(series) == 0 { + t.Fatal("seriesID: no series") + } + return series[0].ID() +} + // mustParseICS parses raw and returns its first VEVENT (panic on error). func mustParseICS(t *testing.T, raw string) ical.Event { t.Helper()