mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
fix(eventstore): omit owners on empty unique constraint inserts (#12785)
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> # 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 <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-31bfb5c4-5888-43f0-85cf-af1af097ca30?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-31bfb5c4-5888-43f0-85cf-af1af097ca30&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> --------- Co-authored-by: Cursor Agent <[email protected]> Co-authored-by: Silvan <[email protected]>
This commit is contained in:
Silvan
co-authored by
Cursor Agent
Silvan
Cursor Agent
Silvan
parent
4bc484dac9
commit
0a08e7269a
@@ -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
|
||||
|
||||
@@ -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,8 +89,17 @@ 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 := 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"
|
||||
@@ -90,7 +108,6 @@ func handleUniqueConstraints(ctx context.Context, tx database.Transaction, comma
|
||||
}
|
||||
return zerrors.ThrowAlreadyExists(err, "V3-DKcYh", errMessage)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,6 @@
|
||||
INSERT INTO eventstore.unique_constraints (
|
||||
instance_id
|
||||
, unique_type
|
||||
, unique_field
|
||||
) VALUES
|
||||
%s
|
||||
@@ -216,6 +216,15 @@ func uniqueConstraints(ctx context.Context, tx *sql.Tx, commands []*command) (er
|
||||
}
|
||||
switch constraint.Action {
|
||||
case eventstore.UniqueConstraintAdd:
|
||||
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(`, `)
|
||||
@@ -225,6 +234,7 @@ func uniqueConstraints(ctx context.Context, tx *sql.Tx, commands []*command) (er
|
||||
stmt.WriteString(`, `)
|
||||
stmt.WriteArg(constraint.Owners)
|
||||
stmt.WriteString(`)`)
|
||||
}
|
||||
case eventstore.UniqueConstraintInstanceRemove:
|
||||
stmt.WriteString(`DELETE FROM eventstore.unique_constraints WHERE instance_id = `)
|
||||
stmt.WriteArgs(instance)
|
||||
|
||||
@@ -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),
|
||||
),
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user