robocanic commented on PR #1582:
URL: https://github.com/apache/dubbo-admin/pull/1582#issuecomment-6229168576
## Review
整体设计清晰，对非 MCP 资源的影响面控制得不错：opt-in 的 `VersionedResource`、仅 MCP 表增加 `version`
列、memory store 读写深拷贝。以下为建议（本次仅阅读 diff，未本地运行测试）。
### Major
1. **store 层泄漏 MCP 领域逻辑，且 memory 与 Gorm 行为不一致**
`GormStore.CompareAndDeleteWithDependents` 在通用 `dbcommon` 里硬编码了
`index.ByMCPCredentialServerID` 与 `len(conditions)==2`；校验 `conditions`
后并不使用，而是自行拼 SQL。memory 实现则完全信任调用方传入的
`conditions`，没有任何校验。`CascadingResourceStore` 看似通用，实际只服务
Server→Credential。建议收窄接口为明确的 `(dependentKind, indexName, indexValue)` 并让两种
store 使用同一套校验，或把 MCP 特定部分下放到 repository 层。
2. **错误类型丢失，Console 层难以映射 HTTP 状态**
`Servers.Get`、`Credentials.Revoke`/`Authenticate` 的 not found / 参数非法均为
`fmt.Errorf` 字符串；`manager.CompareAndSwap` 等的 "不支持" 也不是 `bizerror`。后续 Console
API 无法区分 404/400/409/500。建议使用 `store.ErrorResourceNotFound`（已有
`IsResourceNotFound`）或 `bizerror`，并为 credential 校验失败定义哨兵错误。
3. **Gorm 并发路径缺少测试**
并发 Publish "一个赢家" 只覆盖了 memory。Gorm CAS 是非锁定读 + 带 `version` 条件的
UPDATE，逻辑正确；但 SQLite 的 `FOR UPDATE` 为 no-op，deferred 事务读后升级写锁可能返回
`SQLITE_BUSY`，调用方会得到非 409 错误。`AddWithParent` 与 `CompareAndDeleteWithDependents`
的互斥也只有串行用例。建议补 Gorm 并发 Publish 测试及 "创建 credential vs 删除 server" 的竞争测试。
### Minor
4. `secret_hash` 存在普通 Spec 中，会随通用 List/Get、`String()`/日志序列化出去。本 PR 范围内可接受，后续
Console DTO 必须脱敏，建议注明。
5. `formatExpiration` 使用 `RFC3339`（截断到秒），而 `Create` 校验的是未截断的
`expiresAt.After(now)`；距今不足 1 秒的过期时间存储后可能已在过去。建议用 `RFC3339Nano` 或统一截断后再校验。
6. memory 的 `CompareAndSwap`/`CompareAndDelete` 未校验 `obj.ResourceKind() ==
rs.rk`（Gorm 版本有）。
7. `UpdateDraft` 未检查 `server.Spec == nil`（`Publish` 有），Spec 为 `null` 时会
panic。
8. ResourceVersion 同时存在于 JSON `data` 与 `version` 列；读取以列为准，目前安全，但
`ToResourceWithVersion` 在 `version==0` 时会保留 JSON 中的值，建议统一单一来源。
9. Gorm 级联删除为获取 key 加载了子资源整行（含 `data` blob），只 `Select("resource_key")` 即可。
10. 生成代码 `SetItems` 在类型断言失败分支对 nil `res` 调用 `res.ResourceKind()`，会
panic。属生成模板的既有问题，非本 PR 引入，建议另行修复。
11. credential 的 `displayName` 无长度限制、`serverID` 无格式校验。
### 正面
- Credential：256-bit 随机密钥、仅存 SHA-256、`ConstantTimeCompare`、`Authenticate`
始终读权威存储、统一错误信息避免信息泄露。
- Server 与 Credential 锁顺序一致（先 parent 后 child），无死锁风险；`manager.Add` 的行为变化仅对
versioned kind 生效，兼容性良好。
- 测试覆盖了 Draft/Published 隔离、校验失败不污染已发布版本、跨 store 实例读取、非 MCP 表无 `version` 列。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
