Clean up error handling pattern

Author: sjmiller609

PATTERNS.md

@@ -0,0 +1,293 @@
+# Code Patterns & Best Practices
+
+This document describes the coding patterns used in this project, matching the patterns from kernel/packages/api.
+
+## Project Structure
+
+```
+cmd/api/                    # Main application
+├── api/                   # API handlers (one file per resource)
+│   ├── api.go            # ApiService struct + New() constructor
+│   ├── health.go         # Health check
+│   ├── images.go         # Image resource handlers
+│   ├── instances.go      # Instance resource handlers
+│   └── volumes.go        # Volume resource handlers
+├── config/               # Configuration
+│   └── config.go
+├── main.go               # Entry point, server setup
+├── wire.go               # Dependency injection definition
+└── wire_gen.go           # Generated by wire
+
+lib/                       # Service layer
+├── images/
+│   ├── errors.go         # Sentinel errors
+│   └── manager.go        # Business logic
+├── instances/
+│   ├── errors.go
+│   └── manager.go
+├── volumes/
+│   ├── errors.go
+│   └── manager.go
+├── logger/               # Context-based logging
+│   └── logger.go
+├── providers/            # Wire providers
+│   └── providers.go
+└── oapi/                 # Generated from OpenAPI spec
+    └── oapi.go
+```
+
+## Error Handling Pattern
+
+### Service Layer (lib/)
+
+**Rules:**
+- Accept `context.Context`, never `*http.Request` or HTTP types
+- Return sentinel errors (e.g., `ErrNotFound`, `ErrAlreadyExists`)
+- Use `fmt.Errorf("context: %w", err)` to wrap errors
+
+**Example (lib/images/errors.go):**
+```go
+package images
+
+import "errors"
+
+var (
+    ErrNotFound      = errors.New("image not found")
+    ErrAlreadyExists = errors.New("image already exists")
+)
+```
+
+**Example (lib/images/manager.go):**
+```go
+func (m *manager) GetImage(ctx context.Context, id string) (*oapi.Image, error) {
+    exists := false  // TODO: actual check
+    if !exists {
+        return nil, ErrNotFound  // Return sentinel error
+    }
+    
+    // If something else fails:
+    err := doSomething()
+    if err != nil {
+        return nil, fmt.Errorf("failed to load image metadata: %w", err)
+    }
+    
+    return image, nil
+}
+```
+
+### Handler Layer (cmd/api/api/)
+
+**Rules:**
+- Extract logger from context: `log := logger.FromContext(ctx)`
+- Use inline `switch` with `errors.Is()` to map errors → HTTP responses
+- Default case → 500 + log the error
+- Never expose internal error messages to clients
+
+**Example (cmd/api/api/images.go):**
+```go
+func (s *ApiService) GetImage(ctx context.Context, request oapi.GetImageRequestObject) (oapi.GetImageResponseObject, error) {
+    log := logger.FromContext(ctx)
+    
+    img, err := s.ImageManager.GetImage(ctx, request.Id)
+    if err != nil {
+        switch {
+        case errors.Is(err, images.ErrNotFound):
+            return oapi.GetImage404JSONResponse{
+                Code:    "not_found",
+                Message: "image not found",
+            }, nil
+        default:
+            log.Error("failed to get image", "error", err, "id", request.Id)
+            return oapi.GetImage500JSONResponse{
+                Code:    "internal_error",
+                Message: "failed to get image",
+            }, nil
+        }
+    }
+    return oapi.GetImage200JSONResponse(*img), nil
+}
+```
+
+## Logging Pattern
+
+### Setup (lib/logger/logger.go)
+
+```go
+package logger
+
+import (
+    "context"
+    "log/slog"
+)
+
+type contextKey string
+
+const loggerKey contextKey = "logger"
+
+func AddToContext(ctx context.Context, logger *slog.Logger) context.Context {
+    return context.WithValue(ctx, loggerKey, logger)
+}
+
+func FromContext(ctx context.Context) *slog.Logger {
+    if logger, ok := ctx.Value(loggerKey).(*slog.Logger); ok {
+        return logger
+    }
+    return slog.Default()
+}
+```
+
+### Usage in Providers
+
+```go
+func ProvideLogger() *slog.Logger {
+    return slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{
+        Level: slog.LevelInfo,
+    }))
+}
+
+func ProvideContext(log *slog.Logger) context.Context {
+    return logger.AddToContext(context.Background(), log)
+}
+```
+
+### Usage in Handlers
+
+```go
+func (s *ApiService) SomeHandler(ctx context.Context, ...) (..., error) {
+    log := logger.FromContext(ctx)
+    
+    log.Info("doing something", "key", "value")
+    log.Error("something failed", "error", err, "id", someId)
+}
+```
+
+## Dependency Injection (Wire)
+
+### Define Providers (lib/providers/providers.go)
+
+```go
+func ProvideLogger() *slog.Logger { ... }
+func ProvideContext(log *slog.Logger) context.Context { ... }
+func ProvideConfig() *config.Config { ... }
+func ProvideImageManager(cfg *config.Config) images.Manager { ... }
+```
+
+### Wire Definition (cmd/api/wire.go)
+
+```go
+//go:build wireinject
+
+package main
+
+import (
+    "github.com/google/wire"
+    ...
+)
+
+type application struct {
+    Ctx             context.Context
+    Logger          *slog.Logger
+    Config          *config.Config
+    ImageManager    images.Manager
+    ...
+    ApiService      *api.ApiService
+}
+
+func initializeApp() (*application, func(), error) {
+    panic(wire.Build(
+        providers.ProvideLogger,      // Order matters! Logger first
+        providers.ProvideContext,     // Then context (depends on logger)
+        providers.ProvideConfig,
+        providers.ProvideImageManager,
+        ...
+        api.New,
+        wire.Struct(new(application), "*"),
+    ))
+}
+```
+
+### Generate Wire Code
+
+```bash
+make generate-wire
+```
+
+## Code Generation
+
+### OpenAPI
+
+After modifying `openapi.yaml`:
+```bash
+make oapi-generate
+```
+
+### Wire
+
+After modifying `wire.go` or providers:
+```bash
+make generate-wire
+```
+
+### Both
+
+```bash
+make generate-all
+```
+
+## Testing Pattern
+
+(TODO: To be established when implementing actual functionality)
+
+```go
+package images_test
+
+import (
+    "context"
+    "testing"
+    
+    "github.com/onkernel/cloud-hypervisor-dataplane/lib/images"
+    "github.com/stretchr/testify/require"
+)
+
+func TestGetImage_NotFound(t *testing.T) {
+    mgr := images.NewManager("/tmp/test")
+    _, err := mgr.GetImage(context.Background(), "fake-id")
+    require.ErrorIs(t, err, images.ErrNotFound)
+}
+```
+
+## Comments
+
+From workspace rules:
+
+- Keep comments simple and free of extra formatting
+- Avoid obvious comments
+- Explain the next big chunk of code at a high level
+- Always add comments for exports in Go
+- Never make comments that refer to facts only present in the prompt
+
+**Good:**
+```go
+// Manager handles image lifecycle operations
+type Manager interface { ... }
+```
+
+**Bad:**
+```go
+// ---
+// 1. Get image from disk
+// 2. Parse metadata
+// ---
+```
+
+## Summary
+
+Key principles:
+1. **Separation of concerns**: Handlers in `cmd/api/api/`, business logic in `lib/`
+2. **Clean error handling**: Sentinel errors + switch statements + default to 500
+3. **Context-based logging**: Extract logger from context in handlers
+4. **Dependency injection**: Wire for clean initialization
+5. **Code generation**: OpenAPI spec → Go code, Wire → dependency injection
+6. **No HTTP in services**: Service layer is HTTP-agnostic
+
+These patterns match kernel/packages/api and provide a clean, maintainable codebase.