Keep each model's parent out of its JSON (closes #177)

Every reference from a model to the record it belongs to is now
json:"-", so the webhook and target models (and the other parent and
child pairs in internal/database) can no longer marshal each other in
a loop. Preloading maps associations without reading json tags, so it
still fills these references. One test preloads a webhook with its
targets and entrypoints and a target with its webhook, checks the
references are filled, and checks the JSON leaves them out; another
builds each other model with its parent set and checks the parent's
id is absent from its JSON.

Model: opus-5-5

internal/database/model_apikey.go

@@ -15,6 +15,7 @@ type APIKey struct {
 	Description string     `json:"description"`
 	LastUsedAt  *time.Time `json:"lastUsedAt,omitempty"`
 
-	// Relations
-	User User `json:"user,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// User.APIKeys leads back here, and the JSON could loop.
+	User User `json:"-"`
 }

internal/database/model_delivery.go

@@ -56,8 +56,10 @@ type Delivery struct {
 	// the index.
 	FinishedAt *time.Time `gorm:"index:idx_deliveries_status,priority:3" json:"finishedAt,omitempty"`
 
-	// Relations
-	Event           Event            `json:"event,omitzero"`
-	Target          Target           `json:"target,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// Event.Deliveries and Target.Deliveries lead back here, and the
+	// JSON could loop.
+	Event           Event            `json:"-"`
+	Target          Target           `json:"-"`
 	DeliveryResults []DeliveryResult `json:"deliveryResults,omitempty"`
 }

internal/database/model_delivery_result.go

@@ -23,6 +23,7 @@ type DeliveryResult struct {
 	Error        string `json:"error,omitempty"`
 	Duration     int64  `json:"durationMs"` // Duration in milliseconds
 
-	// Relations
-	Delivery Delivery `json:"delivery,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// Delivery.DeliveryResults leads back here, and the JSON could loop.
+	Delivery Delivery `json:"-"`
 }

internal/database/model_entrypoint.go

@@ -15,6 +15,7 @@ type Entrypoint struct {
 	Description string `json:"description"`
 	Active      bool   `gorm:"default:true" json:"active"`
 
-	// Relations
-	Webhook Webhook `json:"webhook,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// Webhook.Entrypoints leads back here, and the JSON could loop.
+	Webhook Webhook `json:"-"`
 }

internal/database/model_event.go

@@ -44,8 +44,9 @@ type Event struct {
 	// kept as the record of where the copy came from either way.
 	ResubmittedFromID *string `gorm:"type:uuid;index" json:"resubmittedFromId,omitempty"`
 
-	// Relations
-	Webhook    Webhook    `json:"webhook,omitzero"`
-	Entrypoint Entrypoint `json:"entrypoint,omitzero"`
+	// Relations. No model marshals the record it belongs to, so
+	// Webhook and Entrypoint are left out of the JSON.
+	Webhook    Webhook    `json:"-"`
+	Entrypoint Entrypoint `json:"-"`
 	Deliveries []Delivery `json:"deliveries,omitempty"`
 }

internal/database/model_parent_test.go

