refactor: rewrite app with chi and sqlc#10
Conversation
steveiliop56
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded@steveiliop56 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThis pull request executes a major architectural migration from Gin+GORM+Zerolog to Chi+SQLc+slog. The analytics viewer component is removed entirely. Internal service layers (database, cache, rate limit middleware) are replaced with direct implementations. Database access transitions from ORM-based to SQLc-generated query methods. Configuration and environment variables are streamlined, with DB_PATH renamed to DATABASE_PATH. The Dockerfile and Docker Compose configurations are updated to reflect the new stack. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
35-35: Fix environment variable name inconsistency.The Dockerfile uses
DB_PATH, but the application has been refactored to useDATABASE_PATH(as seen in docker-compose.dev.yml line 9 and .vscode/launch.json line 11). This inconsistency will prevent the containerized application from finding the database.Apply this diff to fix the environment variable name:
-ENV DB_PATH=/data/analytics.db +ENV DATABASE_PATH=/data/analytics.db
🧹 Nitpick comments (9)
Dockerfile (1)
43-43: Remove unused Gin framework environment variable.The application has been refactored from Gin to Chi router. The
GIN_MODEenvironment variable is no longer relevant and should be removed.Apply this diff to remove the unused environment variable:
-ENV GIN_MODE=release -query.sql (1)
15-18: Consider uppercase SQL keywords for consistency.While SQL is case-insensitive and
setworks correctly, using uppercaseSETfollows the conventional style seen in your other queries and improves readability.Apply this diff if you'd like to standardize:
-- name: UpdateInstance :exec UPDATE instances -set last_seen = ? +SET last_seen = ? WHERE uuid = ?;health_handler.go (1)
15-21: RedundantWriteHeadercall beforerender.JSON.
render.JSONinternally sets theContent-Typeheader and writes the response. Callingw.WriteHeader(http.StatusOK)beforehand is unnecessary for 200 OK (the default) and could cause issues if headers need to be modified by render. This pattern is inconsistent with how other handlers ininstances_handler.gouse it (where WriteHeader is called for non-200 statuses).func (h *HealthHandler) health(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) render.JSON(w, r, map[string]string{ "status": "200", "message": "up and running", }) }cache.go (1)
64-68: Nitpick: Remove unnecessary capacity hint.The
0capacity hint inmake(map[string]cacheField, 0)is redundant.func (c *Cache) Flush() { c.mutex.Lock() defer c.mutex.Unlock() - c.cache = make(map[string]cacheField, 0) + c.cache = make(map[string]cacheField) }instances_handler.go (1)
41-56: Consider validating UUID and Version are non-empty.The heartbeat payload is decoded but there's no validation that
UUIDandVersionare non-empty strings. Empty values would be persisted to the database.err := render.DecodeJSON(r.Body, &heartbeat) if err != nil { w.WriteHeader(http.StatusBadRequest) render.JSON(w, r, map[string]string{ "status": "400", "message": "Invalid request payload", }) return } + + if heartbeat.UUID == "" || heartbeat.Version == "" { + w.WriteHeader(http.StatusBadRequest) + render.JSON(w, r, map[string]string{ + "status": "400", + "message": "UUID and version are required", + }) + return + }rate_limiter.go (1)
30-34: Performance concern: Global lock serializes all requests.Using
rl.mutex.Lock()for the entire middleware serializes all HTTP requests through the rate limiter, creating a bottleneck. Since theCachealready has its own mutex for thread-safe access, this outer lock appears redundant.Consider removing the RateLimiter-level mutex since Cache operations are already thread-safe, or use more granular per-IP locking if atomicity between Get and Set is required.
main.go (3)
61-67: Missing error handling for SQL statements.The
PRAGMAandCREATE TABLEexecutions ignore errors. If WAL mode cannot be enabled or table creation fails, the application continues with a potentially broken state.- sqlDb.Exec(`PRAGMA journal_mode=WAL;`) + if _, err := sqlDb.Exec(`PRAGMA journal_mode=WAL;`); err != nil { + slog.Warn("failed to enable WAL mode", "error", err) + } - sqlDb.Exec(`CREATE TABLE IF NOT EXISTS "instances" ( + _, err = sqlDb.Exec(`CREATE TABLE IF NOT EXISTS "instances" ( "uuid" TEXT NOT NULL PRIMARY KEY, "version" TEXT NOT NULL, "last_seen" INTEGER NOT NULL - );`) + );`) + if err != nil { + slog.Error("failed to create instances table", "error", err) + os.Exit(1) + }
121-129: Misleading variable name:rowsAffectedholds deleted instances.
DeleteOldInstancesreturns[]Instance(the deleted records), not a count. The variable namerowsAffectedis misleading.- rowsAffected, err := queries.DeleteOldInstances(context.Background(), cutoffTime) + deletedInstances, err := queries.DeleteOldInstances(context.Background(), cutoffTime) if err != nil { slog.Error("failed to clean up old instances: ", "error", err) continue } - slog.Info("old instances cleaned up", "rows_affected", rowsAffected) + slog.Info("old instances cleaned up", "count", len(deletedInstances))
97-111: No graceful shutdown handling.The server starts without signal handling. On SIGTERM/SIGINT, connections are dropped abruptly and the cleanup goroutine cannot exit cleanly. Consider using
signal.NotifyContextandsrv.Shutdown.This is a good-to-have improvement for production deployments to ensure in-flight requests complete and resources are released properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumviewer/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.gitignore(1 hunks).vscode/launch.json(1 hunks)Dockerfile(1 hunks)README.md(2 hunks)cache.go(1 hunks)database/queries/db.go(1 hunks)database/queries/models.go(1 hunks)database/queries/query.sql.go(1 hunks)docker-compose.dev.yml(1 hunks)go.mod(1 hunks)health_handler.go(1 hunks)instances_handler.go(1 hunks)internal/controller/health_controller.go(0 hunks)internal/controller/instances_controller.go(0 hunks)internal/middleware/rate_limit_middleware.go(0 hunks)internal/middleware/zerolog_middleware.go(0 hunks)internal/model/instance_model.go(0 hunks)internal/service/cache_service.go(0 hunks)internal/service/database_service.go(0 hunks)internal/service/migrations/000001_init_sqlite.down.sql(0 hunks)internal/service/migrations/000001_init_sqlite.up.sql(0 hunks)main.go(1 hunks)query.sql(1 hunks)rate_limiter.go(1 hunks)schema.sql(1 hunks)sqlc.yaml(1 hunks)viewer/.gitignore(0 hunks)viewer/README.md(0 hunks)viewer/main.ts(0 hunks)viewer/package.json(0 hunks)viewer/tsconfig.json(0 hunks)
💤 Files with no reviewable changes (14)
- viewer/README.md
- viewer/main.ts
- internal/controller/health_controller.go
- viewer/.gitignore
- internal/controller/instances_controller.go
- viewer/package.json
- internal/model/instance_model.go
- viewer/tsconfig.json
- internal/service/migrations/000001_init_sqlite.up.sql
- internal/middleware/zerolog_middleware.go
- internal/service/database_service.go
- internal/middleware/rate_limit_middleware.go
- internal/service/migrations/000001_init_sqlite.down.sql
- internal/service/cache_service.go
🧰 Additional context used
🧬 Code graph analysis (4)
instances_handler.go (2)
database/queries/db.go (1)
Queries(23-25)database/queries/query.sql.go (2)
CreateInstanceParams(20-24)UpdateInstanceParams(115-118)
rate_limiter.go (1)
cache.go (1)
Cache(13-16)
database/queries/query.sql.go (2)
database/queries/db.go (1)
Queries(23-25)database/queries/models.go (1)
Instance(7-11)
main.go (5)
database/queries/db.go (2)
New(19-21)Queries(23-25)cache.go (1)
NewCache(18-24)rate_limiter.go (2)
NewRateLimiter(23-28)RateLimitConfig(12-15)instances_handler.go (1)
NewInstancesHandler(15-19)health_handler.go (1)
NewHealthHandler(11-13)
🪛 LanguageTool
README.md
[grammar] ~7-~7: Ensure spelling is correct
Context: ...tains the UUID generated by Tinyauth on start up and the version information. The server...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (12)
go.mod (1)
5-12: No sqlc dependency is required in go.mod.The generated sqlc code uses only the standard library (
contextanddatabase/sql), not any sqlc runtime packages. sqlc is a code-generation tool that produces self-contained Go code—it is not a runtime dependency and should not appear ingo.mod..vscode/launch.json (1)
11-11: LGTM!The environment variable rename from
DB_PATHtoDATABASE_PATHaligns with the broader refactoring effort and is consistent with the docker-compose configuration..gitignore (1)
2-11: LGTM!The changes appropriately ignore the entire data directory (more robust than a specific file) and include the compiled binary in the build output section.
Dockerfile (1)
13-18: LGTM!The expanded build context correctly includes all new source files required for the Chi+SQLc architecture.
sqlc.yaml (1)
1-14: LGTM!The sqlc configuration is well-structured. The column override for
instances.idto exclude it from JSON serialization and the UUID rename both follow Go conventions.docker-compose.dev.yml (1)
21-37: LGTM!Commenting out the analytics-dashboard service aligns with the PR's architectural shift to a leaner backend-focused deployment.
database/queries/models.go (1)
1-11: LGTM!This is sqlc-generated code that correctly represents the instances table schema. The Instance struct appropriately maps the database columns.
schema.sql (1)
1-5: LGTM!The instances table schema is clean and appropriate. Using TEXT for UUID and INTEGER for Unix timestamp are both standard SQLite practices. The IF NOT EXISTS clause ensures idempotent execution.
query.sql (1)
1-13: LGTM!All queries are well-formed, use parameterized placeholders for security, and follow SQLite best practices. The
DeleteOldInstancesquery withRETURNING *is particularly good for tracking which instances were pruned.Also applies to: 20-27
database/queries/db.go (1)
1-31: LGTM!This is standard sqlc-generated code providing a clean abstraction over database operations with transaction support via
WithTx.database/queries/query.sql.go (1)
1-123: LGTM!This is well-structured sqlc-generated code with proper resource management (deferred
rows.Close()) and error handling patterns.rate_limiter.go (1)
72-77: Security consideration:CF-Connecting-IPis trusted unconditionally.This header is only reliable when the request comes through Cloudflare. If the server is accessed directly (bypassing Cloudflare), clients can spoof this header. Consider validating that
RemoteAddris a known Cloudflare IP range before trusting this header.Is this service always deployed behind Cloudflare? If not, an attacker could bypass rate limiting by setting a fake
CF-Connecting-IPheader.
1 participant
Summary by CodeRabbit
New Features
Removals
Chores
✏️ Tip: You can customize this high-level summary in your review settings.