Add support for error aggregation for request/response validation

[zlozano]

Here is a draft which shows how an aggregate error might look. There are some places where we still short circuit the evaluation. For example, if there is a decoding error, then it makes no sense to continue. While validating decoded parameters though, we can proceed to evaluate most of the rules applied. For example, for JSON bodies, we can continue to evaluate other properties event if the first field fails validation.

This allows users to iterate through the errors, do some type juggling to get the underlying RequestError or SchemaError and either audit bad input, or communicate a complete/detailed list of errors upstream.

I've included this in the non fast path. I couldn't think of where a dedicated private param would fit into this scenario.

I still need to write tests + do the same for response validation. Mostly wanting to get feedback on the approach + any corner cases I am not accounting for.

[fenollp]

This looks alright so far. We need to find a way to preserve the non-MultiError "fail fast" behavior though.

[zlozano]

Sounds good. I've pushed a new commit proposing how we can preserve the fail fast behavior.

I'm not sold 100% on this idea, but it seemed like a good start.An alternative that you raised in the issue would be to pass a flag using context.Context. Based on the go documentation, I'm not sure this is the best fit, but I will defer to the maintainers on this. Specifically, I am referencing:

Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.

Another alternative, is that this could be an internal value set on the Schema struct on initialization, but it seems like validation handling happens at a couple of different layers, the openapi3filter.Validate* package level functions and the schema Visit* methods. Having a single point to configure this functionality seems to be the most ergonomic for developers which is how I landed on the approach in my most recent commit.

This is still a WIP for demonstration + iterating on some ideas. I still need to handle the remaining visit functions, tests, response validation.

Also, i'd love to hear suggestions on function/param naming if we want to roll with the proposed solution. Thanks!

[fenollp]

I think

