feat: move all queries to sqlc #47
Conversation
WalkthroughThis update significantly refactors and expands the backend data access layer for surveys. It introduces new, strongly-typed SQL queries and corresponding Go methods for survey and session management, replaces hand-written mocks with auto-generated ones using mockery, and updates the storage implementation to leverage the new query interfaces. Supporting utilities and documentation are also updated for consistency and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Storage Layer (Postgres)
participant Queries as db.Queries (Generated)
participant DB as Database
Service->>Queries: Call method (e.g., UpdateSurvey)
Queries->>DB: Execute SQL with typed params
DB-->>Queries: Return result/rows
Queries-->>Service: Return typed result/error
Service-->>Caller: Return mapped domain object/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 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: 3
🔭 Outside diff range comments (1)
api/pkg/storage/postgres.go (1)
445-461: Critical: SQL injection vulnerability with string interpolationThe
GetSurveySessionsWithAnswersmethod constructs SQL queries using string formatting with user-provided sort parameters. This creates a SQL injection vulnerability. Thefilter.SortByandfilter.Ordervalues are directly interpolated into the query without validation.This method should be migrated to sqlc like the other queries, or at minimum, validate the sort parameters against a whitelist.
func (p *Postgres) GetSurveySessionsWithAnswers(surveyUUID string, filter *types.SurveySessionsFilter) ([]types.SurveySession, int, error) { + // Validate sort parameters to prevent SQL injection + validSortColumns := map[string]bool{ + "created_at": true, + "completed_at": true, + "status": true, + } + validOrders := map[string]bool{ + "ASC": true, + "DESC": true, + } + + if !validSortColumns[filter.SortBy] { + return nil, 0, fmt.Errorf("invalid sort column: %s", filter.SortBy) + } + if !validOrders[filter.Order] { + return nil, 0, fmt.Errorf("invalid sort order: %s", filter.Order) + } + query := fmt.Sprintf(`WITH limited_sessions AS (
🧹 Nitpick comments (6)
README.md (1)
288-290: Bullet list feels redundant and loses the actionable detail the previous version providedThe three bullets don’t add information beyond the sentence on Line 286. Either remove them or enrich each item with the concrete deployment hints that were deleted (e.g., container names, default ports, links to Dockerfiles).
api/pkg/db/queries/surveys.sql (2)
108-121: Review complex CTE query for performance.The
GetSurveySessionsWithAnswersquery uses a CTE with multiple LEFT JOINs. While comprehensive, this could be expensive for large datasets.Consider the performance implications and potential for N+1-like issues. Monitor query execution plans in production and consider:
- Adding appropriate indexes
- Breaking into separate queries if performance becomes an issue
- Implementing result caching for frequently accessed data
-- Potential performance indexes to consider: -- CREATE INDEX idx_surveys_sessions_survey_created ON surveys_sessions(survey_id, created_at DESC); -- CREATE INDEX idx_surveys_answers_session ON surveys_answers(session_id); -- CREATE INDEX idx_surveys_webhook_responses_session ON surveys_webhook_responses(session_id);
12-19: Ensure composite index on surveys_sessions(survey_id, status) for optimal performanceI wasn’t able to find an existing index on
surveys_sessions(survey_id, status)in the SQL or Go code—please double-check your migration files or schema to confirm it’s present. If it’s missing, add:-- Index to speed up correlated subqueries in GetSurveys CREATE INDEX idx_surveys_sessions_survey_status ON surveys_sessions(survey_id, status);api/pkg/db/surveys.sql.go (2)
85-88: Rename generic parameter name for clarityThe parameter
Column2inDeleteSurveyQuestionsNotInListParamsis generic and doesn't convey its purpose. Consider renaming it toQuestionIDsorKeepQuestionIDsfor better readability.
223-225: Consider using more descriptive parameter names in SQLThe auto-generated parameter names like
Uuid_2are not very descriptive. Consider updating the SQL query parameter names to be more specific (e.g.,$session_uuidand$survey_uuid) which would result in better generated Go code.Also applies to: 572-576
api/pkg/storage/postgres.go (1)
314-325: Simplify conditional logicThe else block is unnecessary after a return statement.
if newStatus == types.SurveySessionStatus_Completed { return p.queries.UpdateSurveySessionStatusCompleted(p.ctx, db.UpdateSurveySessionStatusCompletedParams{ Status: db.NullSurveysSessionsStatus{Valid: true, SurveysSessionsStatus: db.SurveysSessionsStatus(newStatus)}, Uuid: uuid, }) - } else { - return p.queries.UpdateSurveySessionStatus(p.ctx, db.UpdateSurveySessionStatusParams{ - Status: db.NullSurveysSessionsStatus{Valid: true, SurveysSessionsStatus: db.SurveysSessionsStatus(newStatus)}, - Uuid: uuid, - }) } + + return p.queries.UpdateSurveySessionStatus(p.ctx, db.UpdateSurveySessionStatusParams{ + Status: db.NullSurveysSessionsStatus{Valid: true, SurveysSessionsStatus: db.SurveysSessionsStatus(newStatus)}, + Uuid: uuid, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(1 hunks)api/.mockery.yml(1 hunks)api/Makefile(1 hunks)api/mocks/answer.go(0 hunks)api/mocks/interface.go(0 hunks)api/pkg/db/extend.go(1 hunks)api/pkg/db/mocks_test.go(1 hunks)api/pkg/db/queries/surveys.sql(1 hunks)api/pkg/db/surveys.sql.go(1 hunks)api/pkg/storage/mocks_test.go(1 hunks)api/pkg/storage/postgres.go(4 hunks)api/pkg/types/mocks_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- api/mocks/interface.go
- api/mocks/answer.go
🔇 Additional comments (10)
api/pkg/db/extend.go (1)
26-30: LGTM! Clean UUID decoding utility.The
DecodeUUIDfunction provides a clean abstraction for converting UUID strings topgtype.UUID. The implementation correctly delegates topgtype.UUID.Scan()for proper validation and parsing, maintaining consistency with the existingEncodeUUIDfunction.api/.mockery.yml (1)
1-18: Excellent centralized mock configuration.The mockery configuration follows best practices by:
- Centralizing mock generation settings in a single file
- Using the testify template for comprehensive mock functionality
- Applying consistent naming conventions (
mocks_test.go)- Enabling recursive processing for all interfaces
- Including proper formatting with goimports
This approach significantly improves maintainability compared to scattered manual mock configurations.
api/Makefile (1)
9-9: Good simplification leveraging centralized configuration.Removing the explicit mockery flags in favor of the
.mockery.ymlconfiguration file improves maintainability and follows the DRY principle. The mock generation parameters are now centrally managed and version-controlled.api/pkg/types/mocks_test.go (1)
1-145: Auto-generated mock looks correct.This auto-generated mock file follows standard mockery + testify patterns correctly:
- Proper mock embedding and constructor
- Type-safe fluent API for method expectations
- Appropriate panic handling for missing return values
- Clean separation of concerns with typed call structs
Since this is generated code, the main verification is that the generation process worked correctly, which it has.
api/pkg/db/queries/surveys.sql (4)
7-10: LGTM! Clean update query with proper parameterization.The
UpdateSurveyquery correctly uses parameterized queries to prevent SQL injection and follows a standard UPDATE pattern. All survey fields are updateable via parameters.
55-59: Good use of subquery for referential integrity.The
CreateSurveySessionquery correctly uses a subquery to resolve the survey UUID to its ID, maintaining referential integrity while accepting UUID parameters.
61-64: LGTM! Proper timestamp handling for completion.The
UpdateSurveySessionStatusCompletedquery correctly sets both status and completion timestamp atomically usingNOW().
37-40: Ensure Empty Slice Behavior Is IntentionalThe code in
api/pkg/storage/postgres.goallocatesquestionIdsfrom the length ofsurvey.Config.Questions.Questions, so when there are no questions it passes an empty[]string{}into theDeleteSurveyQuestionsNotInListcall. In PostgreSQL:
question_id != ALL(ARRAY[]::text[])is always true (vacuous truth), so theDELETEwill remove all rows for that survey.- That matches the semantic “no questions remain → delete all existing questions.”
No adjustments are required unless you want different behavior on empty lists. If you do, you could:
• Skip calling
DeleteSurveyQuestionsNotInListwhenlen(questionIds) == 0to preserve existing rows
• Switch toquestion_id = ANY($2::text[])with an empty array check if your intent is “delete none”Otherwise, this is correct.
api/pkg/storage/mocks_test.go (1)
1-1306: Auto-generated mocks look goodThe mockery-generated mocks follow the standard testify template and include all necessary methods for
MockInterfaceandMockFileInterface. The expecter pattern and cleanup functions are properly implemented.api/pkg/db/mocks_test.go (1)
1-279: Auto-generated database mocks are well-structuredThe DBTX mock properly handles variadic arguments and context parameters for database operations. The implementation correctly manages both fixed and variable argument scenarios.
| -- name: StoreWebhookResponse :exec | ||
| INSERT INTO surveys_webhook_responses | ||
| (created_at, session_id, response_status, response) | ||
| VALUES ($1, $2, $3, $4); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify webhook response data handling.
The StoreWebhookResponse query looks correct but ensure the calling code properly handles potential large response payloads and validates the response_status values.
🏁 Script executed:
#!/bin/bash
# Check webhook response handling and any size limits
echo "Searching for webhook response handling..."
rg -A 10 -B 5 "StoreWebhookResponse|webhook.*response"Length of output: 12553
Enforce payload size limit & status code validation for webhook responses
The call in api/pkg/surveys/sessions.go currently uses io.ReadAll(resp.Body) without any cap and stores the raw resp.StatusCode directly. To prevent unbounded memory use and ensure only valid statuses are recorded, please:
- In api/pkg/surveys/sessions.go around the io.ReadAll(resp.Body) call, wrap resp.Body in an io.LimitReader (e.g.
io.LimitReader(resp.Body, maxPayloadBytes)) and define a reasonable maximum (e.g. 1 MB). - After reading, if the payload exceeds your limit, decide whether to truncate, reject, or archive it externally instead of storing the full string.
- Validate resp.StatusCode before passing it to StoreWebhookResponse (e.g. ensure it’s within 100–599, map unexpected codes to an error state, or enforce a whitelist of expected codes).
🤖 Prompt for AI Agents
In api/pkg/surveys/sessions.go near the io.ReadAll(resp.Body) call, wrap
resp.Body with io.LimitReader using a defined maxPayloadBytes (e.g., 1 MB) to
cap the read size. After reading, implement logic to handle payloads exceeding
this limit by truncating, rejecting, or archiving externally instead of storing
the full content. Also, validate resp.StatusCode before passing it to
StoreWebhookResponse to ensure it falls within the 100–599 range or matches an
allowed set; map any invalid codes to an error state or handle accordingly. This
prevents unbounded memory use and ensures only valid status codes are stored.
| const createSurveySession = `-- name: CreateSurveySession :one | ||
| INSERT INTO surveys_sessions | ||
| (status, survey_id, ip_addr) | ||
| VALUES ($1, (SELECT s.id FROM surveys s WHERE s.uuid = $2), $3) | ||
| RETURNING id, uuid | ||
| ` | ||
|
|
||
| type CreateSurveySessionParams struct { | ||
| Status NullSurveysSessionsStatus | ||
| Uuid pgtype.UUID | ||
| IpAddr pgtype.Text | ||
| } | ||
|
|
||
| type CreateSurveySessionRow struct { | ||
| ID int32 | ||
| Uuid pgtype.UUID | ||
| } | ||
|
|
||
| func (q *Queries) CreateSurveySession(ctx context.Context, arg CreateSurveySessionParams) (CreateSurveySessionRow, error) { | ||
| row := q.db.QueryRow(ctx, createSurveySession, arg.Status, arg.Uuid, arg.IpAddr) | ||
| var i CreateSurveySessionRow | ||
| err := row.Scan(&i.ID, &i.Uuid) | ||
| return i, err | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding index on surveys.uuid for subquery performance
The CreateSurveySession query uses a subquery to fetch survey ID by UUID. This operation would benefit from an index on surveys.uuid if not already present.
🏁 Script executed:
#!/bin/bash
# Check if there's an index on surveys.uuid column
rg -A 5 "CREATE.*INDEX.*surveys.*uuid" --glob "migrations/*.sql"Length of output: 215
I wasn’t able to find any SQL files under the migrations/ folder with your first search. Let’s broaden the search to locate your migrations directory and then scan all .sql files for any index on a uuid column:
🏁 Script executed:
#!/bin/bash
# 1. Locate any migrations folders in the repo:
echo "🔍 Looking for migrations directories:"
find . -type d -name migrations
# 2. Search all SQL files for index creation on uuid columns:
echo -e "\n🔍 Searching for CREATE INDEX statements referencing 'uuid':"
rg -Hn "CREATE\s+INDEX" --glob "*.sql" | grep -i uuid || echo "No CREATE INDEX on uuid found in .sql files"Length of output: 433
Add missing index on surveys.uuid for subquery performance
To speed up the lookup in CreateSurveySession—which does
(SELECT s.id FROM surveys s WHERE s.uuid = $2)—please add an index on the uuid column:
• Create a new migration under api/migrations, for example:
20250801T120000_create_surveys_uuid_index.sql
-- Add index on surveys.uuid to optimize session-creation subquery
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_surveys_uuid
ON surveys (uuid);• This ensures the subquery in
api/pkg/db/surveys.sql.go:CreateSurveySession
benefits from index lookups rather than full table scans.
🤖 Prompt for AI Agents
In api/pkg/db/surveys.sql.go around lines 54 to 77, the subquery in
CreateSurveySession uses surveys.uuid without an index, causing slow lookups. To
fix this, add a new migration SQL file under api/migrations named like
20250801T120000_create_surveys_uuid_index.sql that creates a concurrent index on
the surveys.uuid column. This will optimize the subquery by enabling index
lookups instead of full table scans.
| func (p *Postgres) GetSurveyByField(field string, value interface{}) (*types.Survey, error) { | ||
| 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) | ||
|
|
||
| row := p.conn.QueryRow(p.ctx, query, value) | ||
| survey := &types.Survey{} | ||
| err := row.Scan(&survey.ID, &survey.UUID, &survey.CreatedAt, | ||
| &survey.ParseStatus, &survey.DeliveryStatus, &survey.ErrorLog, | ||
| &survey.Name, &survey.Config, &survey.URLSlug) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { | ||
| return nil, nil | ||
| } | ||
| var survey *types.Survey | ||
| var err error | ||
|
|
||
| return nil, err | ||
| switch field { | ||
| case "uuid": | ||
| uuid, err := db.DecodeUUID(value.(string)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode UUID: %w", err) | ||
| } | ||
| row, err := p.queries.GetSurveyByUUID(p.ctx, uuid) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { | ||
| return nil, nil | ||
| } | ||
| return nil, err | ||
| } | ||
| survey = &types.Survey{ | ||
| ID: int64(row.ID), | ||
| UUID: db.EncodeUUID(row.Uuid), | ||
| CreatedAt: row.CreatedAt.Time, | ||
| ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses), | ||
| DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses), | ||
| ErrorLog: row.ErrorLog.String, | ||
| Name: row.Name, | ||
| URLSlug: row.UrlSlug, | ||
| } | ||
| if err := json.Unmarshal(row.Config, &survey.Config); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal survey config: %w", err) | ||
| } | ||
| case "url_slug": | ||
| row, err := p.queries.GetSurveyByURLSlug(p.ctx, value.(string)) | ||
| if err != nil { | ||
| if err == sql.ErrNoRows { | ||
| return nil, nil | ||
| } | ||
| return nil, err | ||
| } | ||
| survey = &types.Survey{ | ||
| ID: int64(row.ID), | ||
| UUID: db.EncodeUUID(row.Uuid), | ||
| CreatedAt: row.CreatedAt.Time, | ||
| ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses), | ||
| DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses), | ||
| ErrorLog: row.ErrorLog.String, | ||
| Name: row.Name, | ||
| URLSlug: row.UrlSlug, | ||
| } | ||
| if err := json.Unmarshal(row.Config, &survey.Config); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal survey config: %w", err) | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("unsupported field: %s", field) | ||
| } | ||
|
|
||
| return survey, nil | ||
| return survey, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated survey construction logic
The survey construction code is duplicated between the uuid and url_slug cases. Consider extracting this into a helper function to improve maintainability.
func (p *Postgres) GetSurveyByField(field string, value interface{}) (*types.Survey, error) {
var survey *types.Survey
var err error
+
+ // Helper function to construct survey from row data
+ constructSurvey := func(id int32, uuid pgtype.UUID, createdAt pgtype.Timestamp,
+ parseStatus db.NullSurveyParseStatuses, deliveryStatus db.NullSurveyDeliveryStatuses,
+ errorLog pgtype.Text, name string, urlSlug string, config []byte) (*types.Survey, error) {
+ s := &types.Survey{
+ ID: int64(id),
+ UUID: db.EncodeUUID(uuid),
+ CreatedAt: createdAt.Time,
+ ParseStatus: types.SurveyParseStatus(parseStatus.SurveyParseStatuses),
+ DeliveryStatus: types.SurveyDeliveryStatus(deliveryStatus.SurveyDeliveryStatuses),
+ ErrorLog: errorLog.String,
+ Name: name,
+ URLSlug: urlSlug,
+ }
+ if err := json.Unmarshal(config, &s.Config); err != nil {
+ return nil, fmt.Errorf("failed to unmarshal survey config: %w", err)
+ }
+ return s, nil
+ }
switch field {
case "uuid":
uuid, err := db.DecodeUUID(value.(string))
if err != nil {
return nil, fmt.Errorf("failed to decode UUID: %w", err)
}
row, err := p.queries.GetSurveyByUUID(p.ctx, uuid)
if err != nil {
if err == sql.ErrNoRows {
return nil, nil
}
return nil, err
}
- survey = &types.Survey{
- ID: int64(row.ID),
- UUID: db.EncodeUUID(row.Uuid),
- CreatedAt: row.CreatedAt.Time,
- ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses),
- DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses),
- ErrorLog: row.ErrorLog.String,
- Name: row.Name,
- URLSlug: row.UrlSlug,
- }
- if err := json.Unmarshal(row.Config, &survey.Config); err != nil {
- return nil, fmt.Errorf("failed to unmarshal survey config: %w", err)
- }
+ survey, err = constructSurvey(row.ID, row.Uuid, row.CreatedAt, row.ParseStatus,
+ row.DeliveryStatus, row.ErrorLog, row.Name, row.UrlSlug, row.Config)
case "url_slug":
row, err := p.queries.GetSurveyByURLSlug(p.ctx, value.(string))
if err != nil {
if err == sql.ErrNoRows {
return nil, nil
}
return nil, err
}
- survey = &types.Survey{
- ID: int64(row.ID),
- UUID: db.EncodeUUID(row.Uuid),
- CreatedAt: row.CreatedAt.Time,
- ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses),
- DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses),
- ErrorLog: row.ErrorLog.String,
- Name: row.Name,
- URLSlug: row.UrlSlug,
- }
- if err := json.Unmarshal(row.Config, &survey.Config); err != nil {
- return nil, fmt.Errorf("failed to unmarshal survey config: %w", err)
- }
+ survey, err = constructSurvey(row.ID, row.Uuid, row.CreatedAt, row.ParseStatus,
+ row.DeliveryStatus, row.ErrorLog, row.Name, row.UrlSlug, row.Config)
default:
return nil, fmt.Errorf("unsupported field: %s", field)
}
return survey, 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.
| func (p *Postgres) GetSurveyByField(field string, value interface{}) (*types.Survey, error) { | |
| 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) | |
| row := p.conn.QueryRow(p.ctx, query, value) | |
| survey := &types.Survey{} | |
| err := row.Scan(&survey.ID, &survey.UUID, &survey.CreatedAt, | |
| &survey.ParseStatus, &survey.DeliveryStatus, &survey.ErrorLog, | |
| &survey.Name, &survey.Config, &survey.URLSlug) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| return nil, nil | |
| } | |
| var survey *types.Survey | |
| var err error | |
| return nil, err | |
| switch field { | |
| case "uuid": | |
| uuid, err := db.DecodeUUID(value.(string)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decode UUID: %w", err) | |
| } | |
| row, err := p.queries.GetSurveyByUUID(p.ctx, uuid) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| return nil, nil | |
| } | |
| return nil, err | |
| } | |
| survey = &types.Survey{ | |
| ID: int64(row.ID), | |
| UUID: db.EncodeUUID(row.Uuid), | |
| CreatedAt: row.CreatedAt.Time, | |
| ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses), | |
| DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses), | |
| ErrorLog: row.ErrorLog.String, | |
| Name: row.Name, | |
| URLSlug: row.UrlSlug, | |
| } | |
| if err := json.Unmarshal(row.Config, &survey.Config); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal survey config: %w", err) | |
| } | |
| case "url_slug": | |
| row, err := p.queries.GetSurveyByURLSlug(p.ctx, value.(string)) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| return nil, nil | |
| } | |
| return nil, err | |
| } | |
| survey = &types.Survey{ | |
| ID: int64(row.ID), | |
| UUID: db.EncodeUUID(row.Uuid), | |
| CreatedAt: row.CreatedAt.Time, | |
| ParseStatus: types.SurveyParseStatus(row.ParseStatus.SurveyParseStatuses), | |
| DeliveryStatus: types.SurveyDeliveryStatus(row.DeliveryStatus.SurveyDeliveryStatuses), | |
| ErrorLog: row.ErrorLog.String, | |
| Name: row.Name, | |
| URLSlug: row.UrlSlug, | |
| } | |
| if err := json.Unmarshal(row.Config, &survey.Config); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal survey config: %w", err) | |
| } | |
| default: | |
| return nil, fmt.Errorf("unsupported field: %s", field) | |
| } | |
| return survey, nil | |
| return survey, err | |
| } | |
| func (p *Postgres) GetSurveyByField(field string, value interface{}) (*types.Survey, error) { | |
| var survey *types.Survey | |
| var err error | |
| // Helper function to construct survey from row data | |
| constructSurvey := func(id int32, uuid pgtype.UUID, createdAt pgtype.Timestamp, | |
| parseStatus db.NullSurveyParseStatuses, deliveryStatus db.NullSurveyDeliveryStatuses, | |
| errorLog pgtype.Text, name string, urlSlug string, config []byte, | |
| ) (*types.Survey, error) { | |
| s := &types.Survey{ | |
| ID: int64(id), | |
| UUID: db.EncodeUUID(uuid), | |
| CreatedAt: createdAt.Time, | |
| ParseStatus: types.SurveyParseStatus(parseStatus.SurveyParseStatuses), | |
| DeliveryStatus: types.SurveyDeliveryStatus(deliveryStatus.SurveyDeliveryStatuses), | |
| ErrorLog: errorLog.String, | |
| Name: name, | |
| URLSlug: urlSlug, | |
| } | |
| if err := json.Unmarshal(config, &s.Config); err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal survey config: %w", err) | |
| } | |
| return s, nil | |
| } | |
| switch field { | |
| case "uuid": | |
| uuid, err := db.DecodeUUID(value.(string)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decode UUID: %w", err) | |
| } | |
| row, err := p.queries.GetSurveyByUUID(p.ctx, uuid) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| return nil, nil | |
| } | |
| return nil, err | |
| } | |
| survey, err = constructSurvey( | |
| row.ID, row.Uuid, row.CreatedAt, | |
| row.ParseStatus, row.DeliveryStatus, | |
| row.ErrorLog, row.Name, row.UrlSlug, row.Config, | |
| ) | |
| case "url_slug": | |
| row, err := p.queries.GetSurveyByURLSlug(p.ctx, value.(string)) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| return nil, nil | |
| } | |
| return nil, err | |
| } | |
| survey, err = constructSurvey( | |
| row.ID, row.Uuid, row.CreatedAt, | |
| row.ParseStatus, row.DeliveryStatus, | |
| row.ErrorLog, row.Name, row.UrlSlug, row.Config, | |
| ) | |
| default: | |
| return nil, fmt.Errorf("unsupported field: %s", field) | |
| } | |
| return survey, err | |
| } |
🤖 Prompt for AI Agents
In api/pkg/storage/postgres.go between lines 179 and 235, the code that
constructs the Survey struct from the database row is duplicated in both the
"uuid" and "url_slug" cases. To fix this, extract the survey construction logic
into a separate helper function that takes the database row as input and returns
a *types.Survey and error. Then call this helper function in both cases to
reduce duplication and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/pkg/db/queries/surveys.sql (3)
52-66: Add index on surveys.uuid for query performanceThis query would benefit from an index on the
surveys.uuidcolumn for optimal performance.
106-117: Add index on surveys.uuid for subquery performanceThe subquery
(SELECT s.id FROM surveys s WHERE s.uuid = $2)requires an index onsurveys.uuidfor optimal performance.
253-255: Validate webhook response data before storageEnsure proper validation of response_status values and response payload size limits in the application layer.
api/pkg/db/surveys.sql.go (1)
54-84: Performance concern inherited from SQL queryThis function executes a subquery to find the survey ID. As mentioned in the SQL review, this requires an index on
surveys.uuid.
🧹 Nitpick comments (8)
api/pkg/db/queries/surveys.sql (5)
20-50: Optimize GetSurveys query with conditional aggregationThe current implementation uses two subqueries that execute for each survey row, which can be inefficient with many surveys. Consider using a LEFT JOIN with conditional aggregation for better performance.
-- name: GetSurveys :many SELECT s.id, s.uuid, s.created_at, s.parse_status, s.delivery_status, s.error_log, s.name, s.config, s.url_slug, - ( - SELECT - COUNT(*) - FROM - surveys_sessions ss - WHERE - ss.survey_id = s.id - AND ss.status = $1) AS sessions_count_in_progress, - ( - SELECT - COUNT(*) - FROM - surveys_sessions ss - WHERE - ss.survey_id = s.id - AND ss.status = $2) AS sessions_count_completed + COALESCE(SUM(CASE WHEN ss.status = $1 THEN 1 ELSE 0 END), 0) AS sessions_count_in_progress, + COALESCE(SUM(CASE WHEN ss.status = $2 THEN 1 ELSE 0 END), 0) AS sessions_count_completed FROM surveys AS s + LEFT JOIN surveys_sessions ss ON ss.survey_id = s.id +GROUP BY + s.id, s.uuid, s.created_at, s.parse_status, s.delivery_status, s.error_log, s.name, s.config, s.url_slug ORDER BY s.created_at DESC;
68-82: Add index on surveys.url_slug for query performanceSimilar to the UUID query, this query would benefit from an index on the
surveys.url_slugcolumn.Create a migration to add the index:
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_surveys_url_slug ON surveys (url_slug);
154-166: Consider index on ip_addr for frequent lookupsIf IP-based session lookups are frequent, consider adding an index on
surveys_sessions.ip_addrto improve query performance.CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_surveys_sessions_ip_addr ON surveys_sessions (survey_id, ip_addr);
168-185: Add index on surveys_sessions.uuid for subquery performanceThe subquery in the WHERE clause would benefit from an index on
surveys_sessions.uuid.CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_surveys_sessions_uuid ON surveys_sessions (uuid);
187-205: Optimize UpsertSurveyQuestionAnswer with JOIN instead of subqueriesThe two subqueries in the VALUES clause execute separately and could be optimized using a single query with JOINs.
Consider refactoring to use a CTE or rewriting the application logic to pass the IDs directly if they're already known, which would eliminate the subqueries entirely.
api/pkg/db/surveys.sql.go (3)
92-95: Improve parameter naming for clarityThe parameter
Column2is auto-generated but unclear. Consider using sqlc's@renamecomment in the SQL file to give it a more descriptive name likeQuestionIDs.In the SQL file, update the query:
-- name: DeleteSurveyQuestionsNotInList :exec DELETE FROM surveys_questions WHERE survey_id = $1 AND question_id != ALL ($2::text[]); -- @rename question_ids
256-259: Clarify parameter names for better readabilityThe parameters
UuidandUuid_2are unclear. Use sqlc's parameter naming feature to make them more descriptive.In the SQL file, update the query to use named parameters:
-- name: GetSurveySession :one SELECT ss.id, ss.uuid, ss.created_at, ss.status, s.uuid AS survey_uuid FROM surveys_sessions AS ss INNER JOIN surveys AS s ON s.id = ss.survey_id WHERE ss.uuid = @session_uuid AND s.uuid = @survey_uuid;
694-698: Improve parameter naming for UpsertSurveyQuestionAnswerSimilar to other functions,
UuidandUuid_2should be renamed toSessionUuidandQuestionUuidfor clarity.Update the SQL query to use named parameters:
-- name: UpsertSurveyQuestionAnswer :exec INSERT INTO surveys_answers (session_id, question_id, answer) VALUES (( SELECT ss.id FROM surveys_sessions ss WHERE ss.uuid = @session_uuid), ( SELECT sq.id FROM surveys_questions sq WHERE sq.uuid = @question_uuid), @answer) ON CONFLICT (session_id, question_id) DO UPDATE SET answer = EXCLUDED.answer;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/Makefile(1 hunks)api/pkg/db/queries/surveys.sql(1 hunks)api/pkg/db/surveys.sql.go(1 hunks)api/pkg/storage/postgres.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/Makefile
- api/pkg/storage/postgres.go
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores