Repository: getkin/kin-openapi
Pull request #217 (2020-06-05 07:17:07)

Add support for `allOf`/etc in validation error handler
Hi - Me again!

Follow-up to https://github.com/getkin/kin-openapi/pull/197

3 commits in here:
1) the current master build is broken, because JSONPointer impl/behavior was improved. just fix the build.
2) add support for errors from anyOf (and likely allOf/oneOf) in validation error handler
3) add explicit test for PATCH with required move from base schema to allOf overlay schema

The usage here is that we need to support `required` on list/get/create but not `patch`. So instead of having a schema like
```
components:
    schemas:
        MyObject1:
            type: object
            required:
            - name
            properties:
                name:
                    type: string
                enabled:
                    type: bool
```
you would want
```
components:
    schemas:
        MyObject1:
            type: object
            properties:
                name:
                    type: string
                enabled:
                    type: bool
        MyObject1RequiredProperties:
            type: object
            required:
            - name
```
and then instead of just using `$ref: "#/components/schemas/MyObject1"` you'd do something like
```
responses:
  200:
    content:
      application/json:
          schema:
             allOf:
             - $ref: "#/components/schemas/MyObject1"
             - $ref: "#/components/schemas/MyObject1RequiredProperties"
```


Patch:
diff --git a/openapi3/schema.go b/openapi3/schema.go
index f068097..427c376 100644
--- a/openapi3/schema.go
+++ b/openapi3/schema.go
@@ -1108,12 +1108,12 @@ func (schema *Schema) visitJSONObject(value map[string]interface{}, fast bool) (
 			if fast {
 				return errSchema
 			}
-			return &SchemaError{
+			return markSchemaErrorKey(&SchemaError{
 				Value:       value,
 				Schema:      schema,
 				SchemaField: "required",
 				Reason:      fmt.Sprintf("Property '%s' is missing", k),
-			}
+			}, k)
 		}
 	}
 	return
diff --git a/openapi3filter/fixtures/petstore.json b/openapi3filter/fixtures/petstore.json
index 00f4a92..6c229a6 100644
--- a/openapi3filter/fixtures/petstore.json
+++ b/openapi3filter/fixtures/petstore.json
@@ -66,10 +66,10 @@
           }
         ],
         "requestBody": {
-          "$ref": "#/components/requestBodies/Pet"
+          "$ref": "#/components/requestBodies/PetWithRequired"
         }
       },
-      "put": {
+      "patch": {
         "tags": [
           "pet"
         ],
@@ -100,6 +100,32 @@
         }
       }
     },
