diff --git a/README.md b/README.md index 1514b61..be43d20 100644 --- a/README.md +++ b/README.md @@ -307,13 +307,20 @@ event routing. | `user_id` | UUID | Foreign key → User | | `name` | string | Human-readable name | | `description` | string | Optional description | -| `retention_days` | integer | Days to retain events (default: 30) | +| `retention_days` | integer | Days to retain events (default: 30; 0 means retain forever) | **Relations:** Belongs to User. Has many Entrypoints. Has many Targets. The `retention_days` field controls how long event data is kept in the webhook's dedicated database before automatic cleanup. +Setting `retention_days` to `0` means "retain events forever". Because +the column carries a default of 30, a literal zero cannot survive an +insert, so a zero is rewritten on save to a sentinel of `365 * 1000` +days (`database.RetentionForeverDays`). The retention reaper recognises +that sentinel and skips the webhook entirely, and the web UI displays +such a webhook's retention as "forever" rather than as a day count. + #### Entrypoint A receiver URL where external services POST webhook events. Each @@ -509,7 +516,7 @@ This separation provides: DB; the event database file is hard-deleted (permanently removed). - **Per-webhook retention** — the `retention_days` field on each webhook controls automatic cleanup of old events in that webhook's database - only. + only, or disables cleanup entirely when set to `0` (retain forever). - **Performance** — each webhook's database has its own WAL, its own page cache, and its own lock, so concurrent event ingestion across webhooks won't contend. diff --git a/TODO.md b/TODO.md index c869649..a88152d 100644 --- a/TODO.md +++ b/TODO.md @@ -28,6 +28,12 @@ databases currently grow without bound. # Completed Steps +- 2026-08-09 Make retain-forever reachable from the normal create and + edit flows (#79): a `RetentionForeverDays = 365 * 1000` sentinel, a + `Webhook.BeforeSave` hook rewriting any non-positive `retention_days` + to it ahead of GORM's own column defaulting, a reaper that skips such + webhooks outright, form validation that honours `0` and rejects + garbage with a 400, and a retention UI that says "forever" - 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in `Dockerfile`, release-archive sha256 pins in `script/bootstrap`), adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 22f7312..3b6a939 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -18,6 +18,11 @@ const ( testVersion = "test" // testContentType is the event content type used in tests. testContentType = "application/json" + // testWebhookName is the Webhook.Name used in tests. + testWebhookName = "test-webhook" + // testForeverLabel is Webhook.RetentionLabel for a retain-forever + // webhook. + testForeverLabel = "forever" ) func setupTestDB( diff --git a/internal/database/model_webhook.go b/internal/database/model_webhook.go index 9b47516..e96991e 100644 --- a/internal/database/model_webhook.go +++ b/internal/database/model_webhook.go @@ -1,6 +1,39 @@ package database +import ( + "strconv" + + "gorm.io/gorm" +) + +const ( + // DefaultRetentionDays is the event retention period applied to a + // webhook created without an explicit retention value. It is the + // single source of truth for that policy and must stay in sync + // with the `gorm:"default:30"` column default on + // Webhook.RetentionDays below; a struct tag cannot reference a + // constant, so a test asserts the two agree. + DefaultRetentionDays = 30 + + // RetentionForeverDays is the sentinel RetentionDays value meaning + // "retain events forever". Users express that intent as 0, which + // Webhook.BeforeSave rewrites to this value: the column default + // substitutes DefaultRetentionDays for a zero value at insert + // time, so a zero can never survive a round trip to the database. + // Nothing outside this file may hardcode the number. + RetentionForeverDays = 365 * 1000 +) + // Webhook represents a webhook processing unit that groups entrypoints and targets +// +// The receiver kinds below are deliberately mixed. BeforeSave has to +// take a pointer because it mutates the record, and GORM only invokes +// hooks declared that way. RetainsForever and RetentionLabel have to +// take values because html/template calls them on webhooks held in a +// template data map, which reflection cannot address; a pointer +// receiver there fails at render time rather than at compile time. +// +//nolint:recvcheck // GORM needs a pointer hook; templates need values. type Webhook struct { BaseModel @@ -8,7 +41,9 @@ type Webhook struct { Name string `gorm:"not null" json:"name"` Description string `json:"description"` - // RetentionDays is the number of days to retain events. + // RetentionDays is the number of days to retain events. A value of + // RetentionForeverDays means retain forever. The column default + // must equal DefaultRetentionDays. RetentionDays int `gorm:"default:30" json:"retentionDays"` // Relations @@ -16,3 +51,44 @@ type Webhook struct { Entrypoints []Entrypoint `json:"entrypoints,omitempty"` Targets []Target `json:"targets,omitempty"` } + +// BeforeSave normalises RetentionDays on every insert and update. A +// non-positive value is the user's way of asking for "retain forever", +// which is stored as the RetentionForeverDays sentinel. +// +// This has to happen in a hook rather than at the call sites. GORM +// substitutes the column default (DefaultRetentionDays) for a zero +// value while building the insert statement, which runs after +// BeforeSave; rewriting any later than this loses that race and the +// row lands at 30 days. Living on the model also means a future call +// site — a REST API, a fixture, a migration — cannot bypass it. +func (w *Webhook) BeforeSave(_ *gorm.DB) error { + if w.RetentionDays <= 0 { + w.RetentionDays = RetentionForeverDays + } + + return nil +} + +// RetainsForever reports whether this webhook's events are kept +// indefinitely. It accepts the RetentionForeverDays sentinel written by +// BeforeSave and, defensively, the non-positive values that rows +// written before the sentinel existed may still carry. +func (w Webhook) RetainsForever() bool { + return w.RetentionDays <= 0 || + w.RetentionDays >= RetentionForeverDays +} + +// RetentionLabel returns the webhook's retention policy as display +// text, so that no template has to know about the sentinel value. +func (w Webhook) RetentionLabel() string { + if w.RetainsForever() { + return "forever" + } + + if w.RetentionDays == 1 { + return "1 day" + } + + return strconv.Itoa(w.RetentionDays) + " days" +} diff --git a/internal/database/model_webhook_test.go b/internal/database/model_webhook_test.go new file mode 100644 index 0000000..240bf06 --- /dev/null +++ b/internal/database/model_webhook_test.go @@ -0,0 +1,184 @@ +package database_test + +import ( + "context" + "reflect" + "strconv" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/database" +) + +// startedTestDB returns a started main database for model-level tests. +func startedTestDB(t *testing.T) *gorm.DB { + t.Helper() + + db, lc := setupTestDB(t) + + ctx := context.Background() + require.NoError(t, lc.Start(ctx)) + t.Cleanup(func() { require.NoError(t, lc.Stop(ctx)) }) + + return db.DB() +} + +// storedRetention reads the retention_days column straight out of the +// row, so the assertion is about what was persisted rather than about +// whatever the in-memory struct happens to hold. +func storedRetention(t *testing.T, db *gorm.DB, id string) int { + t.Helper() + + var got int + + require.NoError( + t, + db.Model(&database.Webhook{}). + Where("id = ?", id). + Pluck("retention_days", &got).Error, + ) + + return got +} + +// newWebhookWithRetention creates a webhook through the ordinary Create +// path, so the BeforeSave hook and the GORM column default both apply +// exactly as they do in production. +func newWebhookWithRetention( + t *testing.T, + db *gorm.DB, + wh *database.Webhook, +) string { + t.Helper() + + wh.UserID = uuid.New().String() + wh.Name = testWebhookName + + require.NoError( + t, + db.Omit(clause.Associations).Create(wh).Error, + ) + + return wh.ID +} + +func TestWebhookBeforeSave_ZeroBecomesForeverSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 0} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + "a zero retention must be stored as the sentinel, "+ + "not replaced by the column default", + ) +} + +func TestWebhookBeforeSave_NegativeBecomesForeverSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: -5} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + ) +} + +func TestWebhookBeforeSave_PositiveIsPreserved(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 7} + id := newWebhookWithRetention(t, db, wh) + + assert.Equal(t, 7, storedRetention(t, db, id)) +} + +// TestWebhookBeforeSave_UpdateToZeroBecomesSentinel proves the hook +// fires on update as well as insert, via the same Save call the edit +// handler makes. +func TestWebhookBeforeSave_UpdateToZeroBecomesSentinel(t *testing.T) { + t.Parallel() + + db := startedTestDB(t) + + wh := &database.Webhook{RetentionDays: 30} + id := newWebhookWithRetention(t, db, wh) + require.Equal(t, 30, storedRetention(t, db, id)) + + wh.RetentionDays = 0 + require.NoError(t, db.Omit(clause.Associations).Save(wh).Error) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetention(t, db, id), + ) +} + +// TestWebhookRetentionColumnDefaultMatchesConstant guards the one place +// the default lives twice: a struct tag cannot reference a constant, so +// this asserts the tag and DefaultRetentionDays agree. +func TestWebhookRetentionColumnDefaultMatchesConstant(t *testing.T) { + t.Parallel() + + field, ok := reflect.TypeFor[database.Webhook](). + FieldByName("RetentionDays") + require.True(t, ok, "Webhook.RetentionDays must exist") + + assert.Equal( + t, + "default:"+strconv.Itoa(database.DefaultRetentionDays), + field.Tag.Get("gorm"), + ) +} + +func TestWebhookRetainsForeverAndLabel(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + days int + forever bool + label string + }{ + { + "sentinel", + database.RetentionForeverDays, true, testForeverLabel, + }, + { + "above sentinel", + database.RetentionForeverDays + 1, true, testForeverLabel, + }, + {"legacy zero", 0, true, testForeverLabel}, + {"legacy negative", -1, true, testForeverLabel}, + {"default", database.DefaultRetentionDays, false, "30 days"}, + {"one day", 1, false, "1 day"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + wh := database.Webhook{RetentionDays: tc.days} + + assert.Equal(t, tc.forever, wh.RetainsForever()) + assert.Equal(t, tc.label, wh.RetentionLabel()) + }) + } +} diff --git a/internal/database/retention.go b/internal/database/retention.go index 23d516f..b910210 100644 --- a/internal/database/retention.go +++ b/internal/database/retention.go @@ -114,7 +114,8 @@ func (r *RetentionReaper) run(ctx context.Context) { } // sweep lists every webhook from the main database and reaps expired -// rows from each per-webhook database whose RetentionDays is positive. +// rows from each per-webhook database that has a finite retention +// policy. Webhooks set to retain forever are skipped entirely. func (r *RetentionReaper) sweep(ctx context.Context) { var webhooks []Webhook @@ -139,8 +140,13 @@ func (r *RetentionReaper) sweep(ctx context.Context) { wh := webhooks[i] - // RetentionDays of zero or less means retain forever. - if wh.RetentionDays <= 0 { + // Skip retain-forever webhooks before building any query. + // RetainsForever covers both the RetentionForeverDays + // sentinel and the non-positive values that predate it: the + // sentinel is a positive number, so without this the reaper + // would compute a cutoff a thousand years in the past and + // issue a DELETE matching nothing on every single sweep. + if wh.RetainsForever() { continue } diff --git a/internal/database/retention_test.go b/internal/database/retention_test.go index 0ff0c90..1b626fc 100644 --- a/internal/database/retention_test.go +++ b/internal/database/retention_test.go @@ -77,7 +77,7 @@ func createWebhook( wh := &database.Webhook{ UserID: uuid.New().String(), - Name: "test-webhook", + Name: testWebhookName, RetentionDays: retentionDays, } require.NoError( @@ -85,10 +85,11 @@ func createWebhook( db.Omit(clause.Associations).Create(wh).Error, ) - // The RetentionDays column carries a GORM default of 30, so a - // zero (or negative) value passed to Create is replaced by that - // default. Force the requested value explicitly so the - // retain-forever (<= 0) path can be exercised. + // Webhook.BeforeSave rewrites a non-positive RetentionDays to the + // retain-forever sentinel, and the column's GORM default would + // otherwise substitute 30. Force the requested value with a + // column-level update so tests can plant legacy rows that predate + // the sentinel and still carry a literal 0 or negative value. require.NoError( t, db.Model(wh). @@ -98,6 +99,30 @@ func createWebhook( return wh.ID } +// createWebhookNormally inserts a webhook through the ordinary Create +// path, with no column-level forcing, so Webhook.BeforeSave applies +// exactly as it does in production. Passing 0 therefore yields a row +// holding the RetentionForeverDays sentinel. +func createWebhookNormally( + t *testing.T, + db *gorm.DB, + retentionDays int, +) string { + t.Helper() + + wh := &database.Webhook{ + UserID: uuid.New().String(), + Name: testWebhookName, + RetentionDays: retentionDays, + } + require.NoError( + t, + db.Omit(clause.Associations).Create(wh).Error, + ) + + return wh.ID +} + // eventChain is the set of row IDs seeded for a single event. type eventChain struct { eventID string @@ -256,12 +281,59 @@ func TestRetentionReaper_ReapsExpiredKeepsRecent(t *testing.T) { assertChainPresent(t, db, recent) } +// TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep covers the +// end-to-end retain-forever path: a webhook created the normal way with +// a requested retention of 0 lands on the RetentionForeverDays +// sentinel, and the reaper leaves its ancient events alone while still +// reaping a finite-retention webhook in the very same sweep. +func TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep( + t *testing.T, +) { + t.Parallel() + + env := setupRetentionTest(t) + + foreverID := createWebhookNormally(t, env.mainDB.DB(), 0) + + var stored database.Webhook + + require.NoError( + t, + env.mainDB.DB().Where("id = ?", foreverID). + First(&stored).Error, + ) + require.Equal( + t, + database.RetentionForeverDays, + stored.RetentionDays, + "a requested retention of 0 must persist as the sentinel", + ) + + finiteID := createWebhookNormally(t, env.mainDB.DB(), 30) + + foreverDB, err := env.mgr.GetDB(foreverID) + require.NoError(t, err) + + finiteDB, err := env.mgr.GetDB(finiteID) + require.NoError(t, err) + + ancient := time.Now().Add(-365 * 24 * time.Hour) + kept := seedEventChain(t, foreverDB, foreverID, ancient) + doomed := seedEventChain(t, finiteDB, finiteID, ancient) + + env.reaper.ExportSweep(context.Background()) + + assertChainPresent(t, foreverDB, kept) + assertChainGone(t, finiteDB, doomed) +} + func TestRetentionReaper_RetainsForeverWhenNonPositive(t *testing.T) { t.Parallel() env := setupRetentionTest(t) - // RetentionDays of zero means retain forever. + // A legacy row written before the sentinel existed still carries a + // literal 0; the <= 0 guard must keep honouring it. webhookID := createWebhook(t, env.mainDB.DB(), 0) db, err := env.mgr.GetDB(webhookID) diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 193856c..b749cb7 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -26,8 +26,6 @@ const ( maxBodyShift = 20 // recentEventLimit is the number of recent events to show. recentEventLimit = 20 - // defaultRetentionDays is the default event retention period. - defaultRetentionDays = 30 // paginationPerPage is the number of items per page. paginationPerPage = 25 diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index eaaf7e9..1c5c155 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -25,6 +25,37 @@ type WebhookListItem struct { // errMissingURL signals that a required URL was not provided. var errMissingURL = errors.New("missing URL") +// errInvalidRetention signals a retention_days form value that is not +// a non-negative whole number. +var errInvalidRetention = errors.New("invalid retention days") + +// retentionErrorMessage is what the create and edit forms show the user +// when parseRetentionDays returns errInvalidRetention. +const retentionErrorMessage = "Retention must be a whole number of " + + "days, or 0 to retain events forever." + +// parseRetentionDays interprets a retention_days form value. +// +// An empty value yields fallback, which lets the create path apply the +// default and the edit path leave the stored value unchanged. A value +// of 0 is returned as 0 and is rewritten to the retain-forever +// sentinel by database.Webhook's BeforeSave hook, so no handler needs +// to know the sentinel. Anything unparseable or negative is an error +// rather than a silently substituted default. +func parseRetentionDays(raw string, fallback int) (int, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return fallback, nil + } + + v, err := strconv.Atoi(raw) + if err != nil || v < 0 { + return 0, errInvalidRetention + } + + return v, nil +} + // EventWithDeliveries holds an event and its deliveries. type EventWithDeliveries struct { database.Event @@ -106,11 +137,20 @@ func (h *Handlers) buildWebhookListItems( // HandleSourceCreate shows the form to create a new webhook. func (h *Handlers) HandleSourceCreate() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - data := map[string]any{ - tmplKeyError: "", - } + h.renderTemplate( + w, r, "sources_new.html", newSourceFormData(""), + ) + } +} - h.renderTemplate(w, r, "sources_new.html", data) +// newSourceFormData builds the template data for the webhook creation +// form, carrying the retention default so the pre-filled value comes +// from database.DefaultRetentionDays rather than being a third +// hardcoded copy of the same policy. +func newSourceFormData(errMsg string) map[string]any { + return map[string]any{ + tmplKeyError: errMsg, + "DefaultRetentionDays": database.DefaultRetentionDays, } } @@ -145,23 +185,26 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { retentionStr := r.FormValue("retention_days") if name == "" { - data := map[string]any{ - tmplKeyError: "Name is required", - } - w.WriteHeader(http.StatusBadRequest) - h.renderTemplate(w, r, "sources_new.html", data) + h.renderTemplate( + w, r, "sources_new.html", + newSourceFormData("Name is required"), + ) return } - retentionDays := defaultRetentionDays + retentionDays, retErr := parseRetentionDays( + retentionStr, database.DefaultRetentionDays, + ) + if retErr != nil { + w.WriteHeader(http.StatusBadRequest) + h.renderTemplate( + w, r, "sources_new.html", + newSourceFormData(retentionErrorMessage), + ) - if retentionStr != "" { - v, convErr := strconv.Atoi(retentionStr) - if convErr == nil && v > 0 { - retentionDays = v - } + return } h.createWebhookWithEntrypoint( @@ -428,7 +471,25 @@ func (h *Handlers) applyWebhookEdit( webhook.Name = name webhook.Description = r.FormValue("description") - h.parseRetention(r, webhook) + + // An empty field falls back to the stored value, so submitting the + // form without touching retention leaves the policy alone. + retentionDays, retErr := parseRetentionDays( + r.FormValue("retention_days"), webhook.RetentionDays, + ) + if retErr != nil { + data := map[string]any{ + tmplKeyWebhook: *webhook, + tmplKeyError: retentionErrorMessage, + } + + w.WriteHeader(http.StatusBadRequest) + h.renderTemplate(w, r, "source_edit.html", data) + + return + } + + webhook.RetentionDays = retentionDays err := h.db.DB().Save(webhook).Error if err != nil { @@ -442,23 +503,6 @@ func (h *Handlers) applyWebhookEdit( ) } -// parseRetention parses and applies retention_days from the -// form. -func (h *Handlers) parseRetention( - r *http.Request, - webhook *database.Webhook, -) { - retStr := r.FormValue("retention_days") - if retStr == "" { - return - } - - v, err := strconv.Atoi(retStr) - if err == nil && v > 0 { - webhook.RetentionDays = v - } -} - // HandleSourceDelete handles webhook deletion. func (h *Handlers) HandleSourceDelete() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/handlers/source_management_test.go b/internal/handlers/source_management_test.go new file mode 100644 index 0000000..4d71fe6 --- /dev/null +++ b/internal/handlers/source_management_test.go @@ -0,0 +1,481 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/go-chi/chi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +const ( + // sourceTestUserID is the session user id used by the webhook + // management tests. + sourceTestUserID = "source-test-user" + // sourceIDParam is the chi URL parameter naming a webhook. + sourceIDParam = "sourceID" +) + +// formRequest builds an urlencoded POST to path carrying the given +// cookies, plus any chi URL parameters the handler reads. +func formRequest( + path string, + cookies []*http.Cookie, + form url.Values, + urlParams map[string]string, +) *http.Request { + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, + path, + strings.NewReader(form.Encode()), + ) + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + + for _, c := range cookies { + req.AddCookie(c) + } + + rctx := chi.NewRouteContext() + for k, v := range urlParams { + rctx.URLParams.Add(k, v) + } + + return req.WithContext( + context.WithValue(req.Context(), chi.RouteCtxKey, rctx), + ) +} + +// getRequest builds a GET to path carrying the given cookies, plus any +// chi URL parameters the handler reads. +func getRequest( + t *testing.T, + path string, + cookies []*http.Cookie, + urlParams map[string]string, +) *http.Request { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, path, nil, + ) + + for _, c := range cookies { + req.AddCookie(c) + } + + rctx := chi.NewRouteContext() + for k, v := range urlParams { + rctx.URLParams.Add(k, v) + } + + return req.WithContext( + context.WithValue(req.Context(), chi.RouteCtxKey, rctx), + ) +} + +// submitCreate posts the webhook creation form with the given +// retention_days value (omitted entirely when retention is nil) and +// returns the recorder. +func submitCreate( + t *testing.T, + h *handlers.Handlers, + cookies []*http.Cookie, + name string, + retention *string, +) *httptest.ResponseRecorder { + t.Helper() + + form := url.Values{} + form.Set("name", name) + + if retention != nil { + form.Set("retention_days", *retention) + } + + req := formRequest("/sources/new", cookies, form, nil) + w := httptest.NewRecorder() + + h.HandleSourceCreateSubmit().ServeHTTP(w, req) + + return w +} + +// onlyWebhook loads the single webhook belonging to the test user. +func onlyWebhook( + t *testing.T, + db *database.Database, +) database.Webhook { + t.Helper() + + var webhooks []database.Webhook + + require.NoError( + t, + db.DB().Where("user_id = ?", sourceTestUserID). + Find(&webhooks).Error, + ) + require.Len(t, webhooks, 1) + + return webhooks[0] +} + +// seedWebhook inserts a webhook owned by the test user with an exact +// stored retention value, bypassing Webhook.BeforeSave via a +// column-level update so that legacy rows can be planted too. +func seedWebhook( + t *testing.T, + db *database.Database, + retentionDays int, +) database.Webhook { + t.Helper() + + wh := &database.Webhook{ + UserID: sourceTestUserID, + Name: "seeded", + RetentionDays: retentionDays, + } + require.NoError( + t, + db.DB().Omit(clause.Associations).Create(wh).Error, + ) + require.NoError( + t, + db.DB().Model(wh). + Update("retention_days", retentionDays).Error, + ) + + wh.RetentionDays = retentionDays + + return *wh +} + +// storedRetentionDays reads the retention_days column for a webhook. +func storedRetentionDays( + t *testing.T, + db *database.Database, + id string, +) int { + t.Helper() + + var got int + + require.NoError( + t, + db.DB().Model(&database.Webhook{}). + Where("id = ?", id). + Pluck("retention_days", &got).Error, + ) + + return got +} + +// sourceTestEnv bundles the handler, session, and database a webhook +// management test drives. +type sourceTestEnv struct { + handlers *handlers.Handlers + db *database.Database + cookies []*http.Cookie +} + +func setupSourceTest(t *testing.T) *sourceTestEnv { + t.Helper() + + var h *handlers.Handlers + + var sess *session.Session + + var db *database.Database + + app := newTestApp(t, &h, &sess, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + return &sourceTestEnv{ + handlers: h, + db: db, + cookies: authenticatedCookies( + t, sess, sourceTestUserID, "sourceuser", + ), + } +} + +// TestHandleSourceCreateSubmit_ZeroRetentionPersistsForever is the core +// regression test for the bug: the create form's 0 must reach the +// database as the retain-forever sentinel rather than being replaced by +// the column's default of 30. +func TestHandleSourceCreateSubmit_ZeroRetentionPersistsForever( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + zero := "0" + + w := submitCreate(t, env.handlers, env.cookies, "forever", &zero) + require.Equal(t, http.StatusSeeOther, w.Code) + + wh := onlyWebhook(t, env.db) + assert.Equal( + t, + database.RetentionForeverDays, + storedRetentionDays(t, env.db, wh.ID), + ) + assert.True(t, wh.RetainsForever()) +} + +func TestHandleSourceCreateSubmit_OmittedRetentionUsesDefault( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + + w := submitCreate(t, env.handlers, env.cookies, "defaulted", nil) + require.Equal(t, http.StatusSeeOther, w.Code) + + wh := onlyWebhook(t, env.db) + assert.Equal( + t, + database.DefaultRetentionDays, + storedRetentionDays(t, env.db, wh.ID), + ) +} + +// TestHandleSourceCreate_PrefillsDefaultFromConstant keeps the create +// form's pre-filled retention from becoming a third hardcoded copy of +// the 30-day policy. +func TestHandleSourceCreate_PrefillsDefaultFromConstant(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + w := httptest.NewRecorder() + env.handlers.HandleSourceCreate().ServeHTTP( + w, getRequest(t, "/sources/new", env.cookies, nil), + ) + + require.Equal(t, http.StatusOK, w.Code) + + body := w.Body.String() + + assert.Contains( + t, body, + `value="`+strconv.Itoa(database.DefaultRetentionDays)+`"`, + ) + assert.NotContains( + t, body, `max="365"`, + "a max below the sentinel would block retain-forever", + ) + assert.Contains(t, body, `min="0"`) +} + +func TestHandleSourceCreateSubmit_InvalidRetentionIsRejected( + t *testing.T, +) { + t.Parallel() + + for _, raw := range []string{"abc", "-1", "3.5"} { + t.Run(raw, func(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + w := submitCreate( + t, env.handlers, env.cookies, "bad", &raw, + ) + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains( + t, w.Body.String(), "Retention must be", + ) + + var count int64 + + require.NoError( + t, + env.db.DB().Model(&database.Webhook{}). + Where("user_id = ?", sourceTestUserID). + Count(&count).Error, + ) + assert.Zero( + t, count, + "no webhook may be created from a rejected form", + ) + }) + } +} + +// submitEdit posts the webhook edit form for the given webhook. +func submitEdit( + t *testing.T, + env *sourceTestEnv, + wh database.Webhook, + retention string, +) *httptest.ResponseRecorder { + t.Helper() + + form := url.Values{} + form.Set("name", wh.Name) + form.Set("description", wh.Description) + form.Set("retention_days", retention) + + req := formRequest( + "/source/"+wh.ID+"/edit", + env.cookies, + form, + map[string]string{sourceIDParam: wh.ID}, + ) + w := httptest.NewRecorder() + + env.handlers.HandleSourceEditSubmit().ServeHTTP(w, req) + + return w +} + +func TestHandleSourceEditSubmit_ZeroRetentionPersistsForever( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + wh := seedWebhook(t, env.db, database.DefaultRetentionDays) + + w := submitEdit(t, env, wh, "0") + require.Equal(t, http.StatusSeeOther, w.Code) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetentionDays(t, env.db, wh.ID), + ) +} + +func TestHandleSourceEditSubmit_InvalidRetentionIsRejected( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + wh := seedWebhook(t, env.db, database.DefaultRetentionDays) + + w := submitEdit(t, env, wh, "not-a-number") + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), "Retention must be") + assert.Equal( + t, + database.DefaultRetentionDays, + storedRetentionDays(t, env.db, wh.ID), + "a rejected form must not change the stored retention", + ) +} + +func TestHandleSourceEditSubmit_EmptyRetentionLeavesValueUnchanged( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + wh := seedWebhook(t, env.db, 7) + + w := submitEdit(t, env, wh, "") + require.Equal(t, http.StatusSeeOther, w.Code) + + assert.Equal(t, 7, storedRetentionDays(t, env.db, wh.ID)) +} + +// TestSourceEditForm_ForeverWebhookRoundTrips walks the exact path that +// the removed max="365" cap used to break: render the edit form for a +// retain-forever webhook, confirm the pre-filled sentinel is not capped +// by browser validation, then submit that pre-filled value straight +// back and confirm the retention policy survives untouched. +func TestSourceEditForm_ForeverWebhookRoundTrips(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + wh := seedWebhook(t, env.db, database.RetentionForeverDays) + + req := getRequest( + t, "/source/"+wh.ID+"/edit", env.cookies, + map[string]string{sourceIDParam: wh.ID}, + ) + w := httptest.NewRecorder() + env.handlers.HandleSourceEdit().ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + + sentinel := strconv.Itoa(database.RetentionForeverDays) + body := w.Body.String() + + assert.Contains( + t, body, `value="`+sentinel+`"`, + "the edit form pre-fills the stored retention", + ) + assert.NotContains( + t, body, `max="365"`, + "a max below the sentinel would block saving any edit", + ) + assert.Contains( + t, body, "forever", + "the form explains what the sentinel means", + ) + + // Submit the pre-filled value back, exactly as a browser would. + post := submitEdit(t, env, wh, sentinel) + require.Equal(t, http.StatusSeeOther, post.Code) + + assert.Equal( + t, + database.RetentionForeverDays, + storedRetentionDays(t, env.db, wh.ID), + ) +} + +// TestSourceListAndDetail_ShowForeverNotTheSentinelNumber checks that +// the retain-forever value is never rendered to the user as a raw day +// count on either read-only view. +func TestSourceListAndDetail_ShowForeverNotTheSentinelNumber( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + wh := seedWebhook(t, env.db, database.RetentionForeverDays) + sentinel := strconv.Itoa(database.RetentionForeverDays) + + listW := httptest.NewRecorder() + env.handlers.HandleSourceList().ServeHTTP( + listW, getRequest(t, "/sources", env.cookies, nil), + ) + + require.Equal(t, http.StatusOK, listW.Code) + assert.Contains(t, listW.Body.String(), "Retention: forever") + assert.NotContains(t, listW.Body.String(), sentinel) + + detailW := httptest.NewRecorder() + env.handlers.HandleSourceDetail().ServeHTTP( + detailW, + getRequest( + t, "/source/"+wh.ID, env.cookies, + map[string]string{sourceIDParam: wh.ID}, + ), + ) + + require.Equal(t, http.StatusOK, detailW.Code) + assert.Contains(t, detailW.Body.String(), "Retention: forever") + assert.NotContains(t, detailW.Body.String(), sentinel) +} diff --git a/templates/source_detail.html b/templates/source_detail.html index 5b9944a..f573e68 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -181,7 +181,7 @@
-

