refactor: use Go 1.22's ServeMux path patterns instead gorilla/mux #684
Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors Pebble's HTTP routing from gorilla/mux to Go 1.22's native ServeMux path patterns to improve performance and reduce dependencies.
- Replaced gorilla/mux with http.ServeMux for HTTP routing
- Updated path parameter extraction from
muxVars(req)["param"]toreq.PathValue("param") - Simplified path patterns to match ServeMux syntax (e.g.,
{task-id}to{taskID})
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internals/daemon/daemon.go | Replace mux.Router with http.ServeMux and simplify route registration |
| internals/daemon/api.go | Remove mux dependency and update path patterns to use camelCase |
| internals/daemon/api_changes.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/api_notices.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/api_tasks.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/daemon_test.go | Refactor tests to use HTTP requests instead of mux internals |
| internals/daemon/export_test.go | Remove mux-related test utilities |
| internals/daemon/api_test.go | Remove mux test setup and helper functions |
| internals/daemon/api_changes_test.go | Update tests to use req.SetPathValue() |
| internals/daemon/api_notices_test.go | Update tests to use req.SetPathValue() |
| go.mod | Remove gorilla/mux dependency |
| docs/specs/openapi.yaml | Update API documentation to reflect new path patterns |
| docs/explanation/api-and-clients.md | Update documentation examples |
| HACKING.md | Remove mux import from example code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| d.router = http.NewServeMux() | ||
|
|
||
| for _, c := range API { | ||
| c.d = d | ||
| if c.PathPrefix == "" { | ||
| d.router.Handle(c.Path, c).Name(c.Path) | ||
| } else { | ||
| d.router.PathPrefix(c.PathPrefix).Handler(c).Name(c.PathPrefix) | ||
| } | ||
| d.router.Handle(c.Path, c) | ||
| } |
There was a problem hiding this comment.
The removal of NotFoundHandler registration means requests to invalid endpoints will now receive Go's default 404 response instead of the custom NotFound() response. This changes the API behavior and error format returned to clients.
| Path string | ||
| PathPrefix string | ||
| // | ||
| Path string |
There was a problem hiding this comment.
The PathPrefix field has been removed from the Command struct, but there's no clear indication of how prefix-based routing is now handled. If any commands previously used PathPrefix, this could be a breaking change.
| Path string | |
| Path string | |
| PathPrefix string |
DRAFT: not yet ready for review. And I still need to figure out how this applies to snapd and run it past them.
Oddly, it doesn't decrease the binary size. In fact, building with the flags we use to build the snap, it's very slightly bigger (exactly 4096 = 4KB). Not sure why, maybe something's bumped it into using another 4KB "page".
In terms of performance, routing is 30% faster, and does fewer memory allocations. Not that that is a big driver here, both are at the millisecond level. But here are the numbers of this benchmark:
Fixes #676.