fix: widen credential_prop.value column to an unbounded text type#4393
Open
reinkrul wants to merge 4 commits into
Open
fix: widen credential_prop.value column to an unbounded text type#4393reinkrul wants to merge 4 commits into
reinkrul wants to merge 4 commits into
Conversation
The column was capped at varchar(500), an arbitrary limit picked when the table was introduced. credentialSubject property values have no natural max length, so on PostgreSQL (and other non-SQLite backends) inserting a VC with a longer property value fails. ALTER COLUMN syntax to change an existing column's type differs per database and SQLite has none at all, so the new migration builds the statement from per-dialect env vars and ships a "_sqlite.sql" no-op counterpart at the same version, swapped in automatically by engine.go. Fixes #4392 Assisted by AI
reinkrul
requested review from
Dirklectisch,
JorisHeadease,
gerardsn,
stevenvegt and
woutslakhorst
as code owners
Contributor
|
Coverage Impact ⬇️ Merging this pull request will decrease total coverage on Modified Files with Diff Coverage (2)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
ALTER TABLE syntax to change an existing column's type isn't portable across databases, and goose already ships a mechanism for exactly this: Go migrations registered via WithGoMigrations, which run plain Go instead of SQL. That's a much smaller diff than templating the SQL with per-dialect env vars and shipping a parallel "_sqlite.sql" no-op file, so replace that approach here. Uses RunTx (not RunDB): the latter needs to acquire a second connection from the pool, which deadlocks against SQLite's single-connection pool. Assisted by AI
Colocates it with the other numbered migrations (naming now matches: 011_...), and includes the migration number in the function names so they're easy to match against the version passed to goose.NewGoMigration. References the originating issue in the doc comment. Assisted by AI
Assisted by AI
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4392
Problem
credential_prop.valuewas capped atvarchar(500)— an arbitrary limit picked when the table was introduced (storage/sql_migrations/001_credential.sql).credentialSubjectproperty values have no natural max length, so on PostgreSQL (and MySQL/SQL Server) inserting a VC whose property value exceeds 500 characters fails with a database error.Fix
Widens the column to an unbounded text type via a goose Go migration.
ALTER TABLE ... ALTER/MODIFY COLUMNsyntax to change an existing column's type is not portable across the databases this project supports (Postgres, MySQL, SQL Server/Azure SQL, SQLite), and SQLite has no such syntax at all (it also doesn't enforce varchar length, so nothing needs to change there). Rather than templating the SQL with per-dialect env vars, this uses goose's own mechanism for exactly this case — a Go migration (goose.NewGoMigration+goose.WithGoMigrations) that runs plain Go instead of SQL, so the right statement is picked with an ordinary map/switch ondbType.The migration lives in
storage/sql_migrations/011_credential_prop_value_type.go, colocated with the other numbered migrations (Migration011CredentialPropValueType), whichengine.goregisters viagoose.WithGoMigrations.It runs via
RunTx(notRunDB):RunDBneeds to acquire a second connection from the pool to run the migration, which deadlocks against SQLite's single-connection pool (db.SetMaxOpenConns(1), set for correctness reasons elsewhere inengine.go) — this is a documented edge case in goose itself.RunTxreuses the connection/transaction goose already holds, avoiding it.Testing
go test ./storage/....varchar(500)cap inserts cleanly.e2e-tests/oauth-flow/rfc021suite against SQL Server with a locally built image from this branch — full VC issuance/storage/status-list/discovery/OAuth/revocation flow passed, confirming the migration applies cleanly on startup there too.