Code Review: User Preferences API

Summary

Overall assessment: NEEDS_FIX

The implementation is well-structured and follows the hexagonal architecture pattern correctly. Domain validation, service layer, handlers, routes, OpenAPI spec, and database migration are all present and properly wired. Tests pass and cover the key scenarios. However, there is one blocker (compiled binary committed to git) and several warnings that should be addressed before merging.

Findings

Blockers

services/preferences-api/server — Compiled binary (12MB) committed to git — A Go compiled binary has been tracked and committed to the repository. This bloats the repo, is platform-specific, and should never be checked in. Remove it from git tracking with git rm services/preferences-api/server and add it to .gitignore.

Warnings

services/preferences-api/internal/domain/preferences.go:34-43 — Map iteration order is non-deterministic — ValidatePreferences iterates over the map and returns on the first error. Due to Go's random map iteration, the same invalid input can produce different error messages across runs. This is not a bug per se, but makes error messages inconsistent for users sending multiple invalid keys. Consider sorting keys before validation or collecting all errors.
services/preferences-api/internal/api/handlers/preferences.go:47-65 — Missing domain error mapping on Get path — The Get handler returns h.svc.Get(...) errors raw (line 60-61), while the Update handler correctly maps domain errors via mapPreferencesDomainError. While Get currently cannot produce domain validation errors, any future change to the service's Get method that introduces domain errors would leak as 500s. Consider applying mapPreferencesDomainError consistently.
services/preferences-api/internal/domain/preferences.go — No domain-level unit tests — The QA plan specifies 8 domain validation unit tests (DV-1 through DV-8), but there are no test files in the domain package. Domain validation is tested indirectly via the service layer tests, but direct domain tests provide better isolation and clearer failure diagnostics. AC-13 specifically requires "Domain layer validates preference keys and values independently of HTTP layer."
services/preferences-api/internal/api/handlers/preferences_test.go:80-82 — auth.SetUser used directly in tests bypasses middleware — The handler tests use withAuthUser to inject the user into context, but there is no test verifying that the auth middleware itself is applied to the route group. Auth enforcement is tested by convention (route registration review) rather than by execution. A test that hits the route without auth context would provide stronger assurance, though this may require middleware integration testing.
services/preferences-api/internal/api/routes.go:32 — Auth is conditionally applied based on cfg.AuthEnabled — If AUTH_ENABLED is not set to "true", all preference endpoints are completely unauthenticated. While this is useful for development, it means auth.MustGetUser in the handler will panic in production if the middleware isn't applied. Consider either: (a) documenting this clearly, (b) using auth.GetUser with a nil check instead of auth.MustGetUser, or (c) making auth always required in production builds.

Suggestions

services/preferences-api/internal/adapter/postgres/preferences.go — No adapter-level tests — The PostgreSQL adapter has no test coverage. While testing against a real database is harder, the adapter could be tested with integration tests or at minimum documented as requiring manual verification.
services/preferences-api/internal/api/handlers/preferences_test.go:21-22 — Test user UUIDs don't match — otherUserID is 550e8400-e29b-41d4-a716-446655440001 which only differs from testUserID (...440000) by the last digit. This works but could be more distinctive for readability (e.g., a completely different UUID).
services/preferences-api/internal/api/spec.go:24-30 — UpdatePreferencesRequest schema doesn't mark individual preference keys as optional — The OpenAPI schema has preferences as required, but individual preference keys within the object aren't marked as optional. Since partial updates are supported, the schema should indicate that individual preference fields are optional to accurately document the API behavior.

Spec Alignment

The implementation matches the spec well across all major requirements:

Spec Requirement	Status	Notes
GET returns preferences as key-value pairs	OK
GET returns empty preferences (not 404) for new users	OK	Service layer creates empty struct
PUT creates or updates (upsert semantics)	OK	PostgreSQL `ON CONFLICT`
PUT supports partial updates (merge)	OK	JSONB `
Supported preference keys validated	OK	Domain layer enforces closed set
Invalid values rejected with 400	OK	Domain validation with descriptive errors
Unknown keys rejected with 400	OK
Both endpoints require authentication (401)	OK	Via `auth.Middleware` (when enabled)
Users can only access own preferences (403)	OK	`checkOwnership` in both handlers
PostgreSQL persistence	OK
Standard `{data, meta}` envelope	OK	Via `httpresponse.OK`
OpenAPI spec documented	OK	Full spec with schemas, params, security
Domain layer validates independently	Partial	Validation exists but no direct unit tests
Hexagonal architecture followed	OK	Clean separation across all layers
Service layer unit tests	OK	10 test cases
Handler integration tests	OK	10 test cases

Test Coverage Assessment

Covered by tests:

Service layer: Get (empty user, existing user, repo error), Update (valid prefs, unknown key, invalid theme, invalid language, invalid notifications_enabled, repo error, partial merge)
Handler layer: Get (200 existing, 200 empty, 400 invalid UUID, 403 mismatch), Update (200 success, 400 unknown key, 400 invalid value, 400 missing prefs, 400 invalid UUID, 403 mismatch)

Missing test coverage:

Domain validation unit tests — DV-1 through DV-8 from QA plan are not implemented
Edge case EC-3 — Empty preferences object {} on PUT (what happens?)
Error cases ER-20, ER-21, ER-22 — Null values, nested objects, arrays as preference values
Auth middleware integration — No test verifying unauthenticated requests get 401
PostgreSQL adapter — No tests for the adapter layer

All tests pass:

go test ./... — 20 tests, all PASS
go vet ./... — clean

6.6 KiB Raw Blame History