From 50512ec3a1d5814319d209d3d65e7ab2c07dd723 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Wed, 28 Oct 2026 21:00:13 +0100 Subject: [PATCH 1/5] fix(storage): stop re-running ALTER COLUMN on char(36) columns GORM's postgres driver reads these columns back from information_schema as `bpchar`, which never prefix-matches the declared `char(36)`, so AutoMigrate emits the same no-op type change on every boot after the first: ALTER TABLE "authorizer_sessions" ALTER COLUMN "user_id" TYPE char(36) USING "user_id"::char(36) On PostgreSQL that is a silent table rewrite under ACCESS EXCLUSIVE on four tables at every restart. On CockroachDB it is fatal: ALTER COLUMN TYPE is gated behind an experimental flag (SQLSTATE XCEXF), so the server starts once and never starts again. varchar(36) matches what the catalog reports, which is why none of the existing varchar columns churn. Values are 36-char uuids either way. Upgrade cost is one ALTER per column, on the first boot after this change, and none after. Verified against postgres 18 with log_statement=all: 5 alters on the upgrade boot, 0 on every boot after. --- internal/storage/schemas/audit_log.go | 4 ++-- internal/storage/schemas/authenticators.go | 2 +- internal/storage/schemas/session.go | 2 +- internal/storage/schemas/webhook_log.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/storage/schemas/audit_log.go b/internal/storage/schemas/audit_log.go index bd004c10f..e4d4c9c82 100644 --- a/internal/storage/schemas/audit_log.go +++ b/internal/storage/schemas/audit_log.go @@ -13,12 +13,12 @@ import ( type AuditLog struct { Key string `json:"_key,omitempty" bson:"_key,omitempty" cql:"_key,omitempty" dynamo:"key,omitempty"` // for arangodb ID string `gorm:"primaryKey;type:char(36)" json:"_id" bson:"_id" cql:"id" dynamo:"id,hash"` - ActorID string `gorm:"type:char(36)" json:"actor_id" bson:"actor_id" cql:"actor_id" dynamo:"actor_id,omitempty" index:"actor_id,hash"` + ActorID string `gorm:"type:varchar(36)" json:"actor_id" bson:"actor_id" cql:"actor_id" dynamo:"actor_id,omitempty" index:"actor_id,hash"` ActorType string `gorm:"type:varchar(30)" json:"actor_type" bson:"actor_type" cql:"actor_type" dynamo:"actor_type"` ActorEmail string `gorm:"type:varchar(256)" json:"actor_email" bson:"actor_email" cql:"actor_email" dynamo:"actor_email"` Action string `gorm:"type:varchar(100)" json:"action" bson:"action" cql:"action" dynamo:"action,omitempty" index:"action,hash"` ResourceType string `gorm:"type:varchar(50)" json:"resource_type" bson:"resource_type" cql:"resource_type" dynamo:"resource_type"` - ResourceID string `gorm:"type:char(36)" json:"resource_id" bson:"resource_id" cql:"resource_id" dynamo:"resource_id"` + ResourceID string `gorm:"type:varchar(36)" json:"resource_id" bson:"resource_id" cql:"resource_id" dynamo:"resource_id"` IPAddress string `gorm:"type:varchar(45)" json:"ip_address" bson:"ip_address" cql:"ip_address" dynamo:"ip_address"` UserAgent string `gorm:"type:text" json:"user_agent" bson:"user_agent" cql:"user_agent" dynamo:"user_agent"` Metadata string `gorm:"type:text" json:"metadata" bson:"metadata" cql:"metadata" dynamo:"metadata"` diff --git a/internal/storage/schemas/authenticators.go b/internal/storage/schemas/authenticators.go index d60314be2..f3e25788d 100644 --- a/internal/storage/schemas/authenticators.go +++ b/internal/storage/schemas/authenticators.go @@ -6,7 +6,7 @@ package schemas type Authenticator struct { Key string `json:"_key,omitempty" bson:"_key,omitempty" cql:"_key,omitempty" dynamo:"key,omitempty"` // for arangodb ID string `gorm:"primaryKey;type:char(36)" json:"_id" bson:"_id" cql:"id" dynamo:"id,hash"` - UserID string `gorm:"type:char(36);uniqueIndex:idx_authenticator_user_id_method" json:"user_id" bson:"user_id" cql:"user_id" dynamo:"user_id" index:"user_id,hash"` + UserID string `gorm:"type:varchar(36);uniqueIndex:idx_authenticator_user_id_method" json:"user_id" bson:"user_id" cql:"user_id" dynamo:"user_id" index:"user_id,hash"` Method string `gorm:"type:varchar(64);uniqueIndex:idx_authenticator_user_id_method" json:"method" bson:"method" cql:"method" dynamo:"method"` Secret string `json:"secret" bson:"secret" cql:"secret" dynamo:"secret"` RecoveryCodes *string `json:"recovery_codes" bson:"recovery_codes" cql:"recovery_codes" dynamo:"recovery_codes"` diff --git a/internal/storage/schemas/session.go b/internal/storage/schemas/session.go index 3400917f8..b0a49d891 100644 --- a/internal/storage/schemas/session.go +++ b/internal/storage/schemas/session.go @@ -6,7 +6,7 @@ package schemas type Session struct { Key string `json:"_key,omitempty" bson:"_key,omitempty" cql:"_key,omitempty" dynamo:"key,omitempty"` // for arangodb ID string `gorm:"primaryKey;type:char(36)" json:"_id" bson:"_id" cql:"id" dynamo:"id,hash"` - UserID string `gorm:"type:char(36)" json:"user_id" bson:"user_id" cql:"user_id" dynamo:"user_id" index:"user_id,hash"` + UserID string `gorm:"type:varchar(36)" json:"user_id" bson:"user_id" cql:"user_id" dynamo:"user_id" index:"user_id,hash"` UserAgent string `json:"user_agent" bson:"user_agent" cql:"user_agent" dynamo:"user_agent"` IP string `json:"ip" bson:"ip" cql:"ip" dynamo:"ip"` CreatedAt int64 `json:"created_at" bson:"created_at" cql:"created_at" dynamo:"created_at"` diff --git a/internal/storage/schemas/webhook_log.go b/internal/storage/schemas/webhook_log.go index 97e90eb04..a35cee736 100644 --- a/internal/storage/schemas/webhook_log.go +++ b/internal/storage/schemas/webhook_log.go @@ -16,7 +16,7 @@ type WebhookLog struct { HttpStatus int64 `json:"http_status" bson:"http_status" cql:"http_status" dynamo:"http_status"` Response string `json:"response" bson:"response" cql:"response" dynamo:"response"` Request string `json:"request" bson:"request" cql:"request" dynamo:"request"` - WebhookID string `gorm:"type:char(36)" json:"webhook_id" bson:"webhook_id" cql:"webhook_id" dynamo:"webhook_id" index:"webhook_id,hash"` + WebhookID string `gorm:"type:varchar(36)" json:"webhook_id" bson:"webhook_id" cql:"webhook_id" dynamo:"webhook_id" index:"webhook_id,hash"` CreatedAt int64 `json:"created_at" bson:"created_at" cql:"created_at" dynamo:"created_at"` UpdatedAt int64 `json:"updated_at" bson:"updated_at" cql:"updated_at" dynamo:"updated_at"` } From 67b650284363d526ae483e28b671a3c1ab5117d0 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Wed, 28 Oct 2026 21:00:24 +0100 Subject: [PATCH 2/5] fix(storage): make cockroachdb survive AutoMigrate, and test it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CockroachDB has been in the supported list and routed to the postgres driver, but nothing ran the storage suite against it. It aborted on startup, so the server came up once on a fresh database and never again. Two independent causes, both in AutoMigrate: - Cockroach lists UNIQUE *indexes* in information_schema .table_constraints; postgres lists only real constraints. GORM reads that row as a stale `unique` tag on every `uniqueIndex` field and issues DROP CONSTRAINT "uni__" — a NamingStrategy name that never exists — so startup dies on SQLSTATE 42704. - `ALTER TABLE ... DROP CONSTRAINT` is unimplemented there for UNIQUE (SQLSTATE 0A000), so the v1 legacy-uniqueness cleanup silently did nothing. That leaves UNIQUE on authorizer_otps.phone_number, where v2 writes '' for every email OTP and the second one is rejected. A cockroach-only dialector skips the bogus drop and retries the real one as DROP INDEX ... CASCADE. Postgres and yugabyte keep the plain driver. `make test-cockroachdb` runs the storage suite against a single-node container, and cockroachdb joins test-all-db and the SQL migration tests. Verified green on a fresh database with cockroach v24.3.13; docker pull was unavailable here, so the make target itself is only make -n validated. --- AGENTS.md | 5 +- Makefile | 21 +++++- internal/storage/db/sql/cockroachdb.go | 75 +++++++++++++++++++ internal/storage/db/sql/provider.go | 16 +++- .../storage/db/sql/provider_migration_test.go | 41 +++++++--- internal/storage/provider_test.go | 6 ++ scripts/wait-for-test-dbs.sh | 2 +- 7 files changed, 148 insertions(+), 18 deletions(-) create mode 100644 internal/storage/db/sql/cockroachdb.go diff --git a/AGENTS.md b/AGENTS.md index cc63e569b..4f0f1cdf2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,11 +71,12 @@ Run `make fmt` before committing; CI runs `make lint`. ```bash make test # Full module test run; TEST_DBS=sqlite (integration tests always SQLite) make test-sqlite # Same as test — explicit SQLite-only, no Docker -make test-all-db # All 8 DBs via Docker (postgres, sqlite, mongodb, arangodb, scylladb, cassandradb, dynamodb, couchbase) +make test-all-db # All 9 DBs via Docker (postgres, cockroachdb, sqlite, mongodb, arangodb, scylladb, cassandradb, dynamodb, couchbase) make smoke # Release e2e smoke tests (build tag `smoke`, 5m timeout) # Single-DB targets (each spins up Docker, runs tests, tears down) make test-postgres +make test-cockroachdb make test-mongodb make test-scylladb make test-cassandradb @@ -86,7 +87,7 @@ make test-couchbase # Docker helpers for test-all-db make test-docker-up # Start all test DB containers + Redis make test-cleanup # Remove all test containers -make test-cleanup-postgres | test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb +make test-cleanup-postgres | test-cleanup-cockroachdb | test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb make test-cleanup-arangodb | test-cleanup-dynamodb | test-cleanup-couchbase ``` diff --git a/Makefile b/Makefile index 621b878fa..6e8d2ea8b 100644 --- a/Makefile +++ b/Makefile @@ -7,6 +7,8 @@ DOCKER_IMAGE ?= quay.io/authorizer/authorizer:$(VERSION) # Integration tests and memory_store/db tests always use SQLite. # Redis memory_store tests run only when TEST_ENABLE_REDIS=1. GO_TEST_ALL := go test -p 1 -v ./... +# Pinned to a release line so the suite does not move under us. +COCKROACH_IMAGE := cockroachdb/cockroach:latest-v24.3 .PHONY: all bootstrap build build-app build-dashboard build-local-image build-push-image trivy-scan @@ -123,6 +125,17 @@ test-postgres: test-cleanup-postgres docker rm -vf authorizer_postgres; \ exit $$status +# CockroachDB is Postgres-wire compatible and runs through the same GORM SQL +# provider, so this target exists to prove that compatibility actually holds +# (AutoMigrate, transactions, LIKE anchoring) rather than being assumed. +test-cockroachdb: test-cleanup-cockroachdb + docker run -d --name authorizer_cockroachdb -p 26257:26257 $(COCKROACH_IMAGE) start-single-node --insecure + sh scripts/wait-for-test-dbs.sh cockroachdb && \ + { go clean --testcache; TEST_DBS="cockroachdb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_cockroachdb; \ + exit $$status + test-sqlite: go clean --testcache && TEST_DBS="sqlite" $(GO_TEST_ALL) @@ -224,7 +237,7 @@ test-all-db: test-cleanup test-docker-up @# the && chain a probe failure is captured by status and still cleans up. sh scripts/wait-for-cql.sh authorizer_scylla_db && \ sh scripts/wait-for-cql.sh authorizer_cassandra_db && \ - { go clean --testcache; TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,cassandradb,dynamodb" $(GO_TEST_ALL); }; \ + { go clean --testcache; TEST_DBS="couchbase,postgres,cockroachdb,sqlite,mongodb,arangodb,scylladb,cassandradb,dynamodb" $(GO_TEST_ALL); }; \ status=$$?; \ $(MAKE) test-cleanup; \ exit $$status @@ -233,6 +246,9 @@ test-all-db: test-cleanup test-docker-up test-docker-up: docker run -d --name authorizer_redis -p 6380:6379 redis docker run -d --name authorizer_postgres -p 5434:5432 -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=postgres postgres + @# Only 26257 (SQL) is published. Cockroach's admin UI is on 8080, which is + @# the port `make dev` binds. + docker run -d --name authorizer_cockroachdb -p 26257:26257 $(COCKROACH_IMAGE) start-single-node --insecure docker run -d --name authorizer_mongodb_db -p 27017:27017 mongo:4.4.15 docker run -d --name authorizer_scylla_db -p 9042:9042 scylladb/scylla docker run -d --name authorizer_cassandra_db -p 9142:9042 -e MAX_HEAP_SIZE=512M -e HEAP_NEWSIZE=128M cassandra:4.1 @@ -256,6 +272,7 @@ test-docker-up: # Remove all test database containers test-cleanup: -docker rm -vf authorizer_postgres + -docker rm -vf authorizer_cockroachdb -docker rm -vf authorizer_scylla_db -docker rm -vf authorizer_cassandra_db -docker rm -vf authorizer_mongodb_db @@ -266,6 +283,8 @@ test-cleanup: test-cleanup-postgres: -docker rm -vf authorizer_postgres +test-cleanup-cockroachdb: + -docker rm -vf authorizer_cockroachdb test-cleanup-mongodb: -docker rm -vf authorizer_mongodb_db test-cleanup-scylladb: diff --git a/internal/storage/db/sql/cockroachdb.go b/internal/storage/db/sql/cockroachdb.go new file mode 100644 index 000000000..bff3a4d38 --- /dev/null +++ b/internal/storage/db/sql/cockroachdb.go @@ -0,0 +1,75 @@ +package sql + +import ( + "fmt" + + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/schema" +) + +// cockroachDialector is the Postgres dialector with one migration behaviour +// changed. It embeds postgres.Dialector rather than the gorm.Dialector +// interface so the driver's optional interfaces — SavePoint/RollbackTo, which +// gorm type-asserts for nested transactions — stay promoted. +type cockroachDialector struct { + postgres.Dialector +} + +// newCockroachDialector returns the Postgres dialector for dbURL with the +// CockroachDB migrator behaviour layered on. +func newCockroachDialector(dbURL string) gorm.Dialector { + return cockroachDialector{postgres.Dialector{Config: &postgres.Config{DSN: dbURL}}} +} + +func (d cockroachDialector) Migrator(db *gorm.DB) gorm.Migrator { + return cockroachMigrator{d.Dialector.Migrator(db)} +} + +type cockroachMigrator struct { + gorm.Migrator +} + +// MigrateColumnUnique keeps AutoMigrate from dropping our unique indexes on +// CockroachDB. +// +// CockroachDB lists UNIQUE *indexes* in information_schema.table_constraints; +// PostgreSQL lists only real UNIQUE constraints there. GORM reads that extra +// row as "this column has a UNIQUE constraint the model no longer asks for" for +// every `gorm:"uniqueIndex"` field, and issues +// DROP CONSTRAINT "uni_
_" — a NamingStrategy-derived name that +// never exists, because the object is an index with its own name. AutoMigrate +// then aborts with SQLSTATE 42704 and the server never starts: +// +// constraint "uni_authorizer_oauth_states_state_key" of relation +// "authorizer_oauth_states" does not exist (SQLSTATE 42704) +// +// Those indexes are declared by the models and are meant to be there, so the +// drop is skipped. The create direction (a `gorm:"unique"` field whose +// constraint is genuinely missing) is delegated untouched. +func (m cockroachMigrator) MigrateColumnUnique(value any, field *schema.Field, columnType gorm.ColumnType) error { + if unique, ok := columnType.Unique(); ok && unique && !field.Unique && !field.PrimaryKey { + return nil + } + return m.Migrator.MigrateColumnUnique(value, field, columnType) +} + +// dropUniqueConstraint removes a single-column UNIQUE constraint on +// CockroachDB, where `ALTER TABLE ... DROP CONSTRAINT` — what +// Migrator.DropConstraint emits — is rejected outright: +// +// unimplemented: cannot drop UNIQUE constraint "authorizer_users_email_key" +// using ALTER TABLE DROP CONSTRAINT, use DROP INDEX CASCADE instead +// (SQLSTATE 0A000) +// +// A UNIQUE constraint there is an index, so it has to go in those terms. +// Without this the legacy-uniqueness cleanup silently does nothing on an +// upgraded v1 database, which leaves UNIQUE on authorizer_otps.phone_number — +// where v2 writes the empty string for every email OTP, so the second one +// would be rejected. +// +// The identifiers are internal: table comes from a fixed list and constraint +// from this database's own information_schema, never from a request. +func dropUniqueConstraint(db *gorm.DB, table, constraint string) error { + return db.Exec(fmt.Sprintf(`DROP INDEX IF EXISTS %q@%q CASCADE`, table, constraint)).Error +} diff --git a/internal/storage/db/sql/provider.go b/internal/storage/db/sql/provider.go index 612fd3cff..8ff209389 100644 --- a/internal/storage/db/sql/provider.go +++ b/internal/storage/db/sql/provider.go @@ -63,8 +63,12 @@ func NewProvider( dbURL := config.DatabaseURL switch dbType { - case constants.DbTypePostgres, constants.DbTypeYugabyte, constants.DbTypeCockroachDB: + case constants.DbTypePostgres, constants.DbTypeYugabyte: sqlDB, err = gorm.Open(postgres.Open(dbURL), ormConfig) + case constants.DbTypeCockroachDB: + // Postgres wire protocol, but AutoMigrate needs one behaviour changed — + // see cockroachdb.go. + sqlDB, err = gorm.Open(newCockroachDialector(dbURL), ormConfig) case constants.DbTypeSqlite: sqlDB, err = gorm.Open(sqlite.Open(dbURL+"?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)"), ormConfig) case constants.DbTypeLibSQL: @@ -93,7 +97,7 @@ func NewProvider( // or any custom name) — failing with "constraint does not exist" (Postgres // SQLSTATE 42704) and aborting startup. Clear the legacy uniqueness up front, // name-agnostically, before AutoMigrate runs. - clearLegacyColumnUniqueness(sqlDB, deps.Log) + clearLegacyColumnUniqueness(sqlDB, dbType, deps.Log) err = sqlDB.AutoMigrate(&schemas.User{}, &schemas.VerificationRequest{}, &schemas.Session{}, &schemas.Env{}, &schemas.Webhook{}, &schemas.WebhookLog{}, &schemas.EmailTemplate{}, &schemas.OTP{}, &schemas.Authenticator{}, &schemas.SessionToken{}, &schemas.MFASession{}, &schemas.OAuthState{}, &schemas.AuditLog{}, &schemas.Client{}, &schemas.TrustedIssuer{}, &schemas.Organization{}, &schemas.OrgMembership{}, &schemas.FederatedIdentity{}, &schemas.ScimEndpoint{}, &schemas.ScimGroup{}, &schemas.WebauthnCredential{}, &schemas.OrgDomain{}, &schemas.SAMLServiceProvider{}, &schemas.SAMLIDPKey{}) if err != nil { @@ -151,7 +155,7 @@ func (p *provider) Close() error { // why this must run before AutoMigrate. Best-effort and non-fatal: fresh // installs, and catalogs without information_schema (sqlite), simply find // nothing to drop. -func clearLegacyColumnUniqueness(db *gorm.DB, log *zerolog.Logger) { +func clearLegacyColumnUniqueness(db *gorm.DB, dbType string, log *zerolog.Logger) { // Run through a discarding GORM logger. The information_schema probe below // intentionally errors on databases without that catalog (sqlite); GORM's // default logger writes such errors to os.Stdout, which corrupts the MCP @@ -197,7 +201,11 @@ func clearLegacyColumnUniqueness(db *gorm.DB, log *zerolog.Logger) { } for name, cols := range columnsByConstraint { if len(cols) == 1 && targets[cols[0]] { - if err := db.Migrator().DropConstraint(t.model, name); err != nil { + err := db.Migrator().DropConstraint(t.model, name) + if err != nil && dbType == constants.DbTypeCockroachDB { + err = dropUniqueConstraint(db, t.name, name) + } + if err != nil { log.Debug().Err(err).Str("constraint", name).Msg("failed to drop legacy unique constraint") } } diff --git a/internal/storage/db/sql/provider_migration_test.go b/internal/storage/db/sql/provider_migration_test.go index bfc621d67..d22fe6ca5 100644 --- a/internal/storage/db/sql/provider_migration_test.go +++ b/internal/storage/db/sql/provider_migration_test.go @@ -2,6 +2,7 @@ package sql import ( "context" + "fmt" "net" "os" "path/filepath" @@ -31,10 +32,11 @@ func sqlTestDeps(t *testing.T) *Dependencies { // TEST_DBS=postgres to cover the engine where the original failure was reported. func sqlMigrationTestDBTypes() []string { supported := map[string]bool{ - constants.DbTypePostgres: true, - constants.DbTypeSqlite: true, - constants.DbTypeMysql: true, - constants.DbTypeMariaDB: true, + constants.DbTypePostgres: true, + constants.DbTypeCockroachDB: true, + constants.DbTypeSqlite: true, + constants.DbTypeMysql: true, + constants.DbTypeMariaDB: true, } env := os.Getenv("TEST_DBS") if env == "" { @@ -65,6 +67,9 @@ func sqlMigrationTestConfig(t *testing.T, dbType string) *config.Config { case constants.DbTypePostgres: cfg.DatabaseURL = "postgres://postgres:postgres@localhost:5434/postgres" skipIfTCPClosed(t, "localhost:5434") + case constants.DbTypeCockroachDB: + cfg.DatabaseURL = "postgres://root@localhost:26257/defaultdb?sslmode=disable" + skipIfTCPClosed(t, "localhost:26257") case constants.DbTypeMysql, constants.DbTypeMariaDB: cfg.DatabaseURL = "root:password@tcp(localhost:3306)/authorizer_test" skipIfTCPClosed(t, "localhost:3306") @@ -175,7 +180,7 @@ func TestProviderEmailPhoneUpdatesAndUniqueness(t *testing.T) { // DDL; sqlite rebuilds tables and has no comparable failure mode. func TestStaleUniqueConstraintMigration(t *testing.T) { for _, dbType := range sqlMigrationTestDBTypes() { - if dbType != constants.DbTypePostgres { + if dbType != constants.DbTypePostgres && dbType != constants.DbTypeCockroachDB { continue } t.Run(dbType, func(t *testing.T) { @@ -199,15 +204,31 @@ func TestStaleUniqueConstraintMigration(t *testing.T) { // be dropped with DROP INDEX — field-reported case) // - otps.email: standalone UNIQUE INDEX "idx_authorizer_otps_email" // Idempotent so reruns against the shared test DB don't clash. + // CockroachDB rejects ALTER TABLE ... DROP CONSTRAINT for a UNIQUE + // constraint (SQLSTATE 0A000) because there it *is* an index, so the + // teardown half of each pair is written in the dialect's own terms — + // the same split the provider's cleanup has to make. + drop := func(table, name string) string { + if dbType == constants.DbTypeCockroachDB { + return fmt.Sprintf("DROP INDEX IF EXISTS %s@%s CASCADE", table, name) + } + return fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT IF EXISTS %s", table, name) + } + dropIndex := func(table, name string) string { + if dbType == constants.DbTypeCockroachDB { + return fmt.Sprintf("DROP INDEX IF EXISTS %s@%s CASCADE", table, name) + } + return fmt.Sprintf("DROP INDEX IF EXISTS %s", name) + } for _, stmt := range []string{ - `ALTER TABLE authorizer_users DROP CONSTRAINT IF EXISTS authorizer_users_email_key`, + drop("authorizer_users", "authorizer_users_email_key"), `ALTER TABLE authorizer_users ADD CONSTRAINT authorizer_users_email_key UNIQUE (email)`, - `ALTER TABLE authorizer_users DROP CONSTRAINT IF EXISTS my_legacy_phone_uq`, + drop("authorizer_users", "my_legacy_phone_uq"), `ALTER TABLE authorizer_users ADD CONSTRAINT my_legacy_phone_uq UNIQUE (phone_number)`, - `ALTER TABLE authorizer_otps DROP CONSTRAINT IF EXISTS idx_authorizer_otps_phone_number`, - `DROP INDEX IF EXISTS idx_authorizer_otps_phone_number`, + drop("authorizer_otps", "idx_authorizer_otps_phone_number"), + dropIndex("authorizer_otps", "idx_authorizer_otps_phone_number"), `ALTER TABLE authorizer_otps ADD CONSTRAINT idx_authorizer_otps_phone_number UNIQUE (phone_number)`, - `DROP INDEX IF EXISTS idx_authorizer_otps_email`, + dropIndex("authorizer_otps", "idx_authorizer_otps_email"), `CREATE UNIQUE INDEX idx_authorizer_otps_email ON authorizer_otps (email)`, } { require.NoError(t, p1.db.Exec(stmt).Error, stmt) diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index 83f098d08..b92bb02b7 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -31,6 +31,7 @@ var allDBTypes = []string{ constants.DbTypeCassandraDB, constants.DbTypeCouchbaseDB, constants.DbTypeDynamoDB, + constants.DbTypeCockroachDB, } // getTestDBTypes returns the list of database types to test against. @@ -63,6 +64,11 @@ func getTestDBConfig(dbType string) *config.Config { switch dbType { case constants.DbTypePostgres: cfg.DatabaseURL = "postgres://postgres:postgres@localhost:5434/postgres" + case constants.DbTypeCockroachDB: + // CockroachDB speaks the Postgres wire protocol and runs through the same + // GORM provider; a separate port keeps it from being mistaken for (or + // colliding with) the Postgres container. + cfg.DatabaseURL = "postgres://root@localhost:26257/defaultdb?sslmode=disable" case constants.DbTypeSqlite: cfg.DatabaseURL = "test.db" case constants.DbTypeMongoDB: diff --git a/scripts/wait-for-test-dbs.sh b/scripts/wait-for-test-dbs.sh index 7e7f99fbc..b80d01f23 100755 --- a/scripts/wait-for-test-dbs.sh +++ b/scripts/wait-for-test-dbs.sh @@ -15,7 +15,7 @@ TIMEOUT_SECONDS="${TEST_DB_WAIT_TIMEOUT:-120}" # name:port pairs. Couchbase is absent on purpose — scripts/couchbase-test.sh # already provisions and waits for it. -ALL_SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 cassandradb:9142 arangodb:8529 dynamodb:8000" +ALL_SERVICES="redis:6380 postgres:5434 cockroachdb:26257 mongodb:27017 scylladb:9042 cassandradb:9142 arangodb:8529 dynamodb:8000" # With no arguments, wait for everything (make test-all-db). With arguments, # wait only for the named services, so a single-backend target does not block on From 4245d90063862c872fca787c8c9733acac87fb34 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Thu, 29 Oct 2026 06:54:04 +0100 Subject: [PATCH 3/5] test(storage): make the user-cascade test rerunnable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It used a fixed email and org id per backend and never deleted the user it re-provisions at the end, so a second run against a database that still holds the first run's rows failed at AddUser — on every SQL backend, not just one. Nobody hit it because the make targets always start a fresh container, but it makes a developer's local database a one-shot and turns an unrelated earlier failure into a confusing "user with given email already exists" on the next run. Fixtures carry a uuid and the re-provisioned user is deleted through the cascade the test just proved. --- internal/storage/db/sql/user_cascade_test.go | 23 +++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/storage/db/sql/user_cascade_test.go b/internal/storage/db/sql/user_cascade_test.go index 2e0118615..c98f20f3a 100644 --- a/internal/storage/db/sql/user_cascade_test.go +++ b/internal/storage/db/sql/user_cascade_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gorm.io/gorm" @@ -45,15 +46,21 @@ func TestDeleteUserCascadesAllUserOwnedRows(t *testing.T) { defer func() { _ = p.Close() }() ctx := context.Background() + + // Everything the test writes is suffixed so a rerun against a + // persistent database (make test-postgres reuses its container only + // within one run, but a developer's local DB does not) starts clean + // instead of colliding with the previous run's fixtures. + uniq := uuid.New().String() user, err := p.AddUser(ctx, &schemas.User{ - Email: refs.NewStringRef("cascade-" + dbType + "@example.com"), + Email: refs.NewStringRef("cascade-" + dbType + "-" + uniq + "@example.com"), SignupMethods: "basic_auth", }) require.NoError(t, err) - const orgID = "org-cascade" - const issuer = "https://idp.example.com" - const subject = "upstream-subject-1" + orgID := "org-cascade-" + uniq + issuer := "https://idp.example.com" + subject := "upstream-subject-" + uniq require.NoError(t, p.AddSession(ctx, &schemas.Session{UserID: user.ID})) _, err = p.AddFederatedIdentity(ctx, &schemas.FederatedIdentity{ @@ -65,7 +72,7 @@ func TestDeleteUserCascadesAllUserOwnedRows(t *testing.T) { _, err = p.AddAuthenticator(ctx, &schemas.Authenticator{UserID: user.ID, Method: "totp", Secret: "s3cret"}) require.NoError(t, err) _, err = p.AddWebauthnCredential(ctx, &schemas.WebauthnCredential{ - UserID: user.ID, CredentialID: "cred-" + dbType, PublicKey: "pk", Name: "laptop", + UserID: user.ID, CredentialID: "cred-" + dbType + "-" + uniq, PublicKey: "pk", Name: "laptop", }) require.NoError(t, err) require.NoError(t, p.AddSessionToken(ctx, &schemas.SessionToken{UserID: user.ID, KeyName: "access", Token: "t"})) @@ -87,7 +94,7 @@ func TestDeleteUserCascadesAllUserOwnedRows(t *testing.T) { _, err = p.GetFederatedIdentity(ctx, orgID, issuer, subject) require.Error(t, err, "federated identity must be gone after the user is deleted") fresh, err := p.AddUser(ctx, &schemas.User{ - Email: refs.NewStringRef("cascade-reprovision-" + dbType + "@example.com"), + Email: refs.NewStringRef("cascade-reprovision-" + dbType + "-" + uniq + "@example.com"), SignupMethods: "basic_auth", }) require.NoError(t, err) @@ -95,6 +102,10 @@ func TestDeleteUserCascadesAllUserOwnedRows(t *testing.T) { OrgID: orgID, Issuer: issuer, Subject: subject, UserID: fresh.ID, }) require.NoError(t, err, "re-provisioning the same (org, issuer, subject) must succeed") + + // Leave nothing behind: the cascade just proven above is what clears + // the re-provisioned identity too. + require.NoError(t, p.DeleteUser(ctx, fresh)) }) } } From 4ed82b7a2a890856649165971a762cbd52711c71 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Thu, 29 Oct 2026 06:54:22 +0100 Subject: [PATCH 4/5] fix(storage): make mysql and mariadb work with gorm automigrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MySQL and MariaDB are in the supported list, but nothing ever ran the storage suite against them and they could not create their schema at all, let alone serve a request. Three defects, each verified fixed against mysql:8.0.40 and mariadb:11: - An unsized string column under `gorm:"uniqueIndex"` became `longtext`, which MySQL refuses to index: *Error 1170 (42000): BLOB/TEXT column 'client_id' used in key specification without a key length*, so CREATE TABLE authorizer_clients failed on a brand new database. The driver already widens such a column to varchar(191) for `index` and `unique`, but checks only those two tags. The dialector now applies the same width to `uniqueIndex` — the 10 affected columns all live in tables that do not exist in v1, so nothing is narrowed under existing data, and three consecutive boots emit zero ALTER TABLE. - MySQL reports COLUMN_KEY='UNI' for unique *indexes*, so AutoMigrate read every `uniqueIndex` field as a stale `unique` tag and tried to drop a constraint name that was never created: *Error 1091: Can't DROP 'uni_authorizer_oauth_states_state_key'*. Same root cause as the CockroachDB abort, so the skip is now shared. - `ESCAPE '\'` is a syntax error on MySQL — backslash escapes inside string literals, so the literal is unterminated (Error 1064) — and both session-token revocation statements carried it. Revocation therefore failed outright rather than revoking. The LIKE escape character is now `!`, which needs no quoting on any engine; the cross-backend anchored-delete test gained a metacharacter case that fails if the escape character itself stops being escaped. Also fixes the CockroachDB wrapper committed earlier: it embedded the gorm.Migrator *interface*, which drops the driver methods gorm type-asserts without an ok-check. That panicked outright on MySQL (`BuildIndexOptions`) and was latent on CockroachDB. Both wrappers now embed the driver's concrete migrator, so every method survives. Adds `make test-mysql` / `make test-mariadb` on separate ports, pulls both plus libsql into test-all-db, and replaces the port probe with scripts/wait-for-sql.sh for the engines whose port opens before they serve queries (MySQL binds 3306, runs its init and then restarts; CockroachDB accepts TCP before it accepts clients). --- AGENTS.md | 7 +- Makefile | 36 ++++++++- internal/storage/db/sql/cockroachdb.go | 28 ++----- internal/storage/db/sql/migrator.go | 39 +++++++++ internal/storage/db/sql/mysql.go | 81 +++++++++++++++++++ internal/storage/db/sql/provider.go | 5 +- .../storage/db/sql/provider_migration_test.go | 5 +- internal/storage/db/sql/session_token.go | 22 ++++- internal/storage/provider_test.go | 35 ++++++++ scripts/wait-for-sql.sh | 76 +++++++++++++++++ scripts/wait-for-test-dbs.sh | 6 +- 11 files changed, 307 insertions(+), 33 deletions(-) create mode 100644 internal/storage/db/sql/migrator.go create mode 100644 internal/storage/db/sql/mysql.go create mode 100755 scripts/wait-for-sql.sh diff --git a/AGENTS.md b/AGENTS.md index 4f0f1cdf2..05e7373a8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,12 +71,14 @@ Run `make fmt` before committing; CI runs `make lint`. ```bash make test # Full module test run; TEST_DBS=sqlite (integration tests always SQLite) make test-sqlite # Same as test — explicit SQLite-only, no Docker -make test-all-db # All 9 DBs via Docker (postgres, cockroachdb, sqlite, mongodb, arangodb, scylladb, cassandradb, dynamodb, couchbase) +make test-all-db # All 12 DBs via Docker (postgres, cockroachdb, mysql, mariadb, sqlite, libsql, mongodb, arangodb, scylladb, cassandradb, dynamodb, couchbase) make smoke # Release e2e smoke tests (build tag `smoke`, 5m timeout) # Single-DB targets (each spins up Docker, runs tests, tears down) make test-postgres make test-cockroachdb +make test-mysql +make test-mariadb make test-mongodb make test-scylladb make test-cassandradb @@ -87,7 +89,8 @@ make test-couchbase # Docker helpers for test-all-db make test-docker-up # Start all test DB containers + Redis make test-cleanup # Remove all test containers -make test-cleanup-postgres | test-cleanup-cockroachdb | test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb +make test-cleanup-postgres | test-cleanup-cockroachdb | test-cleanup-mysql | test-cleanup-mariadb +make test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb make test-cleanup-arangodb | test-cleanup-dynamodb | test-cleanup-couchbase ``` diff --git a/Makefile b/Makefile index 6e8d2ea8b..792920722 100644 --- a/Makefile +++ b/Makefile @@ -9,6 +9,8 @@ DOCKER_IMAGE ?= quay.io/authorizer/authorizer:$(VERSION) GO_TEST_ALL := go test -p 1 -v ./... # Pinned to a release line so the suite does not move under us. COCKROACH_IMAGE := cockroachdb/cockroach:latest-v24.3 +MYSQL_IMAGE := mysql:8.0.40 +MARIADB_IMAGE := mariadb:11 .PHONY: all bootstrap build build-app build-dashboard build-local-image build-push-image trivy-scan @@ -130,12 +132,28 @@ test-postgres: test-cleanup-postgres # (AutoMigrate, transactions, LIKE anchoring) rather than being assumed. test-cockroachdb: test-cleanup-cockroachdb docker run -d --name authorizer_cockroachdb -p 26257:26257 $(COCKROACH_IMAGE) start-single-node --insecure - sh scripts/wait-for-test-dbs.sh cockroachdb && \ + sh scripts/wait-for-sql.sh authorizer_cockroachdb cockroachdb && \ { go clean --testcache; TEST_DBS="cockroachdb" $(GO_TEST_ALL); }; \ status=$$?; \ docker rm -vf authorizer_cockroachdb; \ exit $$status +test-mysql: test-cleanup-mysql + docker run -d --name authorizer_mysql -p 3306:3306 -e MYSQL_ROOT_PASSWORD=password -e MYSQL_DATABASE=authorizer_test $(MYSQL_IMAGE) + sh scripts/wait-for-sql.sh authorizer_mysql mysql && \ + { go clean --testcache; TEST_DBS="mysql" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_mysql; \ + exit $$status + +test-mariadb: test-cleanup-mariadb + docker run -d --name authorizer_mariadb -p 3307:3306 -e MARIADB_ROOT_PASSWORD=password -e MARIADB_DATABASE=authorizer_test $(MARIADB_IMAGE) + sh scripts/wait-for-sql.sh authorizer_mariadb mariadb && \ + { go clean --testcache; TEST_DBS="mariadb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_mariadb; \ + exit $$status + test-sqlite: go clean --testcache && TEST_DBS="sqlite" $(GO_TEST_ALL) @@ -237,7 +255,10 @@ test-all-db: test-cleanup test-docker-up @# the && chain a probe failure is captured by status and still cleans up. sh scripts/wait-for-cql.sh authorizer_scylla_db && \ sh scripts/wait-for-cql.sh authorizer_cassandra_db && \ - { go clean --testcache; TEST_DBS="couchbase,postgres,cockroachdb,sqlite,mongodb,arangodb,scylladb,cassandradb,dynamodb" $(GO_TEST_ALL); }; \ + sh scripts/wait-for-sql.sh authorizer_mysql mysql && \ + sh scripts/wait-for-sql.sh authorizer_mariadb mariadb && \ + sh scripts/wait-for-sql.sh authorizer_cockroachdb cockroachdb && \ + { go clean --testcache; TEST_DBS="couchbase,postgres,cockroachdb,mysql,mariadb,sqlite,libsql,mongodb,arangodb,scylladb,cassandradb,dynamodb" $(GO_TEST_ALL); }; \ status=$$?; \ $(MAKE) test-cleanup; \ exit $$status @@ -249,6 +270,8 @@ test-docker-up: @# Only 26257 (SQL) is published. Cockroach's admin UI is on 8080, which is @# the port `make dev` binds. docker run -d --name authorizer_cockroachdb -p 26257:26257 $(COCKROACH_IMAGE) start-single-node --insecure + docker run -d --name authorizer_mysql -p 3306:3306 -e MYSQL_ROOT_PASSWORD=password -e MYSQL_DATABASE=authorizer_test $(MYSQL_IMAGE) + docker run -d --name authorizer_mariadb -p 3307:3306 -e MARIADB_ROOT_PASSWORD=password -e MARIADB_DATABASE=authorizer_test $(MARIADB_IMAGE) docker run -d --name authorizer_mongodb_db -p 27017:27017 mongo:4.4.15 docker run -d --name authorizer_scylla_db -p 9042:9042 scylladb/scylla docker run -d --name authorizer_cassandra_db -p 9142:9042 -e MAX_HEAP_SIZE=512M -e HEAP_NEWSIZE=128M cassandra:4.1 @@ -268,11 +291,16 @@ test-docker-up: @# there costs nothing: the probe returns in 0s once CQL answers. -sh scripts/wait-for-cql.sh authorizer_scylla_db -sh scripts/wait-for-cql.sh authorizer_cassandra_db + -sh scripts/wait-for-sql.sh authorizer_mysql mysql + -sh scripts/wait-for-sql.sh authorizer_mariadb mariadb + -sh scripts/wait-for-sql.sh authorizer_cockroachdb cockroachdb # Remove all test database containers test-cleanup: -docker rm -vf authorizer_postgres -docker rm -vf authorizer_cockroachdb + -docker rm -vf authorizer_mysql + -docker rm -vf authorizer_mariadb -docker rm -vf authorizer_scylla_db -docker rm -vf authorizer_cassandra_db -docker rm -vf authorizer_mongodb_db @@ -285,6 +313,10 @@ test-cleanup-postgres: -docker rm -vf authorizer_postgres test-cleanup-cockroachdb: -docker rm -vf authorizer_cockroachdb +test-cleanup-mysql: + -docker rm -vf authorizer_mysql +test-cleanup-mariadb: + -docker rm -vf authorizer_mariadb test-cleanup-mongodb: -docker rm -vf authorizer_mongodb_db test-cleanup-scylladb: diff --git a/internal/storage/db/sql/cockroachdb.go b/internal/storage/db/sql/cockroachdb.go index bff3a4d38..21f60813c 100644 --- a/internal/storage/db/sql/cockroachdb.go +++ b/internal/storage/db/sql/cockroachdb.go @@ -22,33 +22,21 @@ func newCockroachDialector(dbURL string) gorm.Dialector { return cockroachDialector{postgres.Dialector{Config: &postgres.Config{DSN: dbURL}}} } +// Migrator keeps the driver's own migrator — and with it every method gorm +// expects to find, including the BuildIndexOptions it type-asserts with no +// ok-check — and replaces only the unique reconciliation. func (d cockroachDialector) Migrator(db *gorm.DB) gorm.Migrator { - return cockroachMigrator{d.Dialector.Migrator(db)} + return cockroachMigrator{d.Dialector.Migrator(db).(postgres.Migrator)} } type cockroachMigrator struct { - gorm.Migrator + postgres.Migrator } -// MigrateColumnUnique keeps AutoMigrate from dropping our unique indexes on -// CockroachDB. -// -// CockroachDB lists UNIQUE *indexes* in information_schema.table_constraints; -// PostgreSQL lists only real UNIQUE constraints there. GORM reads that extra -// row as "this column has a UNIQUE constraint the model no longer asks for" for -// every `gorm:"uniqueIndex"` field, and issues -// DROP CONSTRAINT "uni_
_" — a NamingStrategy-derived name that -// never exists, because the object is an index with its own name. AutoMigrate -// then aborts with SQLSTATE 42704 and the server never starts: -// -// constraint "uni_authorizer_oauth_states_state_key" of relation -// "authorizer_oauth_states" does not exist (SQLSTATE 42704) -// -// Those indexes are declared by the models and are meant to be there, so the -// drop is skipped. The create direction (a `gorm:"unique"` field whose -// constraint is genuinely missing) is delegated untouched. +// MigrateColumnUnique leaves the unique indexes the models declare alone; see +// skipUniqueIndexDrop for why CockroachDB needs this and PostgreSQL does not. func (m cockroachMigrator) MigrateColumnUnique(value any, field *schema.Field, columnType gorm.ColumnType) error { - if unique, ok := columnType.Unique(); ok && unique && !field.Unique && !field.PrimaryKey { + if skipUniqueIndexDrop(field, columnType) { return nil } return m.Migrator.MigrateColumnUnique(value, field, columnType) diff --git a/internal/storage/db/sql/migrator.go b/internal/storage/db/sql/migrator.go new file mode 100644 index 000000000..0820cbd6d --- /dev/null +++ b/internal/storage/db/sql/migrator.go @@ -0,0 +1,39 @@ +package sql + +import ( + "gorm.io/gorm" + "gorm.io/gorm/schema" +) + +// skipUniqueIndexDrop reports whether AutoMigrate is about to drop a unique +// index the models declare, and should be left alone. +// +// GORM reconciles single-column uniqueness by comparing what the catalog +// reports against the field's `unique` tag. On a disagreement it issues +// DROP CONSTRAINT "uni_
_" — a NamingStrategy-derived name that +// only ever exists for a constraint GORM itself created from a `unique` tag. +// +// PostgreSQL lists only real UNIQUE constraints in +// information_schema.table_constraints, so the two agree and nothing happens. +// CockroachDB lists UNIQUE *indexes* there too, and MySQL reports +// COLUMN_KEY='UNI' for them, so on both every `gorm:"uniqueIndex"` field looks +// like a constraint the model no longer wants — and startup dies on a name that +// was never created: +// +// cockroachdb: constraint "uni_authorizer_oauth_states_state_key" of relation +// "authorizer_oauth_states" does not exist (SQLSTATE 42704) +// mysql: Error 1091 (42000): Can't DROP +// 'uni_authorizer_oauth_states_state_key' +// +// Those indexes are declared by the models and are meant to be there, so the +// drop is skipped. The create direction — a `gorm:"unique"` field whose +// constraint is genuinely missing — is always delegated untouched. +// +// Each affected driver wraps its own concrete migrator (see cockroachdb.go and +// mysql.go) rather than the gorm.Migrator interface: gorm type-asserts +// migrator.BuildIndexOptionsInterface with no ok-check, so a wrapper that +// embeds the interface panics the moment a table with an index is created. +func skipUniqueIndexDrop(field *schema.Field, columnType gorm.ColumnType) bool { + unique, ok := columnType.Unique() + return ok && unique && !field.Unique && !field.PrimaryKey +} diff --git a/internal/storage/db/sql/mysql.go b/internal/storage/db/sql/mysql.go new file mode 100644 index 000000000..554597b7d --- /dev/null +++ b/internal/storage/db/sql/mysql.go @@ -0,0 +1,81 @@ +package sql + +import ( + "fmt" + + "gorm.io/driver/mysql" + "gorm.io/gorm" + "gorm.io/gorm/schema" +) + +// indexedStringSize is the column width GORM's mysql driver already gives an +// unsized string that carries an `index` or `unique` tag: 191 characters, the +// widest utf8mb4 column InnoDB can index under the 767-byte prefix limit. +const indexedStringSize = 191 + +// mysqlDialector is the MySQL dialector with two migration behaviours changed. +// It embeds mysql.Dialector (which itself holds a *Config, so Initialize's +// writes are visible here) rather than the gorm.Dialector interface, so the +// driver's methods all stay promoted. +type mysqlDialector struct { + mysql.Dialector +} + +// newMySQLDialector returns the MySQL dialector for dbURL with the MySQL +// migration fixes layered on. It goes through mysql.New so the driver fills in +// DSNConfig from the DSN, as Initialize expects. +func newMySQLDialector(dbURL string) gorm.Dialector { + return mysqlDialector{*mysql.New(mysql.Config{DSN: dbURL}).(*mysql.Dialector)} +} + +// Migrator keeps the driver's own migrator — and with it every method gorm +// expects to find, including the BuildIndexOptions it type-asserts with no +// ok-check — and replaces only the unique reconciliation. +// +// Config.Dialector is repointed at this wrapper because +// migrator.Migrator.DataTypeOf resolves column types through it: left as the +// driver built it, every DDL type decision would route back to the unwrapped +// dialector and skip DataTypeOf below. +func (d mysqlDialector) Migrator(db *gorm.DB) gorm.Migrator { + m := d.Dialector.Migrator(db).(mysql.Migrator) + // Spelled out: mysql.Migrator embeds both migrator.Migrator and the + // driver's Dialector, and each carries a Config, so a bare m.Config is + // ambiguous. + m.Migrator.Config.Dialector = d + return mysqlMigrator{m} +} + +type mysqlMigrator struct { + mysql.Migrator +} + +// MigrateColumnUnique leaves the unique indexes the models declare alone; see +// skipUniqueIndexDrop. +func (m mysqlMigrator) MigrateColumnUnique(value any, field *schema.Field, columnType gorm.ColumnType) error { + if skipUniqueIndexDrop(field, columnType) { + return nil + } + return m.Migrator.MigrateColumnUnique(value, field, columnType) +} + +// DataTypeOf gives an unsized string column under a `uniqueIndex` a width, so +// MySQL can actually index it. +// +// The driver sizes an unsized string to varchar(191) when the field carries +// `index` or `unique`, but it checks only those two tags — a field tagged +// `uniqueIndex` (named or composite) is missed and becomes `longtext`. MySQL +// then rejects the table outright, because a TEXT column needs an explicit key +// length to be indexed, and the server cannot create its schema at all: +// +// Error 1170 (42000): BLOB/TEXT column 'client_id' used in key specification +// without a key length +// +// Same width the driver picks for the tags it does recognise, so an indexed +// column is sized consistently however its index was declared. +func (d mysqlDialector) DataTypeOf(field *schema.Field) string { + if field.DataType == schema.String && field.Size == 0 && + field.TagSettings["UNIQUEINDEX"] != "" && d.DefaultStringSize == 0 { + return fmt.Sprintf("varchar(%d)", indexedStringSize) + } + return d.Dialector.DataTypeOf(field) +} diff --git a/internal/storage/db/sql/provider.go b/internal/storage/db/sql/provider.go index 8ff209389..ac6a98590 100644 --- a/internal/storage/db/sql/provider.go +++ b/internal/storage/db/sql/provider.go @@ -3,7 +3,6 @@ package sql import ( libsql "github.com/ekristen/gorm-libsql" "github.com/rs/zerolog" - "gorm.io/driver/mysql" "gorm.io/driver/postgres" "gorm.io/driver/sqlserver" "gorm.io/gorm" @@ -74,7 +73,9 @@ func NewProvider( case constants.DbTypeLibSQL: sqlDB, err = gorm.Open(libsql.Open(dbURL), ormConfig) case constants.DbTypeMysql, constants.DbTypeMariaDB, constants.DbTypePlanetScaleDB: - sqlDB, err = gorm.Open(mysql.Open(dbURL), ormConfig) + // MySQL wire protocol, but AutoMigrate needs two behaviours changed — + // see mysql.go. + sqlDB, err = gorm.Open(newMySQLDialector(dbURL), ormConfig) case constants.DbTypeSqlserver: sqlDB, err = gorm.Open(sqlserver.Open(dbURL), ormConfig) } diff --git a/internal/storage/db/sql/provider_migration_test.go b/internal/storage/db/sql/provider_migration_test.go index d22fe6ca5..9f93206ba 100644 --- a/internal/storage/db/sql/provider_migration_test.go +++ b/internal/storage/db/sql/provider_migration_test.go @@ -70,9 +70,12 @@ func sqlMigrationTestConfig(t *testing.T, dbType string) *config.Config { case constants.DbTypeCockroachDB: cfg.DatabaseURL = "postgres://root@localhost:26257/defaultdb?sslmode=disable" skipIfTCPClosed(t, "localhost:26257") - case constants.DbTypeMysql, constants.DbTypeMariaDB: + case constants.DbTypeMysql: cfg.DatabaseURL = "root:password@tcp(localhost:3306)/authorizer_test" skipIfTCPClosed(t, "localhost:3306") + case constants.DbTypeMariaDB: + cfg.DatabaseURL = "root:password@tcp(localhost:3307)/authorizer_test" + skipIfTCPClosed(t, "localhost:3307") default: t.Skipf("unsupported SQL db type for migration test: %s", dbType) } diff --git a/internal/storage/db/sql/session_token.go b/internal/storage/db/sql/session_token.go index 93d9f5121..4070251e7 100644 --- a/internal/storage/db/sql/session_token.go +++ b/internal/storage/db/sql/session_token.go @@ -10,9 +10,23 @@ import ( "github.com/authorizerdev/authorizer/internal/storage/schemas" ) +// likeEscapeChar is the ESCAPE character for every LIKE pattern built here. +// +// It is deliberately not a backslash. MySQL treats a backslash as an escape +// character inside string literals, so the literal `ESCAPE '\'` is unterminated +// there and the statement is rejected outright — `Error 1064 (42000): You have +// an error in your SQL syntax ... near ”\”` — while PostgreSQL and SQLite +// accept it. Session-token revocation ran through these two statements, so on +// MySQL it failed instead of revoking. +// +// Naming the escape character explicitly also makes a backslash in a value +// literal on every engine, which is why it is not escaped below: MySQL's LIKE +// otherwise treats backslash as an escape even with no ESCAPE clause. +const likeEscapeChar = "!" + // likeEscaper escapes LIKE metacharacters so a value is matched literally when -// used with an explicit ESCAPE '\' clause. -var likeEscaper = strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) +// used with an explicit ESCAPE clause. +var likeEscaper = strings.NewReplacer(`!`, `!!`, `%`, `!%`, `_`, `!_`) func escapeLike(s string) string { return likeEscaper.Replace(s) @@ -68,7 +82,7 @@ func (p *provider) DeleteSessionTokenByUserIDAndKey(ctx context.Context, userId, // in an id are treated literally. func (p *provider) DeleteAllSessionTokensByUserID(ctx context.Context, userId string) error { escaped := escapeLike(userId) - return p.db.WithContext(ctx).Where(`user_id = ? OR user_id LIKE ? ESCAPE '\'`, userId, "%:"+escaped).Delete(&schemas.SessionToken{}).Error + return p.db.WithContext(ctx).Where(`user_id = ? OR user_id LIKE ? ESCAPE '`+likeEscapeChar+`'`, userId, "%:"+escaped).Delete(&schemas.SessionToken{}).Error } // DeleteSessionTokensByNamespace deletes all session tokens for a namespace (e.g., "auth_provider"). @@ -76,7 +90,7 @@ func (p *provider) DeleteAllSessionTokensByUserID(ctx context.Context, userId st // The namespace is escaped so its metacharacters (auth-recipe names contain "_") // are matched literally rather than as LIKE wildcards. func (p *provider) DeleteSessionTokensByNamespace(ctx context.Context, namespace string) error { - return p.db.WithContext(ctx).Where(`user_id LIKE ? ESCAPE '\'`, escapeLike(namespace)+":%").Delete(&schemas.SessionToken{}).Error + return p.db.WithContext(ctx).Where(`user_id LIKE ? ESCAPE '`+likeEscapeChar+`'`, escapeLike(namespace)+":%").Delete(&schemas.SessionToken{}).Error } // CleanExpiredSessionTokens removes expired session tokens from the database diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index b92bb02b7..f721c998b 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net" "os" + "path/filepath" "strings" "testing" "time" @@ -32,6 +33,8 @@ var allDBTypes = []string{ constants.DbTypeCouchbaseDB, constants.DbTypeDynamoDB, constants.DbTypeCockroachDB, + constants.DbTypeMysql, + constants.DbTypeMariaDB, } // getTestDBTypes returns the list of database types to test against. @@ -64,6 +67,16 @@ func getTestDBConfig(dbType string) *config.Config { switch dbType { case constants.DbTypePostgres: cfg.DatabaseURL = "postgres://postgres:postgres@localhost:5434/postgres" + case constants.DbTypeLibSQL: + // Local file, same as the sqlite case: libsql's embedded mode is a + // sqlite fork, so this exercises the dialect without a sqld server. + cfg.DatabaseURL = "file:" + filepath.Join(os.TempDir(), "authorizer_libsql_test.db") + case constants.DbTypeMysql: + cfg.DatabaseURL = "root:password@tcp(localhost:3306)/authorizer_test" + case constants.DbTypeMariaDB: + // A separate port from MySQL on purpose: one provider backs both DB + // types, so sharing a port would silently test one engine twice. + cfg.DatabaseURL = "root:password@tcp(localhost:3307)/authorizer_test" case constants.DbTypeCockroachDB: // CockroachDB speaks the Postgres wire protocol and runs through the same // GORM provider; a separate port keeps it from being mistaken for (or @@ -392,8 +405,30 @@ func testAnchoredSessionTokenDelete(t *testing.T, ctx context.Context, provider _, err = provider.GetSessionTokenByUserIDAndKey(ctx, targetStored+"\n", "session_token_key") assert.NoError(t, err, "a trailing newline makes the user id a different value; it must not be deleted") + // LIKE metacharacters in the id. The SQL provider matches the ":" + // suffix with LIKE, so "%" and "_" have to be escaped — and so does the + // escape character itself, or "!" is read as an escape sequence and + // the row survives. Every other backend compares literally (Go string + // compare, AQL ==, QuoteMeta'd regex), so this case is what keeps the six + // equivalent rather than each engine's own idea of "literal". + metaUser := "m%_!" + suffix + metaStored := "auth_provider:" + metaUser + // Reachable only if "%" and "_" are treated as wildcards: "%" would swallow + // "ZZZ" and "_" would match "q". + metaDecoy := "auth_provider:mZZZq!" + suffix + addToken(metaStored) + addToken(metaDecoy) + + require.NoError(t, provider.DeleteAllSessionTokensByUserID(ctx, metaUser)) + + _, err = provider.GetSessionTokenByUserIDAndKey(ctx, metaStored, "session_token_key") + assert.Error(t, err, "a user id holding LIKE metacharacters must still have its token deleted") + _, err = provider.GetSessionTokenByUserIDAndKey(ctx, metaDecoy, "session_token_key") + assert.NoError(t, err, "a user id that matches only when %% and _ act as wildcards must survive") + _ = provider.DeleteAllSessionTokensByUserID(ctx, otherUser) _ = provider.DeleteAllSessionTokensByUserID(ctx, targetUser+"\n") + _ = provider.DeleteAllSessionTokensByUserID(ctx, "mZZZq!"+suffix) } // testUserSearchOperations verifies ListUsers' optional case-insensitive diff --git a/scripts/wait-for-sql.sh b/scripts/wait-for-sql.sh new file mode 100755 index 000000000..1e6d53649 --- /dev/null +++ b/scripts/wait-for-sql.sh @@ -0,0 +1,76 @@ +#!/bin/sh +# Wait until a SQL test container actually serves queries. +# +# Same reason wait-for-cql.sh exists: wait-for-test-dbs.sh probes with `nc -z`, +# which on Docker Desktop connects to the published-port proxy and succeeds the +# instant `docker run -p` returns. For these two engines that proves nothing — +# MySQL binds 3306, runs its init scripts and then *restarts*, and CockroachDB +# accepts TCP before it is accepting SQL clients. Both windows are long enough +# for the storage tests to start against a database that is not there yet. This +# asks the server itself, from inside the container, which a proxy cannot answer. +set -e + +CONTAINER="$1" +ENGINE="$2" +TIMEOUT_SECONDS="${TEST_SQL_WAIT_TIMEOUT:-180}" + +if [ -z "$CONTAINER" ] || [ -z "$ENGINE" ]; then + echo "usage: wait-for-sql.sh " >&2 + exit 2 +fi + +probe() { + case "$ENGINE" in + mysql) + docker exec "$CONTAINER" mysqladmin ping -uroot -ppassword >/dev/null 2>&1 + ;; + mariadb) + # MariaDB 11 renamed the client binaries; the mysql* names are only + # compatibility symlinks and are being dropped, so try the real one first. + docker exec "$CONTAINER" mariadb-admin ping -uroot -ppassword >/dev/null 2>&1 || + docker exec "$CONTAINER" mysqladmin ping -uroot -ppassword >/dev/null 2>&1 + ;; + cockroachdb) + docker exec "$CONTAINER" ./cockroach sql --insecure -e 'SELECT 1' >/dev/null 2>&1 + ;; + *) + echo "unknown engine '$ENGINE' (known: mysql, mariadb, cockroachdb)" >&2 + exit 2 + ;; + esac +} + +started=$(date +%s) +deadline=$(( started + TIMEOUT_SECONDS )) + +echo "waiting for $ENGINE in $CONTAINER (up to ${TIMEOUT_SECONDS}s)..." +while :; do + # Fail fast when the container died mid-wait: an exited container otherwise + # looks exactly like a slow boot until the whole timeout burns down. + state=$(docker inspect -f '{{.State.Status}}' "$CONTAINER" 2>/dev/null | tr -d '[:space:]') + [ -n "$state" ] || state=missing + case "$state" in + running) ;; + missing) + echo " no such container: $CONTAINER" >&2 + exit 1 + ;; + *) + echo " $CONTAINER is '$state', not running — it died during startup" >&2 + docker logs --tail 20 "$CONTAINER" 2>&1 | sed 's/^/ /' >&2 + exit 1 + ;; + esac + + if probe; then + echo " $CONTAINER serving $ENGINE (after $(( $(date +%s) - started ))s)" + exit 0 + fi + + if [ "$(date +%s)" -ge "$deadline" ]; then + echo " $CONTAINER never served $ENGINE within ${TIMEOUT_SECONDS}s" >&2 + docker logs --tail 20 "$CONTAINER" 2>&1 | sed 's/^/ /' >&2 + exit 1 + fi + sleep 2 +done diff --git a/scripts/wait-for-test-dbs.sh b/scripts/wait-for-test-dbs.sh index b80d01f23..0eb8d192b 100755 --- a/scripts/wait-for-test-dbs.sh +++ b/scripts/wait-for-test-dbs.sh @@ -14,8 +14,10 @@ set -e TIMEOUT_SECONDS="${TEST_DB_WAIT_TIMEOUT:-120}" # name:port pairs. Couchbase is absent on purpose — scripts/couchbase-test.sh -# already provisions and waits for it. -ALL_SERVICES="redis:6380 postgres:5434 cockroachdb:26257 mongodb:27017 scylladb:9042 cassandradb:9142 arangodb:8529 dynamodb:8000" +# already provisions and waits for it. For mysql, mariadb and cockroachdb this +# probe is only a first gate: their ports open before they serve queries, so +# scripts/wait-for-sql.sh is what actually has to pass before tests run. +ALL_SERVICES="redis:6380 postgres:5434 cockroachdb:26257 mysql:3306 mariadb:3307 mongodb:27017 scylladb:9042 cassandradb:9142 arangodb:8529 dynamodb:8000" # With no arguments, wait for everything (make test-all-db). With arguments, # wait only for the named services, so a single-backend target does not block on From 1ec1116a46ae99df2034503c24ec9b2545734cdd Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Thu, 29 Oct 2026 07:21:59 +0100 Subject: [PATCH 5/5] fix(storage): make sql server create its schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gorm.io/driver/sqlserver sizes an unsized string column to nvarchar(256) when the field carries `index` or `unique`, but it checks only those two tags. A field tagged `uniqueIndex` — named or composite — is missed and becomes nvarchar(MAX), which SQL Server refuses as an index key, so AutoMigrate dies on the first such table and the server never starts: mssql: Column 'client_id' in table 'authorizer_clients' is of a type that is invalid for use as a key column in an index. Ten columns are affected, across clients, organizations, org_memberships, federated_identities, scim_endpoints, trusted_issuers and webauthn_credentials — all tables that arrived in 2.3.0-rc.10, so nothing is narrowed under existing data. Same shape as the mysql fix: wrap the driver's dialector, give an unsized `uniqueIndex` string the width the driver already picks for the tags it does recognise, and repoint the migrator's Config.Dialector so DDL type decisions route through it. No MigrateColumnUnique override is needed here — SQL Server reads column uniqueness from INFORMATION_SCHEMA.TABLE_CONSTRAINTS, which lists real UNIQUE constraints only, so catalog and models agree as they do on PostgreSQL. Verified against mssql/server:2022-latest. A fresh install creates the whole schema and the next two boots emit zero DDL. Both upgrade paths that can carry data are green in one boot, with no churn after: - v1-shaped (char(36) user_id, UNIQUE on users.email and otps.phone_number): rows intact, both legacy UNIQUEs dropped, two phone_number='' email OTPs insertable. - 2.0.0 through 2.3.0, the releases that could still boot on SQL Server because their schema had no unsized `uniqueIndex` string: authenticators.user_id widens from char(36) to varchar(36) with the seeded row intact, and idx_authenticator_user_id_method is created over it afterwards. From 2.3.0-rc.10 on, AutoMigrate could not complete on SQL Server. A database touched by one of those boots has authorizer_authenticators.user_id as char(36) *inside* that composite index, and SQL Server will not retype an indexed column — drop it before upgrading. No boot there ever finished, so it holds no data. Two upsert tests stay red on a separate driver bug: MergeCreate builds its MERGE predicate from the primary key and ignores clause.OnConflict.Columns, so an upsert keyed on a composite unique index inserts a second row instead of replacing one. make test-sqlserver is therefore kept out of test-all-db. --- AGENTS.md | 3 + Makefile | 20 ++++++ internal/storage/db/sql/provider.go | 5 +- .../storage/db/sql/provider_migration_test.go | 6 ++ internal/storage/db/sql/sqlserver.go | 67 +++++++++++++++++++ internal/storage/provider_test.go | 6 ++ scripts/wait-for-sql.sh | 24 +++++-- 7 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 internal/storage/db/sql/sqlserver.go diff --git a/AGENTS.md b/AGENTS.md index 05e7373a8..b4023a72f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,6 +85,8 @@ make test-cassandradb make test-arangodb make test-dynamodb make test-couchbase +make test-sqlserver # Not in test-all-db: 2 upsert tests are known-red + # (driver builds MERGE from the PK, ignores OnConflict.Columns) # Docker helpers for test-all-db make test-docker-up # Start all test DB containers + Redis @@ -92,6 +94,7 @@ make test-cleanup # Remove all test containers make test-cleanup-postgres | test-cleanup-cockroachdb | test-cleanup-mysql | test-cleanup-mariadb make test-cleanup-mongodb | test-cleanup-scylladb | test-cleanup-cassandradb make test-cleanup-arangodb | test-cleanup-dynamodb | test-cleanup-couchbase +make test-cleanup-sqlserver ``` **Test env vars**: diff --git a/Makefile b/Makefile index 792920722..c16b0270a 100644 --- a/Makefile +++ b/Makefile @@ -11,6 +11,8 @@ GO_TEST_ALL := go test -p 1 -v ./... COCKROACH_IMAGE := cockroachdb/cockroach:latest-v24.3 MYSQL_IMAGE := mysql:8.0.40 MARIADB_IMAGE := mariadb:11 +SQLSERVER_IMAGE := mcr.microsoft.com/mssql/server:2022-latest +SQLSERVER_PASSWORD := Str0ng!Passw0rd .PHONY: all bootstrap build build-app build-dashboard build-local-image build-push-image trivy-scan @@ -313,6 +315,22 @@ test-cleanup-postgres: -docker rm -vf authorizer_postgres test-cleanup-cockroachdb: -docker rm -vf authorizer_cockroachdb +test-sqlserver: test-cleanup-sqlserver + @# Not part of test-all-db: two upsert tests are known-red on SQL Server + @# because gorm.io/driver/sqlserver builds its MERGE predicate from the + @# primary key and ignores clause.OnConflict.Columns, so an upsert keyed on + @# a composite unique index inserts a second row instead of replacing one. + @# Tracked separately from the AutoMigrate fixes; everything else passes. + docker run -d --name authorizer_sqlserver -p 1433:1433 \ + -e ACCEPT_EULA=Y -e MSSQL_SA_PASSWORD='$(SQLSERVER_PASSWORD)' $(SQLSERVER_IMAGE) + sh scripts/wait-for-sql.sh authorizer_sqlserver sqlserver && \ + docker exec authorizer_sqlserver /opt/mssql-tools18/bin/sqlcmd -C -S localhost -U sa \ + -P '$(SQLSERVER_PASSWORD)' -Q "CREATE DATABASE authorizer_test" && \ + { go clean --testcache; TEST_DBS="sqlserver" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_sqlserver; \ + exit $$status + test-cleanup-mysql: -docker rm -vf authorizer_mysql test-cleanup-mariadb: @@ -329,6 +347,8 @@ test-cleanup-dynamodb: -docker rm -vf authorizer_dynamodb test-cleanup-couchbase: -docker rm -vf authorizer_couchbase +test-cleanup-sqlserver: + -docker rm -vf authorizer_sqlserver generate-graphql: go run github.com/99designs/gqlgen --verbose generate && go mod tidy generate-db-template: diff --git a/internal/storage/db/sql/provider.go b/internal/storage/db/sql/provider.go index ac6a98590..903cc2b6d 100644 --- a/internal/storage/db/sql/provider.go +++ b/internal/storage/db/sql/provider.go @@ -4,7 +4,6 @@ import ( libsql "github.com/ekristen/gorm-libsql" "github.com/rs/zerolog" "gorm.io/driver/postgres" - "gorm.io/driver/sqlserver" "gorm.io/gorm" "gorm.io/gorm/logger" "gorm.io/gorm/schema" @@ -77,7 +76,9 @@ func NewProvider( // see mysql.go. sqlDB, err = gorm.Open(newMySQLDialector(dbURL), ormConfig) case constants.DbTypeSqlserver: - sqlDB, err = gorm.Open(sqlserver.Open(dbURL), ormConfig) + // MS SQL wire protocol, but AutoMigrate needs one behaviour changed — + // see sqlserver.go. + sqlDB, err = gorm.Open(newSQLServerDialector(dbURL), ormConfig) } if err != nil { diff --git a/internal/storage/db/sql/provider_migration_test.go b/internal/storage/db/sql/provider_migration_test.go index 9f93206ba..5868792b7 100644 --- a/internal/storage/db/sql/provider_migration_test.go +++ b/internal/storage/db/sql/provider_migration_test.go @@ -37,6 +37,7 @@ func sqlMigrationTestDBTypes() []string { constants.DbTypeSqlite: true, constants.DbTypeMysql: true, constants.DbTypeMariaDB: true, + constants.DbTypeSqlserver: true, } env := os.Getenv("TEST_DBS") if env == "" { @@ -76,6 +77,11 @@ func sqlMigrationTestConfig(t *testing.T, dbType string) *config.Config { case constants.DbTypeMariaDB: cfg.DatabaseURL = "root:password@tcp(localhost:3307)/authorizer_test" skipIfTCPClosed(t, "localhost:3307") + case constants.DbTypeSqlserver: + // Fixed password: SQL Server rejects weak ones outright. Matches the + // container `make test-sqlserver` starts. + cfg.DatabaseURL = "sqlserver://sa:Str0ng!Passw0rd@localhost:1433?database=authorizer_test" + skipIfTCPClosed(t, "localhost:1433") default: t.Skipf("unsupported SQL db type for migration test: %s", dbType) } diff --git a/internal/storage/db/sql/sqlserver.go b/internal/storage/db/sql/sqlserver.go new file mode 100644 index 000000000..6fa10ba57 --- /dev/null +++ b/internal/storage/db/sql/sqlserver.go @@ -0,0 +1,67 @@ +package sql + +import ( + "fmt" + + "gorm.io/driver/sqlserver" + "gorm.io/gorm" + "gorm.io/gorm/schema" +) + +// sqlServerIndexedStringSize is the column width GORM's sqlserver driver +// already gives an unsized string that carries an `index` or `unique` tag. +const sqlServerIndexedStringSize = 256 + +// sqlServerDialector is the SQL Server dialector with its unsized-string +// sizing corrected. It embeds sqlserver.Dialector (which holds a *Config, so +// Initialize's writes are visible here) rather than the gorm.Dialector +// interface, so the driver's own methods all stay promoted. +// +// Unlike CockroachDB and MySQL, SQL Server needs no MigrateColumnUnique +// override: its ColumnTypes reads uniqueness from INFORMATION_SCHEMA. +// TABLE_CONSTRAINTS, which lists real UNIQUE constraints only — a +// CREATE UNIQUE INDEX is not a constraint there, so the catalog and the models +// agree, exactly as on PostgreSQL. +type sqlServerDialector struct { + sqlserver.Dialector +} + +// newSQLServerDialector returns the SQL Server dialector for dbURL with the +// sizing fix layered on. +func newSQLServerDialector(dbURL string) gorm.Dialector { + return sqlServerDialector{*sqlserver.Open(dbURL).(*sqlserver.Dialector)} +} + +// Migrator keeps the driver's own migrator and only repoints the dialector the +// migrator resolves column types through: left as the driver built it, every +// DDL type decision would route back to the unwrapped dialector and skip +// DataTypeOf below. +func (d sqlServerDialector) Migrator(db *gorm.DB) gorm.Migrator { + m := d.Dialector.Migrator(db).(sqlserver.Migrator) + // Spelled out: sqlserver.Migrator embeds migrator.Migrator, which carries + // the Config whose Dialector drives DataTypeOf. + m.Migrator.Config.Dialector = d + return m +} + +// DataTypeOf gives an unsized string column under a `uniqueIndex` a width, so +// SQL Server can actually index it. +// +// The driver sizes an unsized string to nvarchar(256) when the field carries +// `index` or `unique`, but it checks only those two tags — a field tagged +// `uniqueIndex` (named or composite) is missed and becomes nvarchar(MAX), +// which SQL Server refuses as an index key, so the server cannot create its +// schema at all: +// +// mssql: Column 'client_id' in table 'authorizer_clients' is of a type that +// is invalid for use as a key column in an index. +// +// Same width the driver picks for the tags it does recognise, so an indexed +// column is sized consistently however its index was declared. +func (d sqlServerDialector) DataTypeOf(field *schema.Field) string { + if field.DataType == schema.String && field.Size == 0 && + field.TagSettings["UNIQUEINDEX"] != "" && d.DefaultStringSize == 0 { + return fmt.Sprintf("nvarchar(%d)", sqlServerIndexedStringSize) + } + return d.Dialector.DataTypeOf(field) +} diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index f721c998b..ddf52b6f4 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -37,6 +37,10 @@ var allDBTypes = []string{ constants.DbTypeMariaDB, } +// sqlServerTestDSN points at the container `make test-sqlserver` starts. The +// password is fixed because SQL Server rejects weak ones outright. +const sqlServerTestDSN = "sqlserver://sa:Str0ng!Passw0rd@localhost:1433?database=authorizer_test" + // getTestDBTypes returns the list of database types to test against. // Reads from TEST_DBS env var (comma-separated). Defaults to allDBTypes if not set. func getTestDBTypes() []string { @@ -67,6 +71,8 @@ func getTestDBConfig(dbType string) *config.Config { switch dbType { case constants.DbTypePostgres: cfg.DatabaseURL = "postgres://postgres:postgres@localhost:5434/postgres" + case constants.DbTypeSqlserver: + cfg.DatabaseURL = sqlServerTestDSN case constants.DbTypeLibSQL: // Local file, same as the sqlite case: libsql's embedded mode is a // sqlite fork, so this exercises the dialect without a sqld server. diff --git a/scripts/wait-for-sql.sh b/scripts/wait-for-sql.sh index 1e6d53649..5262df981 100755 --- a/scripts/wait-for-sql.sh +++ b/scripts/wait-for-sql.sh @@ -3,7 +3,7 @@ # # Same reason wait-for-cql.sh exists: wait-for-test-dbs.sh probes with `nc -z`, # which on Docker Desktop connects to the published-port proxy and succeeds the -# instant `docker run -p` returns. For these two engines that proves nothing — +# instant `docker run -p` returns. For these engines that proves nothing — # MySQL binds 3306, runs its init scripts and then *restarts*, and CockroachDB # accepts TCP before it is accepting SQL clients. Both windows are long enough # for the storage tests to start against a database that is not there yet. This @@ -15,26 +15,38 @@ ENGINE="$2" TIMEOUT_SECONDS="${TEST_SQL_WAIT_TIMEOUT:-180}" if [ -z "$CONTAINER" ] || [ -z "$ENGINE" ]; then - echo "usage: wait-for-sql.sh " >&2 + echo "usage: wait-for-sql.sh " >&2 exit 2 fi probe() { case "$ENGINE" in mysql) - docker exec "$CONTAINER" mysqladmin ping -uroot -ppassword >/dev/null 2>&1 + # A query against the test database, not `mysqladmin ping`: the entrypoint + # starts a temporary server to run its init (which is what creates + # authorizer_test), *then* restarts. Ping answers during that window, so + # it reports ready while the database does not exist yet and the next + # connection is refused mid-restart. + docker exec "$CONTAINER" mysql -uroot -ppassword -e 'SELECT 1' authorizer_test >/dev/null 2>&1 ;; mariadb) # MariaDB 11 renamed the client binaries; the mysql* names are only # compatibility symlinks and are being dropped, so try the real one first. - docker exec "$CONTAINER" mariadb-admin ping -uroot -ppassword >/dev/null 2>&1 || - docker exec "$CONTAINER" mysqladmin ping -uroot -ppassword >/dev/null 2>&1 + docker exec "$CONTAINER" mariadb -uroot -ppassword -e 'SELECT 1' authorizer_test >/dev/null 2>&1 || + docker exec "$CONTAINER" mysql -uroot -ppassword -e 'SELECT 1' authorizer_test >/dev/null 2>&1 ;; cockroachdb) docker exec "$CONTAINER" ./cockroach sql --insecure -e 'SELECT 1' >/dev/null 2>&1 ;; + sqlserver) + # Against the server, not a database: the mssql image has no + # MSSQL_DATABASE equivalent, so authorizer_test is created by the caller + # once the server answers. + docker exec "$CONTAINER" /opt/mssql-tools18/bin/sqlcmd -C -S localhost \ + -U sa -P "${SQLSERVER_PASSWORD:-Str0ng!Passw0rd}" -Q 'SELECT 1' >/dev/null 2>&1 + ;; *) - echo "unknown engine '$ENGINE' (known: mysql, mariadb, cockroachdb)" >&2 + echo "unknown engine '$ENGINE' (known: mysql, mariadb, cockroachdb, sqlserver)" >&2 exit 2 ;; esac