encore-go-code-review

Encore Go Code Review Instructions

When reviewing Encore Go code, check for these common issues:

Critical Issues 1. Infrastructure Inside Functions // WRONG: Infrastructure declared inside function func setup() { db := sqldb.NewDatabase("mydb", sqldb.DatabaseConfig{...}) topic := pubsub.NewTopic*Event }

// CORRECT: Package level declaration var db = sqldb.NewDatabase("mydb", sqldb.DatabaseConfig{ Migrations: "./migrations", })

var topic = pubsub.NewTopic*Event

1. Missing Context Parameter // WRONG: Missing context //encore:api public method=GET path=/users/:id func GetUser(params GetUserParams) (User, error) { // ... }

// CORRECT: Context as first parameter //encore:api public method=GET path=/users/:id func GetUser(ctx context.Context, params GetUserParams) (User, error) { // ... }

1. SQL Injection Risk // WRONG: String interpolation query := fmt.Sprintf("SELECT * FROM users WHERE email = '%s'", email) rows, err := db.Query(ctx, query)

// CORRECT: Parameterized query rows, err := sqldb.QueryUser

1. Wrong Return Types // WRONG: Returning non-pointer struct //encore:api public method=GET path=/users/:id func GetUser(ctx context.Context, params *GetUserParams) (User, error) { // ... }

// CORRECT: Return pointer to struct //encore:api public method=GET path=/users/:id func GetUser(ctx context.Context, params GetUserParams) (User, error) { // ... }

1. Ignoring Errors // WRONG: Ignoring error user, _ := sqldb.QueryRowUser

// CORRECT: Handle error user, err := sqldb.QueryRowUser if err != nil { return nil, err }

Warning Issues 6. Not Checking for ErrNoRows // RISKY: Returns nil without proper error func getUser(ctx context.Context, id string) (*User, error) { user, err := sqldb.QueryRowUser if err != nil { return nil, err // ErrNoRows returns generic error } return user, nil }

// BETTER: Check for not found specifically import "errors"

func getUser(ctx context.Context, id string) (*User, error) { user, err := sqldb.QueryRowUser if errors.Is(err, sqldb.ErrNoRows) { return nil, &errs.Error{ Code: errs.NotFound, Message: "user not found", } } if err != nil { return nil, err } return user, nil }

1. Public Internal Endpoints // CHECK: Should this cron endpoint be public? //encore:api public method=POST path=/internal/cleanup func CleanupJob(ctx context.Context) error { // ... }

// BETTER: Use private for internal endpoints //encore:api private func CleanupJob(ctx context.Context) error { // ... }

1. Non-Idempotent Subscription Handlers // RISKY: Not idempotent (pubsub has at-least-once delivery) var _ = pubsub.NewSubscription(OrderCreated, "process-order", pubsub.SubscriptionConfig[OrderCreatedEvent]{ Handler: func(ctx context.Context, event OrderCreatedEvent) error { return chargeCustomer(ctx, event.OrderID) // Could charge twice! }, }, )

// SAFER: Check before processing var _ = pubsub.NewSubscription(OrderCreated, "process-order", pubsub.SubscriptionConfig[OrderCreatedEvent]{ Handler: func(ctx context.Context, event OrderCreatedEvent) error { order, err := getOrder(ctx, event.OrderID) if err != nil { return err } if order.Status != "pending" { return nil // Already processed } return chargeCustomer(ctx, event.OrderID) }, }, )

1. Not Closing Query Rows // WRONG: Rows not closed func listUsers(ctx context.Context) ([]*User, error) { rows, err := sqldb.QueryUser if err != nil { return nil, err } // Missing: defer rows.Close()

   var users []*User for rows.Next() { users = append(users, rows.Value()) } return users, nil }

// CORRECT: Always close rows func listUsers(ctx context.Context) ([]*User, error) { rows, err := sqldb.QueryUser if err != nil { return nil, err } defer rows.Close()

var users []*User
for rows.Next() {
    users = append(users, rows.Value())
}
return users, rows.Err()

}

Review Checklist All infrastructure at package level All API endpoints have context.Context as first parameter SQL uses parameterized queries ($1, $2, etc.) Response types are pointers Errors are handled, not ignored sqldb.ErrNoRows checked where appropriate Internal endpoints use private not public Subscription handlers are idempotent Query rows are closed with defer rows.Close() Migrations follow naming convention (1_name.up.sql) Output Format

When reviewing, report issues as:

[CRITICAL] [file:line] Description of issue [WARNING] [file:line] Description of concern
[GOOD] Notable good practice observed