Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 58 additions & 8 deletions api_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,63 @@ type withSetBundle interface {
SetBundle(bundle *apiBundle)
}

type statusResponse struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of endpoints return {status: "ok"}, so I made a common struct that they can share so we don't have to write a new response struct for each. statusResponseOK below is also a shortcut for making an OK response.

Status string `json:"status"`
}

var statusResponseOK = &statusResponse{Status: "ok"} //nolint:gochecknoglobals

type healthCheckGetEndpoint struct {
apiBundle
apiendpoint.Endpoint[healthCheckGetRequest, statusResponse]
}

func (*healthCheckGetEndpoint) Meta() *apiendpoint.EndpointMeta {
return &apiendpoint.EndpointMeta{
Pattern: "GET /api/health-checks/{name}",
StatusCode: http.StatusOK,
}
}

type healthCheckName string

const (
healthCheckNameComplete healthCheckName = "complete"
healthCheckNameMinimal healthCheckName = "minimal"
)

type healthCheckGetRequest struct {
Name healthCheckName `json:"-"` // from ExtractRaw
}

func (req *healthCheckGetRequest) ExtractRaw(r *http.Request) error {
req.Name = healthCheckName(r.PathValue("name"))
return nil
}

func (a *healthCheckGetEndpoint) Execute(ctx context.Context, req *healthCheckGetRequest) (*statusResponse, error) {
switch req.Name {
case healthCheckNameComplete:
if _, err := a.dbPool.Exec(ctx, "SELECT 1"); err != nil {
return nil, apierror.WithInternalError(
apierror.NewServiceUnavailable("Unable to query database. Check logs for details."),
err,
)
}

case healthCheckNameMinimal:
// fall through to OK status response below

default:
return nil, apierror.NewNotFound("Health check %q not found. Use either `complete` or `minimal`.", req.Name)
}

return statusResponseOK, nil
}

type jobCancelEndpoint struct {
apiBundle
apiendpoint.Endpoint[jobCancelRequest, jobCancelResponse]
apiendpoint.Endpoint[jobCancelRequest, statusResponse]
}

func (*jobCancelEndpoint) Meta() *apiendpoint.EndpointMeta {
Expand All @@ -58,12 +112,8 @@ type jobCancelRequest struct {
JobIDs []int64String `json:"ids"`
}

type jobCancelResponse struct {
Status string `json:"status"`
}

func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest) (*jobCancelResponse, error) {
return dbutil.WithTxV(ctx, a.dbPool, func(ctx context.Context, tx pgx.Tx) (*jobCancelResponse, error) {
func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest) (*statusResponse, error) {
return dbutil.WithTxV(ctx, a.dbPool, func(ctx context.Context, tx pgx.Tx) (*statusResponse, error) {
updatedJobs := make(map[int64]*rivertype.JobRow)
for _, jobID := range req.JobIDs {
jobID := int64(jobID)
Expand All @@ -78,7 +128,7 @@ func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest)
}

// TODO: return jobs in response, use in frontend instead of invalidating
return &jobCancelResponse{Status: "ok"}, nil
return statusResponseOK, nil
})
}

Expand Down
56 changes: 53 additions & 3 deletions api_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,57 @@ func setupEndpoint[TEndpoint any](ctx context.Context, t *testing.T) (*TEndpoint
}
}

func TestAPIHandlerJobCancel(t *testing.T) {
func TestHandlerHealthCheckGetEndpoint(t *testing.T) {
t.Parallel()

ctx := context.Background()

t.Run("CompleteSuccess", func(t *testing.T) {
t.Parallel()

endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t)

resp, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameComplete})
require.NoError(t, err)
require.Equal(t, statusResponseOK, resp)
})

t.Run("CompleteDatabaseError", func(t *testing.T) {
t.Parallel()

endpoint, bundle := setupEndpoint[healthCheckGetEndpoint](ctx, t)

// Roll back prematurely so we get a database error.
require.NoError(t, bundle.tx.Rollback(ctx))

_, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameComplete})
requireAPIError(t, apierror.WithInternalError(
apierror.NewServiceUnavailable("Unable to query database. Check logs for details."),
pgx.ErrTxClosed,
), err)
})

t.Run("Minimal", func(t *testing.T) {
t.Parallel()

endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t)

resp, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameMinimal})
require.NoError(t, err)
require.Equal(t, statusResponseOK, resp)
})

t.Run("NotFound", func(t *testing.T) {
t.Parallel()

endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t)

_, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: "other"})
requireAPIError(t, apierror.NewNotFound("Health check %q not found. Use either `complete` or `minimal`.", "other"), err)
})
}

func TestJobCancelEndpoint(t *testing.T) {
t.Parallel()

ctx := context.Background()
Expand All @@ -62,7 +112,7 @@ func TestAPIHandlerJobCancel(t *testing.T) {

resp, err := endpoint.Execute(ctx, &jobCancelRequest{JobIDs: []int64String{int64String(insertRes1.Job.ID), int64String(insertRes2.Job.ID)}})
require.NoError(t, err)
require.Equal(t, &jobCancelResponse{Status: "ok"}, resp)
require.Equal(t, statusResponseOK, resp)

updatedJob1, err := bundle.client.JobGetTx(ctx, bundle.tx, insertRes1.Job.ID)
require.NoError(t, err)
Expand All @@ -83,7 +133,7 @@ func TestAPIHandlerJobCancel(t *testing.T) {
})
}

func TestAPIHandlerJobGet(t *testing.T) {
func TestJobGetEndpoint(t *testing.T) {
t.Parallel()

ctx := context.Background()
Expand Down
1 change: 1 addition & 0 deletions handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func NewHandler(opts *HandlerOpts) (http.Handler, error) {
prefix := opts.Prefix

mux := http.NewServeMux()
apiendpoint.Mount(mux, opts.Logger, &healthCheckGetEndpoint{apiBundle: apiBundle})
mux.HandleFunc("GET /api/jobs", handler.JobList)
apiendpoint.Mount(mux, opts.Logger, &jobCancelEndpoint{apiBundle: apiBundle})
mux.HandleFunc("POST /api/jobs/delete", handler.JobDelete)
Expand Down
2 changes: 2 additions & 0 deletions handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ func TestNewHandlerIntegration(t *testing.T) {
// API calls
//

makeAPICall(t, "HealthCheckGetComplete", http.MethodGet, makeURL("/api/health-checks/%s", healthCheckNameComplete), nil)
makeAPICall(t, "HealthCheckGetMinimal", http.MethodGet, makeURL("/api/health-checks/%s", healthCheckNameMinimal), nil)
makeAPICall(t, "JobCancel", http.MethodPost, makeURL("/api/jobs/cancel"), mustMarshalJSON(t, &jobCancelRequest{JobIDs: []int64String{int64String(job.ID)}}))
makeAPICall(t, "JobGet", http.MethodGet, makeURL("/api/jobs/%d", job.ID), nil)
}
11 changes: 10 additions & 1 deletion internal/apiendpoint/api_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,17 @@ func executeAPIEndpoint[TReq any, TResp any](w http.ResponseWriter, r *http.Requ
if err != nil {
var apiErr apierror.Interface
if errors.As(err, &apiErr) {
logAttrs := []any{
slog.String("error", apiErr.Error()),
}

if internalErr := apiErr.GetInternalError(); internalErr != nil {
logAttrs = append(logAttrs, slog.String("internal_error", internalErr.Error()))
}

// Logged at info level because API errors are normal.
logger.InfoContext(ctx, "API error response", slog.String("error", apiErr.Error()))
logger.InfoContext(ctx, "API error response", logAttrs...)

apiErr.Write(ctx, logger, w)
return
}
Expand Down
18 changes: 17 additions & 1 deletion internal/apierror/api_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ import (
//
// APIErrorInterface should be used with errors.As instead of this struct.
type APIError struct {
// InternalError is an additional error that might be associated with the
// API error. It's not returned in the API error response, but is logged in
// API endpoint execution to provide extra information for operators.
InternalError error `json:"-"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a possible internal error that an API error can be tagged with and which will be added to the logs by api_endpoint.go above. This is nice because it lets some additional context be added for operators without the possibility of leaking anything sensitive from an internal error via HTTP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!


// Message is a descriptive, human-friendly message indicating what went
// wrong. Try to make error messages as actionable as possible to help the
// caller easily fix what went wrong.
Expand All @@ -26,7 +31,9 @@ type APIError struct {
StatusCode int `json:"-"`
}

func (e *APIError) Error() string { return e.Message }
func (e *APIError) Error() string { return e.Message }
func (e *APIError) GetInternalError() error { return e.InternalError }
func (e *APIError) SetInternalError(internalErr error) { e.InternalError = internalErr }

// Write writes the API error to an HTTP response, writing to the given logger
// in case of a problem.
Expand All @@ -48,9 +55,18 @@ func (e *APIError) Write(ctx context.Context, logger *slog.Logger, w http.Respon
// won't be usable as an errors.As target.
type Interface interface {
Error() string
GetInternalError() error
SetInternalError(internalErr error)
Write(ctx context.Context, logger *slog.Logger, w http.ResponseWriter)
}

// WithInternalError is a convenience function for assigning an internal error
// to the given API error and returning it.
func WithInternalError[TAPIError Interface](apiErr TAPIError, internalErr error) TAPIError {
apiErr.SetInternalError(internalErr)
return apiErr
}

//
// BadRequest
//
Expand Down
25 changes: 25 additions & 0 deletions internal/apierror/api_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package apierror
import (
"context"
"encoding/json"
"errors"
"net/http/httptest"
"testing"

Expand All @@ -11,6 +12,18 @@ import (
"github.com/riverqueue/riverui/internal/riverinternaltest"
)

func TestAPIError(t *testing.T) {
t.Parallel()

var (
anErr = errors.New("an error")
apiErr = NewBadRequest("Bad request.")
)

apiErr.SetInternalError(anErr)
require.Equal(t, anErr, apiErr.GetInternalError())
}

func TestAPIErrorJSON(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -40,6 +53,18 @@ func TestAPIErrorWrite(t *testing.T) {
)
}

func TestWithInternalError(t *testing.T) {
t.Parallel()

var (
anErr = errors.New("an error")
apiErr = NewBadRequest("Bad request.")
)

apiErr = WithInternalError(apiErr, anErr)
require.Equal(t, anErr, apiErr.InternalError)
}

func mustMarshalJSON(t *testing.T, v any) []byte {
t.Helper()

Expand Down