Skip to content

server: fail fast on duplicate route registration (same method+path) #759

Description

@gaborage

Problem

go-bricks fails fast on duplicate module names (app/module_registry.go) and duplicate messaging queue+consumer_tag+event_type triples (panic at startup), but has no duplicate HTTP route detection. If two modules register the same method + path, echo silently applies last-one-wins (echo.New() hard-codes AllowOverwritingRoute: true), and the losing module's handler is dead on arrival — no error, no warning, no test failure unless the route is exercised.

For a modular framework where independently-authored modules contribute routes to one server, a silent collision is exactly the class of bug the Fail Fast principle exists to catch: "Validation errors crash at startup, never degrade silently."

// module A
server.GET(hr, r, "/users/:id", a.getUser)
// module B — typo'd prefix, silently shadows module A
server.GET(hr, r, "/users/:id", b.getAccount)  // last-one-wins, no error

Proposal

Detect method + full path collisions at registration/startup time in go-bricks' own registration path, and fail startup with an error naming both registrants.

Seam. Since #634, every route — typed (server.GET/POST via the addEcho seam) and raw (RouteRegistrar.Add) — emits a RouteDescriptor into the route registry with the canonical collision key already computed (formatHandlerID"METHOD:/full/path", server/descriptor.go). The descriptor also carries ModuleName, Package, and HandlerName, so the error message can say precisely who collided:

duplicate route registration: GET /users/:id
  first:  module "users"  handler getUser    (github.com/acme/svc/modules/users)
  second: module "orders" handler getAccount (github.com/acme/svc/modules/orders)

Scoping caveat. DefaultRouteRegistry is a package-level global (server/descriptor.go:49); naive dedup there would false-positive across server instances (parallel tests, multi-app processes). The check should be scoped per server instance — e.g. collision state owned by the Server/root routeGroup and threaded through newRouteGroup, or a post-registration validation sweep over only the routes the starting server registered.

Failure mode. Prefer a validation sweep after all modules' RegisterRoutes complete, collecting all collisions and failing startup with one aggregate error (better DX than first-collision panic). Messaging's per-declaration panic is the existing precedent (#366 family) if the sweep proves awkward.

Semantics (initial scope). Exact method + full path string equality — parity with echo's own duplicate check. Same path with different methods is fine. Echo's implicit group 404 catch-alls never pass through the go-bricks registration seam, so they are naturally invisible to the check (no special-casing).

Alternative considered (rejected): echo AllowOverwritingRoute: false

echo v5.3.1 (#753) made this newly viable: Group.Use()'s implicit RouteNotFound routes are now registered with a per-route overwrite exemption (labstack/echo#3049), so strict mode no longer panics on group middleware. Rejected because:

  • echo.New() hard-codes true; we'd have to construct the router ourselves and track echo's defaults forever.
  • The failure surfaces as an echo-flavored panic mid-registration, leaking engine details through the ADR-034 boundary (no echo.* on the consumer surface).
  • Framework-side detection gives provenance-rich errors (module/package/handler) echo cannot produce, and works identically if the engine ever changes.

Open questions

  • Param-name-differing collisions (/users/:id vs /users/:uid) — same radix-tree slot, different strings. Exact-match scope misses them; echo's matcher may or may not reject them. Stretch goal: normalize param segments before comparison.
  • Should framework-registered system routes (/health, /ready, /_sys/*) participate, or only module routes? (They flow through the same seam, so participation is the default; verify no intentional re-registration exists.)
  • Is any intentional route overwrite in the wild (Strangler Fig raw routes replacing typed ones)? If a legitimate case exists, an explicit opt-out (WithAllowOverwrite() route option) beats silent tolerance.

Acceptance criteria

  • Two modules registering the same method + full path → startup fails with an error naming both registrants (module, handler, package).
  • Aggregate reporting: N collisions produce one error listing all N, not N startup attempts.
  • Same path, different methods → no error. Typed + raw registration paths both participate.
  • Per-server scoping: two Server instances in one process (parallel tests) with overlapping routes do not cross-contaminate.
  • Echo's implicit group 404 catch-alls and repeated Use() calls on groups remain unaffected.
  • Docs: CLAUDE.md server stub + wiki/startup_defaults.md note the new startup validation.

Context

Fallout analysis of PR #753 (echo v5.3.0 → v5.3.1): upstream labstack/echo#3049 / labstack/echo#3047. Related: #634 (registry completeness), #678 (route logging), #366 (fail-fast precedent), ADR-034 (echo boundary).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions