Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/main/database/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,7 @@ export function runMigrations(db: Database.Database): void {
const version = db.pragma('user_version', { simple: true }) as number;
if (version >= DB_VERSION) return;

// No migration blocks yet — all schema changes so far are baked into the
// initial schema SQL, so existing pre-production databases can be recreated.
// Wrap in a transaction to establish the correct pattern: when real migration
// DDL blocks are added, each block must stamp user_version atomically with its
// DDL so a mid-migration crash cannot leave the DB partially migrated (fixes #207).
// No migration blocks yet — new DBs receive all schema via initDatabase → LOCAL_SCHEMA_DDL_SQL.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing migration block — this will break existing databases.

sync_tombstones is added to LOCAL_SCHEMA_DDL_SQL, but that SQL only runs for brand-new databases via initDatabase. Existing users whose user_version is already at DB_VERSION will hit the return guard above and skip this function entirely, so their databases will never get the table. Any call to INSERT INTO sync_tombstones or SELECT FROM sync_tombstones will throw no such table: sync_tombstones at runtime.

DB_VERSION needs to be bumped and a migration block needs to be added here:

Suggested change
// No migration blocks yet — new DBs receive all schema via initDatabase → LOCAL_SCHEMA_DDL_SQL.
// Migration to DB_VERSION N: add sync_tombstones for deletion propagation.
if (version < /* new DB_VERSION */) {
db.transaction(() => {
db.prepare(`
CREATE TABLE IF NOT EXISTS sync_tombstones (
table_name TEXT NOT NULL,
row_id TEXT NOT NULL,
deleted_at INTEGER NOT NULL,
PRIMARY KEY (table_name, row_id)
)
`).run();
db.pragma(`user_version = /* new DB_VERSION */`);
})();
}

The DB_VERSION constant (wherever it's defined) must also be incremented to match.

db.transaction(() => {
Comment thread
tomcardoso marked this conversation as resolved.
db.pragma(`user_version = ${DB_VERSION}`);
})();
Expand Down
8 changes: 8 additions & 0 deletions src/main/database/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,15 @@ export const LOCAL_SCHEMA_DDL_SQL = `
UNIQUE(contact_id, tag)
);

CREATE TABLE IF NOT EXISTS sync_tombstones (
table_name TEXT NOT NULL,
row_id TEXT NOT NULL,
deleted_at INTEGER NOT NULL,
PRIMARY KEY (table_name, row_id)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an index on deleted_at to keep the GC query fast as tombstones accumulate:

Suggested change
);
);
CREATE INDEX IF NOT EXISTS idx_sync_tombstones_deleted_at ON sync_tombstones(deleted_at);

DELETE FROM sync_tombstones WHERE deleted_at < ? is a table scan without it. The same index should be added to SHARED_SCHEMA_SQL in shared-schema.ts.


CREATE INDEX IF NOT EXISTS idx_contact_tags_contact_id ON contact_tags(contact_id);
CREATE INDEX IF NOT EXISTS idx_sync_tombstones_deleted_at ON sync_tombstones(deleted_at);
CREATE INDEX IF NOT EXISTS idx_contact_emails_contact_id ON contact_emails(contact_id);
CREATE UNIQUE INDEX IF NOT EXISTS idx_contact_emails_contact_email ON contact_emails(contact_id, email);
CREATE INDEX IF NOT EXISTS idx_contact_phones_contact_id ON contact_phones(contact_id);
Expand Down
10 changes: 0 additions & 10 deletions src/main/database/shared-db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,6 @@ function runSharedMigrations(db: Database.Database): void {
if (version < 1) {
db.pragma('user_version = 1');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing migration for the shared DB. Same issue as the local DB: sync_tombstones was added to SHARED_SCHEMA_SQL, but that only runs when a new shared DB is first created. Existing shared databases will never get the table, causing runtime crashes for any client that tries to sync tombstones.

A new if (version < 2) block is needed here (and the shared user_version must be bumped to match):

Suggested change
}
if (version < 2) {
db.transaction(() => {
db.prepare(`
CREATE TABLE IF NOT EXISTS sync_tombstones (
table_name TEXT NOT NULL,
row_id TEXT NOT NULL,
deleted_at INTEGER NOT NULL,
PRIMARY KEY (table_name, row_id)
)
`).run();
db.pragma('user_version = 2');
})();
}

Note: the example migration template that was deleted from this file in this PR contained an important warning about using INSERT OR REPLACE instead of ON CONFLICT … WHERE value > due to SQLite's lexicographic comparison for min_app_version. Worth retaining that comment somewhere if it's no longer inline here.

// Future migration blocks go here. Example:
// if (version < 2) {
// db.prepare('ALTER TABLE contacts ADD COLUMN foo TEXT').run();
// db.prepare(
// `INSERT OR REPLACE INTO shared_meta (key, value) VALUES ('min_app_version', '0.2.0')`
// ).run();
// // Note: always use INSERT OR REPLACE here, not ON CONFLICT ... WHERE value > ...,
// // because SQLite compares TEXT lexicographically and '0.9.0' > '0.10.0' is true.
// db.pragma('user_version = 2');
// }
}

export function createSharedDb(
Expand Down
8 changes: 8 additions & 0 deletions src/main/database/shared-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ export const SHARED_SCHEMA_SQL = `
UNIQUE(contact_id, tag)
);

CREATE TABLE IF NOT EXISTS sync_tombstones (
table_name TEXT NOT NULL,
row_id TEXT NOT NULL,
deleted_at INTEGER NOT NULL,
PRIMARY KEY (table_name, row_id)
);

CREATE TABLE IF NOT EXISTS contact_alert_rss (
id TEXT PRIMARY KEY,
contact_id TEXT NOT NULL REFERENCES contacts(id) ON DELETE CASCADE,
Expand Down Expand Up @@ -118,6 +125,7 @@ export const SHARED_SCHEMA_SQL = `
);

CREATE INDEX IF NOT EXISTS idx_shared_contact_tags_contact_id ON contact_tags(contact_id);
CREATE INDEX IF NOT EXISTS idx_shared_sync_tombstones_deleted_at ON sync_tombstones(deleted_at);
CREATE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_id ON contact_emails(contact_id);
CREATE UNIQUE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_email ON contact_emails(contact_id, email);
CREATE INDEX IF NOT EXISTS idx_shared_contact_phones_contact_id ON contact_phones(contact_id);
Expand Down
54 changes: 40 additions & 14 deletions src/main/ipc/contacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,20 +446,27 @@ export function registerContactHandlers(): void {
.all(data.id) as { url: string; wayback_url: string | null }[];
existingWaybacks = new Map(existingWebsites.map((r) => [r.url, r.wayback_url]));

// Snapshot created_at values keyed by normalised value so they survive the re-insert.
const existingEmailCreatedAt = new Map(
(stmt(db, 'SELECT email, created_at FROM contact_emails WHERE contact_id = ?').all(data.id) as { email: string; created_at: number }[])
.map((r) => [r.email, r.created_at])
);
const existingPhoneCreatedAt = new Map(
(stmt(db, 'SELECT phone, created_at FROM contact_phones WHERE contact_id = ?').all(data.id) as { phone: string; created_at: number }[])
.map((r) => [r.phone, r.created_at])
);
const existingLinkCreatedAt = new Map(
(stmt(db, 'SELECT url, created_at FROM contact_links WHERE contact_id = ?').all(data.id) as { url: string; created_at: number }[])
.map((r) => [r.url.trim(), r.created_at])
// Snapshot created_at values and row IDs keyed by normalised value so they survive the re-insert.
// Row IDs are used to write tombstones for sub-table rows that get deleted on sync.
const existingEmailRows = stmt(db, 'SELECT id, email, created_at FROM contact_emails WHERE contact_id = ?').all(data.id) as { id: string; email: string; created_at: number }[];
const existingEmailCreatedAt = new Map(existingEmailRows.map((r) => [r.email, r.created_at]));
const existingEmailIdMap = new Map(existingEmailRows.map((r) => [r.email, r.id]));

const existingPhoneRows = stmt(db, 'SELECT id, phone, created_at FROM contact_phones WHERE contact_id = ?').all(data.id) as { id: string; phone: string; created_at: number }[];
const existingPhoneCreatedAt = new Map(existingPhoneRows.map((r) => [r.phone, r.created_at]));
const existingPhoneIdMap = new Map(existingPhoneRows.map((r) => [r.phone, r.id]));

const existingLinkRows = stmt(db, 'SELECT id, url, created_at FROM contact_links WHERE contact_id = ?').all(data.id) as { id: string; url: string; created_at: number }[];
const existingLinkCreatedAt = new Map(existingLinkRows.map((r) => [r.url.trim(), r.created_at]));
const existingLinkIdMap = new Map(existingLinkRows.map((r) => [r.url.trim(), r.id]));

const existingHandleIdMap = new Map(
(stmt(db, 'SELECT id, type, handle FROM contact_handles WHERE contact_id = ?').all(data.id) as { id: string; type: string; handle: string }[])
.map((r) => [`${r.type}:${r.handle}`, r.id])
);

const tombstone = stmt(db, 'INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at)');

stmt(db,
'UPDATE contacts SET name = ?, organization = ?, title = ?, dob = ?, notes = ?, updated_at = ? WHERE id = ?',
).run(data.name.trim(), data.organization?.trim() || null, data.title?.trim() || null, /^\d{4}-\d{2}-\d{2}$/.test(data.dob?.trim() ?? '') ? data.dob!.trim() : null, data.notes?.trim() || null, now, data.id);
Expand All @@ -473,6 +480,10 @@ export function registerContactHandlers(): void {
'INSERT INTO contact_emails (id, contact_id, email, label, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)',
).run(uuidv4(), data.id, e.email, e.label, i, existingEmailCreatedAt.get(e.email) ?? now);
});
const keptEmails = new Set(emails.map((e) => e.email));
for (const [email, id] of existingEmailIdMap) {
if (!keptEmails.has(email)) tombstone.run('contact_emails', id, now);
}

stmt(db, 'DELETE FROM contact_phones WHERE contact_id = ?').run(data.id);
const phones = (data.phones ?? [])
Expand All @@ -484,6 +495,10 @@ export function registerContactHandlers(): void {
'INSERT INTO contact_phones (id, contact_id, phone, label, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)',
).run(uuidv4(), data.id, p.phone, p.label, i, existingPhoneCreatedAt.get(p.phone) ?? now);
});
const keptPhones = new Set(phones.map((p) => p.phone));
for (const [phone, id] of existingPhoneIdMap) {
if (!keptPhones.has(phone)) tombstone.run('contact_phones', id, now);
}

stmt(db, 'DELETE FROM contact_links WHERE contact_id = ?').run(data.id);
const links = (data.links ?? []).filter((l: ContactLinkInput) => l.url.trim() && validateUrl(l.url));
Expand All @@ -492,6 +507,10 @@ export function registerContactHandlers(): void {
'INSERT INTO contact_links (id, contact_id, type, label, url, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)',
).run(uuidv4(), data.id, link.type, link.label ?? null, link.url.trim(), i, existingLinkCreatedAt.get(link.url.trim()) ?? now);
});
const keptLinks = new Set(links.map((l: ContactLinkInput) => l.url.trim()));
for (const [url, id] of existingLinkIdMap) {
if (!keptLinks.has(url)) tombstone.run('contact_links', id, now);
}

stmt(db, 'DELETE FROM contact_handles WHERE contact_id = ?').run(data.id);
const handles = (data.handles ?? []).filter((h: ContactHandleInput) => h.handle.trim() && h.type.trim());
Expand All @@ -500,6 +519,10 @@ export function registerContactHandlers(): void {
'INSERT INTO contact_handles (id, contact_id, type, handle, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)',
).run(uuidv4(), data.id, h.type.trim(), h.handle.trim(), i, now);
});
const keptHandles = new Set(handles.map((h: ContactHandleInput) => `${h.type.trim()}:${h.handle.trim()}`));
for (const [key, id] of existingHandleIdMap) {
if (!keptHandles.has(key)) tombstone.run('contact_handles', id, now);
}

// Restore wayback_urls for previously saved website links
for (const [url, waybackUrl] of existingWaybacks) {
Expand Down Expand Up @@ -775,8 +798,11 @@ export function registerContactHandlers(): void {
const db = getDatabase();
const now = Math.floor(Date.now() / 1000);
db.transaction(() => {
const { changes } = db.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, normalized);
if (changes > 0) db.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(now, contactId);
const row = db.prepare('SELECT id FROM contact_tags WHERE contact_id = ? AND tag = ?').get(contactId, normalized) as { id: string } | undefined;
if (!row) return;
db.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, normalized);
db.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(now, contactId);
db.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at)').run('contact_tags', row.id, now);
})();
broadcastContactsChanged();
});
Expand Down
Loading
Loading