func (schema *Schema) VisitJSON(value interface{}) error {
...
func (schema *Schema) VisitAllJSON(value interface{}) error {

should actually be just

func (schema *Schema) VisitJSON(value interface{}, ...VisitSchemaOption) error {

which I actually did in https://github.com/getkin/kin-openapi/pull/246/files but apparently forgot to merge....

Anyhow, about the fast/failFast thing in schema.go: I believe your flag is about returning multiple errors so it's name should be more like "multiErrors".

I'll finish up on #246 and merge it and I suggest you add a

func MultiErrors() SchemaValidationOption {
	return func(s *schemaValidationSettings) { s.multierrors = true }
}

and work with that.

Sounds good?

[zlozano]

Sounds good to me. I'll get working on a more complete PR with tests. Thanks for your input!

[richard-rance]

I'm glad to see work on this taking place. It is a feature that I have been exploring as well.

[richard-rance]

Suggested change

	type MultiError []error
	type MultiError []error
	//Is allows you to determine if a generic error is in fact a MultiError using `errors.Is()`
	//It will also return true if any of the contained errors match target
	func (me MultiError) Is(target error) bool {
	if _, ok := target.(MultiError); ok {
	return true
	}
	for _, e := range me {
	if errors.Is(e, target) {
	return true
	}
	}
	return false
	}
	//As allows you to use `errors.As()` to set target to the first error within the multi error that matches the target type
	func (me MultiError) As(target interface{}) bool {
	for _, e := range me {
	if errors.As(e, target) {
	return true
	}
	}
	return false
	}
	// StatusCode Calculates the most specific http status code for the list of potential error codes
	func (me MultiError) StatusCode() int {
	code4xx := 0
	code5xx := 0
	for _, err := range me {
	code := 0
	if sc, ok := err.(StatusCoder); ok {
	code = sc.StatusCode()
	}
	if code != 0 {
	if 400 >= code && code <=499 {
	if code4xx == 0 {
	code4xx = code
	} else if code4xx != code {
	code4xx = 400
	}
	} else if 500 >= code && code <=599 {
	if code5xx == 0 {
	code5xx = code
	} else if code5xx != code {
	code5xx = 500
	}
	}
	}
	}
	if code5xx != 0 {
	return code5xx
	}
	return code4xx
	}

[zlozano]

Thanks for the input! I am about to push up the rest of my changes, and I will add this in.

[zlozano]

I added the Is and the As.

The StatusCoder is defined in the openapi3filter package, so including here would introduce an import cycle. I think this is fine for now since the rules used above for calculating the status code are not universal. Also, it looks like the StatusCoder is only implemented by the ValidationError.

I'm not super familiar with the ValidationError/ErrorEncoder, so I am not quite sure how it would look, but I think what would be needed is a way to convert a MultiError into a ValidationError (similar to what is done for SchemaErrors and RequestErrors. It seems like this work could be done separately/as a follow up to this since the error will get passed-through to the encoder, allowing the end user to control this behavior. Related to my previous comment, it's not clear how exactly we would calculate error precedence if there are many errors. So this may be the best course of action for now. I'll defer to those who have more experience with using this feature.

[richard-rance]

Given go's preference for interfaces being defined where they are consumed it would have been acceptable to redefine StatusCoder within this package or use an anonymous interface like some of the core packages. Ex: if sc, ok := err.(interface{StatusCode() string}); ok {

It is fine to leave the StatusCode function out for now. I actually had it as a standalone function in the proof of concept that I was working on.

I agree we should have a separate discussion about conversion to ValidationError. I had a hard time implementing my own ErrorEncoder.

[fenollp]

Most comments I have are the same one repeated so you should be able to go through them pretty quickly.

[crodwell]

@fenollp @zlozano - This looks great but correct me if I'm wrong, it doesn't look like you can unwrap multiple request validation errors?

for example if I have 2 invalid request fields in my request

err := openapi3filter.ValidateRequest(c, &openapi3filter.RequestValidationInput{
		Request:    c.Request,
		PathParams: pathParams,
		Route:      route,
		Options:    &openapi3filter.Options{MultiError: true, ExcludeResponseBody: true, IncludeResponseStatus: false},
	}); err != nil {
        spew.Dump(err)  
/*
(openapi3.MultiError) (len=1 cap=1) request body has an error: doesn't match the schema: Error at "/start_date": string doesn't match the format "date-time" (regular expression "^[0-9]{4}-(0[0-9]|10|11|12)-([0-2][0-9]|30|31)T[0-9]{2}:[0-9]{2}:[0-9]{2}(.[0-9]+)?(Z|(\\+|-)[0-9]{2}:[0-9]{2})?$")
Schema:
  {
    "format": "date-time",
    "type": "string"
  }

Value:
  "abc"
 | Error at "/currency_code": Field must be set to string or not be present
Schema:
  {
    "maxLength": 3,
    "type": "string"
  }

Value:
  "number, integer"
 |  |  
 */
	spew.Dump(errors.Unwrap(err))  
/*
(interface {}) <nil>
*/
}

I can't find any easy way to separate the errors, any ideas?

[zlozano]

@crodwell it is quite possible I missed a use case, but I believe we accomplish what you are doing with some type juggling. Here is an example function we use to unpack the errors into a flat object we can then marshal and write back:

const (
	prefixBody = "@body"
	unknown    = "@unknown"
)

func convertError(me openapi3.MultiError) map[string][]string {
	issues := make(map[string][]string)
	for _, err := range me {
		switch err := err.(type) {
		case *openapi3.SchemaError:
			// Can inspect schema validation errors here, e.g. err.Value
			field := prefixBody
			if path := err.JSONPointer(); len(path) > 0 {
				field = fmt.Sprintf("%s.%s", field, strings.Join(path, "."))
			}
			if _, ok := issues[field]; !ok {
				issues[field] = make([]string, 0, 3)
			}
			issues[field] = append(issues[field], err.Error())
		case *openapi3filter.RequestError: // possible there were multiple issues that failed validation
			if err, ok := err.Err.(openapi3.MultiError); ok {
				for k, v := range convertError(err) {
					if _, ok := issues[k]; !ok {
						issues[k] = make([]string, 0, 3)
					}
					issues[k] = append(issues[k], v...)
				}
				continue
			}

			// check if invalid HTTP parameter
			if err.Parameter != nil {
				prefix := err.Parameter.In
				name := fmt.Sprintf("%s.%s", prefix, err.Parameter.Name)
				if _, ok := issues[name]; !ok {
					issues[name] = make([]string, 0, 3)
				}
				issues[name] = append(issues[name], err.Error())
				continue
			}

			// check if requestBody
			if err.RequestBody != nil {
				if _, ok := issues[prefixBody]; !ok {
					issues[prefixBody] = make([]string, 0, 3)
				}
				issues[prefixBody] = append(issues[prefixBody], err.Error())
				continue
			}
		default:
			reasons, ok := issues[unknown]
			if !ok {
				reasons = make([]string, 0, 3)
			}
			reasons = append(reasons, err.Error())
			issues[unknown] = reasons
		}
	}
	return issues
}

here is a sample main to illustrate:

func main() {
	swagger, err := openapi3.NewSwaggerLoader().LoadSwaggerFromFile("./petstore.yaml")
	if err != nil {
		log.Fatalf("error loading spec %s", err.Error())
	}

	router := openapi3filter.NewRouter()
	if err := router.AddSwagger(swagger); err != nil {
		log.Fatalf("error adding spec %s", err.Error())
	}

	mux := http.NewServeMux()
	mux.Handle("/pet", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		route, pathParams, err := router.FindRoute(r.Method, r.URL)
		if err != nil {
			fmt.Println(err.Error())
			w.WriteHeader(http.StatusInternalServerError)
			return
		}

		err = openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{
			Request:    r,
			PathParams: pathParams,
			Route:      route,
			Options: &openapi3filter.Options{
				MultiError: true,
			},
		})

		switch err := err.(type) {
		case nil:
		case openapi3.MultiError:
			issues := convertError(err)
			for k, msgs := range issues {
				fmt.Println("===== Start New Error =====")
				fmt.Println(k + ":")
				for _, msg := range msgs {
					fmt.Printf("\t%s\n", msg)
				}
			}
		default:
			fmt.Println(err.Error())
		}
	}))

	log.Fatal(http.ListenAndServe(":8282", mux))
}

