Code Review: User Preferences API

Summary

Overall Assessment: PASS

The implementation is well-structured, follows the hexagonal architecture pattern correctly, and satisfies all acceptance criteria from the spec. All 18 unit tests pass. The code follows project conventions for error handling, request binding, response envelopes, and URL parameter syntax. No blockers found.

Findings

Blockers

None.

Warnings

internal/api/handlers/preferences.go:63 — UpdatedAt uses hardcoded Z format ("2006-01-02T15:04:05Z") instead of time.RFC3339. If a non-UTC time is ever passed (e.g., from a database driver returning local time), the Z suffix would be incorrect. Currently safe because time.Now().UTC() is used consistently and PostgreSQL TIMESTAMPTZ stores UTC, but time.RFC3339 would be more robust.
internal/service/preferences.go / internal/api/handlers/preferences_test.go — Mock repository is duplicated between service tests and handler tests. Consider extracting to a shared internal/testutil/ package to avoid maintaining two copies. Low priority since the mock is small and stable.

Suggestions

internal/domain/preferences.go — Domain validation could return all validation errors at once instead of short-circuiting on the first error. This would give clients a better experience when multiple fields are invalid. Low priority for MVP.
internal/adapter/postgres/preferences.go — No integration tests for the PostgreSQL adapter. This is acceptable for now since the SQL is straightforward (PK lookup + upsert), but integration tests would increase confidence for future schema changes.

Spec Alignment

The implementation matches the spec completely:

Acceptance Criterion	Status	Evidence
GET returns stored preferences	PASS	Handler test `returns_stored_preferences`
GET for unknown user returns defaults (not 404)	PASS	Service test `returns_defaults_for_unknown_user`, handler test `returns_defaults_for_unknown_user`
PUT creates preferences (upsert)	PASS	Service test `creates_new_preferences_from_defaults`, handler test `creates_preferences_with_full_update`
PUT merges partial data (deep merge)	PASS	Service test `merges_partial_data_with_existing_preferences`, handler test `creates_preferences_with_partial_update`
PUT validates theme	PASS	Service test `rejects_invalid_theme`, handler test `returns_400_for_invalid_theme`
PUT validates language non-empty	PASS	Service test `rejects_empty_language`
PUT validates notifications.digest	PASS	Service test `rejects_invalid_digest`
notifications.email/push are booleans	PASS	Enforced by Go type system (JSON binding)
Invalid values return 400	PASS	`mapPreferencesDomainError` maps domain errors to `httperror.BadRequest`
Invalid user_id returns 400	PASS	UUID parse check in both handlers, tested in handler tests
Standard {data, meta} envelope	PASS	Uses `httpresponse.OK()`
OpenAPI spec documents endpoints	PASS	`spec.go` defines GET/PUT with schemas, parameters, and responses
Preferences persisted in PostgreSQL	PASS	`adapter/postgres/preferences.go` with JSONB column
Database migration creates table	PASS	`migrations/001_create_user_preferences.sql`
Hexagonal architecture	PASS	domain -> service -> port -> adapter separation is clean
Service and handler unit tests	PASS	10 service tests + 8 handler tests, all passing
Example scaffolding removed	PASS	No example files found

Test Coverage Assessment

Service layer (10 tests):

GetPreferences: defaults for unknown user, stored for existing user
UpdatePreferences: create from defaults, partial merge, theme-only, language-only, all fields, invalid theme, invalid digest, empty language

Handler layer (8 tests):

Get: stored preferences, defaults for unknown, invalid UUID
Upsert: full update, partial update, invalid UUID, invalid theme, empty body

Coverage gaps (non-critical):

No test for invalid digest at handler level (covered at service level)
No test for invalid language at handler level (covered at service level)
No integration tests for PostgreSQL adapter
No test for concurrent access patterns (mock uses sync.RWMutex properly)

All acceptance criteria have at least one corresponding test. The test suite is thorough for an MVP.

Code Quality Notes

Strengths:

Clean hexagonal architecture with proper dependency injection
Pointer-based partial update pattern correctly distinguishes "not provided" from zero values
Deep merge logic is simple and correct
Domain validation is self-contained with clear error types
Error mapping from domain to HTTP is explicit and maintainable
Uses app.Wrap(), app.BindAndValidate(), httpresponse.OK() per project conventions
Uses {user_id} brace syntax for chi URL parameters (not colon syntax)
Compile-time interface verification (var _ port.PreferencesRepository = ...)
SQL uses parameterized queries ($1, $2) — no injection risk
Atomic upsert with ON CONFLICT DO UPDATE

Minor observations:

toPreferencesResponse hardcodes time format rather than using time.RFC3339 (warning above)
Mock repository duplicated across test files (warning above)

5.2 KiB Raw Permalink Blame History