issue: storage/sql: Update writes every column, so concurrent partial updates lose data

[Lutherwaves]

Check Existing Issues

• I have searched the existing issues and discussions.

Expected Behavior

An Update should persist the change the caller made. Two callers that modify different columns of the same row should both survive, regardless of interleaving — this is the ordinary read-modify-write pattern behind any partial-update endpoint (HTTP PATCH, RFC 6902 JSON Patch, a form that submits one field).

Actual Behavior

Update writes every column of the struct it is given, so each caller writes back the values it read earlier for the columns it never touched. The last writer wins on all columns, and the other caller's change is silently reverted — a lost update. No error is returned to either caller and nothing is logged.

storage/sql.go:

func (s *SQLAdapter) UpdateContext(ctx context.Context, item any, filter map[string]any, params ...map[string]any) error {
	// ...
	result := s.dbWithCtx(ctx).Where(query, bindings).Save(item)
	return result.Error
}

Save on a struct with a primary key emits an UPDATE listing all columns. The adapter has no way to express "write only these columns", so a caller that knows exactly which field changed cannot act on that knowledge.

This affects the Memory adapter identically, since MemoryAdapter embeds *SQLAdapter and delegates every *Context method to it.

Steps to Reproduce

Self-contained, runs on the memory adapter — no external database required.

package storage_test

import (
	"testing"

	"github.com/tink3rlabs/magic/storage"
)

type widget struct {
	Id    string `json:"id" gorm:"primaryKey;column:id"`
	Name  string `json:"name" gorm:"column:name"`
	Color string `json:"color" gorm:"column:color"`
}

func (widget) TableName() string { return "widgets" }

func TestConcurrentPartialUpdatesLoseData(t *testing.T) {
	m := storage.GetMemoryAdapterInstance()
	for _, stmt := range []string{
		`CREATE TABLE IF NOT EXISTS widgets (id TEXT PRIMARY KEY, name TEXT, color TEXT)`,
		`DELETE FROM widgets`,
		`INSERT INTO widgets VALUES ('w1', 'original', 'red')`,
	} {
		if err := m.Execute(stmt); err != nil {
			t.Fatalf("setup %q: %v", stmt, err)
		}
	}
	id := map[string]any{"id": "w1"}

	// 1. Two callers each read the row before either writes.
	var a, b widget
	if err := m.Get(&a, id); err != nil {
		t.Fatalf("read a: %v", err)
	}
	if err := m.Get(&b, id); err != nil {
		t.Fatalf("read b: %v", err)
	}

	// 2. Caller A changes only name. Caller B changes only color.
	a.Name = "renamed"
	if err := m.Update(&a, id); err != nil {
		t.Fatalf("update a: %v", err)
	}
	b.Color = "blue"
	if err := m.Update(&b, id); err != nil {
		t.Fatalf("update b: %v", err)
	}

	// 3. Both changes touched different columns, so both should be present.
	var got widget
	if err := m.Get(&got, id); err != nil {
		t.Fatalf("read back: %v", err)
	}
	if got.Name != "renamed" || got.Color != "blue" {
		t.Fatalf("lost update: got name=%q color=%q, want name=%q color=%q",
			got.Name, got.Color, "renamed", "blue")
	}
}

Logs & Screenshots

=== RUN   TestConcurrentPartialUpdatesLoseData
    repro_test.go:55: lost update: got name="original" color="blue", want name="renamed" color="blue"
--- FAIL: TestConcurrentPartialUpdatesLoseData (0.00s)
FAIL

name reverts to original: caller B's write carried the name it read in step 1, before caller A's rename existed. Neither Update returned an error.

Reproduced on v0.19.0 and, unchanged, on the head of #258 (63eb934) — that PR rewrites this same statement but keeps Select("*"), so it does not affect this issue either way.

Additional Information

Scope. Narrowing the write addresses concurrent edits to different columns. Two callers editing the same column remain last-write-wins, which is a separate concern (the adapter would need caller-supplied conditions on the UPDATE to detect it) and is not what this issue asks for.

Suggested fix. Let the caller name the columns to write. Update/UpdateContext already accept params ...map[string]any and currently ignore it entirely, so this can be added without touching a single signature and without changing behavior for any existing caller — omitting the key keeps today's write-everything semantics.

// opt-in; absent means "all columns", exactly as now
err := adapter.Update(&item, filter, map[string]any{
	storage.UpdateColumnsKey: []string{"name", "modified_at"},
})

This composes with #258, which replaces Save with Model(item).Where(...).Select("*").Updates(item): the column list becomes the argument to that existing Select, so the change is small and lands in one place.

Worth deciding as part of it: whether an unknown column name should be rejected with an error or ignored. Rejecting seems better — a typo that silently drops a field is the same class of quiet data loss as this issue.