fix: drop unused oauth_tokens table from 003 (closes #97)#120
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the unused oauth_tokens table and its associated indexes, providing both an upgrade and a rollback migration. Feedback indicates that the up migration contains redundant DROP INDEX statements since PostgreSQL's DROP TABLE automatically removes associated indexes. Additionally, the down migration includes a redundant index on the jti column, which is already covered by a UNIQUE constraint.
|
@rsharath can we just drop these directly without adding a new migration ? |
Remove the never-wired oauth_tokens table directly from the migration
that originally created it, instead of layering a separate drop
migration on top.
Why this is safe — and would NOT be safe in general:
- Migration files become append-only the moment any deployment runs
them, because schema_migrations.version says "this version was
applied" while the actual rows behave however the contemporaneous SQL
dictated. Two deployments at the same migration_version with
different SQL = silent schema divergence.
- Per saucam's review on the prior approach: ZeroID has not been
deployed to any persistent environment yet (no production, no
long-lived dev). Every install runs migrations from scratch on the
current SQL, so editing 003 in place produces the same end state
everywhere.
- Once a real deployment exists, future cleanups of unused tables
must use the layered-migration pattern instead.
Diff is minimal:
003_oauth_clients.up.sql — remove the CREATE TABLE oauth_tokens
block + four CREATE INDEX statements.
003_oauth_clients.down.sql — remove the corresponding DROP INDEX /
DROP TABLE statements.
Verified before edit (zero hits):
grep -rn "oauth_tokens" --include="*.go" .
grep -rn "OauthToken\|OAuthToken" domain/ internal/
## Test plan
- [x] go vet ./... clean
- [x] Full integration suite green ~10s (migration runs fresh in the
testcontainer on every test run, so this exercises the trimmed
003 end-to-end).
Replaces the earlier 015_drop_oauth_tokens migration approach;
015 was reverted along with the original commit.
9fae4c6 to
8915786
Compare
|
You're right — I had the wrong context. Reversed the previous decision and pushed the cleaner approach (
Net diff: Caveat written into the commit message: this only works because no deployment has run 003 yet. Once a real deployment exists, the same cleanup would need a layered drop migration to avoid schema divergence (two deployments at Title + body updated to reflect the new approach. CI re-running. |
Summary
Closes #97. Removes the never-wired
oauth_tokenstable directly frommigrations/003_oauth_clients.up.sql(and the matchingDROPstatements from the down file), instead of layering a separate drop migration on top.Approach changed mid-PR (per @saucam's review)
The original commit added
015_drop_oauth_tokens.{up,down}.sql. Saucam asked: "can we just drop these directly without adding a new migration?" — and in this case, yes: ZeroID has not been deployed to any persistent environment yet, so every install runs migrations from scratch on the current SQL. Editing003in place produces the same end state everywhere.If/when a real deployment exists, future cleanups of unused tables MUST use the layered-migration pattern. Migration files become append-only the moment any deployment runs them, because
schema_migrations.versionsays "this version was applied" while the actual rows behave however the contemporaneous SQL dictated. Two deployments at the samemigration_versionwith different SQL = silent schema divergence.Diff
migrations/003_oauth_clients.up.sql— removed theCREATE TABLE oauth_tokensblock and its fourCREATE INDEXstatements (idx_oauth_tokens_jti,_client_id,_expires_at,_tenant). Header comment updated.migrations/003_oauth_clients.down.sql— removed the correspondingDROP INDEX/DROP TABLEstatements.Net:
2 files changed, 1 insertion(+), 36 deletions(-).Verified
Zero hits in either. No Go domain model binds to the table; no repository references it.
Test plan
go vet ./...— cleanoauth_tokensis ever created).