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

internal/handlers/target_edit_test.go

@@ -2,19 +2,18 @@ package handlers_test
 
 import (
 	"context"
-	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"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/delivery"
 )
 
 // The destinations the target edit tests configure. Both are literal
@@ -150,27 +149,6 @@ func storedTarget(
 	return target
 }
 
-// storedHTTPConfig reloads a target and parses its stored HTTP
-// configuration.
-func storedHTTPConfig(
-	t *testing.T,
-	env *sourceTestEnv,
-	targetID string,
-) delivery.HTTPTargetConfig {
-	t.Helper()
-
-	var cfg delivery.HTTPTargetConfig
-
-	require.NoError(
-		t,
-		json.Unmarshal(
-			[]byte(storedTarget(t, env, targetID).Config), &cfg,
-		),
-	)
-
-	return cfg
-}
-
 // editForm is the fully populated edit submission for an HTTP
 // target.
 func editForm(targetURL, headers, timeout string) url.Values {
@@ -197,12 +175,9 @@ func submitTargetEdit(
 	)
 }
 
-// TestHandleTargetCreate_Configuration covers the half of the gap
-// that is not about editing at all: HTTPTargetConfig has carried
-// Headers and Timeout, and the delivery path has honoured them, but
-// the create form wrote {"url":...} and nothing else, so a
-// destination needing an Authorization header could not be
-// configured through the UI at all.
+// TestHandleTargetCreate_Configuration proves the add target form
+// stores an HTTP target's headers and timeout, and leaves them empty
+// when the form leaves them empty.
 func TestHandleTargetCreate_Configuration(t *testing.T) {
 	t.Parallel()
 
@@ -213,9 +188,9 @@ func TestHandleTargetCreate_Configuration(t *testing.T) {
 		assertCreateStoresHeadersAndTimeout(t, env)
 	})
 
-	t.Run("without them keeps a url-only config", func(t *testing.T) {
+	t.Run("without them stores only the url", func(t *testing.T) {
 		t.Parallel()
-		assertCreateKeepsURLOnlyConfig(t, env)
+		assertCreateStoresOnlyTheURL(t, env)
 	})
 }
 
@@ -228,33 +203,31 @@ func assertCreateStoresHeadersAndTimeout(
 		t, env, editAuthHeader+"\nX-Tenant: acme\n", "12",
 	)
 
-	cfg := storedHTTPConfig(t, env, target.ID)
+	stored := storedTarget(t, env, target.ID)
 
-	assert.Equal(t, editOriginalURL, cfg.URL)
-	assert.Equal(t, 12, cfg.Timeout)
+	assert.Equal(t, editOriginalURL, stored.URL)
+	assert.Equal(t, 12, stored.TimeoutSeconds)
 	assert.Equal(
 		t,
 		map[string]string{
 			"Authorization": "Bearer " + editBearerSecret,
 			"X-Tenant":      "acme",
 		},
-		cfg.Headers,
+		stored.Headers,
 	)
 }
 
