Skip to content

Commit

Permalink
Use error type option instead of err prefixing
Browse files Browse the repository at this point in the history
  • Loading branch information
Marcel Ludwig authored and malud committed May 17, 2021
1 parent 22cbb41 commit 7ee1937
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 43 deletions.
5 changes: 3 additions & 2 deletions handler/transport/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (b *Backend) RoundTrip(req *http.Request) (*http.Response, error) {
if b.openAPIValidator != nil {
// FIXME tc.Origin should be an origin, not just a host!
if err = b.openAPIValidator.ValidateRequest(req, tc.hash(), tc.Scheme+"://"+tc.Origin); err != nil {
return nil, errors.BackendValidation.Label(b.name).With(err)
return nil, errors.BackendValidation.Label(b.name).Kind("backend_request_validation").With(err)
}
}

Expand Down Expand Up @@ -143,7 +143,8 @@ func (b *Backend) RoundTrip(req *http.Request) (*http.Response, error) {

if b.openAPIValidator != nil {
if err = b.openAPIValidator.ValidateResponse(beresp); err != nil {
return nil, errors.BackendValidation.Label(b.name).With(err).Status(http.StatusBadGateway)
return nil, errors.BackendValidation.Label(b.name).Kind("backend_response_validation").
With(err).Status(http.StatusBadGateway)
}
}

Expand Down
20 changes: 11 additions & 9 deletions handler/transport/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,35 +197,35 @@ func TestBackend_RoundTrip_Validation(t *testing.T) {
http.MethodPost,
"/get",
"backend validation error",
"request validation: 'POST /get': Path doesn't support the HTTP method",
"'POST /get': Path doesn't support the HTTP method",
},
{
"invalid request, IgnoreRequestViolations",
&config.OpenAPI{File: "testdata/upstream.yaml", IgnoreRequestViolations: true, IgnoreResponseViolations: true},
http.MethodPost,
"/get",
"",
"request validation: 'POST /get': Path doesn't support the HTTP method",
"'POST /get': Path doesn't support the HTTP method",
},
{
"invalid response",
&config.OpenAPI{File: "testdata/upstream.yaml"},
http.MethodGet,
"/get?404",
"backend validation error",
"response validation: status is not supported",
"status is not supported",
},
{
"invalid response, IgnoreResponseViolations",
&config.OpenAPI{File: "testdata/upstream.yaml", IgnoreResponseViolations: true},
http.MethodGet,
"/get?404",
"",
"response validation: status is not supported",
"status is not supported",
},
}

logger, hook := logrustest.NewNullLogger()
logger, hook := test.NewLogger()
log := logger.WithContext(context.Background())

for _, tt := range tests {
Expand Down Expand Up @@ -260,13 +260,15 @@ func TestBackend_RoundTrip_Validation(t *testing.T) {
entry := hook.LastEntry()
if tt.expectedLogMessage != "" {
if data, ok := entry.Data["validation"]; ok {
for _, err := range data.([]string) {
if err == tt.expectedLogMessage {
for _, errStr := range data.([]string) {
if errStr != tt.expectedLogMessage {
subT.Errorf("\nwant:\t%s\ngot:\t%v", tt.expectedLogMessage, errStr)
return
}
return
}
for _, err := range data.([]string) {
subT.Log(err)
for _, errStr := range data.([]string) {
subT.Log(errStr)
}
}
subT.Errorf("expected matching validation error logs:\n\t%s\n\tgot: nothing", tt.expectedLogMessage)
Expand Down
16 changes: 5 additions & 11 deletions handler/validation/openapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ import (
"net/url"
"sync"

"github.com/avenga/couper/config/request"

"github.com/getkin/kin-openapi/openapi3"
"github.com/getkin/kin-openapi/openapi3filter"

"github.com/avenga/couper/config/request"
"github.com/avenga/couper/eval"
)

Expand Down Expand Up @@ -60,8 +59,7 @@ func (v *OpenAPI) getModifiedSwagger(key, origin string) (*openapi3.Swagger, err
return s, nil
}

err := fmt.Errorf("request validation: swagger wrong type: %v", swagger)
return nil, err
return nil, fmt.Errorf("swagger wrong type: %v", swagger)
}

func cloneSwagger(s *openapi3.Swagger) *openapi3.Swagger {
Expand Down Expand Up @@ -90,7 +88,7 @@ func (v *OpenAPI) ValidateRequest(req *http.Request, key, origin string) error {

route, pathParams, err := router.FindRoute(req.Method, req.URL)
if err != nil {
err = fmt.Errorf("request validation: '%s %s': %w", req.Method, req.URL.Path, err)
err = fmt.Errorf("'%s %s': %w", req.Method, req.URL.Path, err)
if ctx, ok := req.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
ctx.errors = append(ctx.errors, err)
}
Expand All @@ -109,10 +107,7 @@ func (v *OpenAPI) ValidateRequest(req *http.Request, key, origin string) error {
}

// openapi3filter.ValidateRequestBody also handles resetting the req body after reading until EOF.
err = openapi3filter.ValidateRequest(req.Context(), v.requestValidationInput)

if err != nil {
err = fmt.Errorf("request validation: %w", err)
if err = openapi3filter.ValidateRequest(req.Context(), v.requestValidationInput); err != nil {
if ctx, ok := req.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
ctx.errors = append(ctx.errors, err)
}
Expand All @@ -127,7 +122,7 @@ func (v *OpenAPI) ValidateRequest(req *http.Request, key, origin string) error {
func (v *OpenAPI) ValidateResponse(beresp *http.Response) error {
// since a request validation could fail and ignored due to user options, the input route MAY be nil
if v.requestValidationInput == nil || v.requestValidationInput.Route == nil {
err := fmt.Errorf("response validation: '%s %s': invalid route", beresp.Request.Method, beresp.Request.URL.Path)
err := fmt.Errorf("'%s %s': invalid route", beresp.Request.Method, beresp.Request.URL.Path)
if beresp.Request != nil {
if ctx, ok := beresp.Request.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
ctx.errors = append(ctx.errors, err)
Expand Down Expand Up @@ -162,7 +157,6 @@ func (v *OpenAPI) ValidateResponse(beresp *http.Response) error {
}

if err := openapi3filter.ValidateResponse(beresp.Request.Context(), responseValidationInput); err != nil {
err = fmt.Errorf("response validation: %w", err)
if beresp.Request != nil {
if ctx, ok := beresp.Request.Context().Value(request.OpenAPI).(*OpenAPIContext); ok {
ctx.errors = append(ctx.errors, err)
Expand Down
33 changes: 12 additions & 21 deletions handler/validation/openapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestOpenAPIValidator_ValidateRequest(t *testing.T) {
}
}))

log, hook := logrustest.NewNullLogger()
log, hook := test.NewLogger()
logger := log.WithContext(context.Background())
beConf := &config.Backend{
Remain: body.New(&hcl.BodyContent{Attributes: hcl.Attributes{
Expand All @@ -69,10 +69,10 @@ func TestOpenAPIValidator_ValidateRequest(t *testing.T) {
wantBody bool
wantErrLog string
}{
{"GET without required query", "/a?b", nil, false, "request validation: Parameter 'b' in query has an error: must have a value: must have a value"},
{"GET without required query", "/a?b", nil, false, "backend validation error: Parameter 'b' in query has an error: must have a value: must have a value"},
{"GET with required query", "/a?b=value", nil, false, ""},
{"GET with required path", "/a/value", nil, false, ""},
{"GET with required path missing", "/a//", nil, false, "request validation: Parameter 'b' in query has an error: must have a value: must have a value"},
{"GET with required path missing", "/a//", nil, false, "backend validation error: Parameter 'b' in query has an error: must have a value: must have a value"},
{"GET with optional query", "/b", nil, false, ""},
{"GET with optional path param", "/b/a", nil, false, ""},
{"GET with required json body", "/json", strings.NewReader(`["hans", "wurst"]`), true, ""},
Expand All @@ -87,32 +87,23 @@ func TestOpenAPIValidator_ValidateRequest(t *testing.T) {
}

hook.Reset()
res, err := backend.RoundTrip(req)
var res *http.Response
res, err = backend.RoundTrip(req)
if err != nil && tt.wantErrLog == "" {
helper.Must(err)
t.Error(err)
return
}

if tt.wantErrLog != "" {
var found bool
for _, entry := range hook.Entries {
if valEntry, ok := entry.Data["validation"]; ok {
if list, ok := valEntry.([]string); ok {
for _, valMsg := range list {
if valMsg == tt.wantErrLog {
found = true
break
}
}
}
}
}
if !found {
t.Errorf("Expected error log: %q", tt.wantErrLog)
entry := hook.LastEntry()
if entry.Message != tt.wantErrLog {
t.Errorf("Expected error log:\nwant:\t%q\ngot:\t%s", tt.wantErrLog, entry.Message)
}
}

if tt.wantBody {
n, err := io.Copy(ioutil.Discard, res.Body)
var n int64
n, err = io.Copy(ioutil.Discard, res.Body)
if err != nil {
t.Error(err)
}
Expand Down

0 comments on commit 7ee1937

Please sign in to comment.