#41: use sqlc, dump sqlite #46
Conversation
WalkthroughThis change removes all support for SQLite as a backend database, transitioning the project to use only PostgreSQL. All code, configuration, documentation, and migration logic related to SQLite are deleted or replaced. The Go backend now uses the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant PostgresDB
User->>API: Sends request (e.g., create survey)
API->>PostgresDB: Executes sqlc-generated query via pgx
PostgresDB-->>API: Returns result (survey data)
API-->>User: Responds with survey info
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
api/pkg/storage/postgres.go (3)
158-165: Potential SQL injection risk with dynamic field namesUsing
fmt.Sprintfto build queries with dynamic field names can lead to SQL injection if thefieldparameter is not properly validated. Consider using a whitelist approach or a map of allowed fields.func (p *Postgres) GetSurveyByField(field string, value interface{}) (*types.Survey, error) { + // Whitelist allowed fields to prevent SQL injection + allowedFields := map[string]bool{ + "id": true, + "uuid": true, + "url_slug": true, + // add other allowed fields + } + if !allowedFields[field] { + return nil, fmt.Errorf("invalid field name: %s", field) + } + query := fmt.Sprintf(`SELECT s.id, s.uuid, s.created_at, s.parse_status, s.delivery_status, s.error_log, s.name, s.config, s.url_slug FROM surveys AS s WHERE s.%s=$1;`, field)
253-259: Remove incorrect nil check for QueryRow result
pgx.QueryRownever returns nil. It returns aRowthat defers error handling untilScanis called. The nil check is unnecessary and will never be true.row := p.conn.QueryRow(p.ctx, query, session.Status, session.SurveyUUID, session.IPAddr) - if row == nil { - return fmt.Errorf("unable to create survey session") - } return row.Scan(&session.ID, &session.UUID)The error from the query will be returned by
Scanif the query fails.
373-389: Validate sort parameters to prevent SQL injectionThe dynamic ORDER BY clause using
fmt.Sprintfwithfilter.SortByandfilter.Ordercould lead to SQL injection if these values come from user input.Add validation for the sort parameters:
func (p *Postgres) GetSurveySessionsWithAnswers(surveyUUID string, filter *types.SurveySessionsFilter) ([]types.SurveySession, int, error) { + // Validate sort parameters + allowedSortFields := map[string]bool{ + "created_at": true, + "completed_at": true, + "status": true, + // add other allowed fields + } + if !allowedSortFields[filter.SortBy] { + return nil, 0, fmt.Errorf("invalid sort field: %s", filter.SortBy) + } + + allowedOrders := map[string]bool{ + "ASC": true, + "DESC": true, + } + if !allowedOrders[strings.ToUpper(filter.Order)] { + return nil, 0, fmt.Errorf("invalid sort order: %s", filter.Order) + } + query := fmt.Sprintf(`WITH limited_sessions AS (
🧹 Nitpick comments (4)
api/pkg/db/extend.go (1)
9-24: Consider using standard UUID library for better maintainability.The implementation is correct and efficient, but consider using a standard UUID library like
github.com/google/uuidfor better maintainability and error handling.+import ( + "github.com/google/uuid" +) + -func EncodeUUID(uuid pgtype.UUID) string { - src := uuid.Bytes - var buf [36]byte - - hex.Encode(buf[0:8], src[:4]) - buf[8] = '-' - hex.Encode(buf[9:13], src[4:6]) - buf[13] = '-' - hex.Encode(buf[14:18], src[6:8]) - buf[18] = '-' - hex.Encode(buf[19:23], src[8:10]) - buf[23] = '-' - hex.Encode(buf[24:], src[10:]) - - return string(buf[:]) -} +func EncodeUUID(pgUUID pgtype.UUID) string { + if !pgUUID.Valid { + return "" + } + u, err := uuid.FromBytes(pgUUID.Bytes[:]) + if err != nil { + return "" + } + return u.String() +}Note: This would require adding the UUID dependency back to go.mod.
README.md (1)
321-327: Migration instructions simplified appropriately.The migration section correctly reflects the single-database approach with unified migration directory structure.
Consider adding a language specification to the fenced code block to improve documentation clarity:
-``` +```bash cd api migrate create -dir migrations -ext sql -seq name -``` +```api/pkg/storage/postgres.go (2)
261-274: Improve NULL handling in SQL queryInstead of string concatenation for NULL/NOW(), consider using parameterized queries with proper null handling.
func (p *Postgres) UpdateSurveySessionStatus(sessionUUID string, newStatus types.SurveySessionStatus) error { - completedAt := "NULL" - if newStatus == types.SurveySessionStatus_Completed { - completedAt = "NOW()" - } - - query := fmt.Sprintf(`UPDATE surveys_sessions - SET status = $1, completed_at = %s - WHERE uuid = $2;`, completedAt) - - _, err := p.conn.Exec(p.ctx, query, newStatus, sessionUUID) + var query string + var args []interface{} + + if newStatus == types.SurveySessionStatus_Completed { + query = `UPDATE surveys_sessions SET status = $1, completed_at = NOW() WHERE uuid = $2` + args = []interface{}{newStatus, sessionUUID} + } else { + query = `UPDATE surveys_sessions SET status = $1, completed_at = NULL WHERE uuid = $2` + args = []interface{}{newStatus, sessionUUID} + } + + _, err := p.conn.Exec(p.ctx, query, args...) return err }
455-463: Use database timestamp instead of application-generated timestampGenerating timestamps in the application can lead to inconsistencies, especially in distributed systems. Use the database's
NOW()function instead.func (p *Postgres) StoreWebhookResponse(sessionId int, responseStatus int, response string) error { query := `INSERT INTO surveys_webhook_responses - (created_at, session_id, response_status, response) - VALUES ($1, $2, $3, $4);` + (created_at, session_id, response_status, response) + VALUES (NOW(), $1, $2, $3);` - createdAtStr := time.Now().UTC().Format(types.DateTimeFormat) - _, err := p.conn.Exec(p.ctx, query, createdAtStr, sessionId, responseStatus, response) + _, err := p.conn.Exec(p.ctx, query, sessionId, responseStatus, response) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.gitignore(0 hunks)README.md(2 hunks)api/go.mod(2 hunks)api/migrations/sqlite/000001_schema.up.sql(0 hunks)api/pkg/db/db.go(1 hunks)api/pkg/db/extend.go(1 hunks)api/pkg/db/models.go(1 hunks)api/pkg/db/queries/surveys.sql(1 hunks)api/pkg/db/surveys.sql.go(1 hunks)api/pkg/services/services.go(1 hunks)api/pkg/storage/interface.go(0 hunks)api/pkg/storage/postgres.go(18 hunks)api/pkg/storage/sqlite.go(0 hunks)api/sqlc.yaml(1 hunks)compose.yaml(1 hunks)
💤 Files with no reviewable changes (4)
- .gitignore
- api/pkg/storage/interface.go
- api/migrations/sqlite/000001_schema.up.sql
- api/pkg/storage/sqlite.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/pkg/db/surveys.sql.go (3)
api/pkg/db/models.go (3)
NullSurveyParseStatuses(118-121)NullSurveyDeliveryStatuses(75-78)Survey(183-193)api/pkg/db/db.go (1)
Queries(24-26)api/pkg/surveys/manage.go (1)
CreateSurvey(13-40)
🪛 Checkov (3.2.334)
compose.yaml
[MEDIUM] 11-12: Basic Auth Credentials
(CKV_SECRET_4)
🪛 markdownlint-cli2 (0.17.2)
README.md
325-325: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 GitHub Check: Lint Go
api/pkg/storage/postgres.go
[failure] 253-253:
SA4023(related information): the lhs of the comparison is the 1st return value of this function call (staticcheck)
🪛 GitHub Actions: Tests
api/pkg/storage/postgres.go
[error] 253-253: golangci-lint staticcheck SA4023: the lhs of the comparison is the 1st return value of this function call
🔇 Additional comments (18)
api/go.mod (3)
3-3: LGTM: More specific Go version.The change to a more specific Go version (1.24.4) is a good practice for reproducible builds.
25-26: LGTM: Proper pgx indirect dependencies added.The addition of pgpassfile and pgservicefile indirect dependencies is expected with pgx/v5 for PostgreSQL connection management.
5-15: Confirmed safe removal of github.com/google/uuid dependency
Ripgrep searches for both the import path and common uuid calls (New, Parse, Must) returned no matches, so removing this module will not break the build.api/pkg/db/queries/surveys.sql (1)
1-6: LGTM: Well-structured sqlc query.The CreateSurvey query is properly formatted for sqlc with correct parameter placeholders and RETURNING clause. The query structure is clean and follows PostgreSQL best practices.
api/sqlc.yaml (1)
1-12: LGTM: Properly configured sqlc setup.The sqlc configuration is well-structured with appropriate settings for PostgreSQL migration:
- Correct version and engine specification
- Logical directory structure for queries and schema
- Proper pgx/v5 integration matching go.mod dependencies
- Clean package naming convention
This configuration aligns perfectly with the overall migration from SQLite to PostgreSQL.
api/pkg/services/services.go (2)
20-20: LGTM: Simplified storage initialization.The removal of conditional database type selection aligns with the SQLite removal and simplifies the codebase. The unconditional PostgreSQL initialization is cleaner and more maintainable.
17-35: DATABASE_TYPE removal verified—no impact found.
- No occurrences of
DATABASE_TYPEin Go code, YAML, or Dockerfiles.- No remaining references to SQLite.
- Documentation only mentions
DATABASE_URL(Postgres), with no mention of database type selection.compose.yaml (1)
11-11: Database configuration correctly updated for PostgreSQL migration.The environment variable changes properly reflect the transition from SQLite to PostgreSQL-only support. The connection string format is correct and the removal of
DATABASE_TYPEaligns with the simplified single-database approach.README.md (3)
290-290: Documentation correctly updated to reflect PostgreSQL as the sole database.The change from "optional" to required PostgreSQL deployment properly reflects the removal of SQLite support.
296-296: Environment variable documentation accurately updated.The
DATABASE_URLdescription now correctly specifies PostgreSQL connection string format, removing any SQLite references.
318-318: Tech stack section properly updated.The backend description now accurately lists only PostgreSQL, removing SQLite references.
api/pkg/db/db.go (1)
14-32: Well-structured database abstraction layer.The generated sqlc code provides a clean abstraction with proper context support and transaction handling. The DBTX interface correctly abstracts pgx connection types, and the WithTx method enables proper transaction scoping.
api/pkg/db/surveys.sql.go (3)
14-19: SQL query properly structured with complete column selection.The INSERT query correctly uses parameterized inputs and returns all necessary columns including generated fields like
id,uuid, andcreated_at.
21-28: Parameter struct uses appropriate nullable types.The
CreateSurveyParamsstruct correctly uses nullable enum types for optional fields and standard types for required fields, providing proper type safety.
30-52: Query method implements proper context handling and scanning.The
CreateSurveymethod correctly uses context for request scoping and properly scans all returned columns into the Survey struct.api/pkg/db/models.go (3)
14-54: Enum types properly implemented with nullable wrappers.The
CommonStatusesenum and its nullable wrapper correctly implement the Scanner and Valuer interfaces, providing proper database serialization with appropriate error handling.
56-139: Survey status enums follow consistent patterns.Both
SurveyDeliveryStatusesandSurveyParseStatusesenums maintain consistent implementation patterns with proper nullable wrappers and interface compliance.
183-227: Database table structs properly use PostgreSQL types.The table structs correctly use
pgtypefor nullable fields and appropriate Go types for required fields, ensuring proper mapping to the PostgreSQL schema.
| github.com/go-ozzo/ozzo-validation/v4 v4.3.0 | ||
| github.com/golang-migrate/migrate/v4 v4.17.1 | ||
| github.com/google/uuid v1.6.0 | ||
| github.com/jackc/pgx/v5 v5.5.4 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify pgx version for security vulnerabilities.
The addition of pgx/v5 v5.5.4 aligns with the PostgreSQL migration. However, please verify this version is secure and up-to-date.
🏁 Script executed:
#!/bin/bash
# Check for security advisories and latest version of pgx
echo "Checking pgx/v5 version and security..."
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/jackc/pgx/v5") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check latest releases
gh api repos/jackc/pgx/releases/latest | jq '.tag_name'Length of output: 992
To ensure v5.5.4 is the latest release in the Go module ecosystem, let’s fetch the @latest version from the Go proxy.
🏁 Script executed:
#!/bin/bash
# Check the latest pgx/v5 version from the Go module proxy
curl -s https://proxy.golang.org/github.com/jackc/pgx/v5/@latest | jq '.Version'Length of output: 89
Bump pgx/v5 to the latest patch release (v5.7.5)
v5.5.4 addresses the HIGH and MODERATE security advisories (SQL injection overflow and pipeline panic), but the Go proxy shows v5.7.5 as the current latest. Upgrading will include additional bug-fixes and any further hardening.
• api/go.mod (line 9): change
- github.com/jackc/pgx/v5 v5.5.4
+ github.com/jackc/pgx/v5 v5.7.5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| github.com/jackc/pgx/v5 v5.5.4 | |
| github.com/jackc/pgx/v5 v5.7.5 |
🤖 Prompt for AI Agents
In api/go.mod at line 9, update the version of github.com/jackc/pgx/v5 from
v5.5.4 to the latest patch release v5.7.5 to include additional bug fixes and
security improvements. Replace the existing version string with the new version
number.
| func EncodeUUID(uuid pgtype.UUID) string { | ||
| src := uuid.Bytes | ||
| var buf [36]byte | ||
|
|
||
| hex.Encode(buf[0:8], src[:4]) | ||
| buf[8] = '-' | ||
| hex.Encode(buf[9:13], src[4:6]) | ||
| buf[13] = '-' | ||
| hex.Encode(buf[14:18], src[6:8]) | ||
| buf[18] = '-' | ||
| hex.Encode(buf[19:23], src[8:10]) | ||
| buf[23] = '-' | ||
| hex.Encode(buf[24:], src[10:]) | ||
|
|
||
| return string(buf[:]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for robustness.
The function should handle invalid UUID inputs gracefully.
func EncodeUUID(uuid pgtype.UUID) string {
+ if !uuid.Valid {
+ return ""
+ }
src := uuid.Bytes
var buf [36]byte
hex.Encode(buf[0:8], src[:4])
buf[8] = '-'
hex.Encode(buf[9:13], src[4:6])
buf[13] = '-'
hex.Encode(buf[14:18], src[6:8])
buf[18] = '-'
hex.Encode(buf[19:23], src[8:10])
buf[23] = '-'
hex.Encode(buf[24:], src[10:])
return string(buf[:])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func EncodeUUID(uuid pgtype.UUID) string { | |
| src := uuid.Bytes | |
| var buf [36]byte | |
| hex.Encode(buf[0:8], src[:4]) | |
| buf[8] = '-' | |
| hex.Encode(buf[9:13], src[4:6]) | |
| buf[13] = '-' | |
| hex.Encode(buf[14:18], src[6:8]) | |
| buf[18] = '-' | |
| hex.Encode(buf[19:23], src[8:10]) | |
| buf[23] = '-' | |
| hex.Encode(buf[24:], src[10:]) | |
| return string(buf[:]) | |
| } | |
| func EncodeUUID(uuid pgtype.UUID) string { | |
| if !uuid.Valid { | |
| return "" | |
| } | |
| src := uuid.Bytes | |
| var buf [36]byte | |
| hex.Encode(buf[0:8], src[:4]) | |
| buf[8] = '-' | |
| hex.Encode(buf[9:13], src[4:6]) | |
| buf[13] = '-' | |
| hex.Encode(buf[14:18], src[6:8]) | |
| buf[18] = '-' | |
| hex.Encode(buf[19:23], src[8:10]) | |
| buf[23] = '-' | |
| hex.Encode(buf[24:], src[10:]) | |
| return string(buf[:]) | |
| } |
🤖 Prompt for AI Agents
In api/pkg/db/extend.go around lines 9 to 24, the EncodeUUID function lacks
input validation for the UUID parameter, which may cause issues if the input is
invalid. Add a check to verify that the UUID is valid before proceeding with
encoding, and handle invalid inputs gracefully by returning an empty string or
an error indicator to prevent unexpected behavior.
| type Postgres struct { | ||
| conn *sql.DB | ||
| addr string | ||
| conn *pgx.Conn | ||
| queries *db.Queries | ||
| addr string | ||
| ctx context.Context | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid storing context in struct fields
Storing context in struct fields is an anti-pattern in Go. Context should be passed as the first parameter to methods that need it. This allows for proper cancellation, timeouts, and deadline propagation.
Remove the ctx field from the struct and pass context as a parameter to methods:
type Postgres struct {
conn *pgx.Conn
queries *db.Queries
addr string
- ctx context.Context
}Then update methods to accept context as the first parameter, for example:
func (p *Postgres) UpdateSurvey(ctx context.Context, survey *types.Survey) error {
// use ctx instead of p.ctx
}🤖 Prompt for AI Agents
In api/pkg/storage/postgres.go around lines 23 to 28, the Postgres struct
incorrectly stores a context.Context as a field, which is an anti-pattern.
Remove the ctx field from the struct definition and update all methods of
Postgres to accept context.Context as their first parameter instead of relying
on the stored field. Modify method implementations to use the passed context
parameter for cancellation and timeout handling.
| var err error | ||
| p.conn, err = sql.Open("postgres", p.addr) | ||
| p.conn, err = pgx.Connect(context.Background(), p.addr) | ||
| if err != nil { | ||
| return err | ||
| log.Fatalf("cannot connect to postgres: %v", err) | ||
| } |
There was a problem hiding this comment.
Return error instead of calling log.Fatalf
Using log.Fatalf in a library method terminates the entire program, which prevents proper error handling by the caller. Return the error instead.
- p.conn, err = pgx.Connect(context.Background(), p.addr)
- if err != nil {
- log.Fatalf("cannot connect to postgres: %v", err)
- }
+ p.conn, err = pgx.Connect(context.Background(), p.addr)
+ if err != nil {
+ return fmt.Errorf("cannot connect to postgres: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var err error | |
| p.conn, err = sql.Open("postgres", p.addr) | |
| p.conn, err = pgx.Connect(context.Background(), p.addr) | |
| if err != nil { | |
| return err | |
| log.Fatalf("cannot connect to postgres: %v", err) | |
| } | |
| var err error | |
| p.conn, err = pgx.Connect(context.Background(), p.addr) | |
| if err != nil { | |
| return fmt.Errorf("cannot connect to postgres: %v", err) | |
| } |
🤖 Prompt for AI Agents
In api/pkg/storage/postgres.go around lines 38 to 42, replace the log.Fatalf
call with returning the error to the caller instead of terminating the program.
Modify the function signature to return an error if it does not already, and
propagate the connection error by returning it so the caller can handle it
properly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
api/go.mod (1)
9-9: Update pgx/v5 to the latest version for security and bug fixes.As identified in the previous review, the current version v5.5.4 should be updated to v5.7.5 to include additional security improvements and bug fixes.
api/pkg/storage/postgres.go (2)
23-28: Context storage in struct remains an anti-pattern.The issue of storing context in the struct field was identified in the previous review and still needs to be addressed. Context should be passed as parameters to methods for proper cancellation and timeout handling.
38-42: Return error instead of calling log.Fatalf.As identified in the previous review, using
log.Fatalfterminates the program and prevents proper error handling by the caller. This should return the error instead.
🧹 Nitpick comments (1)
ui/src/components/app/SurveyResponsesPage.tsx (1)
305-305: Consider using optional chaining for safer property access.The current conditional rendering can be simplified and made safer using optional chaining.
- <Table.Cell>{question && question.label}</Table.Cell> + <Table.Cell>{question?.label}</Table.Cell>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
api/go.mod(1 hunks)api/pkg/storage/postgres.go(18 hunks)ui/src/components/app/SurveyResponsesPage.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/components/app/SurveyResponsesPage.tsx
[error] 305-305: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
ui/src/components/app/SurveyResponsesPage.tsx (1)
1-329: LGTM! Formatting standardization looks good.The indentation has been consistently standardized to 2 spaces throughout the file. These formatting-only changes improve code consistency without affecting functionality.
api/go.mod (1)
3-15: LGTM! Dependency updates align with PostgreSQL migration.The dependency changes properly reflect the migration from SQLite to PostgreSQL:
- Removed SQLite-specific dependencies (
google/uuid,mattn/go-sqlite3)- Added PostgreSQL driver (
jackc/pgx/v5)- Updated Go version to a more specific patch release
- Updated testify to latest version
These changes support the architectural shift to PostgreSQL-only backend.
api/pkg/storage/postgres.go (2)
1-110: Excellent architectural improvement with sqlc integration.The refactoring to use pgx driver and sqlc-generated queries is well-implemented:
- Proper use of typed parameters with nullable wrappers
- Clean separation of concerns with
db.Queries- Correct error handling and marshaling in
CreateSurvey- Proper assignment of generated IDs and UUIDs back to the struct
This represents a significant improvement in type safety and maintainability.
64-78: Migration logic simplified appropriately.The migration logic has been properly simplified to use a unified migrations directory, removing the complexity of SQLite-specific migration handling. This aligns well with the PostgreSQL-only approach.
TODO:
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores