Skip to content

Commit

Permalink
chore: wrap slog.Logger to reduce boilerplate
Browse files Browse the repository at this point in the history
  • Loading branch information
blgm committed Feb 6, 2024
1 parent 3437960 commit 71859bb
Show file tree
Hide file tree
Showing 22 changed files with 236 additions and 219 deletions.
8 changes: 3 additions & 5 deletions domain/apiresponses/failure_responses.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package apiresponses

import (
"errors"
"fmt"
"log/slog"
"net/http"

"github.com/pivotal-cf/brokerapi/v10/internal/logutil"
)

// FailureResponse can be returned from any of the `ServiceBroker` interface methods
Expand Down Expand Up @@ -44,10 +42,10 @@ func (f *FailureResponse) ErrorResponse() interface{} {
}
}

func (f *FailureResponse) ValidatedStatusCode(prefix string, logger *slog.Logger) int {
func (f *FailureResponse) ValidatedStatusCode(logger interface{ Error(string, error) }) int {
if f.statusCode < 400 || 600 <= f.statusCode {
if logger != nil {
logger.Error(logutil.Join(prefix, "validating-status-code"), slog.String("error", "Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500."))
logger.Error("validating-status-code", errors.New("Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500."))
}
return http.StatusInternalServerError
}
Expand Down
16 changes: 9 additions & 7 deletions domain/apiresponses/failure_responses_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package apiresponses_test

import (
"context"
"errors"
"log/slog"
"net/http"
Expand All @@ -9,6 +10,7 @@ import (
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gbytes"
"github.com/pivotal-cf/brokerapi/v10/domain/apiresponses"
"github.com/pivotal-cf/brokerapi/v10/internal/blog"
)

var _ = Describe("FailureResponse", func() {
Expand Down Expand Up @@ -46,7 +48,7 @@ var _ = Describe("FailureResponse", func() {
newError := failureResponse.AppendErrorMessage("and some more details")

Expect(newError.Error()).To(Equal("my error message and some more details"))
Expect(newError.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(newError.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))

errorResponse, typeCast := newError.ErrorResponse().(apiresponses.ErrorResponse)
Expand All @@ -62,7 +64,7 @@ var _ = Describe("FailureResponse", func() {
newError := failureResponse.AppendErrorMessage("and some more details")

Expect(newError.Error()).To(Equal("my error message and some more details"))
Expect(newError.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(newError.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))
Expect(newError.ErrorResponse()).To(Equal(failureResponse.ErrorResponse()))
})
Expand All @@ -71,25 +73,25 @@ var _ = Describe("FailureResponse", func() {
Describe("ValidatedStatusCode", func() {
It("returns the status code that was passed in", func() {
failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key"))
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
})

It("when error key is provided it returns the status code that was passed in", func() {
failureResponse := apiresponses.NewFailureResponseBuilder(errors.New("my error message"), http.StatusForbidden, "log-key").WithErrorKey("error key").Build()
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
})

Context("when the status code is invalid", func() {
It("returns 500", func() {
failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusInternalServerError))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusInternalServerError))
})

It("logs that the status has been changed", func() {
log := gbytes.NewBuffer()
logger := slog.New(slog.NewJSONHandler(log, nil))
logger := blog.New(context.TODO(), slog.New(slog.NewJSONHandler(log, nil)), "prefix")
failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
failureResponse.ValidatedStatusCode("", logger)
failureResponse.ValidatedStatusCode(logger)
Expect(log).To(gbytes.Say("Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500."))
})
})
Expand Down
16 changes: 9 additions & 7 deletions failure_response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package brokerapi_test

import (
"context"
"errors"
"log/slog"
"net/http"
Expand All @@ -25,6 +26,7 @@ import (
"github.com/onsi/gomega/gbytes"
"github.com/pivotal-cf/brokerapi/v10"
"github.com/pivotal-cf/brokerapi/v10/domain/apiresponses"
"github.com/pivotal-cf/brokerapi/v10/internal/blog"
)

var _ = Describe("FailureResponse", func() {
Expand Down Expand Up @@ -62,7 +64,7 @@ var _ = Describe("FailureResponse", func() {
newError := failureResponse.AppendErrorMessage("and some more details")

Expect(newError.Error()).To(Equal("my error message and some more details"))
Expect(newError.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(newError.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))

errorResponse, typeCast := newError.ErrorResponse().(brokerapi.ErrorResponse)
Expand All @@ -78,7 +80,7 @@ var _ = Describe("FailureResponse", func() {
newError := failureResponse.AppendErrorMessage("and some more details")

Expect(newError.Error()).To(Equal("my error message and some more details"))
Expect(newError.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(newError.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))
Expect(newError.ErrorResponse()).To(Equal(failureResponse.ErrorResponse()))
})
Expand All @@ -87,25 +89,25 @@ var _ = Describe("FailureResponse", func() {
Describe("ValidatedStatusCode", func() {
It("returns the status code that was passed in", func() {
failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key"))
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
})

It("when error key is provided it returns the status code that was passed in", func() {
failureResponse := brokerapi.NewFailureResponseBuilder(errors.New("my error message"), http.StatusForbidden, "log-key").WithErrorKey("error key").Build()
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusForbidden))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusForbidden))
})

Context("when the status code is invalid", func() {
It("returns 500", func() {
failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
Expect(failureResponse.ValidatedStatusCode("", nil)).To(Equal(http.StatusInternalServerError))
Expect(failureResponse.ValidatedStatusCode(nil)).To(Equal(http.StatusInternalServerError))
})

It("logs that the status has been changed", func() {
log := gbytes.NewBuffer()
logger := slog.New(slog.NewJSONHandler(log, nil))
logger := blog.New(context.TODO(), slog.New(slog.NewJSONHandler(log, nil)), "prefix")
failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
failureResponse.ValidatedStatusCode("", logger)
failureResponse.ValidatedStatusCode(logger)
Expect(log).To(gbytes.Say("Invalid failure http response code: 600, expected 4xx or 5xx, returning internal server error: 500."))
})
})
Expand Down
6 changes: 1 addition & 5 deletions handlers/api_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,13 @@ import (
"net/http"

"github.com/pivotal-cf/brokerapi/v10/domain"
"github.com/pivotal-cf/brokerapi/v10/internal/logutil"
)

const (
invalidServiceDetailsErrorKey = "invalid-service-details"
instanceIDLogKey = "instance-id"
serviceIdMissingKey = "service-id-missing"
planIdMissingKey = "plan-id-missing"
unknownErrorKey = "unknown-error"

bindingIDLogKey = "binding-id"
)

var (
Expand Down Expand Up @@ -48,7 +44,7 @@ func (h APIHandler) respond(w http.ResponseWriter, status int, requestIdentity s
encoder.SetEscapeHTML(false)
err := encoder.Encode(response)
if err != nil {
h.logger.Error("encoding response", logutil.Error(err), slog.Int("status", status), slog.Any("response", response))
h.logger.Error("encoding response", slog.Any("error", err), slog.Int("status", status), slog.Any("response", response))
}
}

Expand Down
26 changes: 11 additions & 15 deletions handlers/bind.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,26 @@ package handlers
import (
"encoding/json"
"fmt"
"log/slog"
"net/http"

"github.com/pivotal-cf/brokerapi/v10/internal/blog"

"github.com/go-chi/chi/v5"
"github.com/pivotal-cf/brokerapi/v10/domain"
"github.com/pivotal-cf/brokerapi/v10/domain/apiresponses"
"github.com/pivotal-cf/brokerapi/v10/internal/logutil"
"github.com/pivotal-cf/brokerapi/v10/middlewares"
"github.com/pivotal-cf/brokerapi/v10/utils"
)

const (
bindLogKey = "bind"
invalidBindDetailsErrorKey = "bind.invalid-bind-details"
invalidBindDetailsErrorKey = "invalid-bind-details"
)

func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {
instanceID := chi.URLParam(req, "instance_id")
bindingID := chi.URLParam(req, "binding_id")

logger := h.logger.With(append(
[]any{slog.String(instanceIDLogKey, instanceID), slog.String(bindingIDLogKey, bindingID)},
utils.ContextAttr(req.Context(), middlewares.CorrelationIDKey, middlewares.RequestIdentityKey)...,
)...)
logger := blog.New(req.Context(), h.logger, bindLogKey, blog.InstanceID(instanceID), blog.BindingID(bindingID))

version := getAPIVersion(req)
asyncAllowed := false
Expand All @@ -38,23 +34,23 @@ func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {

var details domain.BindDetails
if err := json.NewDecoder(req.Body).Decode(&details); err != nil {
logger.Error(invalidBindDetailsErrorKey, logutil.Error(err))
logger.Error(invalidBindDetailsErrorKey, err)
h.respond(w, http.StatusUnprocessableEntity, requestId, apiresponses.ErrorResponse{
Description: err.Error(),
})
return
}

if details.ServiceID == "" {
logger.Error(logutil.Join(bindLogKey, serviceIdMissingKey), logutil.Error(serviceIdError))
logger.Error(serviceIdMissingKey, serviceIdError)
h.respond(w, http.StatusBadRequest, requestId, apiresponses.ErrorResponse{
Description: serviceIdError.Error(),
})
return
}

if details.PlanID == "" {
logger.Error(logutil.Join(bindLogKey, planIdMissingKey), logutil.Error(planIdError))
logger.Error(planIdMissingKey, planIdError)
h.respond(w, http.StatusBadRequest, requestId, apiresponses.ErrorResponse{
Description: planIdError.Error(),
})
Expand All @@ -65,7 +61,7 @@ func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {
if err != nil {
switch err := err.(type) {
case *apiresponses.FailureResponse:
statusCode := err.ValidatedStatusCode(bindLogKey, logger)
statusCode := err.ValidatedStatusCode(logger)
errorResponse := err.ErrorResponse()
if err == apiresponses.ErrInstanceDoesNotExist {
// work around ErrInstanceDoesNotExist having different pre-refactor behaviour to other actions
Expand All @@ -74,10 +70,10 @@ func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {
}
statusCode = http.StatusNotFound
}
logger.Error(logutil.Join(bindLogKey, err.LoggerAction()), logutil.Error(err))
logger.Error(err.LoggerAction(), err)
h.respond(w, statusCode, requestId, errorResponse)
default:
logger.Error(logutil.Join(bindLogKey, unknownErrorKey), logutil.Error(err))
logger.Error(unknownErrorKey, err)
h.respond(w, http.StatusInternalServerError, requestId, apiresponses.ErrorResponse{
Description: err.Error(),
})
Expand Down Expand Up @@ -109,7 +105,7 @@ func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {
for _, vol := range binding.VolumeMounts {
experimentalConfig, err := json.Marshal(vol.Device.MountConfig)
if err != nil {
logger.Error(logutil.Join(bindLogKey, unknownErrorKey), logutil.Error(err))
logger.Error(unknownErrorKey, err)
h.respond(w, http.StatusInternalServerError, requestId, apiresponses.ErrorResponse{Description: err.Error()})
return
}
Expand Down
10 changes: 5 additions & 5 deletions handlers/catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,24 @@ import (
"fmt"
"net/http"

"github.com/pivotal-cf/brokerapi/v10/internal/blog"

"github.com/pivotal-cf/brokerapi/v10/domain/apiresponses"
"github.com/pivotal-cf/brokerapi/v10/internal/logutil"
"github.com/pivotal-cf/brokerapi/v10/middlewares"
"github.com/pivotal-cf/brokerapi/v10/utils"
)

const getCatalogLogKey = "getCatalog"

func (h *APIHandler) Catalog(w http.ResponseWriter, req *http.Request) {
logger := h.logger.With(utils.ContextAttr(req.Context(), middlewares.CorrelationIDKey, middlewares.RequestIdentityKey)...)
logger := blog.New(req.Context(), h.logger, getCatalogLogKey)
requestId := fmt.Sprintf("%v", req.Context().Value(middlewares.RequestIdentityKey))

services, err := h.serviceBroker.Services(req.Context())
if err != nil {
switch err := err.(type) {
case *apiresponses.FailureResponse:
logger.Error(logutil.Join(getCatalogLogKey, err.LoggerAction()), logutil.Error(err))
h.respond(w, err.ValidatedStatusCode(getCatalogLogKey, logger), requestId, err.ErrorResponse())
logger.Error(err.LoggerAction(), err)
h.respond(w, err.ValidatedStatusCode(logger), requestId, err.ErrorResponse())
default:
logger.Error(unknownErrorKey, err)
h.respond(w, http.StatusInternalServerError, requestId, apiresponses.ErrorResponse{
Expand Down
20 changes: 8 additions & 12 deletions handlers/deprovision.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,22 @@ package handlers

import (
"fmt"
"log/slog"
"net/http"

"github.com/pivotal-cf/brokerapi/v10/internal/blog"

"github.com/go-chi/chi/v5"
"github.com/pivotal-cf/brokerapi/v10/domain"
"github.com/pivotal-cf/brokerapi/v10/domain/apiresponses"
"github.com/pivotal-cf/brokerapi/v10/internal/logutil"
"github.com/pivotal-cf/brokerapi/v10/middlewares"
"github.com/pivotal-cf/brokerapi/v10/utils"
)

const deprovisionLogKey = "deprovision"

func (h APIHandler) Deprovision(w http.ResponseWriter, req *http.Request) {
instanceID := chi.URLParam(req, "instance_id")

logger := h.logger.With(append(
[]any{slog.String(instanceIDLogKey, instanceID)},
utils.ContextAttr(req.Context(), middlewares.CorrelationIDKey, middlewares.RequestIdentityKey)...,
)...)
logger := blog.New(req.Context(), h.logger, deprovisionLogKey, blog.InstanceID(instanceID))

details := domain.DeprovisionDetails{
PlanID: req.FormValue("plan_id"),
Expand All @@ -35,15 +31,15 @@ func (h APIHandler) Deprovision(w http.ResponseWriter, req *http.Request) {
h.respond(w, http.StatusBadRequest, requestId, apiresponses.ErrorResponse{
Description: serviceIdError.Error(),
})
logger.Error(logutil.Join(deprovisionLogKey, serviceIdMissingKey), logutil.Error(serviceIdError))
logger.Error(serviceIdMissingKey, serviceIdError)
return
}

if details.PlanID == "" {
h.respond(w, http.StatusBadRequest, requestId, apiresponses.ErrorResponse{
Description: planIdError.Error(),
})
logger.Error(logutil.Join(deprovisionLogKey, planIdMissingKey), logutil.Error(planIdError))
logger.Error(planIdMissingKey, planIdError)
return
}

Expand All @@ -53,10 +49,10 @@ func (h APIHandler) Deprovision(w http.ResponseWriter, req *http.Request) {
if err != nil {
switch err := err.(type) {
case *apiresponses.FailureResponse:
logger.Error(logutil.Join(deprovisionLogKey, err.LoggerAction()), logutil.Error(err))
h.respond(w, err.ValidatedStatusCode(deprovisionLogKey, logger), requestId, err.ErrorResponse())
logger.Error(err.LoggerAction(), err)
h.respond(w, err.ValidatedStatusCode(logger), requestId, err.ErrorResponse())
default:
logger.Error(logutil.Join(deprovisionLogKey, unknownErrorKey), logutil.Error(err))
logger.Error(unknownErrorKey, err)
h.respond(w, http.StatusInternalServerError, requestId, apiresponses.ErrorResponse{
Description: err.Error(),
})
Expand Down
Loading

0 comments on commit 71859bb

Please sign in to comment.