uncaged/json-cas
Archived
10
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: apply PR #33 Review Round 2 fixes

Browse Source 
Addresses Review Round 2 feedback for variable model refactor:

1. Remove create() method - set() is now the unified entry point
2. Remove VariableDuplicateError class (only used by create())
3. Clean up dead code: Array.isArray(existing) checks in update()/remove()/tag()
4. Add tag/label conflict validation to set() update path
5. Migrate gc.test.ts from create() to set()

Changes:
- Delete create() method (lines 381-467) and VariableDuplicateError class
- Remove Array.isArray checks from 3 methods (always null, never array)
- Remove orphaned delete() JSDoc comment
- Add 3 new tests for set() update path tag/label conflict validation
- Replace 10 create() calls with set() in gc.test.ts

Test results: 193 pass (190 existing + 3 new)
Build: clean, Lint: clean

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
小橘
2026-05-30 13:25:01 +00:00
parent 31f84a7ab0
commit 5e7db0ef6b
4 changed files with 103 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/json-cas/src/gc.test.ts
+10 -10
View File
@@ -39,7 +39,7 @@ describe("GC - Variable Model Refactoring", () => {
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
varStore.create("config", hashRef);
varStore.set("config", hashRef);
const stats = gc(store, varStore);
@@ -66,8 +66,8 @@ describe("GC - Variable Model Refactoring", () => {
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
varStore.create("config", hashA);
varStore.create("config", hashB);
varStore.set("config", hashA);
varStore.set("config", hashB);
const stats = gc(store, varStore);
@@ -90,7 +90,7 @@ describe("GC - Variable Model Refactoring", () => {
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
varStore.create("config", hashRef);
varStore.set("config", hashRef);
varStore.remove("config", schemaHash);
const stats = gc(store, varStore);
@@ -117,9 +117,9 @@ describe("GC - Variable Model Refactoring", () => {
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
varStore.create("uwf.thread", hash1);
varStore.create("uwf.workflow", hash2);
varStore.create("app.config", hash3);
varStore.set("uwf.thread", hash1);
varStore.set("uwf.workflow", hash2);
varStore.set("app.config", hash3);
const stats = gc(store, varStore);
@@ -151,9 +151,9 @@ describe("GC - Variable Model Refactoring", () => {
const varStore = new VariableStore(dbPath, store);
// Create variables
varStore.create("var1", hashA1);
varStore.create("var2", hashA2);
varStore.create("var3", hashB);
varStore.set("var1", hashA1);
varStore.set("var2", hashA2);
varStore.set("var3", hashB);
// First GC: orphans removed
let stats = gc(store, varStore);
packages/json-cas/src/index.ts
-1
View File
@@ -23,7 +23,6 @@ export {
InvalidVariableNameError,
SchemaMismatchError,
TagLabelConflictError,
VariableDuplicateError,
VariableNotFoundError,
VariableStore,
} from "./variable-store.js";
packages/json-cas/src/variable-store.test.ts
+80
View File
@@ -381,6 +381,86 @@ describe("VariableStore - set() Upsert Method", () => {
varStore.close();
});
test("set() throws TagLabelConflictError when updating with tag key that matches new label", async () => {
store = createMemoryStore();
await bootstrap(store);
const schema = { type: "object", properties: { x: { type: "number" } } };
const schemaHash = await putSchema(store, schema);
const hash1 = await store.put(schemaHash, { x: 1 });
const hash2 = await store.put(schemaHash, { x: 2 });
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
// Create with tags
varStore.set("config", hash1, { tags: { env: "prod" } });
// Try to update with conflicting tag/label
expect(() => {
varStore.set("config", hash2, {
tags: { region: "us" },
labels: ["region"], // conflicts with tag key
});
}).toThrow(TagLabelConflictError);
varStore.close();
});
test("set() throws TagLabelConflictError when updating with label that matches new tag key", async () => {
store = createMemoryStore();
await bootstrap(store);
const schema = { type: "object", properties: { x: { type: "number" } } };
const schemaHash = await putSchema(store, schema);
const hash1 = await store.put(schemaHash, { x: 1 });
const hash2 = await store.put(schemaHash, { x: 2 });
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
// Create with labels
varStore.set("config", hash1, { labels: ["production"] });
// Try to update with conflicting label/tag
expect(() => {
varStore.set("config", hash2, {
tags: { production: "true" }, // conflicts with existing label "production"
// labels not provided - existing ["production"] preserved, causing conflict
});
}).toThrow(TagLabelConflictError);
varStore.close();
});
test("set() allows updating tags/labels when no conflicts", async () => {
store = createMemoryStore();
await bootstrap(store);
const schema = { type: "object", properties: { x: { type: "number" } } };
const schemaHash = await putSchema(store, schema);
const hash1 = await store.put(schemaHash, { x: 1 });
const hash2 = await store.put(schemaHash, { x: 2 });
dbPath = tmpDbPath();
const varStore = new VariableStore(dbPath, store);
// Create with tags and labels
varStore.set("config", hash1, {
tags: { env: "dev" },
labels: ["experimental"],
});
// Update with different tags/labels (no conflicts)
const updated = varStore.set("config", hash2, {
tags: { region: "us", version: "2" },
labels: ["stable", "reviewed"],
});
expect(updated.tags).toEqual({ region: "us", version: "2" });
expect(updated.labels).toEqual(["stable", "reviewed"]);
expect(updated.value).toBe(hash2);
varStore.close();
});
});
describe("VariableStore - get() with Optional Schema", () => {
packages/json-cas/src/variable-store.ts
+13 -109
View File
@@ -15,18 +15,6 @@ export class VariableNotFoundError extends Error {
}
}
export class VariableDuplicateError extends Error {
constructor(
public variableName: string,
public variableSchema: Hash,
) {
super(
`Variable already exists: name=${variableName}, schema=${variableSchema}`,
);
this.name = "VariableDuplicateError";
}
}
export class InvalidVariableNameError extends Error {
constructor(
public variableName: string,
@@ -245,6 +233,16 @@ export class VariableStore {
const tags = options?.tags ?? existing.tags;
const labels = options?.labels ?? existing.labels;
// Check for tag/label conflicts when updating with new options
if (options !== undefined) {
const tagKeys = Object.keys(tags);
for (const key of tagKeys) {
if (labels.includes(key)) {
throw new TagLabelConflictError(key, "label", "tag");
}
}
}
this.db.exec("BEGIN TRANSACTION");
try {
@@ -375,97 +373,6 @@ export class VariableStore {
};
}
/**
* Create a new variable
*/
create(
name: string,
value: string,
options?: {
tags?: Record<string, string>;
labels?: string[];
},
): Variable {
// Validate name format
this.validateName(name);
const schema = this.extractSchema(value);
const tags = options?.tags ?? {};
const labels = options?.labels ?? [];
// Check for tag/label conflicts
const tagKeys = Object.keys(tags);
for (const key of tagKeys) {
if (labels.includes(key)) {
throw new TagLabelConflictError(key, "label", "tag");
}
}
const now = Date.now();
this.db.exec("BEGIN TRANSACTION");
try {
const stmt = this.db.prepare(`
INSERT INTO variables (name, schema, value, created, updated)
VALUES (?, ?, ?, ?, ?)
`);
try {
stmt.run(name, schema, value, now, now);
} catch (e: unknown) {
if (
e !== null &&
typeof e === "object" &&
"message" in e &&
typeof e.message === "string" &&
e.message.includes("UNIQUE constraint failed")
) {
throw new VariableDuplicateError(name, schema);
}
throw e;
}
// Insert tags
if (tagKeys.length > 0) {
const tagStmt = this.db.prepare(`
INSERT INTO variable_tags (variable_name, variable_schema, key, value)
VALUES (?, ?, ?, ?)
`);
for (const [key, val] of Object.entries(tags)) {
tagStmt.run(name, schema, key, val);
}
}
// Insert labels
if (labels.length > 0) {
const labelStmt = this.db.prepare(`
INSERT INTO variable_labels (variable_name, variable_schema, name)
VALUES (?, ?, ?)
`);
for (const labelName of labels) {
labelStmt.run(name, schema, labelName);
}
}
this.db.exec("COMMIT");
} catch (e) {
this.db.exec("ROLLBACK");
throw e;
}
return {
name,
schema,
value,
created: now,
updated: now,
tags,
labels: [...labels],
};
}
/**
* Get a variable by name, optionally with schema
*/
@@ -520,7 +427,7 @@ export class VariableStore {
this.validateName(name);
const existing = this.get(name, schema);
if (existing === null || Array.isArray(existing)) {
if (existing === null) {
throw new VariableNotFoundError(name, schema);
}
@@ -555,7 +462,7 @@ export class VariableStore {
if (schema !== undefined) {
// Remove specific (name, schema) variant
const existing = this.get(name, schema);
if (existing === null || Array.isArray(existing)) {
if (existing === null) {
throw new VariableNotFoundError(name, schema);
}
@@ -584,9 +491,6 @@ export class VariableStore {
return variants;
}
/**
* Delete a variable (deprecated: use remove() instead)
*/
/**
* List variables with optional filters
*/
@@ -790,7 +694,7 @@ export class VariableStore {
// Return updated variable
const updated = this.get(name, schema);
if (updated === null || Array.isArray(updated)) {
if (updated === null) {
throw new VariableNotFoundError(name, schema);
}
return updated;