Store a target's settings as columns (closes #543)

A target's settings move from the per-type JSON string in
targets.config to columns: url, headers, timeout_seconds,
forward_query, archive_expiry and archive_rotation. A column a type
does not use stays empty. headers holds the header map as JSON text
through GORM's JSON serializer, and archive_expiry holds a
time.Duration, 0 meaning never. The forms check each setting once,
with the same refusals and messages, and the edit form is filled from
the columns, so the per-type config structs, their parsers and the
"could not be read" branches are gone. A delivery task carries the
six settings in place of the JSON string; tests check that each copy
into a task, and back out of one, keeps all six.

Model: opus-5-5

README.md

@@ -1626,8 +1626,9 @@ Each type uses only its own settings, and the ones it does not use stay empty:
 `http` uses `url`, `headers`, `timeout_seconds` and `forward_query`; `slack`
 uses `url`; `database` uses `archive_expiry` and `archive_rotation`; `log` uses
 none. The add target form, the new webhook form and the target edit form check
-every setting before it is saved, so nothing that reads a target checks it
+every setting before it is saved, and each setting is a column of its own, so
-again.
+nothing that reads a target parses its settings or has to handle one it cannot
+read.
 
 An `http` target POSTs the event to its `url`. Its `headers` are applied last,
 so they win over the event's own forwarded headers.

internal/delivery/engine_integration_test.go