Retention: {{.Webhook.RetentionDays}} days · Created: {{.Webhook.CreatedAt.Format "2006-01-02 15:04:05 UTC"}}

+

Retention: {{.Webhook.RetentionLabel}} · Created: {{.Webhook.CreatedAt.Format "2006-01-02 15:04:05 UTC"}}

{{end}} diff --git a/templates/source_edit.html b/templates/source_edit.html index 7bce014..5480b05 100644 --- a/templates/source_edit.html +++ b/templates/source_edit.html @@ -28,7 +28,8 @@
- + +

Currently {{.Webhook.RetentionLabel}}. Enter 0 to retain events forever.

diff --git a/templates/sources_list.html b/templates/sources_list.html index 8d9a20c..6fc4cb6 100644 --- a/templates/sources_list.html +++ b/templates/sources_list.html @@ -25,7 +25,7 @@

{{.Description}}

{{end}}
- {{.RetentionDays}}d retention + Retention: {{.RetentionLabel}}
{{.EntrypointCount}} entrypoint{{if ne .EntrypointCount 1}}s{{end}} diff --git a/templates/sources_new.html b/templates/sources_new.html index 60cae8d..8fb94f3 100644 --- a/templates/sources_new.html +++ b/templates/sources_new.html @@ -28,8 +28,8 @@
- -

How long to keep event data.

+ +

How long to keep event data. Enter 0 to retain events forever.