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, so nothing that reads a target parses
it. 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.

Model: opus-5-5

README.md

@@ -1626,9 +1626,8 @@ 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, and each setting is a column of its own, so
-nothing that reads a target parses its settings or has to handle one it cannot
-read.
+every setting before it is saved, so nothing that reads a target checks it
+again.
 
 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,14 +1221,11 @@ 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()
 
-	s := newISetup(t)
+	db := testWebhookDB(t)
+	e := iEngine(t, 1)
 
 	ts := httptest.NewServer(
 		http.HandlerFunc(
@@ -1240,30 +1237,24 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) {
 	)
 	defer ts.Close()
 
-	event := iSeedEvent(
-		t, s.WebhookDB, s.WebhookID, `{"timeout":"test"}`,
-	)
-	targetID := uuid.New().String()
-
-	d := iSeedDelivery(
-		t, s.WebhookDB, event.ID, targetID,
-		database.DeliveryStatusPending,
+	event, del := iSeedEventAndDelivery(
+		t, db, `{"timeout":"test"}`,
 	)
 
-	bodyStr := event.Body
-	task := iTask(
-		d, event, s.WebhookID, targetID,
-		"timeout-target", ts.URL, 1, 1, &bodyStr,
+	task, d := iHTTPTaskAndDelivery(
+		event, del, "timeout-target",
+		ts.URL, 1, 1,
 	)
 	task.TimeoutSeconds = 1
+	d.Target.TimeoutSeconds = 1
 
-	s.Engine.ExportProcessNewTask(context.TODO(), &task)
+	e.ExportDeliverHTTP(context.TODO(), db, d, task)
 
-	iAssertStatus(t, s.WebhookDB, d.ID,
+	iAssertStatus(t, db, del.ID,
 		database.DeliveryStatusFailed,
 	)
 
-	iAssertResultFailed(t, s.WebhookDB, d.ID)
+	iAssertResultFailed(t, db, del.ID)
 }
 
 // TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog pins the size

internal/delivery/recovery_test.go

@@ -205,55 +205,6 @@ 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,7 +1,6 @@
 package delivery_test
 
 import (
-	"context"
 	"database/sql"
 	"fmt"
 	"log/slog"
@@ -21,7 +20,6 @@ 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"
 )
@@ -309,87 +307,3 @@ 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,43 +327,6 @@ 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,7 +10,6 @@ import (
 	"net/netip"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/go-chi/chi"
 	"github.com/google/uuid"
@@ -19,7 +18,6 @@ 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"
@@ -208,71 +206,3 @@ 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)
-}