Skip to content

fix(sqlite): convert source_id raw SQL to Drizzle sync helpers#2634

Merged
Yeraze merged 1 commit into
mainfrom
fix/2631-sqlite-source-id-drizzle
Apr 12, 2026
Merged

fix(sqlite): convert source_id raw SQL to Drizzle sync helpers#2634
Yeraze merged 1 commit into
mainfrom
fix/2631-sqlite-source-id-drizzle

Conversation

@Yeraze

@Yeraze Yeraze commented Apr 11, 2026

Copy link
Copy Markdown
Owner

Summary

Fresh 4.0 alpha SQLite installs crashed with SqliteError: no such column: source_id whenever a call reached getTelemetryByNodeAveraged, purgeChannelMessages, or purgeDirectMessages with a sourceId argument — which happens on every install that auto-creates a default source. Three raw SQL sites in DatabaseService referenced the column as source_id while the SQLite schema defines it as sourceId (camelCase, matching Postgres/MySQL via Drizzle).

Rather than re-rename the raw SQL, this PR converts the three sites to dispatch through new Drizzle-backed sync helpers on the repositories. Column names now come from the schema, so the snake_case/camelCase drift that caused #2631 cannot recur.

Changes

  • src/db/repositories/messages.ts — New sync helpers purgeChannelMessagesSqlite() and purgeDirectMessagesSqlite() using Drizzle delete().where().run()
  • src/db/repositories/telemetry.ts — New sync helper getTelemetryByNodeAveragedSqlite() using Drizzle select().groupBy() with sql templates for the CAST((timestamp / ?) * ? AS INTEGER) time-bucket expression
  • src/services/database.ts — SQLite branches in purgeChannelMessages, purgeDirectMessages, and getTelemetryByNodeAveraged now dispatch to the new sync helpers. Removed ~140 lines of raw SQL
  • src/db/repositories/telemetry.extra.test.ts — 5 new regression tests exercising sourceId scoping (crash regression, per-source filtering, unscoped fetch, raw-type scoping, sinceTimestamp interaction)
  • src/db/repositories/messages.purge.test.ts — New file with 6 tests for both purge sync helpers (crash regression, per-source isolation, broadcast !ffffffff exclusion)
  • CHANGELOG.md — Bug fix entry under the 4.0 multi-source section

Issues Resolved

Fixes #2631

Documentation Updates

  • CHANGELOG.md updated with a bug fix entry under the Unreleased 4.0 multi-source architecture section. No user-facing docs affected.

Testing

  • Unit tests pass (4226 tests total; 11 new regression tests added for this fix)
  • TypeScript compiles cleanly (tsc --noEmit)
  • Dev container built and deployed; container logs clean (no SqliteError, no uncaught exceptions)
  • Reviewer verify: on a fresh SQLite install with a default source, navigate to any node's telemetry charts and confirm no crash in container logs
  • Reviewer verify: delete a channel's messages via the admin UI and confirm only that channel+source's messages are deleted (not other sources)

🤖 Generated with Claude Code

