refactor: use pre-defined utils

This commit is contained in:
mingholy.lmh 2026-03-02 12:38:26 +08:00
parent d0c1547c60
commit d38077423d
6 changed files with 313 additions and 274 deletions

View file

@ -12,6 +12,7 @@ import {
V1_TO_V2_PRESERVE_DISABLE_MAP,
V2_CONTAINER_KEYS,
} from './v1-to-v2-shared.js';
import { setNestedPropertySafe } from '../../../utils/settingsUtils.js';
/**
* Heuristic indicators for deciding whether an object is "V1-like".
@ -29,43 +30,6 @@ import {
* - Primitive/array/null values on indicator keys are treated as legacy V1 signals.
*/
/**
* Best-effort nested setter with collision protection.
*
* Strategy:
* - Create missing intermediate objects while traversing the path.
* - If traversal hits a non-object parent, abort this write.
*
* Rationale:
* - Migration should never coerce scalars into objects implicitly because that can
* destroy user data shape. Skipping the write is safer; later carry-over logic
* preserves original values.
*/
function setNestedProperty(
obj: Record<string, unknown>,
path: string,
value: unknown,
): void {
const keys = path.split('.');
const lastKey = keys.pop();
if (!lastKey) return;
let current: Record<string, unknown> = obj;
for (const key of keys) {
if (current[key] === undefined) {
current[key] = {};
}
const next = current[key];
if (typeof next === 'object' && next !== null) {
current = next as Record<string, unknown>;
} else {
// Path collision with non-object, stop here
return;
}
}
current[lastKey] = value;
}
/**
* V1 -> V2 migration (structural normalization stage).
*
@ -200,10 +164,14 @@ export class V1ToV2Migration implements SettingsMigration {
) {
// Merge nested properties from this partial V2 structure
for (const [nestedKey, nestedValue] of Object.entries(value)) {
setNestedProperty(result, `${v2Path}.${nestedKey}`, nestedValue);
setNestedPropertySafe(
result,
`${v2Path}.${nestedKey}`,
nestedValue,
);
}
} else {
setNestedProperty(result, v2Path, value);
setNestedPropertySafe(result, v2Path, value);
}
processedKeys.add(v1Key);
}
@ -218,10 +186,10 @@ export class V1ToV2Migration implements SettingsMigration {
if (CONSOLIDATED_DISABLE_KEYS.has(v1Key)) {
// Preserve stable behavior: consolidated keys use presence semantics.
// Only literal true remains true; all other present values become false.
setNestedProperty(result, v2Path, value === true);
setNestedPropertySafe(result, v2Path, value === true);
} else if (typeof value === 'boolean') {
// Non-consolidated disable* keys only migrate when explicitly boolean.
setNestedProperty(result, v2Path, value);
setNestedPropertySafe(result, v2Path, value);
}
processedKeys.add(v1Key);
}
@ -274,7 +242,7 @@ export class V1ToV2Migration implements SettingsMigration {
},
);
if (!wasProcessed) {
setNestedProperty(result, fullNestedPath, nestedValue);
setNestedPropertySafe(result, fullNestedPath, nestedValue);
}
}
} else {

View file

@ -272,173 +272,198 @@ describe('V2ToV3Migration', () => {
});
});
it('should preserve string "true" without creating enable key', () => {
it('should coerce string "true" and remove deprecated key', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 'true' },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe('true');
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
).toBe(false);
expect(warnings).toHaveLength(0);
});
it('should preserve string "false" without creating enable key', () => {
it('should coerce string "false" and remove deprecated key', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 'false' },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe('false');
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBe(true);
expect(warnings).toHaveLength(0);
});
it('should coerce case-insensitive strings for consolidated keys', () => {
const v2Settings = {
$version: 2,
general: {
disableAutoUpdate: 'TRUE',
disableUpdateNag: 'FALSE',
},
};
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBeUndefined();
});
it('should preserve string "TRUE" without creating enable key', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 'TRUE' },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
settings: Record<string, unknown>;
warnings: unknown[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe('TRUE');
});
it('should preserve string "FALSE" without creating enable key', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 'FALSE' },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
settings: Record<string, unknown>;
warnings: unknown[];
};
expect(result['$version']).toBe(3);
(result['general'] as Record<string, unknown>)['disableUpdateNag'],
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe('FALSE');
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBe(false);
expect(warnings).toHaveLength(0);
});
it('should preserve number value and not create enable key', () => {
it('should remove number value and emit warning', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 123 },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe(123);
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableAutoUpdate');
});
it('should preserve invalid string value and not create enable key', () => {
it('should remove invalid string value and emit warning', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: 'invalid-string' },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBe('invalid-string');
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableAutoUpdate');
});
it('should preserve disableCacheControl string "true"', () => {
it('should coerce disableCacheControl string "true"', () => {
const v2Settings = {
$version: 2,
model: { generationConfig: { disableCacheControl: 'true' } },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['model'] as Record<string, unknown>)['generationConfig'],
).toEqual({
disableCacheControl: 'true',
enableCacheControl: false,
});
expect(warnings).toHaveLength(0);
});
it('should preserve disableCacheControl string "false"', () => {
it('should coerce disableCacheControl string "false"', () => {
const v2Settings = {
$version: 2,
model: { generationConfig: { disableCacheControl: 'false' } },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['model'] as Record<string, unknown>)['generationConfig'],
).toEqual({
disableCacheControl: 'false',
enableCacheControl: true,
});
expect(warnings).toHaveLength(0);
});
it('should preserve disableCacheControl number value and not create enable key', () => {
it('should remove disableCacheControl number value and emit warning', () => {
const v2Settings = {
$version: 2,
model: { generationConfig: { disableCacheControl: 456 } },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['model'] as Record<string, unknown>)['generationConfig'],
).toEqual({ disableCacheControl: 456 });
).toEqual({});
expect(
(
(result['model'] as Record<string, unknown>)[
@ -446,6 +471,10 @@ describe('V2ToV3Migration', () => {
] as Record<string, unknown>
)['enableCacheControl'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain(
'model.generationConfig.disableCacheControl',
);
});
it('should handle mixed valid and invalid disableAutoUpdate and disableUpdateNag', () => {
@ -457,9 +486,12 @@ describe('V2ToV3Migration', () => {
},
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
@ -473,67 +505,84 @@ describe('V2ToV3Migration', () => {
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['disableUpdateNag'],
).toBe('invalid');
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableUpdateNag');
});
it('should preserve object value for disable key', () => {
it('should remove object value for disable key and emit warning', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: { nested: 'value' } },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toEqual({ nested: 'value' });
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableAutoUpdate');
});
it('should preserve array value for disable key', () => {
it('should remove array value for disable key and emit warning', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: [1, 2, 3] },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toEqual([1, 2, 3]);
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableAutoUpdate');
});
it('should preserve null value for disable key', () => {
it('should remove null value for disable key and emit warning', () => {
const v2Settings = {
$version: 2,
general: { disableAutoUpdate: null },
};
const { settings: result } = migration.migrate(v2Settings, 'user') as {
const { settings: result, warnings } = migration.migrate(
v2Settings,
'user',
) as {
settings: Record<string, unknown>;
warnings: unknown[];
warnings: string[];
};
expect(result['$version']).toBe(3);
expect(
(result['general'] as Record<string, unknown>)['disableAutoUpdate'],
).toBeNull();
).toBeUndefined();
expect(
(result['general'] as Record<string, unknown>)['enableAutoUpdate'],
).toBeUndefined();
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('general.disableAutoUpdate');
});
});

View file

@ -5,14 +5,23 @@
*/
import type { SettingsMigration } from '../types.js';
import {
deleteNestedPropertySafe,
getNestedProperty,
setNestedPropertySafe,
} from '../../../utils/settingsUtils.js';
/**
* Path mapping for boolean polarity migration (V2 disable* -> V3 enable*).
*
* Strategy:
* - For each mapped path, only boolean inputs are transformed.
* - For each mapped path, values are normalized before migration:
* - boolean values are accepted directly
* - string values "true"/"false" (case-insensitive, trim-aware) are coerced
* - all other present values are treated as invalid
* - Transformation is inversion-based: disable=true -> enable=false, disable=false -> enable=true.
* - Non-boolean values are intentionally ignored here to preserve stable compatibility.
* - Deprecated disable* keys are removed whenever present (valid or invalid).
* - Invalid values do not create enable* keys and produce warnings.
*/
const V2_TO_V3_BOOLEAN_MAP: Record<string, string> = {
'general.disableAutoUpdate': 'general.enableAutoUpdate',
@ -31,8 +40,9 @@ const V2_TO_V3_BOOLEAN_MAP: Record<string, string> = {
* Current policy:
* - `general.disableAutoUpdate` and `general.disableUpdateNag` both drive
* `general.enableAutoUpdate`.
* - If any observed boolean source is true, target becomes false.
* - If no boolean source exists, consolidation does not emit a target value.
* - If any valid normalized source is true, target becomes false.
* - If at least one valid normalized source exists, consolidated target is emitted.
* - Invalid present values are removed and warned, and do not contribute to target calculation.
*/
const CONSOLIDATED_V2_PATHS: Record<string, string[]> = {
'general.enableAutoUpdate': [
@ -42,86 +52,34 @@ const CONSOLIDATED_V2_PATHS: Record<string, string[]> = {
};
/**
* Safe nested getter used during migration path inspection.
* Normalizes deprecated disable* values for migration.
*
* Returns `undefined` when traversal cannot continue (missing key or non-object parent).
* Returns:
* - `isPresent=false` when the path does not exist
* - `isPresent=true, isValid=true` when value is boolean or coercible string
* - `isPresent=true, isValid=false` for invalid values (number/object/array/null/other strings)
*/
function getNestedProperty(
obj: Record<string, unknown>,
path: string,
): unknown {
const keys = path.split('.');
let current: unknown = obj;
for (const key of keys) {
if (typeof current !== 'object' || current === null || !(key in current)) {
return undefined;
}
current = (current as Record<string, unknown>)[key];
function normalizeDisableValue(value: unknown): {
isPresent: boolean;
isValid: boolean;
booleanValue?: boolean;
} {
if (value === undefined) {
return { isPresent: false, isValid: false };
}
return current;
}
/**
* Safe nested setter used for emitting migrated paths.
*
* Behavior:
* - Creates intermediate objects when absent.
* - Aborts write if a parent segment is a non-object (collision protection).
*/
function setNestedProperty(
obj: Record<string, unknown>,
path: string,
value: unknown,
): void {
const keys = path.split('.');
const lastKey = keys.pop();
if (!lastKey) return;
let current: Record<string, unknown> = obj;
for (const key of keys) {
if (current[key] === undefined) {
current[key] = {};
if (typeof value === 'boolean') {
return { isPresent: true, isValid: true, booleanValue: value };
}
if (typeof value === 'string') {
const normalized = value.trim().toLowerCase();
if (normalized === 'true') {
return { isPresent: true, isValid: true, booleanValue: true };
}
const next = current[key];
if (typeof next === 'object' && next !== null) {
current = next as Record<string, unknown>;
} else {
// Path collision with non-object, stop here
return;
if (normalized === 'false') {
return { isPresent: true, isValid: true, booleanValue: false };
}
}
current[lastKey] = value;
}
/**
* Best-effort nested delete for removing deprecated disable* keys.
*
* If traversal hits a non-object parent, deletion is skipped.
*/
function deleteNestedProperty(
obj: Record<string, unknown>,
path: string,
): void {
const keys = path.split('.');
const lastKey = keys.pop();
if (!lastKey) return;
let current: Record<string, unknown> = obj;
for (const key of keys) {
const next = current[key];
if (typeof next !== 'object' || next === null) {
return;
}
current = next as Record<string, unknown>;
}
delete current[lastKey];
}
/**
* JSON-based deep clone used to keep `migrate()` input immutable.
*/
function deepClone<T>(obj: T): T {
return JSON.parse(JSON.stringify(obj));
return { isPresent: true, isValid: false };
}
/**
@ -129,14 +87,14 @@ function deepClone<T>(obj: T): T {
*
* Migration contract:
* - Input: V2 settings object (`$version: 2`).
* - Output: `$version: 3` with boolean disable* fields migrated to enable* equivalents.
* - Output: `$version: 3` with deprecated disable* fields removed and
* valid values migrated to enable* equivalents.
*
* Compatibility strategy:
* - Transform only boolean-valued deprecated fields.
* - Preserve non-boolean deprecated values untouched.
* - Accept boolean values and coercible strings "true"/"false".
* - Remove invalid deprecated values (rather than preserving them).
* - Emit warnings for each removed invalid deprecated key.
* - Always bump version to 3 so future loads are idempotent and skip repeated checks.
*
* This implementation mirrors stable behavior and prioritizes parity over aggressive cleanup.
*/
export class V2ToV3Migration implements SettingsMigration {
readonly fromVersion = 2;
@ -161,34 +119,38 @@ export class V2ToV3Migration implements SettingsMigration {
}
/**
* Applies V2 -> V3 transformation with stable-compatible rules.
* Applies V2 -> V3 transformation with deterministic deprecated-key cleanup.
*
* Detailed strategy:
* 1) Clone input.
* 2) Process consolidated paths first:
* - Inspect each source path.
* - If value is boolean, consume it (delete old key) and contribute to aggregate.
* - Emit consolidated target when at least one boolean source was consumed.
* - Normalize each present value (boolean / coercible string / invalid).
* - Always delete present deprecated source key.
* - Valid normalized values contribute to aggregate.
* - Invalid values emit warnings.
* - Emit consolidated target when at least one valid source was consumed.
* 3) Process remaining one-to-one mappings:
* - For each unmapped source, if boolean -> delete old key and write inverted target.
* - If non-boolean -> keep old value unchanged.
* - For each unmapped source, normalize value.
* - If valid -> delete old key and write inverted target.
* - If invalid -> delete old key and emit warning.
* 4) Set `$version = 3`.
*
* Guarantees:
* - Input object is not mutated.
* - Boolean migration is deterministic.
* - Non-boolean legacy values are preserved for compatibility.
* - Valid migration and invalid cleanup are deterministic.
* - Deprecated disable* keys are not retained after migration.
*/
migrate(
settings: unknown,
_scope: string,
scope: string,
): { settings: unknown; warnings: string[] } {
if (typeof settings !== 'object' || settings === null) {
throw new Error('Settings must be an object');
}
// Deep clone to avoid mutating input
const result = deepClone(settings) as Record<string, unknown>;
const result = structuredClone(settings) as Record<string, unknown>;
const processedPaths = new Set<string>();
const warnings: string[] = [];
@ -200,19 +162,29 @@ export class V2ToV3Migration implements SettingsMigration {
for (const oldPath of oldPaths) {
const oldValue = getNestedProperty(result, oldPath);
if (typeof oldValue === 'boolean') {
const normalized = normalizeDisableValue(oldValue);
if (!normalized.isPresent) {
continue;
}
deleteNestedPropertySafe(result, oldPath);
processedPaths.add(oldPath);
if (normalized.isValid) {
hasAnyBooleanValue = true;
if (oldValue === true) {
if (normalized.booleanValue === true) {
hasAnyDisable = true;
}
deleteNestedProperty(result, oldPath);
processedPaths.add(oldPath);
} else {
warnings.push(
`Removed deprecated setting '${oldPath}' from ${scope} settings because the value is invalid. Expected boolean.`,
);
}
}
if (hasAnyBooleanValue) {
// enableAutoUpdate = !hasAnyDisable (if any disable* was true, enable should be false)
setNestedProperty(result, newPath, !hasAnyDisable);
setNestedPropertySafe(result, newPath, !hasAnyDisable);
}
}
@ -223,10 +195,19 @@ export class V2ToV3Migration implements SettingsMigration {
}
const oldValue = getNestedProperty(result, oldPath);
if (typeof oldValue === 'boolean') {
deleteNestedProperty(result, oldPath);
const normalized = normalizeDisableValue(oldValue);
if (!normalized.isPresent) {
continue;
}
deleteNestedPropertySafe(result, oldPath);
if (normalized.isValid) {
// Set new property with inverted value
setNestedProperty(result, newPath, !oldValue);
setNestedPropertySafe(result, newPath, !normalized.booleanValue);
} else {
warnings.push(
`Removed deprecated setting '${oldPath}' from ${scope} settings because the value is invalid. Expected boolean or string "true"/"false".`,
);
}
}

View file

@ -31,6 +31,7 @@ import {
getSettingsSchema,
} from './settingsSchema.js';
import { resolveEnvVarsInObject } from '../utils/envVarResolver.js';
import { setNestedPropertySafe } from '../utils/settingsUtils.js';
import { customDeepMerge } from '../utils/deepMerge.js';
import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';
const debugLogger = createDebugLogger('SETTINGS');
@ -124,31 +125,6 @@ export interface SettingsFile {
rawJson?: string;
}
function setNestedProperty(
obj: Record<string, unknown>,
path: string,
value: unknown,
) {
const keys = path.split('.');
const lastKey = keys.pop();
if (!lastKey) return;
let current: Record<string, unknown> = obj;
for (const key of keys) {
if (current[key] === undefined) {
current[key] = {};
}
const next = current[key];
if (typeof next === 'object' && next !== null) {
current = next as Record<string, unknown>;
} else {
// This path is invalid, so we stop.
return;
}
}
current[lastKey] = value;
}
function getSettingsFileKeyWarnings(
settings: Record<string, unknown>,
settingsFilePath: string,
@ -333,8 +309,8 @@ export class LoadedSettings {
setValue(scope: SettingScope, key: string, value: unknown): void {
const settingsFile = this.forScope(scope);
setNestedProperty(settingsFile.settings, key, value);
setNestedProperty(settingsFile.originalSettings, key, value);
setNestedPropertySafe(settingsFile.settings, key, value);
setNestedPropertySafe(settingsFile.originalSettings, key, value);
this._merged = this.computeMergedSettings();
saveSettings(settingsFile);
}