Skip to content

Commit

Permalink
chore: exclusively use the blog logger internally
Browse files Browse the repository at this point in the history
Externally we take a *slog.Logger, but internally we wrap this
everywhere
  • Loading branch information
blgm committed Feb 6, 2024
1 parent 71859bb commit 62d6db8
Show file tree
Hide file tree
Showing 17 changed files with 416 additions and 55 deletions.
8 changes: 4 additions & 4 deletions domain/apiresponses/failure_responses.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"errors"
"fmt"
"net/http"

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

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

func (f *FailureResponse) ValidatedStatusCode(logger interface{ Error(string, error) }) int {
func (f *FailureResponse) ValidatedStatusCode(logger blog.Blog) int {
if f.statusCode < 400 || 600 <= f.statusCode {
if logger != nil {
logger.Error("validating-status-code", errors.New("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
}
return f.statusCode
Expand Down
20 changes: 13 additions & 7 deletions domain/apiresponses/failure_responses_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package apiresponses_test

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

var _ = Describe("FailureResponse", func() {
var fakeLogger blog.Blog

BeforeEach(func() {
fakeLogger = blog.New(slog.New(&fakes.FakeHandler{}))
})

Describe("ErrorResponse", func() {
It("returns a ErrorResponse containing the error message", func() {
failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key"))
Expand Down Expand Up @@ -48,7 +54,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(fakeLogger)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))

errorResponse, typeCast := newError.ErrorResponse().(apiresponses.ErrorResponse)
Expand All @@ -64,7 +70,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(fakeLogger)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))
Expect(newError.ErrorResponse()).To(Equal(failureResponse.ErrorResponse()))
})
Expand All @@ -73,23 +79,23 @@ 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(fakeLogger)).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(fakeLogger)).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(fakeLogger)).To(Equal(http.StatusInternalServerError))
})

It("logs that the status has been changed", func() {
log := gbytes.NewBuffer()
logger := blog.New(context.TODO(), slog.New(slog.NewJSONHandler(log, nil)), "prefix")
logger := blog.New(slog.New(slog.NewJSONHandler(log, nil)))
failureResponse := asFailureResponse(apiresponses.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
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
22 changes: 15 additions & 7 deletions failure_response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package brokerapi_test

import (
"context"
"errors"
"log/slog"
"net/http"
Expand All @@ -26,10 +25,19 @@ 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/fakes"
"github.com/pivotal-cf/brokerapi/v10/internal/blog"
)

//counterfeiter:generate -o ./fakes log/slog.Handler

var _ = Describe("FailureResponse", func() {
var fakeLogger blog.Blog

BeforeEach(func() {
fakeLogger = blog.New(slog.New(&fakes.FakeHandler{}))
})

Describe("ErrorResponse", func() {
It("returns a ErrorResponse containing the error message", func() {
failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), http.StatusForbidden, "log-key"))
Expand Down Expand Up @@ -64,7 +72,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(fakeLogger)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))

errorResponse, typeCast := newError.ErrorResponse().(brokerapi.ErrorResponse)
Expand All @@ -80,7 +88,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(fakeLogger)).To(Equal(http.StatusForbidden))
Expect(newError.LoggerAction()).To(Equal(failureResponse.LoggerAction()))
Expect(newError.ErrorResponse()).To(Equal(failureResponse.ErrorResponse()))
})
Expand All @@ -89,23 +97,23 @@ 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(fakeLogger)).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(fakeLogger)).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(fakeLogger)).To(Equal(http.StatusInternalServerError))
})

It("logs that the status has been changed", func() {
log := gbytes.NewBuffer()
logger := blog.New(context.TODO(), slog.New(slog.NewJSONHandler(log, nil)), "prefix")
logger := blog.New(slog.New(slog.NewJSONHandler(log, nil)))
failureResponse := asFailureResponse(brokerapi.NewFailureResponse(errors.New("my error message"), 600, "log-key"))
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
Loading

0 comments on commit 62d6db8

Please sign in to comment.