example petstore.yaml:

 openapi: "3.0.0"
info:
  description: "This is a sample server Petstore server.  You can find out more about     Swagger at [http://swagger.io](http://swagger.io) or on [irc.freenode.net, #swagger](http://swagger.io/irc/).      For this sample, you can use the api key `special-key` to test the authorization     filters."
  version: "1.0.0"
  title: "Swagger Petstore"
  termsOfService: "http://swagger.io/terms/"
  contact:
    email: "apiteam@swagger.io"
  license:
    name: "Apache 2.0"
    url: "http://www.apache.org/licenses/LICENSE-2.0.html"
tags:
- name: "pet"
  description: "Everything about your Pets"
  externalDocs:
    description: "Find out more"
    url: "http://swagger.io"
- name: "store"
  description: "Access to Petstore orders"
- name: "user"
  description: "Operations about user"
  externalDocs:
    description: "Find out more about our store"
    url: "http://swagger.io"
paths:
  /pet:
    post:
      tags:
      - "pet"
      summary: "Add a new pet to the store"
      description: ""
      operationId: "addPet"
      requestBody:
        required: true
        content: 
          'application/json':
            schema:
              $ref: '#/components/schemas/Pet'
      responses:
        "405":
          description: "Invalid input"
components:
  schemas:
    Category:
      type: "object"
      properties:
        id:
          type: "integer"
          format: "int64"
        name:
          type: "string"
      xml:
        name: "Category"
    Tag:
      type: "object"
      properties:
        id:
          type: "integer"
          format: "int64"
        name:
          type: "string"
      xml:
        name: "Tag"
    Pet:
      type: "object"
      required:
      - "name"
      - "photoUrls"
      properties:
        id:
          type: "integer"
          format: "int64"
        category:
          $ref: "#/components/schemas/Category"
        name:
          type: "string"
          example: "doggie"
        photoUrls:
          type: "array"
          xml:
            name: "photoUrl"
            wrapped: true
          items:
            type: "string"
        tags:
          type: "array"
          xml:
            name: "tag"
            wrapped: true
          items:
            $ref: "#/components/schemas/Tag"
        status:
          type: "string"
          description: "pet status in the store"
          enum:
          - "available"
          - "pending"
          - "sold"
      xml:
        name: "Pet"

when I curl: curl -v -XPOST "http://localhost:8282/pet" -d '{"name": 100, "photoUrls": [], "status": "invalidStatus"}' --header "Content-Type:application/json"
(note invalid type for name and invalid status)

I receive two errors:

===== Start New Error =====
@body.name:
	Error at "/name":Field must be set to string or not be present
Schema:
  {
    "example": "doggie",
    "type": "string"
  }

Value:
  "number, integer"

===== Start New Error =====
@body.status:
	Error at "/status":JSON value is not one of the allowed values
Schema:
  {
    "description": "pet status in the store",
    "enum": [
      "available",
      "pending",
      "sold"
    ],
    "type": "string"
  }

Value:
  "invalidStatus"

I think this is what you are looking for. Please let me know if I have missed something. We are currently on 0.26.0 so some of the openapi packages may have changed in my example. The type juggling is unfortunate, but at the time this was written I was attempting to work within the existing type system defined by the openapi3 packages. The multi error type just aggregates the existing errors. You'll need to do some type assertions to get the data you are looking for.

[crodwell]

Hey @zlozano . Thanks so much for this! You've helped me get unstuck; I wanted a slice of errors, so that I can transform validation problems into user friendly JSON. I changed your convertError() function to return a map[string][]error instead and now I can iterate over each field that has an (*openapi3.SchemaError) and display the bits I need for the user to rectify.