refactor: replace raw SQL duplication with col() helper for cross-DB quoting#2372
refactor: replace raw SQL duplication with col() helper for cross-DB quoting#2372
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…quoting Adds BaseRepository.col(name) helper that returns correctly quoted column names (PostgreSQL: "camelCase", SQLite/MySQL: bare identifiers). Replaces 4 isPostgres() branched queries in misc.ts with single unified queries: - getPacketLogs: packet_log LEFT JOIN nodes (x2) - getPacketLogById: same double JOIN - getPacketCountsByNode: packet_log LEFT JOIN nodes with GROUP BY - getDistinctRelayNodes: SELECT from nodes with bitwise WHERE Removes ~40 lines of duplicated SQL. Adds 9 new tests verifying JOIN queries return correct node names, handle unknown nodes, respect pagination, and maintain sort order. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * 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.
💡 Excellent helper method design!
This col() method elegantly solves the column quoting differences between databases:
- PostgreSQL requires double quotes for camelCase identifiers
- SQLite/MySQL work without quotes
- Uses
sql.raw()for safe interpolation without SQL injection risks
The method is simple, well-documented, and available to all repository classes. Perfect foundation for eliminating SQL duplication across the codebase.
| ORDER BY pl.timestamp DESC, pl.created_at DESC LIMIT ${limit} OFFSET ${offset} | ||
| `; | ||
| } | ||
| const longName = this.col('longName'); |
There was a problem hiding this comment.
🎯 Great query unification!
This refactoring successfully eliminates the PostgreSQL vs SQLite/MySQL branching that existed here previously. The col() helper handles the column quoting differences transparently.
Before: ~20 lines of duplicated SQL with isPostgres() conditionals
After: Single clean query using the helper
The JOIN logic and result processing remain identical, maintaining full compatibility.
| for (const rv of relayValues) { | ||
| const matchRows = await this.executeQuery(sql.raw(`${matchQuery}${rv}`)); | ||
| const matchRows = await this.executeQuery( | ||
| sql`SELECT ${longName}, ${shortName} FROM nodes WHERE (${nodeNum} & 255) = ${rv}` |
There was a problem hiding this comment.
✨ Smart bitwise operation handling
The bitwise WHERE clause (${nodeNum} & 255) = ${rv} correctly extracts the last byte for relay node matching per the Meshtastic protobuf spec. Using col() ensures the column reference works across all database backends.
This query demonstrates how the helper method enables complex operations while maintaining cross-DB compatibility.
| db.close(); | ||
| }); | ||
|
|
||
| describe('getPacketLogs', () => { |
There was a problem hiding this comment.
📋 Comprehensive test coverage
These tests provide excellent validation for the refactored JOIN queries:
✅ Core Functionality: Verifies node name joins work correctly
✅ Edge Cases: Tests unknown node handling (NULL longName)
✅ Pagination: Validates limit/offset behavior
✅ Sort Order: Confirms timestamp DESC + created_at DESC ordering
The test setup properly creates both nodes and packet_log tables with realistic data, ensuring the JOIN operations are thoroughly validated.
|
|
||
| ## 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. |
There was a problem hiding this comment.
📖 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 examples
The fallback strategy mentioned (keep raw SQL with Drizzle column references) shows good contingency planning, though the actual implementation successfully used the builder approach.
Launches SQLite, PostgreSQL, and MySQL in sequence, monitors logs for errors during a configurable soak period (default 5 minutes each). Logs saved to tests/soak-logs/ for post-test review. Usage: tests/backend-soak-test.sh [duration_seconds] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The PostgreSQL and MySQL baseline migrations omitted the decrypted_by column from the messages CREATE TABLE, causing DrizzleQueryError on any SELECT from messages. SQLite baseline had it correctly. Found by the new backend soak test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Migration 014 now also adds enforceNameValidation and sortOrder to channel_database for PostgreSQL and MySQL — both were omitted from the v3.7 baseline. Also improved soak test exclude patterns for expected ECONNREFUSED errors in dev environment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| * 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.
💡 Excellent helper method design!
This col() method elegantly solves the column quoting differences between databases:
- PostgreSQL requires double quotes for camelCase identifiers
- SQLite/MySQL work without quotes
- Uses
sql.raw()for safe interpolation without SQL injection risks
The method is simple, well-documented, and available to all repository classes. Perfect foundation for eliminating SQL duplication across the codebase.
| ORDER BY pl.timestamp DESC, pl.created_at DESC LIMIT ${limit} OFFSET ${offset} | ||
| `; | ||
| } | ||
| const longName = this.col('longName'); |
There was a problem hiding this comment.
🎯 Great query unification!
This refactoring successfully eliminates the PostgreSQL vs SQLite/MySQL branching that existed here previously. The col() helper handles the column quoting differences transparently.
Before: ~20 lines of duplicated SQL with isPostgres() conditionals
After: Single clean query using the helper
The JOIN logic and result processing remain identical, maintaining full compatibility.
| for (const rv of relayValues) { | ||
| const matchRows = await this.executeQuery(sql.raw(`${matchQuery}${rv}`)); | ||
| const matchRows = await this.executeQuery( | ||
| sql`SELECT ${longName}, ${shortName} FROM nodes WHERE (${nodeNum} & 255) = ${rv}` |
There was a problem hiding this comment.
✨ Smart bitwise operation handling
The bitwise WHERE clause (${nodeNum} & 255) = ${rv} correctly extracts the last byte for relay node matching per the Meshtastic protobuf spec. Using col() ensures the column reference works across all database backends.
This query demonstrates how the helper method enables complex operations while maintaining cross-DB compatibility.
| db.close(); | ||
| }); | ||
|
|
||
| describe('getPacketLogs', () => { |
There was a problem hiding this comment.
📋 Comprehensive test coverage
These tests provide excellent validation for the refactored JOIN queries:
✅ Core Functionality: Verifies node name joins work correctly
✅ Edge Cases: Tests unknown node handling (NULL longName)
✅ Pagination: Validates limit/offset behavior
✅ Sort Order: Confirms timestamp DESC + created_at DESC ordering
The test setup properly creates both nodes and packet_log tables with realistic data, ensuring the JOIN operations are thoroughly validated.
| - 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. |
There was a problem hiding this comment.
📖 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 examples
The fallback strategy mentioned (keep raw SQL with Drizzle column references) shows good contingency planning, though the actual implementation successfully used the builder approach.
1 participant
Summary
Three improvements in one PR:
1. Replace raw SQL duplication with
col()helperAdds
BaseRepository.col(name)that returns correctly quoted column names per database backend. Replaces 4isPostgres()branched queries inMiscRepositorywith unified single-path queries:getPacketLogs— packet_log LEFT JOIN nodes (x2)getPacketLogById— same double JOINgetPacketCountsByNode— packet_log LEFT JOIN nodes with GROUP BY/COUNTgetDistinctRelayNodes— SELECT from nodes with bitwise WHERE2. Fix missing PG/MySQL baseline columns (found by soak test)
3. Backend soak test
New test script (
tests/backend-soak-test.sh) that launches each database backend in sequence, soaks for a configurable duration, and monitors logs for errors. This is how the missing column bugs were discovered.Files Changed
src/db/repositories/base.tscol()helper +sqlimportsrc/db/repositories/misc.tscol()versionssrc/db/repositories/misc.packetlog.test.tssrc/server/migrations/014_add_messages_decrypted_by.tssrc/db/migrations.tssrc/db/migrations.test.tsCLAUDE.mdtests/backend-soak-test.sh.gitignoreTest plan
🤖 Generated with Claude Code