fix: preserve TenantID in PUT handlers to prevent resource disappearance#387
Merged
fix: preserve TenantID in PUT handlers to prevent resource disappearance#387
Conversation
PUT handlers for Provider, Project, RetryConfig, and RoutingStrategy decoded a fresh struct from the request body but only copied ID and CreatedAt from the existing record. Because the frontend never sends tenantID, gorm.Save overwrote tenant_id to 0, making the resource invisible to tenant-scoped queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough修复管理员 PUT 更新时将 TenantID 置零的问题：在更新处理器中解码后从现有记录复制 TenantID（同时保留 ID 和 CreatedAt）；新增 SQLite 迁移修复历史数据；添加 Playwright E2E 测试覆盖 API 与 UI 的更新流程；前端小修避免空用户访问崩溃。 Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Playwright
participant Browser as Browser (UI)
participant AdminAPI as Server (admin handler)
participant Repo as Repository
participant DB as SQLite DB
Tester->>Browser: 在 UI 中登录并打开项目
Browser->>AdminAPI: PUT /api/admin/projects/:id (触发更新)
AdminAPI->>Repo: 读取现有记录（获取 TenantID, CreatedAt）
Repo->>DB: SELECT ... FROM projects WHERE id=...
DB-->>Repo: 返回现有记录
Repo-->>AdminAPI: 返回现有记录
AdminAPI->>AdminAPI: 合并更新体：保留 ID、保留 CreatedAt、设置 TenantID=现有.TenantID
AdminAPI->>Repo: UpdateProject(...)
Repo->>DB: UPDATE projects SET ... WHERE id=...
DB-->>Repo: 更新结果
Repo-->>AdminAPI: 返回更新成功
AdminAPI-->>Browser: 返回 200 OK
Browser->>Tester: 通过 Admin API 查询并验证 tenant_id 与更新后的字段
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Automatically restores tenant_id for providers, projects, retry_configs, and routing_strategies rows that were corrupted (set to 0) by the PUT handler bug fixed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/e2e/playwright/test-project-update-tenant.mjs (3)
117-118: 建议：用显式等待替代waitForTimeout以提高测试稳定性当前测试大量使用
waitForTimeout（固定等待时间），这是 Playwright 测试中的反模式，可能导致：
- 在慢速环境中测试失败
- 在快速环境中浪费执行时间
建议使用显式等待条件，例如：
- await page.waitForTimeout(2000); - console.log('Browser login success'); + await page.waitForURL('**/projects', { timeout: 10000 }); + console.log('Browser login success');或使用
waitForSelector、waitForLoadState('networkidle')等更可靠的等待方式。Also applies to: 123-123, 131-131, 140-141, 149-149, 166-166, 177-177, 185-185, 191-191, 205-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/test-project-update-tenant.mjs` around lines 117 - 118, Replace uses of page.waitForTimeout with explicit waits: identify each call to page.waitForTimeout in this file and swap it for a targeted wait such as page.waitForSelector(selector) for UI elements, page.waitForLoadState('networkidle') for navigation completion, or page.waitForResponse(urlOrPredicate) / locator.waitFor() when awaiting network or specific element state; ensure the chosen selector or predicate matches the next action (e.g., after login wait for a user avatar or dashboard selector), keep logs like console.log('Browser login success') after the explicit wait, and apply the same replacement for all occurrences of page.waitForTimeout in this test file.
213-216: 建议：确保测试失败时也能清理测试数据当前实现中，如果测试在项目创建后但清理步骤前失败，测试项目不会被删除，可能导致测试数据残留。
♻️ 建议使用 try/finally 确保清理
+let projectId = null; + (async () => { // ... setup and test code ... + projectId = project.id; // ... rest of test ... - // Cleanup: delete test project - console.log('\n--- Cleanup ---'); - await adminAPI('DELETE', `/projects/${project.id}`, null, jwt); - console.log('Test project deleted'); - // Screenshot await page.screenshot({ path: '/tmp/project-update-tenant-result.png' }); console.log('Screenshot: /tmp/project-update-tenant-result.png'); console.log(`\n===== Test ${exitCode === 0 ? 'PASSED' : 'FAILED'} =====`); await browser.close(); process.exit(exitCode); })().catch(async (err) => { console.error('Test error:', err.message); if (browser) { try { await browser.close(); } catch {} } process.exit(1); +}).finally(async () => { + // Cleanup test project regardless of test result + if (projectId && jwt) { + try { + await adminAPI('DELETE', `/projects/${projectId}`, null, jwt); + console.log('Test project cleaned up'); + } catch (e) { + console.warn('Failed to cleanup test project:', e.message); + } + } });Also applies to: 225-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/test-project-update-tenant.mjs` around lines 213 - 216, Wrap the test's create/use/delete sequence in a try/finally so cleanup always runs even on failure: keep the project creation and assertions in the try block and move the deletion call to the finally block; call adminAPI('DELETE', `/projects/${project.id}`, null, jwt) only if project and project.id are defined (guard with project != null) to avoid errors; apply the same try/finally pattern to the similar block referenced at lines 225-231 to ensure both test projects are always deleted on test failure.
143-145: 注意：Switch 定位器可能选择错误的开关使用
page.locator('button[role="switch"]').first()可能在页面上有多个开关时选择错误的元素。建议使用更具体的定位策略，例如基于标签文本或父容器定位：
const customRoutesSwitch = page.getByRole('switch', { name: /custom routes|自定义路由/i });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/test-project-update-tenant.mjs` around lines 143 - 145, The locator customRoutesSwitch uses page.locator('button[role="switch"]').first() which can pick the wrong switch when multiple exist; replace this with a more specific locator (e.g., use page.getByRole('switch', { name: /custom routes|自定义路由/i }) or scope to a parent container before querying) so the test targets the intended switch element (update references to customRoutesSwitch and any surrounding code that asserts on switchCountBefore accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/playwright/test-project-update-tenant.mjs`:
- Around line 117-118: Replace uses of page.waitForTimeout with explicit waits:
identify each call to page.waitForTimeout in this file and swap it for a
targeted wait such as page.waitForSelector(selector) for UI elements,
page.waitForLoadState('networkidle') for navigation completion, or
page.waitForResponse(urlOrPredicate) / locator.waitFor() when awaiting network
or specific element state; ensure the chosen selector or predicate matches the
next action (e.g., after login wait for a user avatar or dashboard selector),
keep logs like console.log('Browser login success') after the explicit wait, and
apply the same replacement for all occurrences of page.waitForTimeout in this
test file.
- Around line 213-216: Wrap the test's create/use/delete sequence in a
try/finally so cleanup always runs even on failure: keep the project creation
and assertions in the try block and move the deletion call to the finally block;
call adminAPI('DELETE', `/projects/${project.id}`, null, jwt) only if project
and project.id are defined (guard with project != null) to avoid errors; apply
the same try/finally pattern to the similar block referenced at lines 225-231 to
ensure both test projects are always deleted on test failure.
- Around line 143-145: The locator customRoutesSwitch uses
page.locator('button[role="switch"]').first() which can pick the wrong switch
when multiple exist; replace this with a more specific locator (e.g., use
page.getByRole('switch', { name: /custom routes|自定义路由/i }) or scope to a parent
container before querying) so the test targets the intended switch element
(update references to customRoutesSwitch and any surrounding code that asserts
on switchCountBefore accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4225784-7f62-41bb-b434-b4119ee73576
📒 Files selected for processing (3)
internal/handler/admin.gointernal/repository/sqlite/migrations.gotests/e2e/playwright/test-project-update-tenant.mjs
📜 Review details
🔇 Additional comments (6)
internal/repository/sqlite/migrations.go (1)
186-210: 迁移逻辑正确，修复设计合理！该迁移有效修复了 PUT 处理器导致的
tenant_id=0数据损坏问题：
- 先查询默认租户 ID，若不存在则回退到
1（与迁移 v3 创建的默认租户一致）- 仅更新
tenant_id=0的行，具有幂等性- 错误处理模式与现有迁移保持一致（记录警告但继续执行）
- Down 迁移为空操作是合理的，因为撤销修复没有意义
internal/handler/admin.go (4)
208-211: 修复正确，保留 TenantID 防止资源"消失"此修复正确地从现有数据库记录中保留
TenantID，与保留ID和CreatedAt的模式一致。由于前端不发送tenantID，JSON 解码会将其设为零值，导致 GORM Save 写入tenant_id=0。
465-467: Project 处理器修复正确与 Provider 相同的修复模式，确保项目更新后 TenantID 不会丢失。
640-642: RetryConfig 处理器修复正确保持与其他资源相同的修复模式。
711-713: RoutingStrategy 处理器修复正确四个受影响的 PUT 处理器现在都正确保留 TenantID。
tests/e2e/playwright/test-project-update-tenant.mjs (1)
1-24: 测试设计合理，覆盖了关键的 TenantID 保留场景测试流程完整覆盖了 PR 描述的 bug 场景：
- 通过 API 创建项目
- 通过 UI 切换自定义路由（触发 PUT 更新）
- 验证项目仍然存在
- 编辑项目名称（再次触发 PUT 更新）
- 验证项目仍然存在
- Replace waitForTimeout with explicit waits (waitFor, waitForURL, waitForResponse) - Add try/finally cleanup via cleanup() helper to ensure test project deletion - Keep minimal waitForTimeout(500) only after API response confirmation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@coderabbitai re-review ！Stop making breaking changes, do a proper review！ |
|
✅ Actions performedFull review triggered. |
Member
Author
CI Results ✅All checks passed on latest commit
E2E Test Details
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/playwright/test-project-update-tenant.mjs (2)
166-167: 硬编码的waitForTimeout可能导致测试不稳定。
waitForTimeout(500)是一种测试反模式，可能在慢速 CI 环境中导致竞态条件，或在快速环境中造成不必要的等待。建议使用更明确的等待条件。♻️ 建议使用显式等待条件替代固定延时
- await page.waitForTimeout(500); + // 等待 UI 状态更新完成（如 toast 消失或开关状态变化） + await page.locator('button[role="switch"][data-state="checked"]').first().waitFor({ state: 'visible', timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/test-project-update-tenant.mjs` around lines 166 - 167, The hard-coded sleep using page.waitForTimeout(500) causes flakiness; replace it with an explicit wait that reflects the real condition (e.g., wait for a specific DOM change, network response, or navigation) instead of a fixed timeout. Locate the page.waitForTimeout call in test-project-update-tenant.mjs and change it to an explicit Playwright wait such as page.waitForSelector('<appropriate-selector>'), page.waitForResponse(resp => resp.url().includes('<endpoint>') && resp.status() === 200), or page.waitForFunction(() => /* condition */) that matches the UI update triggered by the action before the console.log('Custom routes toggled'). Ensure the selector/response/predicate uniquely identifies the completed state so the test is stable across CI and local runs.
51-68: 建议为fetch请求添加超时控制。
adminAPI中的fetch调用没有超时设置，在 CI 环境中如果服务器无响应，测试可能会无限期挂起。♻️ 建议使用 AbortController 添加超时
async function adminAPI(method, path, body, token) { const url = `${BASE}/api/admin${path}`; const headers = { 'Content-Type': 'application/json' }; if (token) headers['Authorization'] = `Bearer ${token}`; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + - const res = await fetch(url, { + const res = await fetch(url, { method, headers, body: body ? JSON.stringify(body) : undefined, + signal: controller.signal, }); + + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/playwright/test-project-update-tenant.mjs` around lines 51 - 68, The fetch call that produces const res (inside the adminAPI request flow using method, headers, body) lacks a timeout and can hang in CI; wrap the fetch with an AbortController: create controller and timer before calling fetch, pass controller.signal to fetch, clear the timer after fetch completes, and on timeout call controller.abort() and throw a clear timeout error (handle AbortError separately so the existing JSON/text parsing and res.ok handling still work). Ensure the timer duration is configurable or a reasonable default and reference the existing symbols (fetch, const res, method, headers, body, json, text) when updating the request logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/playwright/test-project-update-tenant.mjs`:
- Around line 166-167: The hard-coded sleep using page.waitForTimeout(500)
causes flakiness; replace it with an explicit wait that reflects the real
condition (e.g., wait for a specific DOM change, network response, or
navigation) instead of a fixed timeout. Locate the page.waitForTimeout call in
test-project-update-tenant.mjs and change it to an explicit Playwright wait such
as page.waitForSelector('<appropriate-selector>'), page.waitForResponse(resp =>
resp.url().includes('<endpoint>') && resp.status() === 200), or
page.waitForFunction(() => /* condition */) that matches the UI update triggered
by the action before the console.log('Custom routes toggled'). Ensure the
selector/response/predicate uniquely identifies the completed state so the test
is stable across CI and local runs.
- Around line 51-68: The fetch call that produces const res (inside the adminAPI
request flow using method, headers, body) lacks a timeout and can hang in CI;
wrap the fetch with an AbortController: create controller and timer before
calling fetch, pass controller.signal to fetch, clear the timer after fetch
completes, and on timeout call controller.abort() and throw a clear timeout
error (handle AbortError separately so the existing JSON/text parsing and res.ok
handling still work). Ensure the timer duration is configurable or a reasonable
default and reference the existing symbols (fetch, const res, method, headers,
body, json, text) when updating the request logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71753cbc-b9aa-41b4-9db9-c1d48bcda291
📒 Files selected for processing (4)
internal/handler/admin.gointernal/repository/sqlite/migrations.gotests/e2e/playwright/test-project-update-tenant.mjsweb/src/components/auth/admin-route.tsx
📜 Review details
🔇 Additional comments (7)
web/src/components/auth/admin-route.tsx (1)
22-24: LGTM！可选链操作符增强了代码的防御性。虽然第 14 行的守卫条件
authEnabled && !user理论上已确保此处user不为undefined，但使用user?.role可防止潜在的竞态条件或未来重构引入的问题。internal/repository/sqlite/migrations.go (1)
186-210: LGTM！迁移逻辑实现正确，与现有 v3 迁移风格保持一致。几点确认：
defaultTenantID的回退逻辑正确处理了查询错误和零值两种情况- 错误处理方式（记录警告但继续执行）与 v3 迁移一致，避免因单表失败阻塞整个迁移
Down函数为空操作是合理的，因为数据修复具有幂等性，且无法可靠地恢复原始的零值状态tests/e2e/playwright/test-project-update-tenant.mjs (1)
86-230: 测试覆盖全面，有效验证了 TenantID 保留逻辑。测试流程设计合理：
- API 和 UI 双重验证确保修复的完整性
- 覆盖了两种 PUT 触发场景（路由切换和名称编辑）
- 清理逻辑健壮，能处理部分失败情况
internal/handler/admin.go (4)
208-211: LGTM！正确保留了 Provider 的 TenantID。修复逻辑正确：从数据库获取的
existing对象确保了 TenantID 的正确性，然后将其复制到解码后的新对象中。这与ID和CreatedAt的保留模式一致。
465-467: LGTM！Project 的 TenantID 保留修复正确。
640-642: LGTM！RetryConfig 的 TenantID 保留修复正确。
711-713: LGTM！RoutingStrategy 的 TenantID 保留修复正确。所有四个受影响的资源（Provider、Project、RetryConfig、RoutingStrategy）都已一致地应用了修复。其他资源（如 Routes、APITokens、ModelMappings）使用部分更新模式，本身就保留了现有字段，因此无需修改。
Contributor
|
@coderabbitai 作为代码逻辑审查助手，判断未按建议修复直接合并的后果，仅输出：是否影响正常逻辑（是/否）、影响原因、建议 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 52376
是否影响正常逻辑：否 影响原因： 两轮 Review 中所有未修复的建议均为 Nitpick（改进建议）级别，且全部集中在 E2E 测试文件
核心业务代码的所有 Review 注释（ 建议： 可直接合并，但建议在后续 PR 中对 |
ymkiux
approved these changes
Mar 10, 2026
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
3 participants
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.
Summary
IDandCreatedAtfrom the existing record, missingTenantIDtenantID,gorm.Saveoverwrotetenant_idto 0, making the resource invisible to tenant-scoped queries — this is why projects "disappear" after toggling custom routes or editing name/slugTenantID = existing.TenantIDpreservation in all 4 affected PUT handlersTest plan
node tests/e2e/playwright/test-project-update-tenant.mjsagainst a running instance🤖 Generated with Claude Code
Summary by CodeRabbit
发布说明