fix: enforce case-sensitive model name matching to support multiple channels with different case variants #2021
Conversation
WalkthroughAdds a MySQL-specific collation-fix step to migrations to set certain columns to utf8mb4_bin when needed, and updates the model-name duplicate check to use a case-sensitive comparison on MySQL while retaining previous behavior for other databases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Migrator as migrateDB
participant ORM as AutoMigrate
participant Collation as fixModelNameCollation
participant MySQL as DB
App->>Migrator: Initialize / migrate
Migrator->>ORM: AutoMigrate(models)
ORM-->>Migrator: Done
Migrator->>Collation: fixModelNameCollation()
Collation->>MySQL: Query current collations (models, abilities)
alt Non-binary collation detected
Collation->>MySQL: ALTER TABLE ... MODIFY COLUMN ... utf8mb4_bin
MySQL-->>Collation: OK / Err
else Already binary or table/column missing
Collation-->>Migrator: No-op / skipped
end
Collation-->>Migrator: Log outcome (warn if failed)
Migrator-->>App: Continue
sequenceDiagram
autonumber
actor Caller
participant Meta as IsModelNameDuplicated
participant DB as Database
Caller->>Meta: Check duplicate(model_name)
alt Using MySQL
Meta->>DB: SELECT COUNT(*) WHERE BINARY model_name = ?
else Other DB
Meta->>DB: SELECT COUNT(*) WHERE model_name = ?
end
DB-->>Meta: cnt, err
Meta-->>Caller: (cnt > 0), err
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
model/main.go (1)
261-261: Consider using suffix check for more precise collation detection.The current check uses
strings.Contains(strings.ToLower(collation), "_bin")which could theoretically match unexpected patterns. While MySQL collations conventionally use_binas a suffix, usingstrings.HasSuffixwould be more precise.Apply this diff for more precise matching:
- } else if !strings.Contains(strings.ToLower(modelCollation), "_bin") { + } else if !strings.HasSuffix(strings.ToLower(modelCollation), "_bin") {And similarly for line 276:
- } else if !strings.Contains(strings.ToLower(abilityCollation), "_bin") { + } else if !strings.HasSuffix(strings.ToLower(abilityCollation), "_bin") {Also applies to: 276-276
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model/main.go(2 hunks)model/model_meta.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
model/model_meta.go (2)
common/database.go (1)
UsingMySQL(12-12)model/main.go (1)
DB(63-63)
model/main.go (2)
common/database.go (1)
UsingMySQL(12-12)common/sys_log.go (1)
SysLog(11-14)
🔇 Additional comments (6)
model/model_meta.go (2)
24-24: Verify that the collation tag is present.The PR description states that a
collation:utf8mb4_bintag should be added to theModelNamefield, but it's not visible in the provided code. This tag is essential for ensuring new installations create tables with case-sensitive collation by default.Please verify that the struct definition includes the collation tag:
ModelName string `json:"model_name" gorm:"size:128;not null;collation:utf8mb4_bin;uniqueIndex:uk_model_name_delete_at,priority:1"`
57-63: LGTM! The conditional case-sensitive comparison is correctly implemented.The use of the
BINARYkeyword for MySQL ensures case-sensitive matching, while PostgreSQL and SQLite maintain their default case-sensitive behavior. The error handling is appropriate.model/main.go (4)
312-317: Consider whether migration failures should be non-fatal.The migration logs warnings but continues on error (line 315-316). While this maintains backwards compatibility, it means installations with migration failures will continue to have case-insensitive collations, potentially causing the original issue (#1990) to persist.
Please verify this is the intended behavior. If the collation fix is critical for correctness, consider:
- Making the error fatal for new installations (checking if tables are empty)
- Adding a system health check that warns admins of misconfigured collations
- Documenting the manual fix procedure for failed migrations
249-286: LGTM on the overall migration approach.The function correctly:
- Checks if MySQL is in use before proceeding
- Queries current collations from
information_schema- Handles missing tables gracefully
- Uses safe SQL with hardcoded identifiers (no injection risk)
- Applies
utf8mb4_bincollation for case-sensitive comparisonThe approach of checking and conditionally updating is good for backwards compatibility.
322-374: Remove collation‐fix suggestion:migrateDBFastisn’t invoked anywhere, so addingfixModelNameCollationhere has no effect.Likely an incorrect or invalid review comment.
249-286: No mismatch inabilities.modelcolumn size
The ALTER TABLE usesVARCHAR(255), which matches theAbility.Modelgorm tag.
- Add case-sensitive collation (utf8mb4_bin) to model_name and ability.model columns - Update IsModelNameDuplicated to use BINARY comparison for MySQL - Add automatic migration to update existing MySQL databases - PostgreSQL and SQLite remain case-sensitive by default Fixes QuantumNous#1990
RedwindA
force-pushed
the
fix/modelNameMatch
branch
from
4d6087d to
da8a5b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
model/main.go (1)
255-283: Handle "no result" cases before issuing ALTERLine 263: when the
information_schemaquery finds no matching row (table or column absent),ScanleavesmodelCollationempty but still falls through to theALTER, which then fails withTable 'models' doesn't exist. The same pattern appears around Line 278. That produces a warning every startup on environments where those tables are legitimately absent (despite the intent stated in the comment) and the collation upgrade never runs. Please detect the “no result” condition explicitly—e.g., scan intosql.NullStringor useRow().Scanand skip whenerr == sql.ErrNoRows—before attempting the ALTER so we truly “ignore missing tables.”+ var modelCollation sql.NullString - err := DB.Raw("SELECT COLLATION_NAME ...").Scan(&modelCollation).Error - if err != nil { - // Table might not exist yet, ignore error - common.SysLog(...) - } else if !strings.HasSuffix(strings.ToLower(modelCollation), "_bin") { + row := DB.Raw("SELECT COLLATION_NAME ...").Row() + switch err := row.Scan(&modelCollation); err { + case nil: + if modelCollation.Valid && !strings.HasSuffix(strings.ToLower(modelCollation.String), "_bin") { ... } + case sql.ErrNoRows: + // nothing to migrate for this column + default: + common.SysLog(...) + }Repeat the same pattern for the
abilities.modelblock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model/main.go(2 hunks)model/model_meta.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
model/main.go (2)
common/database.go (1)
UsingMySQL(12-12)common/sys_log.go (1)
SysLog(11-14)
model/model_meta.go (2)
common/database.go (1)
UsingMySQL(12-12)model/main.go (1)
DB(63-63)
1 participant
PR 类型
PR 是否包含破坏性更新？
PR 描述
修复目的
修复 issue #1990 中报告的模型名称大小写敏感性问题。之前在MySQL 数据库中，
deepseek-ai/DeepSeek-V3.1-Terminus和deepseek-ai/deepseek-v3.1-terminus被视为相同的模型名称，导致无法为不同大小写的模型名称配置不同的渠道（例如硅基流动使用大写版本，英伟达使用小写版本）。实现细节
数据库模式更新
Model结构体的ModelName字段添加collation:utf8mb4_bin标签，使 MySQL使用区分大小写的排序规则Ability结构体的Model字段添加相同的排序规则，确保能力表中的模型匹配也区分大小写查询逻辑优化
IsModelNameDuplicated()函数，对 MySQL 使用BINARY关键字进行大小写敏感的比较自动迁移支持
fixModelNameCollation()迁移函数，在系统启动时自动检测并更新现有 MySQL数据库的字段排序规则影响范围
models.model_name和abilities.model字段更新为大小写敏感测试
Summary by CodeRabbit
New Features
Bug Fixes
Chores