From 2f2e96f90ff16a17801f6a98b43112c4e7bbc5dd Mon Sep 17 00:00:00 2001 From: Reese Norris Date: Sun, 12 Jul 2026 20:37:20 -0400 Subject: [PATCH] fix: address review feedback for server DI handleKickUser return; MetarQueue.Run; registry sentinel re-exports; compile-time store asserts. --- internal/server/conn.go | 3 +- internal/server/deps.go | 13 ++++++ internal/server/deps_test.go | 74 ++++++++++++++++++++++++++------- internal/server/http_service.go | 4 +- internal/server/server.go | 18 ++++---- 5 files changed, 85 insertions(+), 27 deletions(-) diff --git a/internal/server/conn.go b/internal/server/conn.go index cf10645..d08fcdd 100644 --- a/internal/server/conn.go +++ b/internal/server/conn.go @@ -13,7 +13,6 @@ import ( "github.com/renorris/openfsd/db" "github.com/renorris/openfsd/internal/auth" - "github.com/renorris/openfsd/internal/postoffice" "github.com/renorris/openfsd/internal/session" "github.com/renorris/openfsd/pkg/protocol" ) @@ -69,7 +68,7 @@ func (s *Server) handleConn(ctx context.Context, conn net.Conn) { // Attempt to register to registry if err = s.registry.Register(client); err != nil { - if errors.Is(err, postoffice.ErrCallsignInUse) { + if errors.Is(err, ErrCallsignInUse) { sendError(conn, CallsignInUseError, "Callsign already in use") } return diff --git a/internal/server/deps.go b/internal/server/deps.go index 975f80a..ec4eb43 100644 --- a/internal/server/deps.go +++ b/internal/server/deps.go @@ -7,9 +7,18 @@ import ( "time" "github.com/renorris/openfsd/db" + "github.com/renorris/openfsd/internal/postoffice" "github.com/renorris/openfsd/internal/session" ) +// Registry sentinel errors — re-exported from postoffice so handlers/conn/HTTP +// can use errors.Is without importing the concrete registry package. +// Registry implementations should return these (or errors that wrap them). +var ( + ErrCallsignInUse = postoffice.ErrCallsignInUse + ErrCallsignDoesNotExist = postoffice.ErrCallsignDoesNotExist +) + // UserStore is the consumer-side user repository surface used by login. // Signatures match db.UserRepository (no context yet). type UserStore interface { @@ -24,6 +33,7 @@ type ConfigStore interface { } // Registry abstracts the callsign/geo registry (postoffice.PostOffice). +// Register/Find/Send should use ErrCallsignInUse / ErrCallsignDoesNotExist. type Registry interface { Register(s *session.Session) error Release(s *session.Session) @@ -36,8 +46,11 @@ type Registry interface { } // MetarQueue is the METAR fetch queue (metar.Service). +// Run starts workers; Request enqueues a fetch. Both are required so injectors +// cannot silently omit worker startup. type MetarQueue interface { Request(ctx context.Context, s session.Sender, icao string) + Run(ctx context.Context) } // Clock provides the current time (nil Deps.Clock => real wall clock). diff --git a/internal/server/deps_test.go b/internal/server/deps_test.go index c2d1aab..17635a8 100644 --- a/internal/server/deps_test.go +++ b/internal/server/deps_test.go @@ -3,10 +3,12 @@ package server import ( "context" "errors" + "strings" "testing" "time" "github.com/renorris/openfsd/db" + "github.com/renorris/openfsd/internal/postoffice" "github.com/renorris/openfsd/internal/session" ) @@ -37,26 +39,30 @@ func (stubRegistry) Snapshot() []*session.Session { return nil type stubMetar struct{} func (stubMetar) Request(ctx context.Context, s session.Sender, icao string) {} +func (stubMetar) Run(ctx context.Context) {} type fixedClock struct{ t time.Time } func (c fixedClock) Now() time.Time { return c.t } +func fullDeps() Deps { + return Deps{ + Config: &Config{FsdListenAddrs: []string{":0"}}, + Users: stubUserStore{}, + ConfigKV: stubConfigStore{}, + Registry: stubRegistry{}, + Metar: stubMetar{}, + Clock: fixedClock{t: time.Unix(1_700_000_000, 0)}, + } +} + func TestNewRequiresDeps(t *testing.T) { _, err := New(Deps{}) if err == nil { t.Fatal("expected error for empty Deps") } - cfg := &Config{FsdListenAddrs: []string{":0"}} - srv, err := New(Deps{ - Config: cfg, - Users: stubUserStore{}, - ConfigKV: stubConfigStore{}, - Registry: stubRegistry{}, - Metar: stubMetar{}, - Clock: fixedClock{t: time.Unix(1_700_000_000, 0)}, - }) + srv, err := New(fullDeps()) if err != nil { t.Fatalf("New: %v", err) } @@ -74,14 +80,37 @@ func TestNewRequiresDeps(t *testing.T) { } } +func TestNewMissingRequiredFields(t *testing.T) { + cases := []struct { + name string + mutate func(*Deps) + wantSub string + }{ + {"nil Config", func(d *Deps) { d.Config = nil }, "Config"}, + {"nil Users", func(d *Deps) { d.Users = nil }, "Users"}, + {"nil ConfigKV", func(d *Deps) { d.ConfigKV = nil }, "ConfigKV"}, + {"nil Registry", func(d *Deps) { d.Registry = nil }, "Registry"}, + {"nil Metar", func(d *Deps) { d.Metar = nil }, "Metar"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d := fullDeps() + tc.mutate(&d) + _, err := New(d) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), tc.wantSub) { + t.Fatalf("error %q should mention %q", err.Error(), tc.wantSub) + } + }) + } +} + func TestNewNilClockUsesReal(t *testing.T) { - srv, err := New(Deps{ - Config: &Config{}, - Users: stubUserStore{}, - ConfigKV: stubConfigStore{}, - Registry: stubRegistry{}, - Metar: stubMetar{}, - }) + d := fullDeps() + d.Clock = nil + srv, err := New(d) if err != nil { t.Fatal(err) } @@ -94,3 +123,16 @@ func TestNewNilClockUsesReal(t *testing.T) { t.Fatalf("real clock skew too large: %v", delta) } } + +func TestRegistrySentinelsMatchPostoffice(t *testing.T) { + // server re-exports must be identical for errors.Is across package boundaries. + if !errors.Is(ErrCallsignInUse, postoffice.ErrCallsignInUse) { + t.Fatal("ErrCallsignInUse mismatch") + } + if !errors.Is(ErrCallsignDoesNotExist, postoffice.ErrCallsignDoesNotExist) { + t.Fatal("ErrCallsignDoesNotExist mismatch") + } + if !errors.Is(postoffice.ErrCallsignInUse, ErrCallsignInUse) { + t.Fatal("reverse ErrCallsignInUse mismatch") + } +} diff --git a/internal/server/http_service.go b/internal/server/http_service.go index f43fd7f..74dcfaf 100644 --- a/internal/server/http_service.go +++ b/internal/server/http_service.go @@ -11,7 +11,6 @@ import ( "github.com/gin-gonic/gin" "github.com/renorris/openfsd/db" "github.com/renorris/openfsd/internal/auth" - "github.com/renorris/openfsd/internal/postoffice" ) // runServiceHTTP starts the admin service HTTP server used for @@ -155,11 +154,12 @@ func (s *Server) handleKickUser(c *gin.Context) { var reqBody RequestBody if err := c.ShouldBindJSON(&reqBody); err != nil { c.AbortWithStatus(http.StatusBadRequest) + return } client, err := s.registry.Find(reqBody.Callsign) if err != nil { - if !errors.Is(err, postoffice.ErrCallsignDoesNotExist) { + if !errors.Is(err, ErrCallsignDoesNotExist) { c.AbortWithStatus(http.StatusInternalServerError) return } diff --git a/internal/server/server.go b/internal/server/server.go index e87f194..86cc8dd 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -174,10 +174,8 @@ func generateDefaultAdminUser(dbRepo *db.Repositories) (user *db.User, err error // Run starts METAR workers, the admin HTTP service, and FSD listeners. // It blocks until ctx is cancelled (or a listener fails to start). func (s *Server) Run(ctx context.Context) (err error) { - // Start metar workers when the concrete service supports Run. - if r, ok := s.metar.(interface{ Run(context.Context) }); ok { - go r.Run(ctx) - } + // Start metar worker pool (MetarQueue.Run is required on the interface). + go s.metar.Run(ctx) // Start HTTP service go s.runServiceHTTP(ctx) @@ -245,8 +243,14 @@ func (s *Server) listenLoop(ctx context.Context, addr string, errCh chan<- error } } -// Compile-time interface satisfaction checks. +// Compile-time interface satisfaction checks against production implementors. var ( - _ Registry = (*postoffice.PostOffice)(nil) - _ MetarQueue = (*metar.Service)(nil) + _ Registry = (*postoffice.PostOffice)(nil) + _ MetarQueue = (*metar.Service)(nil) + _ UserStore = (*db.SQLiteUserRepository)(nil) + _ UserStore = (*db.PostgresUserRepository)(nil) + _ UserStore = db.UserRepository(nil) + _ ConfigStore = (*db.SQLiteConfigRepository)(nil) + _ ConfigStore = (*db.PostgresConfigRepository)(nil) + _ ConfigStore = db.ConfigRepository(nil) )