From 0a08e7269ac9fa659b821d20cd6443ad05a1011c Mon Sep 17 00:00:00 2001 From: Silvan <27845747+adlerhurst@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:36:44 +0200 Subject: [PATCH] fix(eventstore): omit owners on empty unique constraint inserts (#12785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Which Problems Are Solved After #12740, setup on existing deployments (v4.17.x → v4.18.0) fails before migration 78 can add `eventstore.unique_constraints.owners`. Starting any setup step pushes `system.migration.started`, which inserts a unique constraint that names the `owners` column. On upgraded databases that column does not exist yet, so setup dies on `76_users14_login_equality_indexes` with `SQLSTATE 42703`. # How the Problems Are Solved Empty-owner unique-constraint inserts no longer name the `owners` column: - `len(Owners) == 0` → `INSERT (instance_id, unique_type, unique_field)` - tagged `WithOwners(...)` → unchanged `INSERT (..., owners)` Setup started/done events never stamp owners, so 76–78 can start. Migration 78 still adds the column; after that, omitted `owners` uses `DEFAULT '{}'`. Both v2 and v3 pushers apply the same rule. Mixed pushes split into two INSERT batches so tagged and empty rows do not share a column list. # Additional Changes - v2 SQL mocks now expect the three-column INSERT for untagged adds, plus tagged and mixed cases - Pusher tests cover mixed tagged+empty adds and adds against a table that does not have `owners` yet - The missing-column pusher test drops `owners` and pushes in one transaction (`PushWithClient`), then rolls back so the shared test schema is not mutated. Other tests may stall on `AccessExclusiveLock` until rollback. # Additional Context - Closes #12780 - Follow-up for PR #12740 - Operators do not need to re-run init or apply DDL outside setup Verified locally: - `go test ./internal/eventstore ./internal/v2/eventstore/postgres ./internal/eventstore/v3` (including `-race` on those packages) - Empty-owner insert succeeds when the `owners` column is missing; tagged insert still requires the column
Open in Web Open in Cursor 
--------- Co-authored-by: Cursor Agent Co-authored-by: Silvan --- .../eventstore_pusher_owners_test.go | 87 ++++++++++++++++++ internal/eventstore/v3/unique_constraints.go | 43 ++++++--- .../v3/unique_constraints_add_test.go | 15 ++++ .../unique_constraints_add_without_owners.sql | 6 ++ internal/v2/eventstore/postgres/push.go | 28 ++++-- internal/v2/eventstore/postgres/push_test.go | 90 ++++++++++++++++--- 6 files changed, 233 insertions(+), 36 deletions(-) create mode 100644 internal/eventstore/v3/unique_constraints_add_test.go create mode 100644 internal/eventstore/v3/unique_constraints_add_without_owners.sql diff --git a/internal/eventstore/eventstore_pusher_owners_test.go b/internal/eventstore/eventstore_pusher_owners_test.go index 085a75357..7fb004645 100644 --- a/internal/eventstore/eventstore_pusher_owners_test.go +++ b/internal/eventstore/eventstore_pusher_owners_test.go @@ -3,8 +3,11 @@ package eventstore_test import ( "context" "database/sql" + "errors" "testing" + "github.com/jackc/pgx/v5/pgconn" + "github.com/zitadel/zitadel/internal/database" "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/zitadel/internal/zerrors" @@ -222,6 +225,22 @@ var uniqueConstraintOwnerCases = []uniqueConstraintOwnerCase{ assertOwners(t, client, instanceID, "usernames", "untagged", nil) }, }, + { + name: "mixed add tagged and empty owners", + run: func(t *testing.T, db *eventstore.Eventstore, client *database.DB, instanceID string) { + _, err := db.Push(context.Background(), generateCommand("owners-mixed-add", "1", + withInstanceID(instanceID), + generateAddUniqueConstraint("usernames", "tagged", "org:org-1", "user:u1"), + generateAddUniqueConstraint("usernames", "untagged"), + )) + if err != nil { + t.Fatal(err) + } + assertUniqueCount(t, client, instanceID, 2) + assertOwners(t, client, instanceID, "usernames", "tagged", []string{"org:org-1", "user:u1"}) + assertOwners(t, client, instanceID, "usernames", "untagged", nil) + }, + }, { name: "mixed batch deletes then inserts", run: func(t *testing.T, db *eventstore.Eventstore, client *database.DB, instanceID string) { @@ -246,6 +265,74 @@ var uniqueConstraintOwnerCases = []uniqueConstraintOwnerCase{ }, } +func TestEventstore_Push_UniqueConstraintOwners_MissingColumn(t *testing.T) { + for pusherName, pusher := range pushers { + t.Run(pusherName, func(t *testing.T) { + client := clients[pusherName] + tx, err := client.Begin() + if err != nil { + t.Fatal(err) + } + // DROP COLUMN takes AccessExclusiveLock until rollback. Other tests + // against the same database may stall; they still complete afterwards. + t.Cleanup(func() { + if err := tx.Rollback(); err != nil && !errors.Is(err, sql.ErrTxDone) { + t.Errorf("rollback missing-column tx: %v", err) + } + }) + if _, err := tx.Exec("ALTER TABLE eventstore.unique_constraints DROP COLUMN IF EXISTS owners"); err != nil { + t.Fatal(err) + } + + db := eventstore.NewEventstore(&eventstore.Config{ + Querier: queriers["v2(inmemory)"], + Pusher: pusher, + }) + const instanceID = "owners-missing-col" + + t.Run("empty owners add succeeds", func(t *testing.T) { + _, err := db.PushWithClient(context.Background(), tx, generateCommand("owners-missing-empty", "1", + withInstanceID(instanceID), + generateAddUniqueConstraint("usernames", "bob"), + )) + if err != nil { + t.Fatal(err) + } + assertUniqueCountTx(t, tx, instanceID, 1) + }) + + t.Run("tagged owners add fails", func(t *testing.T) { + _, err := db.PushWithClient(context.Background(), tx, generateCommand("owners-missing-tagged", "1", + withInstanceID(instanceID), + generateAddUniqueConstraint("usernames", "alice", "org:org-1", "user:u1"), + )) + if err == nil { + t.Fatal("expected missing owners column error") + } + if !isUndefinedColumn(err) { + t.Fatalf("expected undefined column, got %v", err) + } + }) + }) + } +} + +func isUndefinedColumn(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == "42703" +} + +func assertUniqueCountTx(t *testing.T, tx *sql.Tx, instanceID string, want int) { + t.Helper() + var count int + if err := tx.QueryRow("SELECT COUNT(*) FROM eventstore.unique_constraints WHERE instance_id = $1", instanceID).Scan(&count); err != nil { + t.Fatal(err) + } + if count != want { + t.Errorf("unique count = %d, want %d", count, want) + } +} + func assertUniqueCount(t *testing.T, db *database.DB, instanceID string, want int) { t.Helper() var count int diff --git a/internal/eventstore/v3/unique_constraints.go b/internal/eventstore/v3/unique_constraints.go index d29589df8..f3733d996 100644 --- a/internal/eventstore/v3/unique_constraints.go +++ b/internal/eventstore/v3/unique_constraints.go @@ -23,6 +23,8 @@ var ( deleteConstraintPlaceholdersStmt string //go:embed unique_constraints_add.sql addConstraintStmt string + //go:embed unique_constraints_add_without_owners.sql + addConstraintWithoutOwnersStmt string ) func handleUniqueConstraints(ctx context.Context, tx database.Transaction, commands []eventstore.Command) (err error) { @@ -32,8 +34,10 @@ func handleUniqueConstraints(ctx context.Context, tx database.Transaction, comma deletePlaceholders := make([]string, 0) deleteArgs := make([]any, 0) - addPlaceholders := make([]string, 0) - addArgs := make([]any, 0) + addWithOwnersPlaceholders := make([]string, 0) + addWithOwnersArgs := make([]any, 0) + addWithoutOwnersPlaceholders := make([]string, 0) + addWithoutOwnersArgs := make([]any, 0) addConstraints := map[string]*eventstore.UniqueConstraint{} deleteConstraints := map[string]*eventstore.UniqueConstraint{} @@ -46,9 +50,14 @@ func handleUniqueConstraints(ctx context.Context, tx database.Transaction, comma switch constraint.Action { case eventstore.UniqueConstraintAdd: constraint.UniqueField = strings.ToLower(constraint.UniqueField) - addPlaceholders = append(addPlaceholders, fmt.Sprintf("($%d, $%d, $%d, $%d)", len(addArgs)+1, len(addArgs)+2, len(addArgs)+3, len(addArgs)+4)) - addArgs = append(addArgs, instanceID, constraint.UniqueType, constraint.UniqueField, constraint.Owners) addConstraints[fmt.Sprintf(uniqueConstraintPlaceholderFmt, instanceID, constraint.UniqueType, constraint.UniqueField)] = constraint + if len(constraint.Owners) == 0 { + addWithoutOwnersPlaceholders = append(addWithoutOwnersPlaceholders, fmt.Sprintf("($%d, $%d, $%d)", len(addWithoutOwnersArgs)+1, len(addWithoutOwnersArgs)+2, len(addWithoutOwnersArgs)+3)) + addWithoutOwnersArgs = append(addWithoutOwnersArgs, instanceID, constraint.UniqueType, constraint.UniqueField) + } else { + addWithOwnersPlaceholders = append(addWithOwnersPlaceholders, fmt.Sprintf("($%d, $%d, $%d, $%d)", len(addWithOwnersArgs)+1, len(addWithOwnersArgs)+2, len(addWithOwnersArgs)+3, len(addWithOwnersArgs)+4)) + addWithOwnersArgs = append(addWithOwnersArgs, instanceID, constraint.UniqueType, constraint.UniqueField, constraint.Owners) + } case eventstore.UniqueConstraintRemove: deletePlaceholders = append(deletePlaceholders, fmt.Sprintf(deleteConstraintPlaceholdersStmt, len(deleteArgs)+1, len(deleteArgs)+2, len(deleteArgs)+3)) deleteArgs = append(deleteArgs, instanceID, constraint.UniqueType, constraint.UniqueField) @@ -80,16 +89,24 @@ func handleUniqueConstraints(ctx context.Context, tx database.Transaction, comma return zerrors.ThrowInternal(err, "V3-C8l3V", errMessage) } } - if len(addPlaceholders) > 0 { - _, err := tx.Exec(ctx, fmt.Sprintf(addConstraintStmt, strings.Join(addPlaceholders, ", ")), addArgs...) - if err != nil { - logging.WithError(err).Warn("add unique constraint failed") - errMessage := "Errors.Internal" - if constraint := constraintFromErr(err, addConstraints); constraint != nil { - errMessage = constraint.ErrorMessage - } - return zerrors.ThrowAlreadyExists(err, "V3-DKcYh", errMessage) + if err := execAddConstraints(ctx, tx, addConstraintWithoutOwnersStmt, addWithoutOwnersPlaceholders, addWithoutOwnersArgs, addConstraints); err != nil { + return err + } + return execAddConstraints(ctx, tx, addConstraintStmt, addWithOwnersPlaceholders, addWithOwnersArgs, addConstraints) +} + +func execAddConstraints(ctx context.Context, tx database.Transaction, stmt string, placeholders []string, args []any, addConstraints map[string]*eventstore.UniqueConstraint) error { + if len(placeholders) == 0 { + return nil + } + _, err := tx.Exec(ctx, fmt.Sprintf(stmt, strings.Join(placeholders, ", ")), args...) + if err != nil { + logging.WithError(err).Warn("add unique constraint failed") + errMessage := "Errors.Internal" + if constraint := constraintFromErr(err, addConstraints); constraint != nil { + errMessage = constraint.ErrorMessage } + return zerrors.ThrowAlreadyExists(err, "V3-DKcYh", errMessage) } return nil } diff --git a/internal/eventstore/v3/unique_constraints_add_test.go b/internal/eventstore/v3/unique_constraints_add_test.go new file mode 100644 index 000000000..e6d973427 --- /dev/null +++ b/internal/eventstore/v3/unique_constraints_add_test.go @@ -0,0 +1,15 @@ +package eventstore + +import ( + "strings" + "testing" +) + +func TestAddConstraintStmts(t *testing.T) { + if strings.Contains(addConstraintWithoutOwnersStmt, "owners") { + t.Fatalf("without-owners insert must not name owners: %s", addConstraintWithoutOwnersStmt) + } + if !strings.Contains(addConstraintStmt, "owners") { + t.Fatalf("tagged insert must name owners: %s", addConstraintStmt) + } +} diff --git a/internal/eventstore/v3/unique_constraints_add_without_owners.sql b/internal/eventstore/v3/unique_constraints_add_without_owners.sql new file mode 100644 index 000000000..087b11cb7 --- /dev/null +++ b/internal/eventstore/v3/unique_constraints_add_without_owners.sql @@ -0,0 +1,6 @@ +INSERT INTO eventstore.unique_constraints ( + instance_id + , unique_type + , unique_field +) VALUES + %s diff --git a/internal/v2/eventstore/postgres/push.go b/internal/v2/eventstore/postgres/push.go index cb2027d53..154eeb2f5 100644 --- a/internal/v2/eventstore/postgres/push.go +++ b/internal/v2/eventstore/postgres/push.go @@ -216,15 +216,25 @@ func uniqueConstraints(ctx context.Context, tx *sql.Tx, commands []*command) (er } switch constraint.Action { case eventstore.UniqueConstraintAdd: - stmt.WriteString(`INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES (`) - stmt.WriteArg(instance) - stmt.WriteString(`, `) - stmt.WriteArg(constraint.UniqueType) - stmt.WriteString(`, `) - stmt.WriteArg(constraint.UniqueField) - stmt.WriteString(`, `) - stmt.WriteArg(constraint.Owners) - stmt.WriteString(`)`) + if len(constraint.Owners) == 0 { + stmt.WriteString(`INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES (`) + stmt.WriteArg(instance) + stmt.WriteString(`, `) + stmt.WriteArg(constraint.UniqueType) + stmt.WriteString(`, `) + stmt.WriteArg(constraint.UniqueField) + stmt.WriteString(`)`) + } else { + stmt.WriteString(`INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES (`) + stmt.WriteArg(instance) + stmt.WriteString(`, `) + stmt.WriteArg(constraint.UniqueType) + stmt.WriteString(`, `) + stmt.WriteArg(constraint.UniqueField) + stmt.WriteString(`, `) + stmt.WriteArg(constraint.Owners) + stmt.WriteString(`)`) + } case eventstore.UniqueConstraintInstanceRemove: stmt.WriteString(`DELETE FROM eventstore.unique_constraints WHERE instance_id = `) stmt.WriteArgs(instance) diff --git a/internal/v2/eventstore/postgres/push_test.go b/internal/v2/eventstore/postgres/push_test.go index f5960d4e9..19b9e9c85 100644 --- a/internal/v2/eventstore/postgres/push_test.go +++ b/internal/v2/eventstore/postgres/push_test.go @@ -64,10 +64,38 @@ func Test_uniqueConstraints(t *testing.T) { }, }, }, + expectations: []mock.Expectation{ + mock.ExpectExec( + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id"), + mock.WithExecRowsAffected(1), + ), + }, + }, + assertErr: expectNoErr, + }, + { + name: "add 1 constraint with owners", + args: args{ + commands: []*command{ + { + intent: &intent{ + PushAggregate: eventstore.NewPushIntent( + "instance", + eventstore.AppendAggregate("", "", ""), + ).Aggregates()[0], + }, + Command: &eventstore.Command{ + UniqueConstraints: []*eventstore.UniqueConstraint{ + eventstore.NewAddEventUniqueConstraint("test", "id", "error").WithOwners("org:org-1", "user:u1"), + }, + }, + }, + }, expectations: []mock.Expectation{ mock.ExpectExec( "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id", []string{}), + mock.WithExecArgs("instance", "test", "id", []string{"org:org-1", "user:u1"}), mock.WithExecRowsAffected(1), ), }, @@ -94,8 +122,8 @@ func Test_uniqueConstraints(t *testing.T) { }, expectations: []mock.Expectation{ mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("", "test", "id", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("", "test", "id"), mock.WithExecRowsAffected(1), ), }, @@ -123,13 +151,47 @@ func Test_uniqueConstraints(t *testing.T) { }, expectations: []mock.Expectation{ mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id"), mock.WithExecRowsAffected(1), ), + mock.ExpectExec( + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id2"), + mock.WithExecRowsAffected(1), + ), + }, + }, + assertErr: expectNoErr, + }, + { + name: "add tagged and empty owners 1 command", + args: args{ + commands: []*command{ + { + intent: &intent{ + PushAggregate: eventstore.NewPushIntent( + "instance", + eventstore.AppendAggregate("", "", ""), + ).Aggregates()[0], + }, + Command: &eventstore.Command{ + UniqueConstraints: []*eventstore.UniqueConstraint{ + eventstore.NewAddEventUniqueConstraint("test", "id", "error").WithOwners("org:org-1"), + eventstore.NewAddEventUniqueConstraint("test", "id2", "error"), + }, + }, + }, + }, + expectations: []mock.Expectation{ mock.ExpectExec( "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id2", []string{}), + mock.WithExecArgs("instance", "test", "id", []string{"org:org-1"}), + mock.WithExecRowsAffected(1), + ), + mock.ExpectExec( + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id2"), mock.WithExecRowsAffected(1), ), }, @@ -169,13 +231,13 @@ func Test_uniqueConstraints(t *testing.T) { }, expectations: []mock.Expectation{ mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id"), mock.WithExecRowsAffected(1), ), mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id2", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id2"), mock.WithExecRowsAffected(1), ), }, @@ -440,8 +502,8 @@ func Test_uniqueConstraints(t *testing.T) { }, expectations: []mock.Expectation{ mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id"), mock.WithExecErr(execErr), ), }, @@ -474,8 +536,8 @@ func Test_uniqueConstraints(t *testing.T) { }, expectations: []mock.Expectation{ mock.ExpectExec( - "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field, owners) VALUES ($1, $2, $3, $4)", - mock.WithExecArgs("instance", "test", "id", []string{}), + "INSERT INTO eventstore.unique_constraints (instance_id, unique_type, unique_field) VALUES ($1, $2, $3)", + mock.WithExecArgs("instance", "test", "id"), mock.WithExecErr(execErr), ), },