@@ -0,0 +1,126 @@
+package database_test
+
+import (
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// TestPreloadedModelsMarshalWithoutTheirParent pins that a child's
+// reference to the record it belongs to is left out of the JSON, so a
+// webhook and its targets cannot marshal each other in a loop, and that
+// GORM still preloads that reference, since it ignores json tags.
+func TestPreloadedModelsMarshalWithoutTheirParent(t *testing.T) {
+	t.Parallel()
+
+	db := startedTestDB(t)
+
+	stored := database.Webhook{
+		UserID:      uuid.New().String(),
+		Name:        testWebhookName,
+		Entrypoints: []database.Entrypoint{{Path: uuid.New().String()}},
+		Targets: []database.Target{{
+			Name: "log",
+			Type: database.TargetTypeLog,
+		}},
+	}
+	require.NoError(t, db.Create(&stored).Error)
+
+	entrypointID := stored.Entrypoints[0].ID
+	targetID := stored.Targets[0].ID
+
+	var webhook database.Webhook
+
+	require.NoError(t, db.
+		Preload("Entrypoints.Webhook").
+		Preload("Targets.Webhook").
+		First(&webhook, "id = ?", stored.ID).Error)
+
+	require.Len(t, webhook.Entrypoints, 1)
+	require.Len(t, webhook.Targets, 1)
+	assert.Equal(t, stored.ID, webhook.Entrypoints[0].Webhook.ID)
+	assert.Equal(t, stored.ID, webhook.Targets[0].Webhook.ID)
+
+	encoded := marshalModel(t, webhook)
+
+	assert.Contains(t, encoded, entrypointID)
+	assert.Contains(t, encoded, targetID)
+
+	// Each child holds the parent's id as its webhookId, so the parent
+	// is looked for by its own id field.
+	parentIDField := `"id":"` + stored.ID + `"`
+
+	assert.NotContains(t, marshalModel(t, webhook.Entrypoints[0]), parentIDField)
+	assert.NotContains(t, marshalModel(t, webhook.Targets[0]), parentIDField)
+
+	var target database.Target
+
+	require.NoError(t, db.
+		Preload("Webhook").
+		First(&target, "id = ?", targetID).Error)
+
+	assert.Equal(t, stored.ID, target.Webhook.ID)
+
+	encoded = marshalModel(t, target)
+
+	assert.Contains(t, encoded, stored.ID)
+	assert.NotContains(t, encoded, parentIDField)
+}
+
+// TestModelsMarshalWithoutTheirParent covers the other references to a
+// parent: each model is built with its parent set, and the parent's id
+// must not appear in the JSON.
+func TestModelsMarshalWithoutTheirParent(t *testing.T) {
+	t.Parallel()
+
+	parent := database.BaseModel{ID: uuid.New().String()}
+
+	cases := []struct {
+		name  string
+		model any
+	}{
+		{
+			name:  "Webhook.User",
+			model: database.Webhook{User: database.User{BaseModel: parent}},
+		},
+		{
+			name:  "APIKey.User",
+			model: database.APIKey{User: database.User{BaseModel: parent}},
+		},
+		{
+			name:  "Delivery.Event",
+			model: database.Delivery{Event: database.Event{BaseModel: parent}},
+		},
+		{
+			name:  "Delivery.Target",
+			model: database.Delivery{Target: database.Target{BaseModel: parent}},
+		},
+		{
+			name: "DeliveryResult.Delivery",
+			model: database.DeliveryResult{
+				Delivery: database.Delivery{BaseModel: parent},
+			},
+		},
+		{
+			name:  "Event.Webhook",
+			model: database.Event{Webhook: database.Webhook{BaseModel: parent}},
+		},
+		{
+			name: "Event.Entrypoint",
+			model: database.Event{
+				Entrypoint: database.Entrypoint{BaseModel: parent},
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.NotContains(t, marshalModel(t, tc.model), parent.ID)
+		})
+	}
+}

internal/database/model_target.go

@@ -34,7 +34,8 @@ type Target struct {
 	MaxRetries   int `json:"maxRetries,omitempty"`
 	MaxQueueSize int `json:"maxQueueSize,omitempty"`
 
-	// Relations
-	Webhook    Webhook    `json:"webhook,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// Webhook.Targets leads back here, and the JSON could loop.
+	Webhook    Webhook    `json:"-"`
 	Deliveries []Delivery `json:"deliveries,omitempty"`
 }

internal/database/model_webhook.go

@@ -66,8 +66,9 @@ type Webhook struct {
 	// must equal DefaultRetentionDays.
 	RetentionDays int `gorm:"default:30" json:"retentionDays"`
 
-	// Relations
-	User        User         `json:"user,omitzero"`
+	// Relations. No model marshals the record it belongs to:
+	// User.Webhooks leads back here, and the JSON could loop.
+	User        User         `json:"-"`
 	Entrypoints []Entrypoint `json:"entrypoints,omitempty"`
 	Targets     []Target     `json:"targets,omitempty"`
 }

internal/handlers/source_management.go

@@ -1911,9 +1911,13 @@ func (h *Handlers) HandleTargetToggle() http.HandlerFunc {
 				return false, err
 			}
 
-			tgt.Active = !tgt.Active
+			// Only the active column: saving the whole row would
+			// write back the name and settings read above over an
+			// edit saved since.
+			active := !tgt.Active
 
-			return tgt.Active, h.db.DB().Save(&tgt).Error
+			return active, h.db.DB().Model(&tgt).
+				Update("active", active).Error
 		},
 		"failed to toggle target",
 		targetActivated, targetDeactivated,

internal/handlers/target_toggle_test.go

@@ -0,0 +1,66 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+)
+
+// TestHandleTargetToggle_DoesNotUndoAnEdit proves that a toggle which
+// loaded the target before an edit of it was saved does not write the
+// old name and settings back over the edit. The edit is submitted from
+// a callback on the toggle's own read of the target, so it is saved
+// after that read and before the toggle writes.
+func TestHandleTargetToggle_DoesNotUndoAnEdit(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	wh, tgt := seedHTTPTarget(t, env, "", "")
+	require.True(t, tgt.Active)
+
+	var (
+		edited   bool
+		editCode int
+	)
+
+	require.NoError(t, env.db.DB().Callback().Query().
+		After("gorm:query").
+		Register("test:edit_after_toggle_read", func(tx *gorm.DB) {
+			// The edit reads the target too; only the toggle's read,
+			// the first, submits it.
+			if tx.Statement.Table != "targets" || edited {
+				return
+			}
+
+			edited = true
+			editCode = submitTargetEdit(
+				env, wh.ID, tgt.ID,
+				editForm(editReplacedURL, "", ""),
+			).Code
+		}),
+	)
+
+	req := postRequest(
+		"/hook/"+wh.ID+"/targets/"+tgt.ID+"/toggle",
+		env.cookies,
+		map[string]string{paramSourceID: wh.ID, paramTargetID: tgt.ID},
+	)
+	w := httptest.NewRecorder()
+
+	env.handlers.HandleTargetToggle().ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	require.Equal(t, http.StatusSeeOther, editCode)
+
+	stored := storedTarget(t, env, tgt.ID)
+	assert.False(t, stored.Active)
+	assert.Equal(t, "edited-name", stored.Name)
+	assert.Equal(t, 5, stored.MaxRetries)
+	assert.Equal(
+		t, editReplacedURL, storedHTTPConfig(t, env, tgt.ID).URL,
+	)
+}

internal/middleware/middleware_test.go

@@ -12,7 +12,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/gorilla/sessions"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"sneak.berlin/go/webhooker/internal/config"
@@ -78,14 +77,7 @@ func newTestSessionManager(
 		key[i] = byte(i)
 	}
 
-	store := sessions.NewCookieStore(key)
-	store.Options = &sessions.Options{
-		Path:     "/",
-		MaxAge:   86400 * 7,
-		HttpOnly: true,
-		Secure:   false,
-		SameSite: http.SameSiteLaxMode,
-	}
+	store := session.NewStore(key)
 
 	var now func() time.Time
 
@@ -931,8 +923,7 @@ func metricsAuthMiddleware(
 	}
 
 	key := make([]byte, testKeySize)
-	store := sessions.NewCookieStore(key)
-	store.Options = &sessions.Options{Path: "/", MaxAge: 86400}
+	store := session.NewStore(key)
 
 	sessManager := session.NewForTest(store, cfg, log, key, nil)
 

internal/session/export_test.go

@@ -1,10 +0,0 @@
-package session
-
-import "github.com/gorilla/sessions"
-
-// NewStore exposes the production cookie-store constructor so tests
-// exercise the store the application actually runs with, rather than a
-// lookalike assembled in the test.
-func NewStore(key []byte) *sessions.CookieStore {
-	return newStore(key)
-}

internal/session/testing.go

@@ -8,6 +8,13 @@ import (
 	"sneak.berlin/go/webhooker/internal/config"
 )
 
+// NewStore exposes the production cookie-store constructor so tests
+// exercise the store the application actually runs with, rather than a
+// lookalike assembled in the test.
+func NewStore(key []byte) *sessions.CookieStore {
+	return newStore(key)
+}
+
 // NewForTest creates a Session with a pre-configured cookie store for use
 // in tests. This bypasses the fx lifecycle and database dependency, allowing
 // middleware and handler tests to use real session functionality. The key