Skip to content

Commit

Permalink
Add error handling for modifiers
Browse files Browse the repository at this point in the history
- Modifiers can append to errors slice that is checked when creating
  routes
- This allows for cleaner error handling when building APIs and could
  enable dynamically creating APIs at runtime without panic risk

Closes #32
  • Loading branch information
calvinmclean committed Feb 25, 2024
1 parent db6840c commit f5801df
Show file tree
Hide file tree
Showing 7 changed files with 111 additions and 39 deletions.
27 changes: 20 additions & 7 deletions babyapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package babyapi
import (
"context"
"errors"
"fmt"
"log"
"log/slog"
"net/http"
Expand Down Expand Up @@ -79,6 +80,8 @@ type API[T Resource] struct {
rootAPI bool

readOnly sync.Mutex

errors []error
}

// NewAPI initializes an API using the provided name, base URL path, and function to create a new instance of
Expand Down Expand Up @@ -115,6 +118,7 @@ func NewAPI[T Resource](name, base string, instance func() T) *API[T] {
nil,
false,
sync.Mutex{},
nil,
}

api.GetAll = api.defaultGetAll()
Expand Down Expand Up @@ -250,7 +254,8 @@ func (a *API[T]) AddCustomRootRoute(route chi.Route) *API[T] {
a.panicIfReadOnly()

if a.parent != nil {
panic("cannot be applied to child APIs")
a.errors = append(a.errors, fmt.Errorf("AddCustomRootRoute: cannot be applied to child APIs"))
return a

Check warning on line 258 in babyapi.go

View check run for this annotation

Codecov / codecov/patch

babyapi.go#L257-L258

Added lines #L257 - L258 were not covered by tests
}
a.rootRoutes = append(a.rootRoutes, route)
return a
Expand All @@ -270,7 +275,8 @@ func (a *API[T]) AddCustomIDRoute(route chi.Route) *API[T] {
a.panicIfReadOnly()

if a.rootAPI {
panic("ID routes cannot be used with a root API")
a.errors = append(a.errors, fmt.Errorf("AddCustomIDRoute: ID routes cannot be used with a root API"))
return a
}
a.customIDRoutes = append(a.customIDRoutes, route)
return a
Expand All @@ -289,19 +295,24 @@ func (a *API[T]) AddIDMiddleware(m func(http.Handler) http.Handler) *API[T] {
a.panicIfReadOnly()

if a.rootAPI {
panic("ID middleware cannot be used with a root API")
a.errors = append(a.errors, fmt.Errorf("AddIDMiddleware: ID middleware cannot be used with a root API"))
return a
}
a.idMiddlewares = append(a.idMiddlewares, m)
return a
}

// Serve will serve the API on the given port
func (a *API[T]) Serve(address string) {
func (a *API[T]) Serve(address string) error {
if address == "" {
address = ":8080"
}

a.server = &http.Server{Addr: address, Handler: a.Router()}
router, err := a.Router()
if err != nil {
return fmt.Errorf("error creating router: %w", err)
}
a.server = &http.Server{Addr: address, Handler: router}

var serverStopCtx context.CancelFunc
a.serverCtx, serverStopCtx = context.WithCancel(context.Background())
Expand Down Expand Up @@ -330,12 +341,14 @@ func (a *API[T]) Serve(address string) {
}()

slog.Info("starting server", "address", address, "api", a.name)
err := a.server.ListenAndServe()
err = a.server.ListenAndServe()
if err != nil && err != http.ErrServerClosed {
slog.Error("error starting the server", "error", err)
return fmt.Errorf("error starting the server: %w", err)

Check warning on line 346 in babyapi.go

View check run for this annotation

Codecov / codecov/patch

babyapi.go#L346

Added line #L346 was not covered by tests
}

<-a.serverCtx.Done()

return nil
}

// Stop will stop the API
Expand Down
38 changes: 26 additions & 12 deletions babyapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ func TestBabyAPI(t *testing.T) {

album1 := &Album{Title: "Album1"}

go api.Serve("localhost:8080")
go func() {
err := api.Serve("localhost:8080")
require.NoError(t, err)
}()
serverURL := "http://localhost:8080"

waitForAPI(serverURL)
Expand Down Expand Up @@ -980,19 +983,24 @@ func TestAPIModifierErrors(t *testing.T) {
}

func TestRootAPIWithMiddlewareAndCustomHandlers(t *testing.T) {
api := babyapi.NewRootAPI("root", "/")

t.Run("CustomizationsForIDsPanics", func(t *testing.T) {
require.Panics(t, func() {
api.AddCustomIDRoute(chi.Route{})
})
require.Panics(t, func() {
api.AddIDMiddleware(func(h http.Handler) http.Handler {
return nil
})
t.Run("CustomizationsForIDsCauseError", func(t *testing.T) {
api := babyapi.NewRootAPI("root", "/")
api.AddCustomIDRoute(chi.Route{})
api.AddIDMiddleware(func(h http.Handler) http.Handler {
return nil
})

err := api.RunWithArgs(os.Stdout, []string{"serve"}, "", "", false, nil, "")
require.Error(t, err)
require.ErrorAs(t, err, &babyapi.BuilderError{})
require.Equal(t, `error creating router: encountered 2 errors constructing API:
- AddCustomIDRoute: ID routes cannot be used with a root API
- AddIDMiddleware: ID middleware cannot be used with a root API
`, err.Error())
})

api := babyapi.NewRootAPI("root", "/")

api.Get = func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(201)
}
Expand Down Expand Up @@ -1345,11 +1353,17 @@ func TestRootAPICLI(t *testing.T) {
func TestReadOnlyPanicAfterStart(t *testing.T) {
api := babyapi.NewAPI[*Album]("Albums", "/albums", func() *Album { return &Album{} })

_ = api.Router()
_, err := api.Router()
require.NoError(t, err)

require.PanicsWithError(t, "API cannot be modified after starting", func() {
api.SetOnCreateOrUpdate(func(_ *http.Request, _ *Album) *babyapi.ErrResponse {
return nil
})
})
}

// func TestInvalidUseOfModifiersReturnsErrorAtStart(t *testing.T) {
// api := babyapi.NewRootAPI[*Album]("Albums", "/albums", func() *Album { return &Album{} })
// api
// }
3 changes: 1 addition & 2 deletions cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ func (a *API[T]) RunWithArgs(out io.Writer, args []string, bindAddress string, a
}

if args[0] == "serve" {
a.Serve(bindAddress)
return nil
return a.Serve(bindAddress)
}

return a.runClientCLI(out, args, address, pretty, headers, query)
Expand Down
5 changes: 2 additions & 3 deletions extensions.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ func (a *API[T]) ApplyExtension(e Extension[T]) *API[T] {

err := e.Apply(a)
if err != nil {
// TODO: when #32 is implemented, extensions will be added to a list and applied in the function
// that finalizes the API and returns an error
panic(fmt.Sprintf("error applying extension: %v", err))
a.errors = append(a.errors, fmt.Errorf("ApplyExtension: error applying extension: %w", err))
return a

Check warning on line 18 in extensions.go

View check run for this annotation

Codecov / codecov/patch

extensions.go#L17-L18

Added lines #L17 - L18 were not covered by tests
}
return a
}
7 changes: 4 additions & 3 deletions related_apis.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
// RelatedAPI declares a subset of methods from the API struct that are required to enable
// nested/parent-child API relationships
type RelatedAPI interface {
Router() chi.Router
Route(chi.Router)
Router() (chi.Router, error)
Route(chi.Router) error
Base() string
Name() string
GetIDParam(*http.Request) string
Expand Down Expand Up @@ -43,7 +43,8 @@ func (a *API[T]) AddNestedAPI(childAPI RelatedAPI) *API[T] {

relAPI, ok := childAPI.(relatedAPI)
if !ok {
panic(fmt.Sprintf("incompatible type for child API: %T", childAPI))
a.errors = append(a.errors, fmt.Errorf("AddNestedAPI: incompatible type for child API: %T", childAPI))
return a

Check warning on line 47 in related_apis.go

View check run for this annotation

Codecov / codecov/patch

related_apis.go#L46-L47

Added lines #L46 - L47 were not covered by tests
}

a.subAPIs[childAPI.Name()] = relAPI
Expand Down
55 changes: 46 additions & 9 deletions router.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"net/http"
"strings"
"sync"

"github.com/go-chi/chi/v5"
Expand All @@ -22,16 +23,38 @@ func defaultResponseCodes() map[string]int {
}
}

// BuilderError is used for combining errors that may occur when constructing a new API
type BuilderError struct {
errors []error
}

func (e BuilderError) Error() string {
var sb strings.Builder
sb.WriteString(fmt.Sprintf("encountered %d errors constructing API:\n", len(e.errors)))

for _, err := range e.errors {
sb.WriteString(fmt.Sprintf("- %v\n", err))
}

return sb.String()
}

var _ error = BuilderError{}

// HTMLer allows for easily represending reponses as HTML strings when accepted content
// type is text/html
type HTMLer interface {
HTML(*http.Request) string
}

// Create API routes on the given router
func (a *API[T]) Route(r chi.Router) {
func (a *API[T]) Route(r chi.Router) error {
a.readOnly.TryLock()

if len(a.errors) > 0 {
return BuilderError{a.errors}
}

respondOnce.Do(func() {
render.Respond = func(w http.ResponseWriter, r *http.Request, v interface{}) {
if render.GetAcceptedContentType(r) == render.ContentTypeHTML {
Expand All @@ -54,14 +77,15 @@ func (a *API[T]) Route(r chi.Router) {
a.doCustomRoutes(r, a.rootRoutes)
}

var returnErr error
r.Route(a.base, func(r chi.Router) {
// Only set these middleware for root-level API
if a.parent == nil {
a.DefaultMiddleware(r)
}

if a.rootAPI {
a.rootAPIRoutes(r)
returnErr = a.rootAPIRoutes(r)
return
}

Expand All @@ -79,38 +103,51 @@ func (a *API[T]) Route(r chi.Router) {
routeIfNotNil(r.With(a.requestBodyMiddleware).Patch, "/", a.Patch)

for _, subAPI := range a.subAPIs {
subAPI.Route(r)
err := subAPI.Route(r)
if err != nil {
returnErr = fmt.Errorf("error creating routes for %q: %w", subAPI.Name(), err)
return

Check warning on line 109 in router.go

View check run for this annotation

Codecov / codecov/patch

router.go#L108-L109

Added lines #L108 - L109 were not covered by tests
}
}

a.doCustomRoutes(r, a.customIDRoutes)
})
if returnErr != nil {
return

Check warning on line 116 in router.go

View check run for this annotation

Codecov / codecov/patch

router.go#L116

Added line #L116 was not covered by tests
}

a.doCustomRoutes(r, a.customRoutes)
})

return returnErr
}

// rootAPIRoutes creates different routes for a root API that doesn't deal with any resources
func (a *API[T]) rootAPIRoutes(r chi.Router) {
func (a *API[T]) rootAPIRoutes(r chi.Router) error {
routeIfNotNil(r.Post, "/", a.Post)
routeIfNotNil(r.Get, "/", a.Get)
routeIfNotNil(r.Delete, "/", a.Delete)
routeIfNotNil(r.Put, "/", a.Put)
routeIfNotNil(r.Patch, "/", a.Patch)

for _, subAPI := range a.subAPIs {
subAPI.Route(r)
err := subAPI.Route(r)
if err != nil {
return fmt.Errorf("error creating routes for %q: %w", subAPI.Name(), err)

Check warning on line 136 in router.go

View check run for this annotation

Codecov / codecov/patch

router.go#L136

Added line #L136 was not covered by tests
}
}

a.doCustomRoutes(r, a.rootRoutes)
a.doCustomRoutes(r, a.customRoutes)

return nil
}

// Create a new router with API routes
func (a *API[T]) Router() chi.Router {
func (a *API[T]) Router() (chi.Router, error) {
r := chi.NewRouter()
a.Route(r)

return r
err := a.Route(r)
return r, err
}

func (a *API[T]) doCustomRoutes(r chi.Router, routes []chi.Route) {
Expand Down
15 changes: 12 additions & 3 deletions test/testing_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,26 @@ import (

"github.com/calvinmclean/babyapi"
"github.com/go-chi/chi/v5"
"github.com/stretchr/testify/require"
)

// Test is meant to be used in external tests to automatically handle setting up routes and using httptest
func TestServe[T babyapi.Resource](t *testing.T, api *babyapi.API[T]) (string, func()) {
server := httptest.NewServer(api.Router())
router, err := api.Router()
require.NoError(t, err)
server := httptest.NewServer(router)
return server.URL, server.Close
}

// TestRequest is meant to be used in external tests to automatically handle setting up routes and using httptest
func TestRequest[T babyapi.Resource](t *testing.T, api *babyapi.API[T], r *http.Request) *httptest.ResponseRecorder {
w := httptest.NewRecorder()

apiRouter, err := api.Router()
require.NoError(t, err)

router := chi.NewRouter()
router.Mount("/", api.Router())
router.Mount("/", apiRouter)
router.ServeHTTP(w, r)

return w
Expand All @@ -36,10 +42,13 @@ func TestWithParentRoute[T, P babyapi.Resource](t *testing.T, api *babyapi.API[T
parentAPI := babyapi.NewAPI[P](parentName, parentBasePath, func() P { return parent })
parentAPI.AddNestedAPI(api)

apiRouter, err := api.Router()
require.NoError(t, err)

router := chi.NewRouter()
api.DefaultMiddleware(router)
router.Route(fmt.Sprintf("%s/{%s}", parentBasePath, babyapi.IDParamKey(parentName)), func(r chi.Router) {
r.Mount("/", api.Router())
r.Mount("/", apiRouter)
})

router.ServeHTTP(w, r.WithContext(context.WithValue(context.Background(), babyapi.ContextKey(parentName), parent)))
Expand Down

0 comments on commit f5801df

Please sign in to comment.