From bf4993de8bc81fa14fe2f0643019c909e380df36 Mon Sep 17 00:00:00 2001 From: Richard Carson Derr Date: Sun, 29 Sep 2024 18:50:17 -0400 Subject: [PATCH 1/4] refactor(issue-293): only register endpoints in Run to avoid panic --- rest/rest.go | 111 ++++++++++++++++++++++------------ rest/rest_example_test.go | 4 +- rest/rest_test.go | 124 ++++++++++++++++++++++---------------- 3 files changed, 146 insertions(+), 93 deletions(-) diff --git a/rest/rest.go b/rest/rest.go index c01e77f..fc8ea9d 100644 --- a/rest/rest.go +++ b/rest/rest.go @@ -40,7 +40,9 @@ func Listener(ls net.Listener) Option { // meant for serving the OpenAPI schema. func OpenApiEndpoint(method, pattern string, f func(*openapi3.Spec) http.Handler) Option { return func(a *App) { - a.mux.Handle(fmt.Sprintf("%s %s", method, pattern), f(a.spec)) + a.openApiEndpoint = func(mux *http.ServeMux) { + mux.Handle(fmt.Sprintf("%s %s", method, pattern), f(a.spec)) + } } } @@ -83,6 +85,12 @@ type Operation interface { OpenApi() openapi3.Operation } +type endpoint struct { + method string + pattern string + op Operation +} + // Endpoint registers the [Operation] with both // the App wide OpenAPI spec and the App wide HTTP server. // @@ -90,33 +98,11 @@ type Operation interface { // match too broadly and cause conflicts with other paths. func Endpoint(method, pattern string, op Operation) Option { return func(app *App) { - // Per the net/http.ServeMux docs, https://pkg.go.dev/net/http#ServeMux: - // - // The special wildcard {$} matches only the end of the URL. - // For example, the pattern "/{$}" matches only the path "/", - // whereas the pattern "/" matches every path. - // - // This means that when registering the pattern with the OpenAPI spec - // the {$} needs to be stripped because OpenAPI will believe it's - // an actual path parameter. - trimmedPattern := strings.TrimSuffix(pattern, "{$}") - err := app.spec.AddOperation(method, trimmedPattern, op.OpenApi()) - if err != nil { - panic(err) - } - - // enforce strict matching for top-level path - // otherwise "/" would match too broadly and http.ServeMux - // will panic when other paths are registered e.g. /openapi.json - if pattern == "/" { - pattern = "/{$}" - } - app.pathMethods[pattern] = append(app.pathMethods[pattern], method) - - app.mux.Handle( - fmt.Sprintf("%s %s", method, pattern), - otelhttp.WithRouteTag(trimmedPattern, op), - ) + app.endpoints = append(app.endpoints, endpoint{ + method: method, + pattern: pattern, + op: op, + }) } } @@ -124,7 +110,7 @@ func Endpoint(method, pattern string, op Operation) Option { // any HTTP requests that do not match any other method-pattern combinations. func NotFoundHandler(h http.Handler) Option { return func(app *App) { - app.mux.Handle("/{path...}", h) + app.notFoundHandler = h } } @@ -161,8 +147,12 @@ func Version(s string) Option { type App struct { ls net.Listener - spec *openapi3.Spec - mux *http.ServeMux + spec *openapi3.Spec + endpoints []endpoint + + openApiEndpoint func(*http.ServeMux) + + notFoundHandler http.Handler pathMethods map[string][]string methodNotAllowedHandler http.Handler @@ -176,9 +166,9 @@ func NewApp(opts ...Option) *App { spec: &openapi3.Spec{ Openapi: "3.0.3", }, - mux: http.NewServeMux(), - pathMethods: make(map[string][]string), - listen: net.Listen, + pathMethods: make(map[string][]string), + listen: net.Listen, + openApiEndpoint: func(sm *http.ServeMux) {}, } for _, opt := range opts { opt(app) @@ -193,11 +183,23 @@ func (app *App) Run(ctx context.Context) error { return err } - app.registerMethodNotAllowedHandler() + mux := http.NewServeMux() + app.openApiEndpoint(mux) + + err = app.registerEndpoints(mux) + if err != nil { + return err + } + + if app.notFoundHandler != nil { + mux.Handle("/{path...}", app.notFoundHandler) + } + + app.registerMethodNotAllowedHandler(mux) httpServer := &http.Server{ Handler: otelhttp.NewHandler( - app.mux, + mux, "server", otelhttp.WithMessageEvents(otelhttp.ReadEvents, otelhttp.WriteEvents), ), @@ -226,7 +228,40 @@ func (app *App) listener() (net.Listener, error) { return app.listen("tcp", ":80") } -func (app *App) registerMethodNotAllowedHandler() { +func (app *App) registerEndpoints(mux *http.ServeMux) error { + for _, e := range app.endpoints { + // Per the net/http.ServeMux docs, https://pkg.go.dev/net/http#ServeMux: + // + // The special wildcard {$} matches only the end of the URL. + // For example, the pattern "/{$}" matches only the path "/", + // whereas the pattern "/" matches every path. + // + // This means that when registering the pattern with the OpenAPI spec + // the {$} needs to be stripped because OpenAPI will believe it's + // an actual path parameter. + trimmedPattern := strings.TrimSuffix(e.pattern, "{$}") + err := app.spec.AddOperation(e.method, trimmedPattern, e.op.OpenApi()) + if err != nil { + return err + } + + // enforce strict matching for top-level path + // otherwise "/" would match too broadly and http.ServeMux + // will panic when other paths are registered e.g. /openapi.json + if e.pattern == "/" { + e.pattern = "/{$}" + } + app.pathMethods[e.pattern] = append(app.pathMethods[e.pattern], e.method) + + mux.Handle( + fmt.Sprintf("%s %s", e.method, e.pattern), + otelhttp.WithRouteTag(trimmedPattern, e.op), + ) + } + return nil +} + +func (app *App) registerMethodNotAllowedHandler(mux *http.ServeMux) { if app.methodNotAllowedHandler == nil { return } @@ -246,7 +281,7 @@ func (app *App) registerMethodNotAllowedHandler() { for path, methods := range app.pathMethods { unsupportedMethods := diffSets(supportedMethods, methods) for _, method := range unsupportedMethods { - app.mux.Handle(fmt.Sprintf("%s %s", method, path), app.methodNotAllowedHandler) + mux.Handle(fmt.Sprintf("%s %s", method, path), app.methodNotAllowedHandler) } } } diff --git a/rest/rest_example_test.go b/rest/rest_example_test.go index dcbfb2b..3569a6b 100644 --- a/rest/rest_example_test.go +++ b/rest/rest_example_test.go @@ -14,7 +14,7 @@ import ( "net/http" "strings" - "github.com/z5labs/bedrock/rest/endpoint" + re "github.com/z5labs/bedrock/rest/endpoint" "golang.org/x/sync/errgroup" ) @@ -62,7 +62,7 @@ func Example() { Endpoint( http.MethodPost, "/", - endpoint.NewOperation( + re.NewOperation( echoService{}, ), ), diff --git a/rest/rest_test.go b/rest/rest_test.go index a8e7ee6..c9bb60c 100644 --- a/rest/rest_test.go +++ b/rest/rest_test.go @@ -16,7 +16,6 @@ import ( "path" "testing" - "github.com/z5labs/bedrock" "github.com/z5labs/bedrock/pkg/ptr" "github.com/stretchr/testify/assert" @@ -561,8 +560,22 @@ func (h operationHandler) OpenApi() openapi3.Operation { return (openapi3.Operation)(h) } -func TestEndpoint(t *testing.T) { - t.Run("will panic", func(t *testing.T) { +func TestApp_Run(t *testing.T) { + t.Run("will return an error", func(t *testing.T) { + t.Run("if it fails to create the default net.Listener", func(t *testing.T) { + app := NewApp() + + listenErr := errors.New("failed to listen") + app.listen = func(network, addr string) (net.Listener, error) { + return nil, listenErr + } + + err := app.Run(context.Background()) + if !assert.Equal(t, listenErr, err) { + return + } + }) + t.Run("if a path parameter defined in the openapi3.Operation is not in the path pattern", func(t *testing.T) { h := operationHandler(openapi3.Operation{ Parameters: []openapi3.ParameterOrRef{ @@ -580,28 +593,41 @@ func TestEndpoint(t *testing.T) { }, }) - f := func() (err error) { - defer bedrock.Recover(&err) - - NewApp( - Endpoint(http.MethodGet, "/", h), - ) - return nil + ls, err := net.Listen("tcp", ":0") + if !assert.Nil(t, err) { + return } - err := f() + app := NewApp( + Listener(ls), + Endpoint(http.MethodGet, "/", h), + ) - var perr bedrock.PanicError - if !assert.ErrorAs(t, err, &perr) { + err = app.Run(context.Background()) + if !assert.NotNil(t, err) { return } - if !assert.NotNil(t, perr.Unwrap()) { + }) + }) + + t.Run("will not return an error", func(t *testing.T) { + t.Run("if the context.Context is cancelled", func(t *testing.T) { + ls, err := net.Listen("tcp", ":0") + if !assert.Nil(t, err) { + return + } + + app := NewApp(Listener(ls)) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err = app.Run(ctx) + if !assert.Nil(t, err) { return } }) - }) - t.Run("will not panic", func(t *testing.T) { t.Run("if a path parameter defined in the openapi3.Operation is in the path pattern", func(t *testing.T) { h := operationHandler(openapi3.Operation{ Parameters: []openapi3.ParameterOrRef{ @@ -619,54 +645,46 @@ func TestEndpoint(t *testing.T) { }, }) - f := func() (err error) { - defer bedrock.Recover(&err) - - NewApp( - Endpoint(http.MethodGet, "/{id}", h), - ) - return nil - } - - err := f() + ls, err := net.Listen("tcp", ":0") if !assert.Nil(t, err) { return } - }) - }) -} -func TestApp_Run(t *testing.T) { - t.Run("will return an error", func(t *testing.T) { - t.Run("if it fails to create the default net.Listener", func(t *testing.T) { - app := NewApp() + app := NewApp( + Listener(ls), + Endpoint(http.MethodGet, "/{id}", h), + ) - listenErr := errors.New("failed to listen") - app.listen = func(network, addr string) (net.Listener, error) { - return nil, listenErr - } + ctx, cancel := context.WithCancel(context.Background()) + eg, egctx := errgroup.WithContext(ctx) + eg.Go(func() error { + return app.Run(egctx) + }) - err := app.Run(context.Background()) - if !assert.Equal(t, listenErr, err) { - return - } - }) - }) + respCh := make(chan *http.Response, 1) + eg.Go(func() error { + defer cancel() + defer close(respCh) - t.Run("will not return an error", func(t *testing.T) { - t.Run("if the context.Context is cancelled", func(t *testing.T) { - ls, err := net.Listen("tcp", ":0") + resp, err := http.Get(fmt.Sprintf("http://%s/123", ls.Addr())) + if err != nil { + return err + } + + respCh <- resp + return nil + }) + + err = eg.Wait() if !assert.Nil(t, err) { return } - app := NewApp(Listener(ls)) - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - err = app.Run(ctx) - if !assert.Nil(t, err) { + resp := <-respCh + if !assert.NotNil(t, resp) { + return + } + if !assert.Equal(t, http.StatusOK, resp.StatusCode) { return } }) From ebc55498e7a8bdc74c6394319243ce4598f5914f Mon Sep 17 00:00:00 2001 From: Richard Carson Derr Date: Sun, 29 Sep 2024 18:53:41 -0400 Subject: [PATCH 2/4] test(issue-293): bug replicated with test case --- rest/rest_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/rest/rest_test.go b/rest/rest_test.go index c9bb60c..57458f9 100644 --- a/rest/rest_test.go +++ b/rest/rest_test.go @@ -666,7 +666,88 @@ func TestApp_Run(t *testing.T) { defer cancel() defer close(respCh) - resp, err := http.Get(fmt.Sprintf("http://%s/123", ls.Addr())) + req, err := http.NewRequestWithContext( + egctx, + http.MethodGet, + fmt.Sprintf("http://%s/123", ls.Addr()), + nil, + ) + if err != nil { + return err + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + + respCh <- resp + return nil + }) + + err = eg.Wait() + if !assert.Nil(t, err) { + return + } + + resp := <-respCh + if !assert.NotNil(t, resp) { + return + } + if !assert.Equal(t, http.StatusOK, resp.StatusCode) { + return + } + }) + + t.Run("if a wildcard path parameter is used", func(t *testing.T) { + h := operationHandler(openapi3.Operation{ + Parameters: []openapi3.ParameterOrRef{ + { + Parameter: &openapi3.Parameter{ + In: openapi3.ParameterInPath, + Name: "id", + Schema: &openapi3.SchemaOrRef{ + Schema: &openapi3.Schema{ + Type: ptr.Ref(openapi3.SchemaTypeString), + }, + }, + }, + }, + }, + }) + + ls, err := net.Listen("tcp", ":0") + if !assert.Nil(t, err) { + return + } + + app := NewApp( + Listener(ls), + Endpoint(http.MethodGet, "/{id...}", h), + ) + + ctx, cancel := context.WithCancel(context.Background()) + eg, egctx := errgroup.WithContext(ctx) + eg.Go(func() error { + return app.Run(egctx) + }) + + respCh := make(chan *http.Response, 1) + eg.Go(func() error { + defer cancel() + defer close(respCh) + + req, err := http.NewRequestWithContext( + egctx, + http.MethodGet, + fmt.Sprintf("http://%s/123", ls.Addr()), + nil, + ) + if err != nil { + return err + } + + resp, err := http.DefaultClient.Do(req) if err != nil { return err } From 27fc8b0bad3829c4109a403be88d710ffd2d0d89 Mon Sep 17 00:00:00 2001 From: Richard Carson Derr Date: Sun, 29 Sep 2024 18:59:00 -0400 Subject: [PATCH 3/4] fix(issue-293): remove ... wildcard before registering open api operation --- rest/rest.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rest/rest.go b/rest/rest.go index fc8ea9d..a818c08 100644 --- a/rest/rest.go +++ b/rest/rest.go @@ -240,6 +240,15 @@ func (app *App) registerEndpoints(mux *http.ServeMux) error { // the {$} needs to be stripped because OpenAPI will believe it's // an actual path parameter. trimmedPattern := strings.TrimSuffix(e.pattern, "{$}") + + // Per the net/http.ServeMux docs, https://pkg.go.dev/net/http#ServeMux: + // + // A path can include wildcard segments of the form {NAME} or {NAME...}. + // + // The '...' wildcard has no equivalent in OpenAPI so we must remove it + // before registering the OpenAPI operation with the spec. + trimmedPattern = strings.ReplaceAll(trimmedPattern, "...", "") + err := app.spec.AddOperation(e.method, trimmedPattern, e.op.OpenApi()) if err != nil { return err From ce51744c547a0f860b85d582633abb604a06fe70 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Sun, 29 Sep 2024 22:59:49 +0000 Subject: [PATCH 4/4] chore(docs): updated coverage badge. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b3f386c..8760181 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Mentioned in Awesome Go](https://awesome.re/mentioned-badge.svg)](https://github.com/avelino/awesome-go) [![Go Reference](https://pkg.go.dev/badge/github.com/z5labs/bedrock.svg)](https://pkg.go.dev/github.com/z5labs/bedrock) [![Go Report Card](https://goreportcard.com/badge/github.com/z5labs/bedrock)](https://goreportcard.com/report/github.com/z5labs/bedrock) -![Coverage](https://img.shields.io/badge/Coverage-97.2%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-97.0%25-brightgreen) [![build](https://github.com/z5labs/bedrock/actions/workflows/build.yaml/badge.svg)](https://github.com/z5labs/bedrock/actions/workflows/build.yaml) **bedrock provides a minimal, modular and composable foundation for