-
-
Notifications
You must be signed in to change notification settings - Fork 43
refactor: replace raw SQL duplication with col() helper for cross-DB quoting #2372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
fd75a4f
4432b67
8754d72
0a0550b
71d881f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # Drizzle JOIN Refactor (Phase 1) — Design Spec | ||
|
|
||
| **Date:** 2026-03-21 | ||
| **Status:** Approved | ||
|
|
||
| ## Overview | ||
|
|
||
| Replace 4 raw SQL queries in `src/db/repositories/misc.ts` that branch on database type (PostgreSQL vs SQLite/MySQL) for column quoting with unified Drizzle ORM query builder calls. This eliminates ~80 lines of duplicated SQL and 4 `isPostgres()` conditional blocks. | ||
|
|
||
| ## Scope | ||
|
|
||
| Only queries where the branching is purely about column name quoting. Excludes queries with genuine SQL syntax differences (DISTINCT ON, DELETE RETURNING, etc.) — those are future phases. | ||
|
|
||
| ## Queries to Refactor | ||
|
|
||
| ### 1. `getPacketLogs` (lines ~965-984) | ||
| - `packet_log` LEFT JOIN `nodes` twice (as from_nodes, to_nodes) | ||
| - Selects `longName` from each joined node | ||
| - Has WHERE clause, ORDER BY timestamp DESC + created_at DESC, LIMIT/OFFSET | ||
|
|
||
| ### 2. `getPacketLogById` (lines ~999-1015) | ||
| - Same double LEFT JOIN as above | ||
| - Filtered by `pl.id = ${id}` | ||
|
|
||
| ### 3. `getPacketCountsByNode` (lines ~1175-1195) | ||
| - `packet_log` LEFT JOIN `nodes` once | ||
| - GROUP BY + COUNT(*) + ORDER BY count DESC | ||
| - Postgres uses `COUNT(*)::int` cast (unnecessary with Drizzle) | ||
|
|
||
| ### 4. `getDistinctRelayNodes` (lines ~1100-1104) | ||
| - Simple SELECT from nodes with bitwise WHERE | ||
| - Not a JOIN, just quoting difference on column names | ||
|
|
||
| ## Design | ||
|
|
||
| ### Approach | ||
| Use Drizzle's `alias()` function for double-joining the nodes table, and the standard `.select().from().leftJoin()` builder for all queries. Drizzle handles column quoting per-backend automatically. | ||
|
|
||
| ### Key Drizzle patterns needed | ||
|
|
||
| **Table alias for double-join:** | ||
| ```typescript | ||
| import { alias } from 'drizzle-orm/sqlite-core'; // or pg-core, mysql-core | ||
| // Use the active schema's nodes table | ||
| const fromNodes = alias(this.tables.nodes, 'from_nodes'); | ||
| const toNodes = alias(this.tables.nodes, 'to_nodes'); | ||
| ``` | ||
|
|
||
| Note: `alias()` is backend-specific in Drizzle. Since we have three backends, we need to use the correct `alias` import or find a backend-agnostic approach. The `sql` tagged template with `this.tables.nodes` column references may be simpler — Drizzle's `sql` helper auto-quotes column references from table schemas. | ||
|
|
||
| **Unified column references:** | ||
| ```typescript | ||
| const { packetLog, nodes } = this.tables; | ||
| // Drizzle auto-quotes: nodes.longName → "longName" (PG) or longName (SQLite) | ||
| ``` | ||
|
|
||
| **COUNT without cast:** | ||
| ```typescript | ||
| sql<number>`COUNT(*)` // Works across all backends | ||
| ``` | ||
|
|
||
| ### What changes | ||
| - Remove 4 `if (this.isPostgres()) { ... } else { ... }` blocks | ||
| - Replace with single Drizzle query builder call per method | ||
| - No changes to method signatures or return types | ||
| - `normalizePacketLogRow()` continues to handle result normalization | ||
|
|
||
| ### Files Modified | ||
|
|
||
| | File | Change | | ||
| |------|--------| | ||
| | `src/db/repositories/misc.ts` | Replace 4 branched queries with Drizzle query builder | | ||
|
|
||
| ## Testing | ||
|
|
||
| - Existing tests cover these methods — they must continue to pass | ||
| - Run full test suite (3052+ tests) | ||
| - Build verification (TypeScript clean) | ||
|
|
||
| ## Risk | ||
|
|
||
| Low — method signatures and return types don't change. The Drizzle query builder generates the same SQL that's currently hardcoded, just with correct quoting per backend. If any query doesn't translate cleanly to the Drizzle builder, we keep the raw SQL but use Drizzle column references for auto-quoting instead of full builder conversion. | ||
|
Comment on lines
+78
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📖 Excellent design documentation This design spec demonstrates thorough planning: ✅ Clear scope definition - Phase 1 focuses only on column quoting differences The fallback strategy mentioned (keep raw SQL with Drizzle column references) shows good contingency planning, though the actual implementation successfully used the builder approach. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| * Provides common functionality for all repository implementations. | ||
| * Supports SQLite, PostgreSQL, and MySQL through Drizzle ORM. | ||
| */ | ||
| import { sql } from 'drizzle-orm'; | ||
| import { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; | ||
| import { NodePgDatabase } from 'drizzle-orm/node-postgres'; | ||
| import { MySql2Database } from 'drizzle-orm/mysql2'; | ||
|
|
@@ -117,6 +118,15 @@ export abstract class BaseRepository { | |
| return this.mysqlDb; | ||
| } | ||
|
|
||
| /** | ||
| * Quote a column name for use in raw SQL. | ||
| * PostgreSQL requires double-quoted "camelCase" identifiers; SQLite/MySQL do not. | ||
| * Returns a raw SQL fragment that can be interpolated into sql`` templates. | ||
| */ | ||
| protected col(name: string) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Excellent helper method design! This
The method is simple, well-documented, and available to all repository classes. Perfect foundation for eliminating SQL duplication across the codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Excellent helper method design! This
The method is simple, well-documented, and available to all repository classes. Perfect foundation for eliminating SQL duplication across the codebase. |
||
| return this.isPostgres() ? sql.raw(`"${name}"`) : sql.raw(name); | ||
| } | ||
|
|
||
| /** | ||
| * Execute a raw SQL query that returns rows (SELECT) across all dialects. | ||
| * SQLite's Drizzle driver doesn't have .execute() — uses .all() instead. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| /** | ||
| * Misc Repository - Packet Log Query Tests | ||
| * | ||
| * Tests the refactored Drizzle JOIN queries for packet log methods. | ||
| * Verifies that column references are correctly quoted across database backends. | ||
| */ | ||
| import { describe, it, expect, beforeEach, afterEach } from 'vitest'; | ||
| import Database from 'better-sqlite3'; | ||
| import { drizzle, BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; | ||
| import { MiscRepository } from './misc.js'; | ||
| import * as schema from '../schema/index.js'; | ||
|
|
||
| describe('MiscRepository - Packet Log Queries', () => { | ||
| let db: Database.Database; | ||
| let drizzleDb: BetterSQLite3Database<typeof schema>; | ||
| let repo: MiscRepository; | ||
|
|
||
| beforeEach(() => { | ||
| db = new Database(':memory:'); | ||
|
|
||
| // Create tables needed for JOIN queries | ||
| db.exec(` | ||
| CREATE TABLE IF NOT EXISTS nodes ( | ||
| nodeNum INTEGER PRIMARY KEY, | ||
| nodeId TEXT, | ||
| longName TEXT, | ||
| shortName TEXT, | ||
| lastHeard INTEGER | ||
| ) | ||
| `); | ||
|
|
||
| db.exec(` | ||
| CREATE TABLE IF NOT EXISTS packet_log ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| packet_id INTEGER, | ||
| timestamp INTEGER NOT NULL, | ||
| from_node INTEGER NOT NULL, | ||
| from_node_id TEXT, | ||
| to_node INTEGER, | ||
| to_node_id TEXT, | ||
| channel INTEGER, | ||
| portnum INTEGER NOT NULL, | ||
| portnum_name TEXT, | ||
| encrypted INTEGER DEFAULT 0, | ||
| snr REAL, | ||
| rssi INTEGER, | ||
| hop_limit INTEGER, | ||
| hop_start INTEGER, | ||
| relay_node INTEGER, | ||
| payload_size INTEGER, | ||
| want_ack INTEGER DEFAULT 0, | ||
| priority INTEGER, | ||
| payload_preview TEXT, | ||
| metadata TEXT, | ||
| direction TEXT DEFAULT 'rx', | ||
| created_at INTEGER, | ||
| transport_mechanism TEXT, | ||
| decrypted_by TEXT, | ||
| decrypted_channel_id INTEGER | ||
| ) | ||
| `); | ||
|
|
||
| // Create settings table (needed by MiscRepository) | ||
| db.exec(` | ||
| CREATE TABLE IF NOT EXISTS settings ( | ||
| key TEXT PRIMARY KEY, | ||
| value TEXT | ||
| ) | ||
| `); | ||
|
|
||
| drizzleDb = drizzle(db, { schema }); | ||
| repo = new MiscRepository(drizzleDb as any, 'sqlite'); | ||
|
|
||
| // Insert test nodes | ||
| db.exec(`INSERT INTO nodes (nodeNum, nodeId, longName, shortName) VALUES (100, '!00000064', 'Node Alpha', 'ALPH')`); | ||
| db.exec(`INSERT INTO nodes (nodeNum, nodeId, longName, shortName) VALUES (200, '!000000c8', 'Node Beta', 'BETA')`); | ||
| db.exec(`INSERT INTO nodes (nodeNum, nodeId, longName, shortName) VALUES (300, '!0000012c', 'Node Gamma', 'GAMM')`); | ||
|
|
||
| // Insert test packets | ||
| const now = Math.floor(Date.now() / 1000); | ||
| const nowMs = Date.now(); | ||
| db.exec(`INSERT INTO packet_log (packet_id, timestamp, from_node, from_node_id, to_node, to_node_id, portnum, portnum_name, direction, created_at, relay_node) VALUES (1, ${now}, 100, '!00000064', 200, '!000000c8', 1, 'TEXT_MESSAGE_APP', 'rx', ${nowMs}, 100)`); | ||
| db.exec(`INSERT INTO packet_log (packet_id, timestamp, from_node, from_node_id, to_node, to_node_id, portnum, portnum_name, direction, created_at, relay_node) VALUES (2, ${now}, 200, '!000000c8', 100, '!00000064', 1, 'TEXT_MESSAGE_APP', 'rx', ${nowMs + 1}, 200)`); | ||
| db.exec(`INSERT INTO packet_log (packet_id, timestamp, from_node, from_node_id, to_node, to_node_id, portnum, portnum_name, direction, created_at) VALUES (3, ${now - 60}, 100, '!00000064', 4294967295, '!ffffffff', 3, 'POSITION_APP', 'rx', ${nowMs - 60000})`); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| db.close(); | ||
| }); | ||
|
|
||
| describe('getPacketLogs', () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📋 Comprehensive test coverage These tests provide excellent validation for the refactored JOIN queries: ✅ Core Functionality: Verifies node name joins work correctly The test setup properly creates both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📋 Comprehensive test coverage These tests provide excellent validation for the refactored JOIN queries: ✅ Core Functionality: Verifies node name joins work correctly The test setup properly creates both |
||
| it('returns packets with joined node names', async () => { | ||
| const packets = await repo.getPacketLogs({}); | ||
| expect(packets.length).toBe(3); | ||
|
|
||
| // Check that longName was joined from nodes table | ||
| const pkt1 = packets.find(p => p.packet_id === 1); | ||
| expect(pkt1).toBeDefined(); | ||
| expect(pkt1!.from_node_longName).toBe('Node Alpha'); | ||
| expect(pkt1!.to_node_longName).toBe('Node Beta'); | ||
| }); | ||
|
|
||
| it('returns null longName for unknown nodes', async () => { | ||
| // Insert packet from unknown node | ||
| const now = Math.floor(Date.now() / 1000); | ||
| db.exec(`INSERT INTO packet_log (packet_id, timestamp, from_node, from_node_id, to_node, portnum, direction, created_at) VALUES (99, ${now}, 999, '!000003e7', NULL, 1, 'rx', ${Date.now()})`); | ||
|
|
||
| const packets = await repo.getPacketLogs({}); | ||
| const unknownPkt = packets.find(p => p.packet_id === 99); | ||
| expect(unknownPkt).toBeDefined(); | ||
| expect(unknownPkt!.from_node_longName).toBeNull(); | ||
| }); | ||
|
|
||
| it('respects limit and offset', async () => { | ||
| const packets = await repo.getPacketLogs({ limit: 2, offset: 0 }); | ||
| expect(packets.length).toBe(2); | ||
| }); | ||
|
|
||
| it('orders by timestamp DESC then created_at DESC', async () => { | ||
| const packets = await repo.getPacketLogs({}); | ||
| // First two packets have same timestamp, ordered by created_at DESC | ||
| expect(packets[0].packet_id).toBe(2); // higher created_at | ||
| expect(packets[1].packet_id).toBe(1); | ||
| expect(packets[2].packet_id).toBe(3); // older timestamp | ||
| }); | ||
| }); | ||
|
|
||
| describe('getPacketLogById', () => { | ||
| it('returns a single packet with joined node names', async () => { | ||
| const packets = await repo.getPacketLogs({}); | ||
| const firstId = packets[0].id; | ||
|
|
||
| const pkt = await repo.getPacketLogById(firstId!); | ||
| expect(pkt).not.toBeNull(); | ||
| expect(pkt!.from_node_longName).toBeDefined(); | ||
| }); | ||
|
|
||
| it('returns null for non-existent id', async () => { | ||
| const pkt = await repo.getPacketLogById(99999); | ||
| expect(pkt).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getPacketCountsByNode', () => { | ||
| it('returns counts with joined node names', async () => { | ||
| const counts = await repo.getPacketCountsByNode({}); | ||
| expect(counts.length).toBeGreaterThan(0); | ||
|
|
||
| const alpha = counts.find(c => c.from_node === 100); | ||
| expect(alpha).toBeDefined(); | ||
| expect(alpha!.from_node_longName).toBe('Node Alpha'); | ||
| expect(alpha!.count).toBe(2); // packets 1 and 3 | ||
| }); | ||
|
|
||
| it('respects limit', async () => { | ||
| const counts = await repo.getPacketCountsByNode({ limit: 1 }); | ||
| expect(counts.length).toBe(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getDistinctRelayNodes', () => { | ||
| it('returns relay nodes with matched node names', async () => { | ||
| const relays = await repo.getDistinctRelayNodes(); | ||
| expect(relays.length).toBeGreaterThan(0); | ||
|
|
||
| // relay_node 100 & 0xFF = 100, matches node 100 (Node Alpha) | ||
| const relay100 = relays.find(r => r.relay_node === 100); | ||
| expect(relay100).toBeDefined(); | ||
| expect(relay100!.matching_nodes.length).toBeGreaterThan(0); | ||
| expect(relay100!.matching_nodes[0].longName).toBe('Node Alpha'); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📖 Excellent design documentation
This design spec demonstrates thorough planning:
✅ Clear scope definition - Phase 1 focuses only on column quoting differences
✅ Risk assessment - Correctly identifies this as low-risk due to unchanged signatures
✅ Detailed analysis - Lists all 4 queries with line numbers and complexity
✅ Implementation strategy - Explains the
col()helper approach with examplesThe fallback strategy mentioned (keep raw SQL with Drizzle column references) shows good contingency planning, though the actual implementation successfully used the builder approach.