Skip to content

Wire main with config, db, auth, jellyfin adapter#16

Merged
koinsaari merged 1 commit into
mainfrom
feat/wire-main
May 30, 2026
Merged

Wire main with config, db, auth, jellyfin adapter#16
koinsaari merged 1 commit into
mainfrom
feat/wire-main

Conversation

@koinsaari
Copy link
Copy Markdown
Contributor

@koinsaari koinsaari commented May 30, 2026

Summary

  • Flatten db.Open to return *sql.DB directly — the DB wrapper struct had no methods and no second consumer
  • Add jellyfin.AsAuthAdapter to bridge *jellyfin.Client to auth.JellyfinAuthenticator, translating all three error sentinels (ErrInvalidCredentials, ErrUpstreamUnavailable, ErrQuickConnectPending) at the package boundary
  • Replace placeholder main.go with full wiring: config load, db open, Jellyfin client + adapter, auth service, HTTP server, graceful shutdown on SIGINT/SIGTERM
  • Delete cmd/api-proxy/main_test.go — toy healthz test superseded by internal/http tests

Test plan

  • CI passes (test, lint, codegen-check, docker-build, gitleaks)
  • grep -rn "db\.DB\b" --include="*.go" . — no matches
  • go build ./... — clean, no import cycles
  • go test -race -count=1 ./... — all pass locally
  • golangci-lint run ./... — 0 issues
  • docker build -t api-proxy:ci . — succeeds

🤖 Generated with Claude Code

Closes #12

@claude
Copy link
Copy Markdown

claude Bot commented May 30, 2026

Claude finished @koinsaari's task in 2m 35s —— View job


PR Review: Wire main with config, db, auth, jellyfin adapter

  • Read REVIEW.md and repository context
  • Analyze all changed files
  • Check import cycles (auth ↔ jellyfin)
  • Check JWT keyfunc signing-method validation
  • Check SQL parameterization
  • Post review

No blocking issues.

Summary

The PR is clean and well-structured. The db.DB wrapper removal is a straightforward simplification with correct callsite updates. The jellyfin.AsAuthAdapter pattern is the right way to bridge packages without creating an import cycle, and the compile-time interface assertion on line 10 of adapter.go confirms the contract. Graceful shutdown, signal handling, and the HTTP server wiring in main.go all look correct.

Import cycle check (internal/auth must never import internal/clients/jellyfin): ✅ Clean. The adapter lives in the jellyfin package and imports auth — the correct direction.

JWT keyfunc (jwt.go:41-46): ✅ Signs method is validated as *jwt.SigningMethodHMAC before the key is returned.

SQL parameterization: ✅ All queries use ? placeholders throughout internal/auth/.


🟡 Nit 1 — Shutdown error silently dropped

main.go:64:

_ = httpSrv.Shutdown(shutdownCtx)

If graceful shutdown times out (connections don't drain within 10 s), the context deadline error is swallowed. Consider logging it so an ops team can tell the shutdown timed out:

if err := httpSrv.Shutdown(shutdownCtx); err != nil {
    logger.Error("shutdown", "err", err)
}

Fix this →


🟡 Nit 2 — QuickConnectInitiate upstream-error path not covered in adapter test

adapter_test.go tests the happy-path for QuickConnectInitiate and all three error translations for AuthenticateByName / QuickConnectAuthenticate, but there is no test that confirms QuickConnectInitiate translates ErrUpstreamUnavailableauth.ErrJellyfinUnavailable. The translateErr helper is shared so the logic is indirectly verified, but a one-case test would close the gap:

func TestAdapter_QuickConnectInitiate_TranslatesUpstreamUnavailable(t *testing.T) {
    a := newAdapterServer(t, func(w http.ResponseWriter, r *http.Request) {
        http.Error(w, "", http.StatusInternalServerError)
    })
    _, err := a.QuickConnectInitiate(context.Background())
    if !errors.Is(err, auth.ErrJellyfinUnavailable) {
        t.Fatalf("want auth.ErrJellyfinUnavailable, got %v", err)
    }
}

Fix this →

@koinsaari koinsaari added the enhancement New feature or request label May 30, 2026
@koinsaari koinsaari self-assigned this May 30, 2026
Flatten db.Open to return *sql.DB directly (the DB wrapper struct had no
methods and no second consumer). Add jellyfin.AsAuthAdapter to translate
between the clients and auth packages without an import cycle. Replace
the placeholder main.go with full wiring: config load, db open, Jellyfin
client, auth service, HTTP server, graceful shutdown. Delete the toy
healthz test superseded by internal/http tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 30, 2026

Claude finished @koinsaari's task in 1m 23s —— View job


PR Review: Wire main with config, db, auth, jellyfin adapter

  • Read REVIEW.md and repository context
  • Analyze all changed files
  • Check import cycles (auth ↔ jellyfin)
  • Check JWT keyfunc signing-method validation
  • Check SQL parameterization
  • Post review

No blocking issues. Both nits from the previous review have been addressed.

Summary

The PR is clean and the follow-up changes are correct. The previous nits are fixed: the shutdown error is now logged (main.go:64), and TestAdapter_QuickConnectInitiate_TranslatesUpstreamUnavailable is present in adapter_test.go:74.

Import cycle check (internal/auth must never import internal/clients/jellyfin): ✅ Clean. The adapter lives in the jellyfin package and imports auth — the correct direction.

JWT keyfunc (jwt.go:41-46): ✅ Signing method is validated as *jwt.SigningMethodHMAC before the key is returned.

SQL parameterization: ✅ All queries use ? placeholders throughout internal/auth/.

db.DB wrapper removal: ✅ Callsites in lockout_test.go and login_test.go correctly updated to use *sql.DB directly (.DB field access dropped).

Test coverage for adapter: ✅ All three error translation paths and the happy path are covered for all three methods, including the previously missing QuickConnectInitiate upstream-unavailable case.

@koinsaari koinsaari merged commit eeb8ab5 into main May 30, 2026
7 checks passed
@koinsaari koinsaari deleted the feat/wire-main branch May 30, 2026 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire main with config, db, auth, jellyfin adapter

1 participant