-// Without the new fields the stored shape must be the same
-// {"url":...} the create form wrote before they existed, so no
-// existing target's configuration is rewritten by this change.
-func assertCreateKeepsURLOnlyConfig(
+func assertCreateStoresOnlyTheURL(
 	t *testing.T, env *sourceTestEnv,
 ) {
 	t.Helper()
 
 	_, target := seedHTTPTarget(t, env, "", "")
 
-	assert.JSONEq(
-		t, `{"url":"`+editOriginalURL+`"}`, target.Config,
-	)
+	assert.Equal(t, editOriginalURL, target.URL)
+	assert.Empty(t, target.Headers)
+	assert.Zero(t, target.TimeoutSeconds)
+	assert.False(t, target.ForwardQuery)
 }
 
 // TestHandleTargetEditSubmit_Saves is the round trip the issue asks
@@ -299,13 +272,8 @@ func assertEditChangesDestination(
 	)
 	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
 
-	assert.Equal(
-		t,
-		editReplacedURL,
-		storedHTTPConfig(t, env, target.ID).URL,
-	)
-
 	reloaded := storedTarget(t, env, target.ID)
+	assert.Equal(t, editReplacedURL, reloaded.URL)
 	assert.Equal(t, "edited-name", reloaded.Name)
 	assert.Equal(t, 5, reloaded.MaxAttempts)
 	assert.Equal(
@@ -333,16 +301,16 @@ func assertEditRoundTripsHeadersAndTimeout(
 	)
 	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
 
-	cfg := storedHTTPConfig(t, env, target.ID)
+	stored := storedTarget(t, env, target.ID)
 
-	assert.Equal(t, 21, cfg.Timeout)
+	assert.Equal(t, 21, stored.TimeoutSeconds)
 	assert.Equal(
 		t,
 		map[string]string{
 			"Authorization": "Bearer rotated",
 			"X-Trace":       "on",
 		},
-		cfg.Headers,
+		stored.Headers,
 	)
 }
 
@@ -362,10 +330,10 @@ func assertEditClearingRemovesThem(
 	)
 	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
 
-	cfg := storedHTTPConfig(t, env, target.ID)
+	stored := storedTarget(t, env, target.ID)
 
-	assert.Empty(t, cfg.Headers)
-	assert.Zero(t, cfg.Timeout)
+	assert.Empty(t, stored.Headers)
+	assert.Zero(t, stored.TimeoutSeconds)
 }
 
 // Delivery attempts are offered only by the forms for target types
@@ -442,6 +410,90 @@ func TestHandleTargetEdit_CallsTheDatabaseTypeArchive(t *testing.T) {
 	assert.Contains(t, page, `class="label">Archive rotation</label>`)
 }
 
+// TestHandleTargetEdit_EveryType edits a target of each type: the edit
+// form starts on the settings the add target form stored, and saving
+// it stores the settings submitted.
+func TestHandleTargetEdit_EveryType(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	// created and edited are the add target form's and the edit
+	// form's fields, as query strings; shown is what the edit form
+	// must show of created; want is the settings edited stores.
+	cases := []struct {
+		targetType      database.TargetType
+		created, edited string
+		shown           []string
+		want            database.Target
+	}{
+		{
+			database.TargetTypeHTTP,
+			"url=" + editOriginalURL +
+				"&headers=X-Tenant:+acme&timeout=7&forward_query=on",
+			"url=" + editReplacedURL + "&timeout=9",
+			[]string{
+				`value="` + editOriginalURL + `"`,
+				"X-Tenant: acme",
+				`value="7"`,
+				`name="forward_query" value="on" checked`,
+			},
+			database.Target{
+				URL:            editReplacedURL,
+				Headers:        map[string]string{},
+				TimeoutSeconds: 9,
+			},
+		},
+		{
+			database.TargetTypeSlack,
+			"url=" + editOriginalURL,
+			"url=" + editReplacedURL,
+			[]string{`value="` + editOriginalURL + `"`},
+			database.Target{URL: editReplacedURL},
+		},
+		{
+			database.TargetTypeDatabase,
+			"expiry=720h&rotation=daily",
+			"expiry=24h&rotation=hourly",
+			[]string{
+				`<option value="720h" selected>`,
+				`<option value="daily" selected>`,
+			},
+			database.Target{
+				ArchiveExpiry:   24 * time.Hour,
+				ArchiveRotation: rotationHourly,
+			},
+		},
+		{database.TargetTypeLog, "", "", nil, database.Target{}},
+	}
+
+	for _, tc := range cases {
+		t.Run(string(tc.targetType), func(t *testing.T) {
+			t.Parallel()
+
+			webhook, target := addTargetThroughForm(
+				t, env, tc.targetType, tc.created,
+			)
+
+			page := targetEditPage(t, env, webhook.ID, target.ID)
+			for _, shown := range tc.shown {
+				assert.Contains(t, page, shown)
+			}
+
+			form, err := url.ParseQuery(tc.edited)
+			require.NoError(t, err)
+			form.Set("name", "edited")
+
+			w := submitTargetEdit(env, webhook.ID, target.ID, form)
+			require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+			got := storedTarget(t, env, target.ID)
+			assert.Equal(t, "edited", got.Name)
+			assert.Equal(t, tc.want, targetSettingsOf(got))
+		})
+	}
+}
+
 // TestHandleTarget_ForwardQuery covers the HTTP target's setting that
 // passes each event's query string on to it: the add target form
 // stores it checked, the edit form starts with it checked and turns it
