-
-
Notifications
You must be signed in to change notification settings - Fork 179
fix: 修复供应商克隆时因浅拷贝引用共享导致源供应商数据被意外污染的问题 #767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,8 @@ export function createInitialState( | |
| } | ||
| ): ProviderFormState { | ||
| const isEdit = mode === "edit"; | ||
| const sourceProvider = isEdit ? provider : cloneProvider; | ||
| const raw = isEdit ? provider : cloneProvider; | ||
| const sourceProvider = raw ? structuredClone(raw) : undefined; | ||
|
|
||
| return { | ||
| basic: { | ||
|
|
@@ -322,11 +323,13 @@ export function providerFormReducer( | |
| return { ...state, ui: { ...state.ui, showFailureThresholdConfirm: action.payload } }; | ||
|
|
||
| // Reset | ||
| case "RESET_FORM": | ||
| case "RESET_FORM": { | ||
| const fresh = structuredClone(defaultInitialState); | ||
| return { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] [TEST-MISSING-CRITICAL] RESET_FORM deep-copy change is untested Why this is a problem: This PR changes reset behavior to avoid shared references ( Violated guideline: "Test Coverage - All new features must have unit test coverage of at least 80%" (CLAUDE.md) Suggested fix (extend import { describe, expect, it } from "vitest";
import {
createInitialState,
providerFormReducer,
} from "@/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context";
describe("providerFormReducer RESET_FORM deep-copy safety", () => {
it("returns fresh nested objects on each reset", () => {
const base = createInitialState("create");
const s1 = providerFormReducer(base, { type: "RESET_FORM" });
const s2 = providerFormReducer(base, { type: "RESET_FORM" });
expect(s1.routing.modelRedirects).not.toBe(s2.routing.modelRedirects);
expect(s1.routing.allowedModels).not.toBe(s2.routing.allowedModels);
expect(s1.routing.groupPriorities).not.toBe(s2.routing.groupPriorities);
});
it("preserves activeTab", () => {
const withTab = providerFormReducer(createInitialState("create"), {
type: "SET_ACTIVE_TAB",
payload: "routing",
});
const reset = providerFormReducer(withTab, { type: "RESET_FORM" });
expect(reset.ui.activeTab).toBe("routing");
});
}); |
||
| ...defaultInitialState, | ||
| ui: { ...defaultInitialState.ui, activeTab: state.ui.activeTab }, | ||
| ...fresh, | ||
| ui: { ...fresh.ui, activeTab: state.ui.activeTab }, | ||
| }; | ||
| } | ||
hank9999 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Load provider data | ||
| case "LOAD_PROVIDER": | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { createInitialState } from "@/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context"; | ||
| import type { ProviderDisplay } from "@/types/provider"; | ||
|
|
||
| function makeProvider(overrides?: Partial<ProviderDisplay>): ProviderDisplay { | ||
| return { | ||
| id: 1, | ||
| name: "TestProvider", | ||
| url: "https://api.example.com", | ||
| maskedKey: "sk-****1234", | ||
| isEnabled: true, | ||
| weight: 1, | ||
hank9999 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| priority: 0, | ||
| groupPriorities: { groupA: 10, groupB: 20 }, | ||
| costMultiplier: 1.0, | ||
| groupTag: "groupA,groupB", | ||
| providerType: "claude", | ||
| providerVendorId: null, | ||
| preserveClientIp: false, | ||
| modelRedirects: { "claude-3": "claude-3.5" }, | ||
| allowedModels: ["claude-3", "claude-3.5"], | ||
| mcpPassthroughType: "none", | ||
| mcpPassthroughUrl: null, | ||
| limit5hUsd: null, | ||
| limitDailyUsd: null, | ||
| dailyResetMode: "fixed", | ||
| dailyResetTime: "00:00", | ||
| limitWeeklyUsd: null, | ||
| limitMonthlyUsd: null, | ||
| limitTotalUsd: null, | ||
| limitConcurrentSessions: 0, | ||
| maxRetryAttempts: null, | ||
| circuitBreakerFailureThreshold: 3, | ||
| circuitBreakerOpenDuration: 60000, | ||
| circuitBreakerHalfOpenSuccessThreshold: 2, | ||
| proxyUrl: null, | ||
| proxyFallbackToDirect: false, | ||
| firstByteTimeoutStreamingMs: 30000, | ||
| streamingIdleTimeoutMs: 60000, | ||
| requestTimeoutNonStreamingMs: 120000, | ||
| websiteUrl: null, | ||
| faviconUrl: null, | ||
| cacheTtlPreference: null, | ||
| context1mPreference: null, | ||
| codexReasoningEffortPreference: null, | ||
| codexReasoningSummaryPreference: null, | ||
| codexTextVerbosityPreference: null, | ||
| codexParallelToolCallsPreference: null, | ||
| anthropicMaxTokensPreference: null, | ||
| anthropicThinkingBudgetPreference: null, | ||
| anthropicAdaptiveThinking: { | ||
| effort: "high", | ||
| modelMatchMode: "specific", | ||
| models: ["claude-opus-4-6"], | ||
| }, | ||
| geminiGoogleSearchPreference: null, | ||
| tpm: null, | ||
| rpm: null, | ||
| rpd: null, | ||
| cc: null, | ||
| createdAt: "2025-01-01T00:00:00.000Z", | ||
| updatedAt: "2025-01-01T00:00:00.000Z", | ||
| ...overrides, | ||
| } as ProviderDisplay; | ||
| } | ||
|
|
||
| describe("createInitialState deep-copy safety", () => { | ||
| describe("clone mode", () => { | ||
| it("modelRedirects is a distinct object with equal values", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.modelRedirects).toEqual(source.modelRedirects); | ||
| expect(state.routing.modelRedirects).not.toBe(source.modelRedirects); | ||
| }); | ||
|
|
||
| it("allowedModels is a distinct array with equal values", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.allowedModels).toEqual(source.allowedModels); | ||
| expect(state.routing.allowedModels).not.toBe(source.allowedModels); | ||
| }); | ||
|
|
||
| it("groupPriorities is a distinct object with equal values", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.groupPriorities).toEqual(source.groupPriorities); | ||
| expect(state.routing.groupPriorities).not.toBe(source.groupPriorities); | ||
| }); | ||
|
|
||
| it("anthropicAdaptiveThinking is a distinct object with distinct models array", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.anthropicAdaptiveThinking).toEqual(source.anthropicAdaptiveThinking); | ||
| expect(state.routing.anthropicAdaptiveThinking).not.toBe(source.anthropicAdaptiveThinking); | ||
| expect(state.routing.anthropicAdaptiveThinking!.models).not.toBe( | ||
| source.anthropicAdaptiveThinking!.models | ||
| ); | ||
| }); | ||
|
|
||
| it("null anthropicAdaptiveThinking stays null", () => { | ||
| const source = makeProvider({ anthropicAdaptiveThinking: null }); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.anthropicAdaptiveThinking).toBeNull(); | ||
| }); | ||
|
|
||
| it("null modelRedirects falls back to empty object", () => { | ||
| const source = makeProvider({ modelRedirects: null }); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.modelRedirects).toEqual({}); | ||
| }); | ||
|
|
||
| it("null allowedModels falls back to empty array", () => { | ||
| const source = makeProvider({ allowedModels: null }); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.allowedModels).toEqual([]); | ||
| }); | ||
|
|
||
| it("null groupPriorities falls back to empty object", () => { | ||
| const source = makeProvider({ groupPriorities: null }); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.routing.groupPriorities).toEqual({}); | ||
| }); | ||
|
|
||
| it("name gets _Copy suffix", () => { | ||
| const source = makeProvider({ name: "MyProvider" }); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.basic.name).toBe("MyProvider_Copy"); | ||
| }); | ||
|
|
||
| it("key is always empty", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("create", undefined, source); | ||
| expect(state.basic.key).toBe(""); | ||
| }); | ||
| }); | ||
|
|
||
| describe("edit mode", () => { | ||
| it("nested objects are isolated from source provider", () => { | ||
| const source = makeProvider(); | ||
| const state = createInitialState("edit", source); | ||
| expect(state.routing.modelRedirects).toEqual(source.modelRedirects); | ||
| expect(state.routing.modelRedirects).not.toBe(source.modelRedirects); | ||
| expect(state.routing.allowedModels).not.toBe(source.allowedModels); | ||
| expect(state.routing.groupPriorities).not.toBe(source.groupPriorities); | ||
| expect(state.routing.anthropicAdaptiveThinking).not.toBe(source.anthropicAdaptiveThinking); | ||
| }); | ||
| }); | ||
|
|
||
| describe("create mode without clone source", () => { | ||
| it("nested objects use fresh defaults", () => { | ||
| const state = createInitialState("create"); | ||
| expect(state.routing.modelRedirects).toEqual({}); | ||
| expect(state.routing.allowedModels).toEqual([]); | ||
| expect(state.routing.groupPriorities).toEqual({}); | ||
| expect(state.routing.anthropicAdaptiveThinking).toBeNull(); | ||
| }); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Medium] [TEST-MISSING-CRITICAL] No test coverage for the Why this is a problem: The PR description explicitly identifies the Per CLAUDE.md: "All new features must have unit test coverage of at least 80%" Suggested fix: Add test cases for the import { providerFormReducer, createInitialState } from "@/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context";
describe("providerFormReducer RESET_FORM", () => {
it("returns a state with no shared references across resets", () => {
const initial = createInitialState("create");
const reset1 = providerFormReducer(initial, { type: "RESET_FORM" });
const reset2 = providerFormReducer(initial, { type: "RESET_FORM" });
expect(reset1.routing.modelRedirects).not.toBe(reset2.routing.modelRedirects);
expect(reset1.routing.allowedModels).not.toBe(reset2.routing.allowedModels);
expect(reset1.routing.groupPriorities).not.toBe(reset2.routing.groupPriorities);
});
it("preserves the active tab from current state", () => {
const initial = createInitialState("create");
const withTab = { ...initial, ui: { ...initial.ui, activeTab: "routing" as const } };
const reset = providerFormReducer(withTab, { type: "RESET_FORM" });
expect(reset.ui.activeTab).toBe("routing");
});
}); |
||
Uh oh!
There was an error while loading. Please reload this page.