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

internal/handlers/target_create.go

@@ -2,7 +2,6 @@ package handlers
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
@@ -65,20 +64,14 @@ func (h *Handlers) processTargetCreate(
 ) {
 	in := targetFormInputFrom(r)
 
-	target, errMsg, err := h.newTarget(r.Context(), webhook.ID, in)
-	if err != nil {
-		h.serverError(w, r, "failed to encode target config", err)
-
-		return
-	}
-
+	target, errMsg := h.newTarget(r.Context(), webhook.ID, in)
 	if errMsg != "" {
 		h.renderSourceDetail(w, r, webhook, in, errMsg)
 
 		return
 	}
 
-	err = h.db.DB().Create(target).Error
+	err := h.db.DB().Create(target).Error
 	if err != nil {
 		h.serverError(w, r, "failed to create target", err)
 
@@ -93,15 +86,13 @@ func (h *Handlers) processTargetCreate(
 
 // newTarget validates a new target for a webhook and returns the row
 // to create, or, when it refuses the target, the message the form
-// shows. An error is the server's fault, not a refusal: the accepted
-// configuration could not be encoded. Every form that creates a
-// target goes through here, so they all accept and refuse the same
-// things.
+// shows. Every form that creates a target goes through here, so they
+// all accept and refuse the same things.
 func (h *Handlers) newTarget(
 	ctx context.Context,
 	webhookID string,
 	in targetFormInput,
-) (*database.Target, string, error) {
+) (*database.Target, string) {
 	target := &database.Target{
 		WebhookID:   webhookID,
 		Type:        in.Type,
@@ -109,34 +100,33 @@ func (h *Handlers) newTarget(
 		MaxAttempts: 1,
 	}
 
-	errMsg, err := h.setTargetFromForm(ctx, target, in)
-	if err != nil || errMsg != "" {
-		return nil, errMsg, err
+	errMsg := h.setTargetFromForm(ctx, target, in)
+	if errMsg != "" {
+		return nil, errMsg
 	}
 
-	return target, "", nil
+	return target, ""
 }
 
 // setTargetFromForm validates a target form against the target's type
-// and, when it accepts it, sets the target's name, configuration and
-// number of delivery attempts from it. It returns the message the form
-// shows for anything it refuses, an unknown type among them, and then
-// leaves the target unchanged; an error is the server's fault, as for
-// newTarget.
+// and sets the target's name, settings and number of delivery attempts
+// from it. It returns the message the form shows for anything it
+// refuses, an unknown type among them; the caller then discards the
+// target, which may be partly set.
 // The add target form and the target edit form both go through here,
 // so the two cannot come to disagree about what a target may be.
 func (h *Handlers) setTargetFromForm(
 	ctx context.Context,
 	target *database.Target,
 	in targetFormInput,
-) (string, error) {
+) string {
 	if in.Name == "" {
-		return "Name is required", nil
+		return "Name is required"
 	}
 
-	configJSON, errMsg, err := h.buildTargetConfig(ctx, target.Type, in)
-	if err != nil || errMsg != "" {
-		return errMsg, err
+	errMsg := h.setTargetSettings(ctx, target, in)
+	if errMsg != "" {
+		return errMsg
 	}
 
 	// An empty max_attempts keeps the target's count: the default of
@@ -147,14 +137,13 @@ func (h *Handlers) setTargetFromForm(
 	// target is delivering with.
 	maxAttempts, err := parseMaxAttempts(in.MaxAttempts, target.MaxAttempts)
 	if err != nil {
-		return "Invalid delivery attempts: " + attemptsErrorMessage(err), nil
+		return "Invalid delivery attempts: " + attemptsErrorMessage(err)
 	}
 
 	target.Name = in.Name
-	target.Config = configJSON
 	target.MaxAttempts = maxAttempts
 
-	return "", nil
+	return ""
 }
 
 // targetFormInput carries the raw values of a target form. Both the
@@ -213,84 +202,81 @@ func targetFormInputFrom(r *http.Request) targetFormInput {
 	}
 }
 
-// buildTargetConfig builds the JSON config string for a target from
-// the submitted form values, or returns the message the form shows
-// for a value it refuses. An error is the server's fault, not a
-// refusal: the accepted configuration could not be encoded. Which
-// fields of in apply depends on the target type; a type without a URL
-// ignores any URL submitted.
-func (h *Handlers) buildTargetConfig(
+// setTargetSettings sets a target's settings from the submitted form
+// values, or returns the message the form shows for a value it
+// refuses. Which fields of in apply depends on the target type; a
+// type without a URL ignores any URL submitted, and the settings a
+// type does not use are left empty.
+func (h *Handlers) setTargetSettings(
 	ctx context.Context,
-	targetType database.TargetType,
+	target *database.Target,
 	in targetFormInput,
-) (string, string, error) {
-	switch targetType {
+) string {
+	switch target.Type {
 	case database.TargetTypeHTTP:
-		return h.buildHTTPTargetConfig(ctx, in)
+		return h.setHTTPTargetSettings(ctx, target, in)
 	case database.TargetTypeSlack:
-		return h.buildSlackTargetConfig(ctx, in.URL)
+		return h.setSlackTargetSettings(ctx, target, in.URL)
 	case database.TargetTypeDatabase:
-		return buildDatabaseTargetConfig(in.Expiry, in.Rotation)
+		return setDatabaseTargetSettings(target, in.Expiry, in.Rotation)
 	case database.TargetTypeLog:
-		return "", "", nil
+		return ""
 	default:
-		return "", "Invalid target type", nil
+		return "Invalid target type"
 	}
 }
 
-// buildHTTPTargetConfig builds config JSON for an HTTP target: an
-// SSRF-validated destination plus the optional headers, timeout and
-// query string setting the delivery path honours.
-func (h *Handlers) buildHTTPTargetConfig(
+// setHTTPTargetSettings sets an HTTP target's SSRF-validated
+// destination plus the optional headers, timeout and query string
+// setting the delivery path honours.
+func (h *Handlers) setHTTPTargetSettings(
 	ctx context.Context,
+	target *database.Target,
 	in targetFormInput,
-) (string, string, error) {
+) string {
 	errMsg := h.validateTargetURL(
 		ctx, in.URL, "URL is required for HTTP targets",
 	)
 	if errMsg != "" {
-		return "", errMsg, nil
+		return errMsg
 	}
 
 	headers, err := delivery.ParseTargetHeaders(in.Headers)
 	if err != nil {
-		return "", fmt.Sprintf("Invalid headers: %v", err), nil
+		return fmt.Sprintf("Invalid headers: %v", err)
 	}
 
 	timeout, err := delivery.ParseTargetTimeout(in.Timeout)
 	if err != nil {
-		return "", fmt.Sprintf("Invalid timeout: %v", err), nil
+		return fmt.Sprintf("Invalid timeout: %v", err)
 	}
 
-	configJSON, err := marshalTargetConfig(delivery.HTTPTargetConfig{
-		URL:          in.URL,
-		Headers:      headers,
-		Timeout:      timeout,
-		ForwardQuery: in.ForwardQuery,
-	})
+	target.URL = in.URL
+	target.Headers = headers
+	target.TimeoutSeconds = timeout
+	target.ForwardQuery = in.ForwardQuery
 
-	return configJSON, "", err
+	return ""
 }
 
-// buildSlackTargetConfig builds config JSON for a Slack target,
-// whose whole configuration is one SSRF-validated webhook URL.
-func (h *Handlers) buildSlackTargetConfig(
+// setSlackTargetSettings sets a Slack target's one setting, its
+// SSRF-validated webhook URL.
+func (h *Handlers) setSlackTargetSettings(
 	ctx context.Context,
+	target *database.Target,
 	targetURL string,
-) (string, string, error) {
+) string {
 	errMsg := h.validateTargetURL(
 		ctx, targetURL,
 		"Webhook URL is required for Slack targets",
 	)
 	if errMsg != "" {
-		return "", errMsg, nil
+		return errMsg
 	}
 
-	configJSON, err := marshalTargetConfig(delivery.SlackTargetConfig{
-		WebhookURL: targetURL,
-	})
+	target.URL = targetURL
 
-	return configJSON, "", err
+	return ""
 }
 
 // validateTargetURL refuses an empty or SSRF-blocked destination,
@@ -341,52 +327,31 @@ func (h *Handlers) validateTargetURL(
 	return ""
 }
 
-// marshalTargetConfig serialises a target configuration for storage.
-func marshalTargetConfig(cfg any) (string, error) {
-	configBytes, err := json.Marshal(cfg)
-	if err != nil {
-		return "", err
-	}
-
-	return string(configBytes), nil
-}
-
-// buildDatabaseTargetConfig builds config JSON for a database
-// (archive) target. The optional expiry and rotation are validated
-// here, at creation time, so a bad value is refused instead of
-// failing every subsequent delivery. Each is stored only when set,
-// and with neither the config is empty (the keep-forever, one-file
-// default).
-func buildDatabaseTargetConfig(
+// setDatabaseTargetSettings sets a database (archive) target's
+// expiry and rotation. Both are optional: an empty or never expiry
+// keeps the archive forever, and an empty rotation is stored as none,
+// one file. A bad value is refused here, at the form, instead of
+// being stored.
+func setDatabaseTargetSettings(
+	target *database.Target,
 	expiry, rotation string,
-) (string, string, error) {
-	expiry = strings.TrimSpace(expiry)
-
-	err := archive.ValidateExpiry(expiry)
+) string {
+	archiveExpiry, err := archive.ParseExpiry(strings.TrimSpace(expiry))
 	if err != nil {
-		return "", fmt.Sprintf("Invalid archive expiry: %v", err), nil
+		return fmt.Sprintf("Invalid archive expiry: %v", err)
 	}
 
 	err = archive.ValidateRotation(rotation)
 	if err != nil {
-		return "", fmt.Sprintf("Invalid archive rotation: %v", err), nil
+		return fmt.Sprintf("Invalid archive rotation: %v", err)
 	}
 
-	cfg := map[string]any{}
-
-	if expiry != "" {
-		cfg["expiry"] = expiry
+	if rotation == "" {
+		rotation = archiveRotationNone
 	}
 
-	if rotation != "" {
-		cfg["rotation"] = rotation
-	}
+	target.ArchiveExpiry = archiveExpiry
+	target.ArchiveRotation = rotation
 
-	if len(cfg) == 0 {
-		return "", "", nil
-	}
-
-	configJSON, err := marshalTargetConfig(cfg)
-
-	return configJSON, "", err
+	return ""
 }