@@ -1221,11 +1221,14 @@ func TestDeliverHTTP_CustomTargetHeaders(t *testing.T) {
 
 // --- HTTP delivery with custom timeout ---
 
+// TestDeliverHTTP_TargetTimeout proves a task's TimeoutSeconds reaches
+// the delivery through the engine. The destination answers after two
+// seconds, inside the engine's own five-second client timeout, so only
+// the task's one second can fail the delivery.
 func TestDeliverHTTP_TargetTimeout(t *testing.T) {
 	t.Parallel()
 
-	db := testWebhookDB(t)
+	s := newISetup(t)
-	e := iEngine(t, 1)
 
 	ts := httptest.NewServer(
 		http.HandlerFunc(
@@ -1237,24 +1240,30 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) {
 	)
 	defer ts.Close()
 
-	event, del := iSeedEventAndDelivery(
+	event := iSeedEvent(
-		t, db, `{"timeout":"test"}`,
+		t, s.WebhookDB, s.WebhookID, `{"timeout":"test"}`,
+	)
+	targetID := uuid.New().String()
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
 	)
 
-	task, d := iHTTPTaskAndDelivery(
+	bodyStr := event.Body
-		event, del, "timeout-target",
+	task := iTask(
-		ts.URL, 1, 1,
+		d, event, s.WebhookID, targetID,
+		"timeout-target", ts.URL, 1, 1, &bodyStr,
 	)
 	task.TimeoutSeconds = 1
-	d.Target.TimeoutSeconds = 1
 
-	e.ExportDeliverHTTP(context.TODO(), db, d, task)
+	s.Engine.ExportProcessNewTask(context.TODO(), &task)
 
-	iAssertStatus(t, db, del.ID,
+	iAssertStatus(t, s.WebhookDB, d.ID,
 		database.DeliveryStatusFailed,
 	)
 
-	iAssertResultFailed(t, db, del.ID)
+	iAssertResultFailed(t, s.WebhookDB, d.ID)
 }
 
 // TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog pins the size

internal/delivery/recovery_test.go

@@ -205,6 +205,55 @@ func TestRecovery_PendingOlderThanTheAgeBound(t *testing.T) {
 	}
 }
 
+// TestRecovery_TaskCarriesEveryTargetSetting: the task the recovery
+// routine queues carries every one of the target's settings, all six
+// set here.
+// They are copied into the task one field at a time, and a field left
+// out is sent empty without any error.
+func TestRecovery_TaskCarriesEveryTargetSetting(t *testing.T) {
+	t.Parallel()
+
+	for _, run := range recoveryRuns() {
+		t.Run(run.name, func(t *testing.T) {
+			t.Parallel()
+
+			s := newISetup(t)
+			iCreateWebhook(t, s.MainDB, s.WebhookID, "recovery")
+
+			target := database.Target{
+				WebhookID:       s.WebhookID,
+				Name:            "recovery",
+				Type:            database.TargetTypeHTTP,
+				Active:          true,
+				URL:             "http://example.com/hook",
+				Headers:         map[string]string{headerAuthorization: "Bearer t"},
+				TimeoutSeconds:  7,
+				ForwardQuery:    true,
+				ArchiveExpiry:   time.Hour,
+				ArchiveRotation: "daily",
+				MaxAttempts:     1,
+			}
+			require.NoError(t, s.MainDB.Create(&target).Error)
+
+			id := seedRecoveryDelivery(
+				t, s, target.ID, database.DeliveryStatusPending, 0, false,
+			)
+			rAgePending(t, s.WebhookDB, id)
+
+			s.Engine.ExportRecoverDeliveries(context.Background(), run.atStart)
+
+			task := requireQueued(t, s.Engine.ExportDeliveryCh())
+			assert.Equal(t, target.ID, task.TargetID)
+			assert.Equal(t, target.URL, task.TargetURL)
+			assert.Equal(t, target.Headers, task.TargetHeaders)
+			assert.Equal(t, target.TimeoutSeconds, task.TimeoutSeconds)
+			assert.Equal(t, target.ForwardQuery, task.ForwardQuery)
+			assert.Equal(t, target.ArchiveExpiry, task.ArchiveExpiry)
+			assert.Equal(t, target.ArchiveRotation, task.ArchiveRotation)
+		})
+	}
+}
+
 // TestRecovery_PendingYoungerThanTheAgeBound: a pending delivery touched
 // a moment ago, as one a stop cut off is, is sent at start, which is how
 // it reaches its target on the next start

internal/delivery/target_database_test.go

@@ -1,6 +1,7 @@
 package delivery_test
 
 import (
+	"context"
 	"database/sql"
 	"fmt"
 	"log/slog"
@@ -20,6 +21,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/archive"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/database/databasetest"
+	"sneak.berlin/go/webhooker/internal/db/migrations"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/gormlog"
 )
@@ -307,3 +309,87 @@ func TestDeliverDatabase_ArchiveFailureFailsDelivery(
 		"no archive file should exist for a failed write",
 	)
 }
+
+// seedArchivedRow creates the archive file at path holding one row, for
+// eventID, archived at archivedAt.
+func seedArchivedRow(
+	t *testing.T, path, eventID string, archivedAt time.Time,
+) {
+	t.Helper()
+
+	sqlDB, err := sql.Open(
+		"sqlite", fmt.Sprintf("file:%s?mode=rwc", path),
+	)
+	require.NoError(t, err)
+
+	defer func() { _ = sqlDB.Close() }()
+
+	gdb, err := gorm.Open(
+		sqlite.Dialector{Conn: sqlDB},
+		&gorm.Config{Logger: gormlog.New(slog.New(slog.DiscardHandler))},
+	)
+	require.NoError(t, err)
+
+	require.NoError(t, migrations.Apply(gdb, migrations.Archive))
+	require.NoError(t, gdb.Exec(
+		"INSERT INTO archived_events (event_id, archived_at) VALUES (?, ?)",
+		eventID, archivedAt,
+	).Error)
+}
+
+// TestDeliverDatabase_TaskArchiveSettings proves a task's ArchiveExpiry
+// and ArchiveRotation reach the archive through the engine. The
+// target's row has neither, so only the task can bring them: with daily
+// rotation the event goes to the file for its day, and opening that
+// file with a one-hour expiry prunes the row it already held, archived
+// two hours ago.
+func TestDeliverDatabase_TaskArchiveSettings(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	webhook := database.Webhook{UserID: uuid.NewString(), Name: "settings"}
+	webhook.ID = s.WebhookID
+	require.NoError(t, s.MainDB.Create(&webhook).Error)
+
+	target := database.Target{
+		WebhookID:   s.WebhookID,
+		Name:        "archive",
+		Type:        database.TargetTypeDatabase,
+		Active:      true,
+		MaxAttempts: 1,
+	}
+	require.NoError(t, s.MainDB.Create(&target).Error)
+
+	event := iSeedEvent(t, s.WebhookDB, s.WebhookID, `{"archived":true}`)
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, target.ID,
+		database.DeliveryStatusPending,
+	)
+
+	task := iTask(
+		d, event, s.WebhookID, target.ID, target.Name, "", 1, 1,
+		&event.Body,
+	)
+	task.TargetType = database.TargetTypeDatabase
+	task.ArchiveExpiry = time.Hour
+	task.ArchiveRotation = "daily"
+
+	daily := target
+	daily.ArchiveRotation = task.ArchiveRotation
+	dailyFile := archive.PathAt(s.DBMgr, &webhook, &daily, event.CreatedAt)
+	seedArchivedRow(t, dailyFile, "expired", time.Now().Add(-2*time.Hour))
+
+	s.Engine.ExportProcessNewTask(context.TODO(), &task)
+
+	iAssertStatus(t, s.WebhookDB, d.ID, database.DeliveryStatusDelivered)
+
+	var rows []archivedRow
+
+	require.NoError(t, openArchiveDBForRead(t, dailyFile).
+		Table("archived_events").Find(&rows).Error)
+	require.Len(t, rows, 1)
+	assert.Equal(t, event.ID, rows[0].EventID,
+		"the event goes to its day's file and the expired row is pruned",
+	)
+}

internal/handlers/delivery_replay_test.go

@@ -327,6 +327,43 @@ func assertNoLeakedTarget(t *testing.T, webhookDB *gorm.DB) {
 	)
 }
 
+// TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting proves the
+// task a replay hands the delivery engine carries every one of the
+// target's settings.
+func TestHandleDeliveryReplay_TaskCarriesEveryTargetSetting(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+		notif *recordingNotifier
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr, &notif)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID, database.TargetTypeHTTP,
+		targetWithEverySetting(),
+	)
+
+	_, original := seedFailedDelivery(t, dbMgr, wh.ID, tgt.ID)
+
+	w := postReplay(t, h, sess, wh.ID, original.ID)
+	require.Equal(t, http.StatusSeeOther, w.Code)
+
+	tasks := notif.Tasks()
+	require.Len(t, tasks, 1)
+	assertTaskCarriesTargetSettings(t, tasks[0], tgt)
+}
+
 // TestHandleDeliveryReplay_RefusesDeletedTarget proves the required
 // refusal: a target deleted since the delivery ran is reported as
 // deleted rather than erroring, and nothing is created or queued.

internal/handlers/webhook_test.go

@@ -10,6 +10,7 @@ import (
 	"net/netip"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/go-chi/chi"
 	"github.com/google/uuid"
@@ -18,6 +19,7 @@ import (
 	"gorm.io/gorm/clause"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/middleware/middlewaretest"
@@ -206,3 +208,71 @@ func receivedLine(t *testing.T, buf *bytes.Buffer) map[string]any {
 
 	return found[0]
 }
+
+// targetWithEverySetting returns target settings with all six set, the
+// archive ones included, so a task that drops one of them differs from
+// the target.
+func targetWithEverySetting() database.Target {
+	return database.Target{
+		URL:             replayTargetURL,
+		Headers:         map[string]string{"X-Api-Key": "k"},
+		TimeoutSeconds:  7,
+		ForwardQuery:    true,
+		ArchiveExpiry:   time.Hour,
+		ArchiveRotation: "daily",
+	}
+}
+
+// assertTaskCarriesTargetSettings proves a delivery task carries every
+// one of the target's settings. They are copied into the task one field
+// at a time, and a field left out is sent empty without any error.
+func assertTaskCarriesTargetSettings(
+	t *testing.T, task delivery.Task, target *database.Target,
+) {
+	t.Helper()
+
+	assert.Equal(t, target.ID, task.TargetID)
+	assert.Equal(t, target.URL, task.TargetURL)
+	assert.Equal(t, target.Headers, task.TargetHeaders)
+	assert.Equal(t, target.TimeoutSeconds, task.TimeoutSeconds)
+	assert.Equal(t, target.ForwardQuery, task.ForwardQuery)
+	assert.Equal(t, target.ArchiveExpiry, task.ArchiveExpiry)
+	assert.Equal(t, target.ArchiveRotation, task.ArchiveRotation)
+}
+
+// TestHandleWebhook_TaskCarriesEveryTargetSetting proves the task a
+// received event hands the delivery engine carries every one of the
+// target's settings.
+func TestHandleWebhook_TaskCarriesEveryTargetSetting(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		db    *database.Database
+		notif *recordingNotifier
+	)
+
+	app := newTestApp(t, &h, &db, &notif)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	webhook := seedWebhook(t, db)
+	entrypoint := seedEntrypoint(t, db, webhook.ID)
+	target := seedConfiguredTarget(
+		t, db, webhook.ID, database.TargetTypeHTTP,
+		targetWithEverySetting(),
+	)
+
+	w := httptest.NewRecorder()
+	h.HandleWebhook().ServeHTTP(w, postRequest(
+		"/h/"+entrypoint.Path, nil,
+		map[string]string{"uuid": entrypoint.Path},
+	))
+
+	require.Equal(t, http.StatusOK, w.Code)
+
+	tasks := notif.Tasks()
+	require.Len(t, tasks, 1)
+	assertTaskCarriesTargetSettings(t, tasks[0], target)
+}