@@ -467,7 +519,7 @@ func TestHandleTarget_ForwardQuery(t *testing.T) {
 
 	targets := targetsForWebhook(t, env.db, webhook.ID)
 	require.Len(t, targets, 1)
-	assert.True(t, storedHTTPConfig(t, env, targets[0].ID).ForwardQuery)
+	assert.True(t, storedTarget(t, env, targets[0].ID).ForwardQuery)
 
 	w = serveTarget(
 		env, http.MethodGet, targetsPath+"/"+targets[0].ID+"/edit", nil,
@@ -479,7 +531,7 @@ func TestHandleTarget_ForwardQuery(t *testing.T) {
 
 	w = submitTargetEdit(env, webhook.ID, targets[0].ID, edit)
 	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
-	assert.False(t, storedHTTPConfig(t, env, targets[0].ID).ForwardQuery)
+	assert.False(t, storedTarget(t, env, targets[0].ID).ForwardQuery)
 
 	edit.Set("url", editBlockedURL)
 	edit.Set("forward_query", "on")
@@ -568,7 +620,7 @@ func assertEditRejectsBlockedDestination(
 	assert.Contains(t, w.Body.String(), "Invalid target URL")
 	assert.Equal(
 		t, editOriginalURL,
-		storedHTTPConfig(t, env, target.ID).URL,
+		storedTarget(t, env, target.ID).URL,
 		"a rejected edit must leave the stored config alone",
 	)
 }
@@ -597,9 +649,9 @@ func assertEditIgnoresQueryString(
 
 	assert.Equal(t, http.StatusBadRequest, w.Code)
 
-	cfg := storedHTTPConfig(t, env, target.ID)
-	assert.Equal(t, editOriginalURL, cfg.URL)
-	assert.Empty(t, cfg.Headers)
+	stored := storedTarget(t, env, target.ID)
+	assert.Equal(t, editOriginalURL, stored.URL)
+	assert.Empty(t, stored.Headers)
 }
 
 func assertEditRejectsHeaders(
@@ -617,7 +669,7 @@ func assertEditRejectsHeaders(
 	assert.Equal(t, http.StatusBadRequest, w.Code)
 	assert.Contains(t, w.Body.String(), "Invalid headers")
 	assert.Empty(
-		t, storedHTTPConfig(t, env, target.ID).Headers,
+		t, storedTarget(t, env, target.ID).Headers,
 		"a rejected header must not be stored",
 	)
 }
@@ -637,7 +689,7 @@ func assertEditRejectsTimeout(
 	assert.Equal(t, http.StatusBadRequest, w.Code)
 	assert.Contains(t, w.Body.String(), "Invalid timeout")
 	assert.Equal(
-		t, 9, storedHTTPConfig(t, env, target.ID).Timeout,
+		t, 9, storedTarget(t, env, target.ID).TimeoutSeconds,
 		"a rejected timeout must leave the stored one alone",
 	)
 }
@@ -757,7 +809,7 @@ func assertTargetOfAnotherWebhook404s(
 
 	assert.Equal(
 		t, editOriginalURL,
-		storedHTTPConfig(t, env, target.ID).URL,
+		storedTarget(t, env, target.ID).URL,
 	)
 }
 
@@ -778,7 +830,7 @@ func assertWebhookOfAnotherUser404s(
 
 	target := seedConfiguredTarget(
 		t, env.db, other.ID, database.TargetTypeHTTP,
-		`{"url":"`+editOriginalURL+`"}`,
+		database.Target{URL: editOriginalURL},
 	)
 
 	w := serveTarget(