+    "/pet2": {
+      "post": {
+        "tags": [
+          "pet"
+        ],
+        "summary": "Add a new pet to the store",
+        "description": "",
+        "operationId": "addPet",
+        "responses": {
+          "405": {
+            "description": "Invalid input"
+          }
+        },
+        "security": [
+          {
+            "petstore_auth": [
+              "write:pets",
+              "read:pets"
+            ]
+          }
+        ],
+        "requestBody": {
+          "$ref": "#/components/requestBodies/PetAllOfRequiredProperties"
+        }
+      }
+    },
     "/pet/findByStatus": {
       "get": {
         "tags": [
@@ -137,7 +163,10 @@
                 "schema": {
                   "type": "array",
                   "items": {
-                    "$ref": "#/components/schemas/Pet"
+                    "allOf": [
+                      {"$ref": "#/components/schemas/Pet"},
+                      {"$ref": "#/components/schemas/PetRequiredProperties"}
+                    ]
                   }
                 }
               },
@@ -145,7 +174,10 @@
                 "schema": {
                   "type": "array",
                   "items": {
-                    "$ref": "#/components/schemas/Pet"
+                    "allOf": [
+                      {"$ref": "#/components/schemas/Pet"},
+                      {"$ref": "#/components/schemas/PetRequiredProperties"}
+                    ]
                   }
                 }
               }
@@ -1092,6 +1124,62 @@
         }
       },
       "Pet": {
+        "type": "object",
+        "properties": {
+          "id": {
+            "type": "integer",
+            "format": "int64",
+            "readOnly": true
+          },
+          "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"
+        }
+      },
+      "PetRequiredProperties": {
+        "type": "object",
+        "required": [
+          "name",
+          "photoUrls"
+        ]
+      },
+      "PetWithRequired": {
         "type": "object",
         "required": [
           "name",
@@ -1108,7 +1196,7 @@
           },
           "name": {
             "type": "string",
-            "example": "doggie"
+            "example": "doggie",
           },
           "photoUrls": {
             "type": "array",
@@ -1146,6 +1234,35 @@
       }
     },
     "requestBodies": {
+      "PetAllOfRequiredProperties": {
+        "content": {
+          "application/json": {
+            "schema": {
+              "allOf": [
+                {
+                  "$ref": "#/components/schemas/Pet"
+                },
+                {
+                  "$ref": "#/components/schemas/PetRequiredProperties"
+                }
+              ]
+            }
+          },
+          "application/xml": {
+            "schema": {
+              "allOf": [
+                {
+                  "$ref": "#/components/schemas/Pet"
+                },
+                {
+                  "$ref": "#/components/schemas/PetRequiredProperties"
+                }
+              ]
+            }
+          }
+        },
+        "required": true
+      },
       "Pet": {
         "content": {
           "application/json": {
@@ -1162,6 +1279,22 @@
         "description": "Pet object that needs to be added to the store",
         "required": true
       },
+      "PetWithRequired": {
+        "content": {
+          "application/json": {
+            "schema": {
+              "$ref": "#/components/schemas/PetWithRequired"
+            }
+          },
+          "application/xml": {
+            "schema": {
+              "$ref": "#/components/schemas/PetWithRequired"
+            }
+          }
+        },
+        "description": "Pet object that needs to be added to the store",
+        "required": true
+      },
       "UserArray": {
         "content": {
           "application/json": {
@@ -1197,4 +1330,4 @@
       }
     }
   }
-}
+}
\ No newline at end of file
diff --git a/openapi3filter/validation_error_encoder.go b/openapi3filter/validation_error_encoder.go
index 556bcc8..49459ef 100644
--- a/openapi3filter/validation_error_encoder.go
+++ b/openapi3filter/validation_error_encoder.go
@@ -135,6 +135,11 @@ var propertyMissingNameRE = regexp.MustCompile(`Property '(?P<name>[^']*)' is mi
 func convertSchemaError(e *RequestError, innerErr *openapi3.SchemaError) *ValidationError {
 	cErr := &ValidationError{Title: innerErr.Reason}
 
+	// Handle "Origin" error
+	if originErr, ok := innerErr.Origin.(*openapi3.SchemaError); ok {
+		cErr = convertSchemaError(e, originErr)
+	}
+
 	// Add http status code
 	if e.Parameter != nil {
 		cErr.Status = http.StatusBadRequest
@@ -152,16 +157,6 @@ func convertSchemaError(e *RequestError, innerErr *openapi3.SchemaError) *Valida
 	} else if innerErr.JSONPointer() != nil {
 		pointer := innerErr.JSONPointer()
 
-		// JSONPointer is rarely what you expect.
-		// 1. for "property is missing" errors, its an empty array
-		// 2. for nested attributes, it leaves off the last element from the JSONPointer
-		matches := propertyMissingNameRE.FindStringSubmatch(innerErr.Reason)
-		if matches != nil && len(matches) > 1 {
-			if len(pointer) == 0 || matches[1] != pointer[0] {
-				pointer = append(pointer, matches[1])
-			}
-		}
-
 		cErr.Source = &ValidationErrorSource{
 			Pointer: toJSONPointer(pointer),
 		}


Test patch:
diff --git a/openapi3filter/validation_error_test.go b/openapi3filter/validation_error_test.go
index 74e5a55..420831d 100644
--- a/openapi3filter/validation_error_test.go
+++ b/openapi3filter/validation_error_test.go
@@ -34,21 +34,24 @@ type validationArgs struct {
 	r *http.Request
 }
 type validationTest struct {
-	name                string
-	fields              validationFields
-	args                validationArgs
-	wantErr             bool
-	wantErrBody         string
-	wantErrReason       string
-	wantErrSchemaReason string
-	wantErrSchemaPath   string
-	wantErrSchemaValue  interface{}
-	wantErrParam        string
-	wantErrParamIn      string
-	wantErrParseKind    ParseErrorKind
-	wantErrParseValue   interface{}
-	wantErrParseReason  string
-	wantErrResponse     *ValidationError
+	name                      string
+	fields                    validationFields
+	args                      validationArgs
+	wantErr                   bool
+	wantErrBody               string
+	wantErrReason             string
+	wantErrSchemaReason       string
+	wantErrSchemaPath         string
+	wantErrSchemaValue        interface{}
+	wantErrSchemaOriginReason string
+	wantErrSchemaOriginPath   string
+	wantErrSchemaOriginValue  interface{}
+	wantErrParam              string
+	wantErrParamIn            string
+	wantErrParseKind          ParseErrorKind
+	wantErrParseValue         interface{}
+	wantErrParseReason        string
+	wantErrResponse           *ValidationError
 }
 
 func getValidationTests(t *testing.T) []*validationTest {
@@ -239,9 +242,9 @@ func getValidationTests(t *testing.T) []*validationTest {
 			wantErrSchemaPath:   "/1",
 			wantErrSchemaValue:  "watdis",
 			wantErrResponse: &ValidationError{Status: http.StatusBadRequest,
-				Title: "JSON value is not one of the allowed values",
+				Title:  "JSON value is not one of the allowed values",
 				Detail: "Value 'watdis' at /1 must be one of: available, pending, sold",
-				Source:  &ValidationErrorSource{Parameter: "status"}},
+				Source: &ValidationErrorSource{Parameter: "status"}},
 		},
 		{
 			name: "error - invalid enum value, allowing commas (without 'perhaps you intended' recommendation)",
@@ -255,9 +258,9 @@ func getValidationTests(t *testing.T) []*validationTest {
 			wantErrSchemaPath:   "/1",
 			wantErrSchemaValue:  "fish,with,commas",
 			wantErrResponse: &ValidationError{Status: http.StatusBadRequest,
-				Title: "JSON value is not one of the allowed values",
+				Title:  "JSON value is not one of the allowed values",
 				Detail: "Value 'fish,with,commas' at /1 must be one of: dog, cat, turtle, bird,with,commas",
-					// No 'perhaps you intended' because its the right serialization format
+				// No 'perhaps you intended' because its the right serialization format
 				Source: &ValidationErrorSource{Parameter: "kind"}},
 		},
 		{
@@ -279,10 +282,10 @@ func getValidationTests(t *testing.T) []*validationTest {
 			wantErrReason:       "doesn't match the schema",
 			wantErrSchemaReason: "Property 'photoUrls' is missing",
 			wantErrSchemaValue:  map[string]string{"name": "Bahama"},
-			wantErrSchemaPath:   "/",
+			wantErrSchemaPath:   "/photoUrls",
 			wantErrResponse: &ValidationError{Status: http.StatusUnprocessableEntity,
-				Title: "Property 'photoUrls' is missing",
-				Source:  &ValidationErrorSource{Pointer: "/photoUrls"}},
+				Title:  "Property 'photoUrls' is missing",
+				Source: &ValidationErrorSource{Pointer: "/photoUrls"}},
 		},
 		{
 			name: "error - missing required nested object attribute",
@@ -293,10 +296,10 @@ func getValidationTests(t *testing.T) []*validationTest {
 			wantErrReason:       "doesn't match the schema",
 			wantErrSchemaReason: "Property 'name' is missing",
 			wantErrSchemaValue:  map[string]string{},
-			wantErrSchemaPath:   "/category",
+			wantErrSchemaPath:   "/category/name",
 			wantErrResponse: &ValidationError{Status: http.StatusUnprocessableEntity,
-				Title: "Property 'name' is missing",
-				Source:  &ValidationErrorSource{Pointer: "/category/name"}},
+				Title:  "Property 'name' is missing",
+				Source: &ValidationErrorSource{Pointer: "/category/name"}},
 		},
 		{
 			name: "error - missing required deeply nested object attribute",
@@ -307,10 +310,10 @@ func getValidationTests(t *testing.T) []*validationTest {
 			wantErrReason:       "doesn't match the schema",
 			wantErrSchemaReason: "Property 'name' is missing",
 			wantErrSchemaValue:  map[string]string{},
-			wantErrSchemaPath:   "/category/tags/0",
+			wantErrSchemaPath:   "/category/tags/0/name",
 			wantErrResponse: &ValidationError{Status: http.StatusUnprocessableEntity,
-				Title: "Property 'name' is missing",
-				Source:  &ValidationErrorSource{Pointer: "/category/tags/0/name"}},
+				Title:  "Property 'name' is missing",
+				Source: &ValidationErrorSource{Pointer: "/category/tags/0/name"}},
 		},
 		{
 			// TODO: Add support for validating readonly properties to upstream validator.
@@ -334,8 +337,23 @@ func getValidationTests(t *testing.T) []*validationTest {
 			// TODO: this shouldn't say "or not be present", but this requires recursively resolving
 			//  innerErr.JSONPointer() against e.RequestBody.Content["application/json"].Schema.Value (.Required, .Properties)
 			wantErrResponse: &ValidationError{Status: http.StatusUnprocessableEntity,
-				Title: "Field must be set to array or not be present",
-				Source:  &ValidationErrorSource{Pointer: "/photoUrls"}},
+				Title:  "Field must be set to array or not be present",
+				Source: &ValidationErrorSource{Pointer: "/photoUrls"}},
+		},
+		{
+			name: "error - missing required object attribute from allOf required overlay",
+			args: validationArgs{
+				r: newPetstoreRequest(t, http.MethodPost, "/pet2", bytes.NewBufferString(`{"name":"Bahama"}`)),
+			},
+			wantErrReason:             "doesn't match the schema",
+			wantErrSchemaPath:         "/",
+			wantErrSchemaValue:        map[string]string{"name": "Bahama"},
+			wantErrSchemaOriginReason: "Property 'photoUrls' is missing",
+			wantErrSchemaOriginValue:  map[string]string{"name": "Bahama"},
+			wantErrSchemaOriginPath:   "/photoUrls",
+			wantErrResponse: &ValidationError{Status: http.StatusUnprocessableEntity,
+				Title:  "Property 'photoUrls' is missing",
+				Source: &ValidationErrorSource{Pointer: "/photoUrls"}},
 		},
 		{
 			name: "success - ignores unknown object attribute",
@@ -351,6 +369,12 @@ func getValidationTests(t *testing.T) []*validationTest {
 					bytes.NewBufferString(`{"name":"Bahama","photoUrls":[]}`)),
 			},
 		},
+		{
+			name: "success - required properties are not required on PATCH if required overlaid using allOf elsewhere",
+			args: validationArgs{
+				r: newPetstoreRequest(t, http.MethodPatch, "/pet", bytes.NewBufferString(`{}`)),
+			},
+		},
 
 		//
 		// Path params
@@ -429,9 +453,18 @@ func TestValidationHandler_validateRequest(t *testing.T) {
 					pointer := toJSONPointer(innerErr.JSONPointer())
 					req.Equal(tt.wantErrSchemaPath, pointer)
 					req.Equal(fmt.Sprintf("%v", tt.wantErrSchemaValue), fmt.Sprintf("%v", innerErr.Value))
+
+					if originErr, ok := innerErr.Origin.(*openapi3.SchemaError); ok {
+						req.Equal(tt.wantErrSchemaOriginReason, originErr.Reason)
+						pointer := toJSONPointer(originErr.JSONPointer())
+						req.Equal(tt.wantErrSchemaOriginPath, pointer)
+						req.Equal(fmt.Sprintf("%v", tt.wantErrSchemaOriginValue), fmt.Sprintf("%v", originErr.Value))
+					}
 				} else {
 					req.False(tt.wantErrSchemaReason != "" || tt.wantErrSchemaPath != "",
 						"error = %v, not a SchemaError -- %#v", e.Err, e.Err)
+					req.False(tt.wantErrSchemaOriginReason != "" || tt.wantErrSchemaOriginPath != "",
+						"error = %v, not a SchemaError with Origin -- %#v", e.Err, e.Err)
 				}
 
 				if innerErr, ok := e.Err.(*ParseError); ok {