`initializeGuidedPath` opened BEGIN, read the flag, then COMMIT at every mount of the provider: start-up, and every profile switch, creation or deletion. While a `withTransaction` callback runs, `db.ts` lets every other query bypass its lock, the overlap its serialization exists to prevent. The flag is now read first, outside any transaction, through the queued connection: once D9 ran, an opening costs that one read. Without it, the transaction re-reads the flag before writing, so two first loads still cannot apply the rule twice (CWE-362). Pinned: no transaction and no write with the flag set; the flag counts only for "1", on both reads; a first load that sets the flag between the two reads wins; the in-transaction reads still follow BEGIN. Review of #396 (PR #402). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
470 lines
18 KiB
TypeScript
470 lines
18 KiB
TypeScript
import { describe, it, expect, vi, beforeEach } from "vitest";
|
|
|
|
vi.mock("./db", () => {
|
|
const getDb = vi.fn();
|
|
return {
|
|
getDb,
|
|
withTransaction: vi.fn(async (fn: (db: any) => any) => fn(await getDb())),
|
|
};
|
|
});
|
|
|
|
vi.mock("./logService", () => ({
|
|
logInfo: vi.fn(),
|
|
logWarn: vi.fn(),
|
|
logError: vi.fn(),
|
|
}));
|
|
|
|
import { getDb, withTransaction } from "./db";
|
|
import { logInfo } from "./logService";
|
|
import { subscribeDataChanged } from "./dataEvents";
|
|
import {
|
|
ONBOARDING_CHECKLIST_HIDDEN_KEY,
|
|
ONBOARDING_GUIDED_INITIALIZED_KEY,
|
|
getOnboardingSnapshot,
|
|
hideChecklist,
|
|
initializeGuidedPath,
|
|
resumeStep,
|
|
showChecklist,
|
|
skipStep,
|
|
skippedStepKey,
|
|
} from "./onboardingService";
|
|
|
|
const countersRow = {
|
|
sources: 2,
|
|
imports: 3,
|
|
manual_categorized: 4,
|
|
budget_entries: 5,
|
|
transactions: 60,
|
|
};
|
|
|
|
interface FakeDbOptions {
|
|
prefs?: Record<string, string>;
|
|
/** Rows of the aggregated counters query; null = the driver returns no row. */
|
|
counters?: typeof countersRow | null;
|
|
/** Answer of `SELECT COUNT(*) AS transactions` (rule D9). */
|
|
transactions?: number;
|
|
/** Makes the matching `execute` reject, as SQLite would. */
|
|
failOn?: (sql: string, params: unknown[]) => boolean;
|
|
}
|
|
|
|
/**
|
|
* Just enough of `user_preferences` and of a transaction to watch what the
|
|
* service writes: the preferences live in a map, BEGIN keeps a copy that
|
|
* ROLLBACK restores, and every `execute` is logged.
|
|
*/
|
|
function makeFakeDb(options: FakeDbOptions = {}) {
|
|
let prefs: Record<string, string> = { ...options.prefs };
|
|
let beforeTransaction: Record<string, string> | null = null;
|
|
const statements: string[] = [];
|
|
|
|
const select = vi.fn(async (sql: string, params?: unknown[]) => {
|
|
if (sql.includes("COUNT(*) AS transactions FROM transactions")) {
|
|
return [{ transactions: options.transactions ?? 0 }];
|
|
}
|
|
if (sql.includes("LIKE 'onboarding_skipped_%'")) {
|
|
return Object.entries(prefs)
|
|
.filter(([key]) => /^onboarding.skipped./.test(key))
|
|
.map(([key, value]) => ({ key, value }));
|
|
}
|
|
if (sql.includes("FROM user_preferences WHERE key = $1")) {
|
|
const key = String(params?.[0]);
|
|
return key in prefs ? [{ key, value: prefs[key], updated_at: "" }] : [];
|
|
}
|
|
if (sql.includes("FROM import_sources")) {
|
|
const row = options.counters === undefined ? countersRow : options.counters;
|
|
return row === null ? [] : [row];
|
|
}
|
|
throw new Error(`unexpected SELECT: ${sql}`);
|
|
});
|
|
|
|
const execute = vi.fn(async (sql: string, params: unknown[] = []) => {
|
|
statements.push(sql.trim().split(/\s+/).slice(0, 3).join(" "));
|
|
if (options.failOn?.(sql, params)) throw new Error("database is locked");
|
|
if (sql === "BEGIN") {
|
|
beforeTransaction = { ...prefs };
|
|
} else if (sql === "COMMIT") {
|
|
beforeTransaction = null;
|
|
} else if (sql === "ROLLBACK") {
|
|
if (beforeTransaction) prefs = beforeTransaction;
|
|
beforeTransaction = null;
|
|
} else if (sql.includes("INSERT INTO user_preferences")) {
|
|
prefs[String(params[0])] = String(params[1]);
|
|
} else if (sql.includes("DELETE FROM user_preferences")) {
|
|
delete prefs[String(params[0])];
|
|
} else {
|
|
throw new Error(`unexpected execute: ${sql}`);
|
|
}
|
|
return { rowsAffected: 1 };
|
|
});
|
|
|
|
return { db: { select, execute }, statements, prefs: () => prefs };
|
|
}
|
|
|
|
function useFakeDb(options: FakeDbOptions = {}) {
|
|
const fake = makeFakeDb(options);
|
|
vi.mocked(getDb).mockResolvedValue(fake.db as never);
|
|
return fake;
|
|
}
|
|
|
|
beforeEach(() => {
|
|
vi.mocked(getDb).mockReset();
|
|
vi.mocked(withTransaction).mockClear();
|
|
vi.mocked(logInfo).mockReset();
|
|
});
|
|
|
|
describe("getOnboardingSnapshot (#351, #396)", () => {
|
|
it("maps the aggregated row to camelCase counters", async () => {
|
|
useFakeDb();
|
|
const snapshot = await getOnboardingSnapshot();
|
|
expect(snapshot.counters).toEqual({
|
|
sources: 2,
|
|
imports: 3,
|
|
manualCategorized: 4,
|
|
budgetEntries: 5,
|
|
transactions: 60,
|
|
});
|
|
});
|
|
|
|
it("runs the five sub-counts in one query, plus the two preference reads and the skipped steps", async () => {
|
|
const { db } = useFakeDb();
|
|
await getOnboardingSnapshot();
|
|
expect(db.select).toHaveBeenCalledTimes(4);
|
|
const sqls = db.select.mock.calls.map((c) => String(c[0]));
|
|
const aggregate = sqls.find((s) => s.includes("import_sources"))!;
|
|
expect(aggregate).toContain("FROM transactions WHERE is_manually_categorized = 1");
|
|
expect(aggregate).toContain("FROM budget_entries");
|
|
expect(aggregate).toMatch(/\(SELECT COUNT\(\*\) FROM transactions\)\s+AS transactions/);
|
|
const prefKeys = db.select.mock.calls
|
|
.filter((c) => String(c[0]).includes("WHERE key = $1"))
|
|
.map((c) => (c[1] as unknown[])[0]);
|
|
expect(prefKeys).toEqual(
|
|
expect.arrayContaining(["import_folder", ONBOARDING_CHECKLIST_HIDDEN_KEY]),
|
|
);
|
|
expect(sqls.filter((s) => s.includes("LIKE 'onboarding_skipped_%'"))).toHaveLength(1);
|
|
});
|
|
|
|
// The tests run without SQLite, so the rule is pinned on the query itself.
|
|
// `executeImport` writes the file row `completed` before its first insert: a
|
|
// file whose rows were all in error keeps that row and adds no transaction,
|
|
// and must not tick "Import a first file" (review of #398).
|
|
it("counts a completed import only once the file brought a transaction", async () => {
|
|
const { db } = useFakeDb();
|
|
await getOnboardingSnapshot();
|
|
const aggregate = db.select.mock.calls
|
|
.map((c) => String(c[0]))
|
|
.find((s) => s.includes("import_sources"))!;
|
|
expect(aggregate).toMatch(
|
|
/\(SELECT COUNT\(\*\) FROM imported_files f\s+WHERE f\.status = 'completed'\s+AND EXISTS \(SELECT 1 FROM transactions t WHERE t\.file_id = f\.id\)\)\s+AS imports/,
|
|
);
|
|
});
|
|
|
|
it("is a pure read: it never writes, even on a profile D9 would hide", async () => {
|
|
const { db } = useFakeDb({ transactions: 60 });
|
|
const unsubscribe = subscribeDataChanged(() => {
|
|
throw new Error("a snapshot must not announce anything");
|
|
});
|
|
try {
|
|
await getOnboardingSnapshot();
|
|
} finally {
|
|
unsubscribe();
|
|
}
|
|
expect(db.execute).not.toHaveBeenCalled();
|
|
expect(withTransaction).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("reports importFolderSet=false, hidden=false and no skipped step without preferences", async () => {
|
|
useFakeDb();
|
|
const snapshot = await getOnboardingSnapshot();
|
|
expect(snapshot.importFolderSet).toBe(false);
|
|
expect(snapshot.hidden).toBe(false);
|
|
expect(snapshot.skipped).toEqual([]);
|
|
});
|
|
|
|
it("reports importFolderSet=true when a folder is stored", async () => {
|
|
useFakeDb({ prefs: { import_folder: "/home/max/releves" } });
|
|
expect((await getOnboardingSnapshot()).importFolderSet).toBe(true);
|
|
});
|
|
|
|
it("treats an empty import_folder value as unset", async () => {
|
|
useFakeDb({ prefs: { import_folder: "" } });
|
|
expect((await getOnboardingSnapshot()).importFolderSet).toBe(false);
|
|
});
|
|
|
|
it('reports hidden=true only for the value "1"', async () => {
|
|
useFakeDb({ prefs: { [ONBOARDING_CHECKLIST_HIDDEN_KEY]: "1" } });
|
|
expect((await getOnboardingSnapshot()).hidden).toBe(true);
|
|
useFakeDb({ prefs: { [ONBOARDING_CHECKLIST_HIDDEN_KEY]: "0" } });
|
|
expect((await getOnboardingSnapshot()).hidden).toBe(false);
|
|
});
|
|
|
|
it("falls back to zero counters when the driver returns no row", async () => {
|
|
useFakeDb({ counters: null });
|
|
expect((await getOnboardingSnapshot()).counters).toEqual({
|
|
sources: 0,
|
|
imports: 0,
|
|
manualCategorized: 0,
|
|
budgetEntries: 0,
|
|
transactions: 0,
|
|
});
|
|
});
|
|
|
|
it("reads the skipped steps in checklist order", async () => {
|
|
useFakeDb({
|
|
prefs: {
|
|
[skippedStepKey("budget")]: "1",
|
|
[skippedStepKey("source")]: "1",
|
|
},
|
|
});
|
|
expect((await getOnboardingSnapshot()).skipped).toEqual(["source", "budget"]);
|
|
});
|
|
|
|
it("keeps only the six keys: an unknown step, or what LIKE's `_` wildcard lets through, is ignored", async () => {
|
|
useFakeDb({
|
|
prefs: {
|
|
onboarding_skipped_dragons: "1",
|
|
onboarding_skipped_: "1",
|
|
onboarding_skipped_import_extra: "1",
|
|
onboardingXskippedYimport: "1",
|
|
[skippedStepKey("import")]: "1",
|
|
},
|
|
});
|
|
expect((await getOnboardingSnapshot()).skipped).toEqual(["import"]);
|
|
});
|
|
|
|
it('counts a skipped step only for the value "1"', async () => {
|
|
useFakeDb({
|
|
prefs: {
|
|
[skippedStepKey("source")]: "0",
|
|
[skippedStepKey("import")]: "true",
|
|
[skippedStepKey("budget")]: "",
|
|
[skippedStepKey("manualCategory")]: "1",
|
|
},
|
|
});
|
|
expect((await getOnboardingSnapshot()).skipped).toEqual(["manualCategory"]);
|
|
});
|
|
});
|
|
|
|
describe("initializeGuidedPath — rule D9 (#396)", () => {
|
|
it("without the flag and with transactions: hides, then flags, inside one transaction", async () => {
|
|
const fake = useFakeDb({ transactions: 3 });
|
|
await expect(initializeGuidedPath()).resolves.toEqual({ hidden: true });
|
|
|
|
expect(withTransaction).toHaveBeenCalledTimes(1);
|
|
const writes = fake.db.execute.mock.calls.map(([sql, params]) => [
|
|
String(sql).trim().split(/\s+/)[0],
|
|
(params as unknown[] | undefined)?.[0],
|
|
]);
|
|
expect(writes).toEqual([
|
|
["BEGIN", undefined],
|
|
["INSERT", ONBOARDING_CHECKLIST_HIDDEN_KEY],
|
|
["INSERT", ONBOARDING_GUIDED_INITIALIZED_KEY],
|
|
["COMMIT", undefined],
|
|
]);
|
|
// The flag is read once outside any transaction, then again after BEGIN,
|
|
// with the transaction count.
|
|
const begin = fake.db.execute.mock.invocationCallOrder[0];
|
|
const reads = fake.db.select.mock.calls.map(([sql], i) => ({
|
|
sql: String(sql),
|
|
order: fake.db.select.mock.invocationCallOrder[i],
|
|
}));
|
|
const flagReads = reads.filter((r) => r.sql.includes("FROM user_preferences WHERE key = $1"));
|
|
expect(flagReads).toHaveLength(2);
|
|
expect(flagReads[0].order).toBeLessThan(begin);
|
|
expect(flagReads[1].order).toBeGreaterThan(begin);
|
|
const count = reads.find((r) => r.sql.includes("COUNT(*) AS transactions FROM transactions"))!;
|
|
expect(count.order).toBeGreaterThan(begin);
|
|
expect(fake.prefs()).toEqual({
|
|
[ONBOARDING_CHECKLIST_HIDDEN_KEY]: "1",
|
|
[ONBOARDING_GUIDED_INITIALIZED_KEY]: "1",
|
|
});
|
|
expect(logInfo).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("without the flag and without transactions: flags only, the path stays visible", async () => {
|
|
const fake = useFakeDb({ transactions: 0 });
|
|
await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false });
|
|
expect(fake.statements).toEqual(["BEGIN", "INSERT INTO user_preferences", "COMMIT"]);
|
|
expect(fake.prefs()).toEqual({ [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" });
|
|
expect(logInfo).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("with the flag already set: no transaction and no write, even with transactions", async () => {
|
|
const fake = useFakeDb({
|
|
transactions: 3,
|
|
prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" },
|
|
});
|
|
await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false });
|
|
expect(withTransaction).not.toHaveBeenCalled();
|
|
expect(fake.statements).toEqual([]);
|
|
expect(fake.db.select).toHaveBeenCalledTimes(1);
|
|
expect(fake.prefs()).toEqual({ [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" });
|
|
expect(logInfo).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it('counts the flag as set only for the value "1", outside and inside the transaction', async () => {
|
|
for (const value of ["0", "true", ""]) {
|
|
vi.mocked(withTransaction).mockClear();
|
|
const fake = useFakeDb({
|
|
transactions: 3,
|
|
prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: value },
|
|
});
|
|
await expect(initializeGuidedPath()).resolves.toEqual({ hidden: true });
|
|
expect(withTransaction).toHaveBeenCalledTimes(1);
|
|
expect(fake.statements).toEqual([
|
|
"BEGIN",
|
|
"INSERT INTO user_preferences",
|
|
"INSERT INTO user_preferences",
|
|
"COMMIT",
|
|
]);
|
|
expect(fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY]).toBe("1");
|
|
}
|
|
});
|
|
|
|
it("re-reads the flag inside the transaction: a first load that set it in between wins (CWE-362)", async () => {
|
|
const fake = useFakeDb({ transactions: 3 });
|
|
const read = fake.db.select.getMockImplementation()!;
|
|
let flagReads = 0;
|
|
fake.db.select.mockImplementation(async (sql: string, params?: unknown[]) => {
|
|
const rows = await read(sql, params);
|
|
if (params?.[0] === ONBOARDING_GUIDED_INITIALIZED_KEY && ++flagReads === 1) {
|
|
// Another opening runs D9 to its end right after this read.
|
|
fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY] = "1";
|
|
}
|
|
return rows;
|
|
});
|
|
await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false });
|
|
expect(flagReads).toBe(2);
|
|
expect(fake.statements).toEqual(["BEGIN", "COMMIT"]);
|
|
expect(fake.prefs()[ONBOARDING_CHECKLIST_HIDDEN_KEY]).toBeUndefined();
|
|
expect(logInfo).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("a profile shown again after D9 is not hidden a second time", async () => {
|
|
// D9 hid it, the user showed it again from Settings → Users.
|
|
const fake = useFakeDb({ transactions: 3 });
|
|
await initializeGuidedPath();
|
|
await showChecklist();
|
|
await initializeGuidedPath();
|
|
expect(fake.prefs()[ONBOARDING_CHECKLIST_HIDDEN_KEY]).toBeUndefined();
|
|
});
|
|
|
|
it("a failed hiding write rolls back and leaves the flag unset", async () => {
|
|
const fake = useFakeDb({
|
|
transactions: 3,
|
|
failOn: (sql, params) =>
|
|
sql.includes("INSERT INTO user_preferences") && params[0] === ONBOARDING_CHECKLIST_HIDDEN_KEY,
|
|
});
|
|
await expect(initializeGuidedPath()).rejects.toThrow("database is locked");
|
|
expect(fake.statements).toEqual(["BEGIN", "INSERT INTO user_preferences", "ROLLBACK"]);
|
|
expect(fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY]).toBeUndefined();
|
|
expect(fake.prefs()[ONBOARDING_CHECKLIST_HIDDEN_KEY]).toBeUndefined();
|
|
expect(logInfo).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("a failed flag write rolls the hiding back too", async () => {
|
|
const fake = useFakeDb({
|
|
transactions: 3,
|
|
failOn: (sql, params) =>
|
|
sql.includes("INSERT INTO user_preferences") && params[0] === ONBOARDING_GUIDED_INITIALIZED_KEY,
|
|
});
|
|
await expect(initializeGuidedPath()).rejects.toThrow("database is locked");
|
|
expect(fake.statements[fake.statements.length - 1]).toBe("ROLLBACK");
|
|
expect(fake.prefs()).toEqual({});
|
|
});
|
|
|
|
it("the next snapshot comes back hidden", async () => {
|
|
useFakeDb({ transactions: 3, counters: { ...countersRow, transactions: 3 } });
|
|
expect((await getOnboardingSnapshot()).hidden).toBe(false);
|
|
await initializeGuidedPath();
|
|
expect((await getOnboardingSnapshot()).hidden).toBe(true);
|
|
});
|
|
|
|
it("does not announce anything on the data channel", async () => {
|
|
useFakeDb({ transactions: 3 });
|
|
const listener = vi.fn();
|
|
const unsubscribe = subscribeDataChanged(listener);
|
|
try {
|
|
await initializeGuidedPath();
|
|
} finally {
|
|
unsubscribe();
|
|
}
|
|
expect(listener).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
describe("hideChecklist / showChecklist (#351)", () => {
|
|
it('hideChecklist writes "1" under onboarding_checklist_hidden and logs', async () => {
|
|
const fake = useFakeDb();
|
|
await hideChecklist();
|
|
expect(fake.db.execute).toHaveBeenCalledTimes(1);
|
|
const [sql, params] = fake.db.execute.mock.calls[0];
|
|
expect(String(sql)).toContain("INSERT INTO user_preferences");
|
|
expect(params).toEqual([ONBOARDING_CHECKLIST_HIDDEN_KEY, "1"]);
|
|
expect(logInfo).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("showChecklist deletes the preference and logs", async () => {
|
|
const fake = useFakeDb({ prefs: { [ONBOARDING_CHECKLIST_HIDDEN_KEY]: "1" } });
|
|
await showChecklist();
|
|
expect(fake.db.execute).toHaveBeenCalledTimes(1);
|
|
const [sql, params] = fake.db.execute.mock.calls[0];
|
|
expect(String(sql)).toContain("DELETE FROM user_preferences");
|
|
expect(params).toEqual([ONBOARDING_CHECKLIST_HIDDEN_KEY]);
|
|
expect(logInfo).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("does not log when the write fails", async () => {
|
|
useFakeDb({ failOn: () => true });
|
|
await expect(hideChecklist()).rejects.toThrow("database is locked");
|
|
expect(logInfo).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
describe("skipStep / resumeStep (#396, D3)", () => {
|
|
it('skipStep writes "1" under onboarding_skipped_<key>, without reading first', async () => {
|
|
const fake = useFakeDb();
|
|
await skipStep("budget");
|
|
expect(fake.db.select).not.toHaveBeenCalled();
|
|
expect(fake.db.execute).toHaveBeenCalledTimes(1);
|
|
expect(fake.db.execute.mock.calls[0][1]).toEqual(["onboarding_skipped_budget", "1"]);
|
|
expect(fake.prefs()).toEqual({ onboarding_skipped_budget: "1" });
|
|
});
|
|
|
|
it("skipStep is idempotent", async () => {
|
|
const fake = useFakeDb();
|
|
await skipStep("source");
|
|
await skipStep("source");
|
|
expect(fake.prefs()).toEqual({ onboarding_skipped_source: "1" });
|
|
});
|
|
|
|
it("resumeStep deletes the preference, and is idempotent", async () => {
|
|
const fake = useFakeDb({ prefs: { onboarding_skipped_source: "1", onboarding_skipped_budget: "1" } });
|
|
await resumeStep("source");
|
|
await resumeStep("source");
|
|
expect(fake.prefs()).toEqual({ onboarding_skipped_budget: "1" });
|
|
expect(String(fake.db.execute.mock.calls[0][0])).toContain("DELETE FROM user_preferences");
|
|
});
|
|
|
|
it("skip then resume round-trips through the snapshot", async () => {
|
|
useFakeDb();
|
|
await skipStep("manualCategory");
|
|
expect((await getOnboardingSnapshot()).skipped).toEqual(["manualCategory"]);
|
|
await resumeStep("manualCategory");
|
|
expect((await getOnboardingSnapshot()).skipped).toEqual([]);
|
|
});
|
|
|
|
it("neither announces on the data channel", async () => {
|
|
useFakeDb();
|
|
const listener = vi.fn();
|
|
const unsubscribe = subscribeDataChanged(listener);
|
|
try {
|
|
await skipStep("budget");
|
|
await resumeStep("budget");
|
|
await hideChecklist();
|
|
await showChecklist();
|
|
} finally {
|
|
unsubscribe();
|
|
}
|
|
expect(listener).not.toHaveBeenCalled();
|
|
});
|
|
});
|