SeniorDebuggingRareNot answered yet
Code review: fix this Postgres order-status-history store
Review this storage-layer method that should persist an order's status change. Find the production bugs and state how you would fix each.
package domain
type OrderStatusHistoryStore struct{}
func NewStore() OrderStatusHistoryStore {
return OrderStatusHistoryStore{}
}
func (s OrderStatusHistoryStore) saveOrderStatusHistory(orderID, status, partition string) {
db, err := sql.Open("postgres", "user=foo dbname=bar sslmode=disable")
if err != nil {
fmt.Errorf("%v", err)
}
query := fmt.Sprintf(
"insert into order_status_history (order_id, status, partition, inserted_at) "+
"values (%v, %v, %v, %v)",
orderID, status, partition, time.Now())
go db.Exec(query)
}Find and fix the bugs.
Five bugs. sql.Open runs per call, leaking a new pool each time — open one *sql.DB in NewStore and reuse it. The fmt.Errorf result is discarded and the method returns no error — return a wrapped error instead. The fmt.Sprintf query is SQL-injectable — use a parameterized $1..$4 query with the args passed to Exec. go db.Exec is fire-and-forget, dropping the error and ordering — call it synchronously. And there is no context — accept ctx and use ExecContext.
- ✗Believing
db.Execsanitizes afmt.Sprintf-built query, so injection is impossible - ✗Thinking a fire-and-forget
go db.Execis acceptable because the insert runs 'eventually' - ✗Calling
sql.Openper request, assuming it opens and owns a single real connection
- →Why does
sql.Opennot actually open a connection, and what doesdb.Pingadd? - →How does a
$1placeholder stop injection that escaping the string cannot?
Contents
Find the bugs
func (s OrderStatusHistoryStore) saveOrderStatusHistory(orderID, status, partition string) {
db, err := sql.Open("postgres", "user=foo dbname=bar sslmode=disable") // ❌ a new pool every call
if err != nil {
fmt.Errorf("%v", err) // ❌ error built and thrown away
}
query := fmt.Sprintf( // ❌ SQL injection
"insert into order_status_history (order_id, status, partition, inserted_at) values (%v, %v, %v, %v)",
orderID, status, partition, time.Now())
go db.Exec(query) // ❌ fire-and-forget: error and ordering lost; ❌ no context
}Review and fixes
sql.Openper call.sql.Opencreates a connection pool, not a single connection, and must not run per operation — the pool is never closed and leaks. Open one*sql.DBat startup and store it.- Swallowed error + no return.
fmt.Errorfonly builds an error value; here it is discarded (go vetcatches this). The method must return anerror. - SQL injection.
fmt.Sprintfof values into the query text is both injectable and invalid SQL for strings. Use a parameterized query with$1..$4; the driver binds the values safely. - Fire-and-forget goroutine.
go db.Exec(...)loses the error, gives no ordering, and can outlive the request. Call it synchronously (or schedule it deliberately, with error handling). - No
context. A long operation must be cancellable — acceptctxand callExecContext.
✅ Fixed version:
type OrderStatusHistoryStore struct {
db *sql.DB // one pool, reused
}
func NewStore(db *sql.DB) (*OrderStatusHistoryStore, error) {
if db == nil {
return nil, fmt.Errorf("store: nil db")
}
return &OrderStatusHistoryStore{db: db}, nil
}
func (s *OrderStatusHistoryStore) SaveOrderStatusHistory(
ctx context.Context, orderID, status, partition string,
) error {
const q = `insert into order_status_history
(order_id, status, partition, inserted_at)
values ($1, $2, $3, now())`
if _, err := s.db.ExecContext(ctx, q, orderID, status, partition); err != nil {
return fmt.Errorf("save order status history: %w", err)
}
return nil
}Contents
