Relax token key column length for legacy migration compatibility - #4401
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplit test DB setup, added dialect-aware migration-compatibility tests that seed a legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant DB
participant Migrator
Test->>DB: Create legacy `tokens` table (key CHAR(48)) and seed row
Test->>DB: Inspect declared column type (expect CHAR(48))
Test->>Migrator: Call AutoMigrate for `Token`
Migrator->>DB: Apply schema change -> `VARCHAR(128)`
Test->>DB: Re-inspect declared column type (expect VARCHAR(128))
Test->>DB: Read seeded row and insert/read back longer key to verify preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/token_test.go (1)
132-150: Consider covering the actual legacy migration path.This test proves fresh SQLite tables are created with
varchar(128), but the PR’s risk is upgrading an existingchar(48)schema. A small migration-path test would better verify the compatibility claim.Example direction
func TestTokenAutoMigrateUsesVarchar128KeyColumn(t *testing.T) { db := setupTokenControllerTestDB(t) var columns []sqliteColumnInfo if err := db.Raw("PRAGMA table_info(tokens)").Scan(&columns).Error; err != nil { t.Fatalf("failed to inspect token table schema: %v", err) } for _, column := range columns { if column.Name == "key" { if strings.ToLower(column.Type) != "varchar(128)" { t.Fatalf("expected key column type varchar(128), got %q", column.Type) } return } } t.Fatal("key column not found in token table schema") } + +func TestTokenAutoMigrateWidensLegacyChar48KeyColumn(t *testing.T) { + // Create a DB with the previous schema shape, run AutoMigrate, + // then assert the key column is widened to varchar(128). +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/token_test.go` around lines 132 - 150, Add a test that exercises the legacy migration path by creating a tokens table with the old schema (key CHAR(48)) directly on the test SQLite DB, then invoking the same auto-migration code used by setupTokenControllerTestDB (the project’s token migration entry point—call the same migration function your tests currently invoke) and finally assert the resulting PRAGMA table_info(tokens) shows the key column is varchar(128); place this as a new test (e.g., TestTokenMigrationFromChar48ToVarchar128) alongside TestTokenAutoMigrateUsesVarchar128KeyColumn and reuse setupTokenControllerTestDB to obtain the DB connection and migration invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controller/token_test.go`:
- Around line 132-150: Add a test that exercises the legacy migration path by
creating a tokens table with the old schema (key CHAR(48)) directly on the test
SQLite DB, then invoking the same auto-migration code used by
setupTokenControllerTestDB (the project’s token migration entry point—call the
same migration function your tests currently invoke) and finally assert the
resulting PRAGMA table_info(tokens) shows the key column is varchar(128); place
this as a new test (e.g., TestTokenMigrationFromChar48ToVarchar128) alongside
TestTokenAutoMigrateUsesVarchar128KeyColumn and reuse setupTokenControllerTestDB
to obtain the DB connection and migration invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33346ab9-e205-4aeb-9648-1f1299b5d7c8
📒 Files selected for processing (2)
controller/token_test.gomodel/token.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/token_test.go (1)
194-242: Please verify this migration on MySQL and PostgreSQL too.These tests only exercise SQLite
AutoMigrate/PRAGMA behavior, but the production migration must also widentokens.keyunder MySQL and PostgreSQL where GORM uses different DDL and length enforcement differs. Please add integration coverage or documented CI/manual verification for MySQL >= 5.7.8 and PostgreSQL >= 9.6; ideally include a post-migration insert/read of a >48-character key so the legacy-compatibility target is covered. As per coding guidelines, “All database migrations MUST work on all three supported databases (SQLite, MySQL, PostgreSQL).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/token_test.go` around lines 194 - 242, TestTokenMigrationFromChar48ToVarchar128 only covers SQLite; extend verification to MySQL and PostgreSQL by adding equivalent integration checks that run migrateTokenControllerTestDB and then insert/read a >48-character key to confirm tokens.key is widened and data preserved. Update or add tests (or CI steps) that reuse helpers like openTokenControllerTestDB (or add openTokenControllerTestDBMySQL/openTokenControllerTestDBPostgres) and assert post-migration column type/behavior (or attempt an insert+select of a 64/128-char key) and that migratedToken fields (Key, Name) remain unchanged; if real DBs aren’t available in CI, add documented manual verification steps for MySQL >=5.7.8 and PostgreSQL >=9.6 alongside the SQLite test. Ensure migrateTokenControllerTestDB is exercised for each DB backend and that any DB-specific DDL verification replaces getSQLiteColumnType with equivalent column-type checks for MySQL/Postgres or uses insert/read assertions to prove the widening.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controller/token_test.go`:
- Around line 194-242: TestTokenMigrationFromChar48ToVarchar128 only covers
SQLite; extend verification to MySQL and PostgreSQL by adding equivalent
integration checks that run migrateTokenControllerTestDB and then insert/read a
>48-character key to confirm tokens.key is widened and data preserved. Update or
add tests (or CI steps) that reuse helpers like openTokenControllerTestDB (or
add openTokenControllerTestDBMySQL/openTokenControllerTestDBPostgres) and assert
post-migration column type/behavior (or attempt an insert+select of a
64/128-char key) and that migratedToken fields (Key, Name) remain unchanged; if
real DBs aren’t available in CI, add documented manual verification steps for
MySQL >=5.7.8 and PostgreSQL >=9.6 alongside the SQLite test. Ensure
migrateTokenControllerTestDB is exercised for each DB backend and that any
DB-specific DDL verification replaces getSQLiteColumnType with equivalent
column-type checks for MySQL/Postgres or uses insert/read assertions to prove
the widening.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6f097a6-c0a9-40de-884b-a47051a163b7
📒 Files selected for processing (1)
controller/token_test.go
|
补充说明：已按 CodeRabbit 的建议补上跨数据库迁移测试。 新增测试：
这两个测试通过环境变量注入 DSN：
本地我已用 Docker 起临时 MySQL 8.4 / PostgreSQL 16 实际跑过： TEST_MYSQL_DSN='root:codex@tcp(127.0.0.1:13306)/newapi_test?charset=utf8mb4&parseTime=True&loc=Local' \
TEST_POSTGRES_DSN='host=127.0.0.1 port=15432 user=postgres password=codex dbname=newapi_test sslmode=disable' \
go test ./controller -run 'TestToken(MigrationFromChar48ToVarchar128(MySQL|Postgres)?|AutoMigrateUsesVarchar128KeyColumn)$' -count=1
TEST_MYSQL_DSN='root:codex@tcp(127.0.0.1:13306)/newapi_test?charset=utf8mb4&parseTime=True&loc=Local' \
TEST_POSTGRES_DSN='host=127.0.0.1 port=15432 user=postgres password=codex dbname=newapi_test sslmode=disable' \
go test ./controller -count=1结果： 测试覆盖点：
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/token_test.go`:
- Around line 145-147: The test currently unconditionally executes db.Exec("DROP
TABLE IF EXISTS tokens") in t.Cleanup (and similarly at lines ~269-280), which
can wipe a real external DB; update the test harness to guard external DB runs
by detecting TEST_MYSQL_DSN or TEST_POSTGRES_DSN and refusing to drop or modify
an existing "tokens" table: if an external DSN is present, first check for the
existence of the "tokens" table via the DB connection and fail the test (or
skip) if it already exists, and for this test only create/cleanup a uniquely
named table (or use a test-specific prefix) instead of the global "tokens" name;
apply the same guard logic to the setup and t.Cleanup blocks referencing
db.Exec("DROP TABLE IF EXISTS tokens") so only tables created by this test are
removed and external DBs are protected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2932f24-f750-41f6-877e-e3905ddc7278
📒 Files selected for processing (1)
controller/token_test.go
|
@Calcium-Ion 打扰了，这个 PR 这边已经按 CodeRabbit 的建议补齐了迁移路径测试、MySQL/PostgreSQL 覆盖以及外部库保护逻辑；最新一轮 CodeRabbit 也已经没有 actionable comments。\n\n如果你方便的话，麻烦帮看一下这个 PR 是否可以继续推进，感谢。 |
007c418 to
fc98d36
Compare
fc98d36 to
4947452
Compare
|
提交没签名导致不能合并,签名后重新提交了 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/token_test.go (1)
252-270: Postgres branch: fragile formatting whencharacter_maximum_lengthis NULL.For
character varying/characterwith no explicit length (e.g., unboundedvarchar),maxLength.Validwill be false andmaxLength.Int64is0, so Lines 261-264 would emit misleading strings likevarchar(0)orchar(0). The current migration tests only exercise bounded lengths (char(48)→varchar(128)), so this isn't a bug today — just brittle if the helper is reused for other columns later.♻️ Optional hardening
switch strings.ToLower(dataType) { case "character varying": + if !maxLength.Valid { + return "varchar" + } return fmt.Sprintf("varchar(%d)", maxLength.Int64) case "character": + if !maxLength.Valid { + return "char" + } return fmt.Sprintf("char(%d)", maxLength.Int64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/token_test.go` around lines 252 - 270, The helper that inspects the Postgres column currently reads dataType and maxLength and formats "varchar(%d)" or "char(%d)" even when maxLength.Valid is false; update the switch in the token_test.go inspection logic (the block that scans into dataType and maxLength and the switch on strings.ToLower(dataType)) to check maxLength.Valid before using maxLength.Int64 and, for unbounded types (character varying / character) when maxLength.Valid is false, return the unparameterized type name ("varchar" or "char") instead of "varchar(0)" or "char(0)"; also keep the existing fallback that uses maxLength only when maxLength.Valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controller/token_test.go`:
- Around line 252-270: The helper that inspects the Postgres column currently
reads dataType and maxLength and formats "varchar(%d)" or "char(%d)" even when
maxLength.Valid is false; update the switch in the token_test.go inspection
logic (the block that scans into dataType and maxLength and the switch on
strings.ToLower(dataType)) to check maxLength.Valid before using maxLength.Int64
and, for unbounded types (character varying / character) when maxLength.Valid is
false, return the unparameterized type name ("varchar" or "char") instead of
"varchar(0)" or "char(0)"; also keep the existing fallback that uses maxLength
only when maxLength.Valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67f4803e-ab23-4981-973c-929c3a9554ec
📒 Files selected for processing (2)
controller/token_test.gomodel/token.go
✅ Files skipped from review due to trivial changes (1)
- model/token.go
Relax token key column length for legacy migration compatibility
3 participants
Important
📝 变更描述 / Description
将
model.Token.Key的数据库列类型从char(48)调整为varchar(128)，并补充一个针对AutoMigrate结果的最小测试。这样做的原因是：当前鉴权流程本身已经会去掉
sk-前缀再查库，但一些历史部署在迁移到 new-api 时会保留更长的 legacy token key（例如从 sub2api 迁移时，去掉前缀后仍可能是 64 字符）。在现有char(48)限制下，这类 key 无法导入；放宽列长度后，不影响现有 48 位 key 的使用，同时为外部迁移脚本保留兼容空间。AutoMigrate，因此已有部署升级到该版本后，首次启动会尝试把tokens.key列从char(48)放宽到varchar(128)。char(48)。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
go test ./controller -run TestTokenAutoMigrateUsesVarchar128KeyColumn -count=1Summary by CodeRabbit
Bug Fixes
Tests