`insertEdge` has always used `INSERT OR IGNORE`, but the edges table carried no UNIQUE constraint — only an autoincrement PK and non-unique indexes — so `OR IGNORE` had nothing to conflict on and behaved like a plain INSERT. Whenever two extraction/resolution passes emitted the same edge (e.g. a return type captured by both a type-reference and a value-reference pass), the graph stored byte-identical duplicate rows: ~527 on this repo, inflating edge counts and letting callers/impact list the same relationship twice. Add a UNIQUE identity index on (source, target, kind, IFNULL(line,-1), IFNULL(col,-1)) — in schema.sql for fresh databases and migration v6 (dedup existing rows, then create the index) for existing ones. IFNULL folds the nullable line/col so coordinate-less edges (synthesized / file-level) dedup too; SQLite otherwise treats each NULL as distinct. Distinct call sites (same source/target/kind, different line/col) are preserved — only byte-identical structural duplicates collapse. This is the storage-layer invariant the reporter identified: it makes OR IGNORE keep its promise and catches every double-emit, present and future, rather than chasing each emitting pass. Migration v6 is deterministic (keeps the lowest id per identity group) and idempotent (IF NOT EXISTS index; no-op DELETE once unique). The DELETE's GROUP BY matches the index expression exactly so creation can't fail on a leftover pair. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
2176a7a439
commit
0da2dcec8e
+110
-1
@@ -16,7 +16,8 @@ import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { DatabaseConnection } from '../src/db';
|
||||
import { QueryBuilder } from '../src/db/queries';
|
||||
import { Node } from '../src/types';
|
||||
import { runMigrations, getCurrentVersion } from '../src/db/migrations';
|
||||
import { Node, Edge } from '../src/types';
|
||||
|
||||
function makeNode(id: string, name = id): Node {
|
||||
return {
|
||||
@@ -249,3 +250,111 @@ describe('runMaintenance', () => {
|
||||
expect(() => db.runMaintenance()).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
// The edges table carried no UNIQUE constraint, so `insertEdge`'s
|
||||
// `INSERT OR IGNORE` had nothing to conflict on and silently admitted
|
||||
// byte-identical duplicate rows when two passes emitted the same edge (#1034).
|
||||
// A UNIQUE identity index — `(source, target, kind, IFNULL(line,-1),
|
||||
// IFNULL(col,-1))` — makes OR IGNORE actually dedup.
|
||||
describe('edge identity uniqueness (#1034)', () => {
|
||||
let dir: string;
|
||||
let db: DatabaseConnection;
|
||||
let q: QueryBuilder;
|
||||
|
||||
beforeEach(() => {
|
||||
dir = fs.mkdtempSync(path.join(os.tmpdir(), 'db-edge-uniq-'));
|
||||
db = DatabaseConnection.initialize(path.join(dir, 'test.db'));
|
||||
q = new QueryBuilder(db.getDb());
|
||||
q.insertNodes([makeNode('A'), makeNode('B')]);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
db.close();
|
||||
if (fs.existsSync(dir)) fs.rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
const edgeCount = () =>
|
||||
(db.getDb().prepare('SELECT count(*) AS c FROM edges').get() as { c: number }).c;
|
||||
const mk = (over: Partial<Edge> = {}): Edge => ({
|
||||
source: 'A',
|
||||
target: 'B',
|
||||
kind: 'references',
|
||||
line: 153,
|
||||
column: 12,
|
||||
metadata: { resolvedBy: 'exact-match' },
|
||||
...over,
|
||||
});
|
||||
|
||||
it('a fresh database has the identity index', () => {
|
||||
const idx = db
|
||||
.getDb()
|
||||
.prepare("SELECT name FROM sqlite_master WHERE type='index' AND name='idx_edges_identity'")
|
||||
.get();
|
||||
expect(idx).toBeTruthy();
|
||||
});
|
||||
|
||||
it('collapses byte-identical edges to a single row', () => {
|
||||
q.insertEdges([mk(), mk(), mk()]);
|
||||
expect(edgeCount()).toBe(1);
|
||||
});
|
||||
|
||||
it('dedups even when only the metadata differs (same structural identity)', () => {
|
||||
q.insertEdges([mk({ metadata: { resolvedBy: 'exact-match' } }), mk({ metadata: { resolvedBy: 'import' } })]);
|
||||
expect(edgeCount()).toBe(1);
|
||||
});
|
||||
|
||||
it('keeps edges that differ in line/col — distinct call sites are not duplicates', () => {
|
||||
q.insertEdges([mk({ column: 12 }), mk({ column: 99 }), mk({ line: 200, column: 1 })]);
|
||||
expect(edgeCount()).toBe(3);
|
||||
});
|
||||
|
||||
it('dedups coordinate-less edges, folding NULL line/col via IFNULL', () => {
|
||||
q.insertEdges([mk({ line: undefined, column: undefined }), mk({ line: undefined, column: undefined })]);
|
||||
expect(edgeCount()).toBe(1);
|
||||
});
|
||||
|
||||
it('dedups across separate insert calls (storage constraint, not a per-batch dedup)', () => {
|
||||
q.insertEdges([mk()]);
|
||||
q.insertEdges([mk()]);
|
||||
expect(edgeCount()).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('migration v6: dedup edges + add identity index on upgrade (#1034)', () => {
|
||||
it('collapses pre-existing duplicate rows, keeps distinct ones, and restores the constraint', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'db-mig6-'));
|
||||
const db = DatabaseConnection.initialize(path.join(dir, 'test.db'));
|
||||
const raw = db.getDb();
|
||||
const q = new QueryBuilder(raw);
|
||||
q.insertNodes([makeNode('A'), makeNode('B')]);
|
||||
|
||||
// Recreate a pre-v6 database: without the identity index, `INSERT OR IGNORE`
|
||||
// admits duplicates. Revert the recorded version so migration v6 will re-run.
|
||||
raw.exec('DROP INDEX IF EXISTS idx_edges_identity');
|
||||
raw.prepare('DELETE FROM schema_versions WHERE version >= 6').run();
|
||||
q.insertEdges([
|
||||
{ source: 'A', target: 'B', kind: 'references', line: 153, column: 12, metadata: { resolvedBy: 'exact-match' } },
|
||||
{ source: 'A', target: 'B', kind: 'references', line: 153, column: 12, metadata: { resolvedBy: 'exact-match' } },
|
||||
{ source: 'A', target: 'B', kind: 'calls', line: 200, column: 4 },
|
||||
]);
|
||||
const count = () => (raw.prepare('SELECT count(*) AS c FROM edges').get() as { c: number }).c;
|
||||
expect(count()).toBe(3); // duplicate admitted while the index was absent
|
||||
|
||||
runMigrations(raw, 5);
|
||||
|
||||
expect(count()).toBe(2); // duplicate collapsed, the distinct `calls` edge kept
|
||||
expect(getCurrentVersion(raw)).toBe(6);
|
||||
const idx = raw
|
||||
.prepare("SELECT name FROM sqlite_master WHERE type='index' AND name='idx_edges_identity'")
|
||||
.get();
|
||||
expect(idx).toBeTruthy();
|
||||
// The constraint now holds — re-inserting the duplicate is a no-op.
|
||||
q.insertEdges([
|
||||
{ source: 'A', target: 'B', kind: 'references', line: 153, column: 12, metadata: { resolvedBy: 'x' } },
|
||||
]);
|
||||
expect(count()).toBe(2);
|
||||
|
||||
db.close();
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -282,7 +282,7 @@ describe('Database Connection', () => {
|
||||
|
||||
const version = db.getSchemaVersion();
|
||||
expect(version).not.toBeNull();
|
||||
expect(version?.version).toBe(5);
|
||||
expect(version?.version).toBe(6);
|
||||
|
||||
db.close();
|
||||
});
|
||||
|
||||
@@ -299,7 +299,7 @@ describe('Best-Candidate Resolution', () => {
|
||||
describe('Schema v2 Migration', () => {
|
||||
it.skipIf(!HAS_SQLITE)('should have correct current schema version', async () => {
|
||||
const { CURRENT_SCHEMA_VERSION } = await import('../src/db/migrations');
|
||||
expect(CURRENT_SCHEMA_VERSION).toBe(5);
|
||||
expect(CURRENT_SCHEMA_VERSION).toBe(6);
|
||||
});
|
||||
|
||||
it.skipIf(!HAS_SQLITE)('should have migration for version 2', async () => {
|
||||
|
||||
Reference in New Issue
Block a user