@Yeraze @claude
fix(sqlite): convert source_id raw SQL to Drizzle sync helpers (#2631)
dc91a8d 
Fresh SQLite installs crashed with `SqliteError: no such column: source_id`
whenever a call reached `getTelemetryByNodeAveraged`, `purgeChannelMessages`,
or `purgeDirectMessages` with a `sourceId` argument — which happens on any
4.0 alpha install that auto-creates a default source. The SQLite branches
in `DatabaseService` were hand-rolled raw SQL that referenced `source_id`,
but the schema column is `sourceId`.

Rather than re-rename the raw SQL, convert the three sites to dispatch
through Drizzle-backed sync helpers on the repositories so column names
come from the schema and the drift can't recur:

- messages.ts: new `purgeChannelMessagesSqlite()` and
  `purgeDirectMessagesSqlite()` using Drizzle delete/where/run
- telemetry.ts: new `getTelemetryByNodeAveragedSqlite()` using Drizzle
  select/groupBy with `sql` templates for CAST-based bucketing
- services/database.ts: SQLite branches in purge+averaged methods now
  call the new helpers; removed ~140 lines of raw SQL

Regression tests (11 new):
- telemetry.extra.test.ts: 5 tests exercising sourceId scoping (crash
  regression, per-source filtering, unscoped fetch, raw-type scoping,
  sinceTimestamp interaction)
- messages.purge.test.ts: 6 tests covering both purge sync helpers
  (crash regression, per-source isolation, broadcast exclusion)

Fixes #2631

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Apr 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job

PR Review

This PR effectively addresses issue #2631 by converting problematic raw SQL queries to type-safe Drizzle helpers. The solution is architecturally sound and includes comprehensive regression testing.

Todo List

  • Read and analyze all changed files
  • Review conversion from raw SQL to Drizzle helpers
  • Analyze test coverage and regression tests
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Review security aspects
  • Provide comprehensive feedback

✅ Strengths

Excellent architectural approach: Instead of just fixing the column name in raw SQL, this PR properly moves the logic into repository layers where it belongs, following established patterns.

Type safety improvement: Converting from raw SQL to Drizzle query builders provides compile-time safety and prevents the schema/query drift that caused the original issue.

Comprehensive test coverage: 11 new regression tests specifically target the crash scenario and edge cases around sourceId scoping.

Clean implementation:

  • src/db/repositories/messages.ts:266-298 - Sync helpers properly validate SQLite-only usage and use appropriate Drizzle conditions
  • src/db/repositories/telemetry.ts:722-829 - Complex averaging logic properly translated with time-bucket SQL expressions
  • src/services/database.ts:3700-3735 - Clean delegation pattern with proper error handling

⚠️ Potential Issues

Performance consideration (src/db/repositories/telemetry.ts:756-763): The type counting query runs on every call to calculate the limit. While this won't be a bottleneck for typical node counts, consider caching this value if it becomes problematic.

SQLite-only validation: The new sync helpers correctly throw errors when called on non-SQLite databases, but the error messages could be more descriptive about why this restriction exists.

🧪 Test Quality

Excellent regression coverage:

  • src/db/repositories/messages.purge.test.ts - 179 lines of tests covering channel/DM purging with sourceId scoping
  • src/db/repositories/telemetry.extra.test.ts - Tests specifically for the crash scenario and per-source isolation

Good test patterns: Tests properly set up isolated in-memory databases and verify both positive and negative cases.

🔒 Security Assessment

No security concerns: The conversion from raw SQL to parameterized Drizzle queries actually improves security by eliminating any possibility of SQL injection.

🚀 Performance Assessment

Negligible performance impact: The Drizzle queries generate equivalent SQL to the original raw queries. The additional type counting query is offset by the elimination of ~140 lines of complex raw SQL.

📋 Minor Suggestions

  1. Documentation: Consider adding JSDoc comments to the new sync helpers explaining why they're SQLite-only
  2. Error messages: Make SQLite-only error messages more descriptive: 
    throw new Error('purgeChannelMessagesSqlite is SQLite-only - use async purgeChannelMessages for PostgreSQL/MySQL');

✅ Overall Assessment

This is a high-quality fix that addresses the root cause properly. The PR demonstrates excellent engineering practices:

  • Root cause analysis leading to architectural improvement
  • Comprehensive testing preventing regression
  • Following established codebase patterns
  • Clean separation of concerns

Recommendation: APPROVE - This fix is ready to merge and will prevent similar issues in the future.

@github-actions

github-actions Bot commented Apr 11, 2026

Copy link
Copy Markdown
System Test Results

MeshMonitor System Test Results

Test Run: 2026-04-11 19:56:41 EDT

Test Summary

Test Suite Result
Configuration Import ✅ PASSED
Quick Start Test ✅ PASSED
Security Test ✅ PASSED
V1 API Test ✅ PASSED
Reverse Proxy Test ✅ PASSED
Reverse Proxy + OIDC ✅ PASSED
Virtual Node CLI Test ✅ PASSED
Backup & Restore Test ✅ PASSED
Database Migration Test ✅ PASSED
DB Backing Consistency ✅ PASSED
API Exercise (3 DBs) ✅ PASSED

✅ Overall Result: PASSED

All deployment configurations are working correctly!

Test Details

Configuration Import:

  • Tests configuration import and device reboot cycle
  • Verifies channel roles, PSKs, and LoRa configuration
  • Note: Channel name verification skipped due to architectural limitation

Quick Start Test:

  • Zero-config deployment (no SESSION_SECRET or COOKIE_SECURE required)
  • HTTP access without HSTS
  • Auto-generated admin user with default credentials
  • Session cookies work over HTTP
  • Meshtastic node connection and message exchange verified

Security Test:

  • Verifies Node IP address hidden from anonymous users in API responses
  • Verifies MQTT configuration hidden from anonymous users
  • Verifies Node IP address visible to authenticated users
  • Verifies MQTT configuration visible to authenticated users
  • Verifies protected endpoints require authentication

V1 API Test:

  • Tests v1 REST API endpoints with Bearer token authentication
  • Verifies Bearer token requests bypass CSRF protection
  • Verifies POST/PUT/DELETE work without CSRF token when using Bearer auth
  • Verifies session-based requests still require CSRF token

Reverse Proxy Test:

  • Production deployment with COOKIE_SECURE=true
  • HTTPS-ready configuration
  • Trust proxy enabled for reverse proxy compatibility
  • CORS configured for HTTPS domain
  • Meshtastic node connection and message exchange verified

Reverse Proxy + OIDC Test:

  • OIDC authentication integration
  • Mock OIDC provider health checks
  • Authorization flow and session creation
  • Hybrid mode (OIDC + local auth)
  • Meshtastic node connection verified

Virtual Node CLI Test:

  • Virtual Node Server enabled on TCP port 4404
  • Meshtastic Python client successfully connects
  • Node data download and synchronization verified
  • Test message sent on gauntlet channel (index 3)
  • Message delivery confirmed via Web UI API
  • Virtual Node Server connection logging verified

Backup & Restore Test:

  • System backup created from running dev container
  • New container spun up with RESTORE_FROM_BACKUP env var
  • Data integrity verified (node count, message count, settings)
  • Restore event logged in audit log
  • Dev container unaffected by restore test

Database Migration Test:

  • SQLite to PostgreSQL migration verified
  • SQLite to MySQL migration verified
  • Data integrity confirmed for both target databases
  • Row counts match between source and target

DB Backing Consistency Test:

  • SQLite, PostgreSQL, and MySQL backends tested with same device
  • Node counts within ±10 across all three backends
  • Favorite counts identical across all backends
  • Key station verified as favorite on all backends

@Yeraze Yeraze merged commit 018cbd1 into main Apr 12, 2026
18 checks passed
@Yeraze Yeraze deleted the fix/2631-sqlite-source-id-drizzle branch April 12, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Error fetching telemetry: SqliteError: no such column: source_id

1 participant

@Yeraze