Repository: getkin/kin-openapi
Pull request #718 (2022-12-19 14:25:25)

feat: improve error reporting for bad/missing discriminator
First, the error messages themselves are better: they give the name of the discriminator property itself, which gives the reader a clue where the problem lies. And they give the invalid value when there is one.

Second, using SchemaError rather than errors.New() makes it possible for downstream interpreters to add more value to the error.


Patch:
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
index 8094763..e1648fc 100644
--- a/.github/workflows/go.yml
+++ b/.github/workflows/go.yml
@@ -105,7 +105,7 @@ jobs:
     - if: runner.os == 'Linux'
       name: Missing specification object link to definition
       run: |
-        [[ 30 -eq $(git grep -InE '^// See https:.+OpenAPI-Specification.+3[.]0[.]3[.]md#.+bject$' openapi3/*.go | grep -v _test.go | grep -v doc.go | wc -l) ]]
+        [[ 31 -eq $(git grep -InE '^// See https:.+OpenAPI-Specification.+3[.]0[.]3[.]md#.+bject$' openapi3/*.go | grep -v _test.go | grep -v doc.go | wc -l) ]]
 
     - if: runner.os == 'Linux'
       name: Style around ExtensionProps embedding
diff --git a/openapi2/openapi2.go b/openapi2/openapi2.go
index 430e398..4927ade 100644
--- a/openapi2/openapi2.go
+++ b/openapi2/openapi2.go
@@ -40,15 +40,13 @@ func (doc *T) UnmarshalJSON(data []byte) error {
 }
 
 func (doc *T) AddOperation(path string, method string, operation *Operation) {
-	paths := doc.Paths
-	if paths == nil {
-		paths = make(map[string]*PathItem)
-		doc.Paths = paths
+	if doc.Paths == nil {
+		doc.Paths = make(map[string]*PathItem)
 	}
-	pathItem := paths[path]
+	pathItem := doc.Paths[path]
 	if pathItem == nil {
 		pathItem = &PathItem{}
-		paths[path] = pathItem
+		doc.Paths[path] = pathItem
 	}
 	pathItem.SetOperation(method, operation)
 }
diff --git a/openapi2conv/openapi2_conv.go b/openapi2conv/openapi2_conv.go
index b81e3c2..989b685 100644
--- a/openapi2conv/openapi2_conv.go
+++ b/openapi2conv/openapi2_conv.go
@@ -1204,15 +1204,13 @@ func stripNonCustomExtensions(extensions map[string]interface{}) {
 }
 
 func addPathExtensions(doc2 *openapi2.T, path string, extensionProps openapi3.ExtensionProps) {
-	paths := doc2.Paths
-	if paths == nil {
-		paths = make(map[string]*openapi2.PathItem)
-		doc2.Paths = paths
+	if doc2.Paths == nil {
+		doc2.Paths = make(map[string]*openapi2.PathItem)
 	}
-	pathItem := paths[path]
+	pathItem := doc2.Paths[path]
 	if pathItem == nil {
 		pathItem = &openapi2.PathItem{}
-		paths[path] = pathItem
+		doc2.Paths[path] = pathItem
 	}
 	pathItem.ExtensionProps = extensionProps
 }
diff --git a/openapi3/internalize_refs.go b/openapi3/internalize_refs.go
index 6ff6b85..dcec43b 100644
--- a/openapi3/internalize_refs.go
+++ b/openapi3/internalize_refs.go
@@ -193,14 +193,11 @@ func (doc *T) addCallbackToSpec(c *CallbackRef, refNameResolver RefNameResolver,
 		return false
 	}
 	name := refNameResolver(c.Ref)
-	if _, ok := doc.Components.Callbacks[name]; ok {
-		c.Ref = "#/components/callbacks/" + name
-	}
 	if doc.Components.Callbacks == nil {
 		doc.Components.Callbacks = make(Callbacks)
 	}
-	doc.Components.Callbacks[name] = &CallbackRef{Value: c.Value}
 	c.Ref = "#/components/callbacks/" + name
+	doc.Components.Callbacks[name] = &CallbackRef{Value: c.Value}
 	return true
 }
 
diff --git a/openapi3/loader.go b/openapi3/loader.go
index 4cc83c0..ecc2ef2 100644
--- a/openapi3/loader.go
+++ b/openapi3/loader.go
@@ -281,12 +281,7 @@ func isSingleRefElement(ref string) bool {
 	return !strings.Contains(ref, "#")
 }
 
-func (loader *Loader) resolveComponent(
-	doc *T,
-	ref string,
-	path *url.URL,
-	resolved interface{},
-) (
+func (loader *Loader) resolveComponent(doc *T, ref string, path *url.URL, resolved interface{}) (
 	componentDoc *T,
 	componentPath *url.URL,
 	err error,
@@ -928,11 +923,10 @@ func (loader *Loader) resolveCallbackRef(doc *T, component *CallbackRef, documen
 					}
 					id := unescapeRefString(rest)
 
-					definitions := doc.Components.Callbacks
-					if definitions == nil {
+					if doc.Components.Callbacks == nil {
 						return failedToResolveRefFragmentPart(ref, "callbacks")
 					}
-					resolved := definitions[id]
+					resolved := doc.Components.Callbacks[id]
 					if resolved == nil {
 						return failedToResolveRefFragmentPart(ref, id)
 					}
@@ -1022,15 +1016,13 @@ func (loader *Loader) resolvePathItemRef(doc *T, entrypoint string, pathItem *Pa
 			}
 			id := unescapeRefString(rest)
 
-			definitions := doc.Paths
-			if definitions == nil {
+			if doc.Paths == nil {
 				return failedToResolveRefFragmentPart(ref, "paths")
 			}
-			resolved := definitions[id]
+			resolved := doc.Paths[id]
 			if resolved == nil {
 				return failedToResolveRefFragmentPart(ref, id)
 			}
-
 			*pathItem = *resolved
 		}
 	}
diff --git a/openapi3/openapi3.go b/openapi3/openapi3.go
index d9506a9..6622ef0 100644
--- a/openapi3/openapi3.go
+++ b/openapi3/openapi3.go
@@ -36,15 +36,13 @@ func (doc *T) UnmarshalJSON(data []byte) error {
 }
 
 func (doc *T) AddOperation(path string, method string, operation *Operation) {
-	paths := doc.Paths
-	if paths == nil {
-		paths = make(Paths)
-		doc.Paths = paths
+	if doc.Paths == nil {
+		doc.Paths = make(Paths)
 	}
-	pathItem := paths[path]
+	pathItem := doc.Paths[path]
 	if pathItem == nil {
 		pathItem = &PathItem{}
-		paths[path] = pathItem
+		doc.Paths[path] = pathItem
 	}
 	pathItem.SetOperation(method, operation)
 }
diff --git a/openapi3/paths.go b/openapi3/paths.go
index 2af59f2..0986b05 100644
--- a/openapi3/paths.go
+++ b/openapi3/paths.go
@@ -29,8 +29,8 @@ func (paths Paths) Validate(ctx context.Context, opts ...ValidationOption) error
 		}
 
 		if pathItem == nil {
-			paths[path] = &PathItem{}
-			pathItem = paths[path]
+			pathItem = &PathItem{}
+			paths[path] = pathItem
 		}
 
 		normalizedPath, _, varsInPath := normalizeTemplatedPath(path)
@@ -109,6 +109,37 @@ func (paths Paths) Validate(ctx context.Context, opts ...ValidationOption) error
 	return nil
 }
 
+// InMatchingOrder returns paths in the order they are matched against URLs.
+// See https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#paths-object
+// When matching URLs, concrete (non-templated) paths would be matched
+// before their templated counterparts.
+func (paths Paths) InMatchingOrder() []string {
+	// NOTE: sorting by number of variables ASC then by descending lexicographical
+	// order seems to be a good heuristic.
+	if paths == nil {
+		return nil
+	}
+
+	vars := make(map[int][]string)
+	max := 0
+	for path := range paths {
+		count := strings.Count(path, "}")
+		vars[count] = append(vars[count], path)
+		if count > max {
+			max = count
+		}
+	}
+
+	ordered := make([]string, 0, len(paths))
+	for c := 0; c <= max; c++ {
+		if ps, ok := vars[c]; ok {
+			sort.Sort(sort.Reverse(sort.StringSlice(ps)))
+			ordered = append(ordered, ps...)
+		}
+	}
+	return ordered
+}
+
 // Find returns a path that matches the key.
 //
 // The method ignores differences in template variable names (except possible "*" suffix).
diff --git a/openapi3/schema.go b/openapi3/schema.go
index f2b4ea2..20c1fa5 100644
--- a/openapi3/schema.go
+++ b/openapi3/schema.go
@@ -922,16 +922,35 @@ func (schema *Schema) visitSetOperations(settings *schemaValidationSettings, val
 			if valuemap, okcheck := value.(map[string]interface{}); okcheck {
 				discriminatorVal, okcheck := valuemap[pn]
 				if !okcheck {
-					return errors.New("input does not contain the discriminator property")
+					return &SchemaError{
+						Schema:      schema,
+						SchemaField: "discriminator",
+						Reason:      fmt.Sprintf("input does not contain the discriminator property %q", pn),
+					}
 				}
 
 				discriminatorValString, okcheck := discriminatorVal.(string)
 				if !okcheck {
-					return errors.New("descriminator value is not a string")
+					valStr := "null"
+					if discriminatorVal != nil {
+						valStr = fmt.Sprintf("%v", discriminatorVal)
+					}
+
+					return &SchemaError{
+						Value:       discriminatorVal,
+						Schema:      schema,
+						SchemaField: "discriminator",
+						Reason:      fmt.Sprintf("value of discriminator property %q is not a string: %v", pn, valStr),
+					}
 				}
 
 				if discriminatorRef, okcheck = schema.Discriminator.Mapping[discriminatorValString]; len(schema.Discriminator.Mapping) > 0 && !okcheck {
-					return errors.New("input does not contain a valid discriminator value")
+					return &SchemaError{
+						Value:       discriminatorVal,
+						Schema:      schema,
+						SchemaField: "discriminator",
+						Reason:      fmt.Sprintf("discriminator property %q has invalid value: %q", pn, discriminatorVal),
+					}
 				}
 			}
 		}
diff --git a/routers/gorillamux/router.go b/routers/gorillamux/router.go
index 6977808..bbf81ce 100644
--- a/routers/gorillamux/router.go
+++ b/routers/gorillamux/router.go
@@ -56,7 +56,7 @@ func NewRouter(doc *openapi3.T) (routers.Router, error) {
 
 	muxRouter := mux.NewRouter().UseEncodedPath()
 	r := &Router{}
-	for _, path := range orderedPaths(doc.Paths) {
+	for _, path := range doc.Paths.InMatchingOrder() {
 		pathItem := doc.Paths[path]
 		if len(pathItem.Servers) > 0 {
 			if servers, err = makeServers(pathItem.Servers); err != nil {
@@ -203,31 +203,6 @@ func newSrv(serverURL string, server *openapi3.Server, varsUpdater varsf) (srv,
 	return svr, nil
 }
 
-func orderedPaths(paths map[string]*openapi3.PathItem) []string {
-	// https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#pathsObject
-	// When matching URLs, concrete (non-templated) paths would be matched
-	// before their templated counterparts.
-	// NOTE: sorting by number of variables ASC then by descending lexicographical
-	// order seems to be a good heuristic.
-	vars := make(map[int][]string)
-	max := 0
-	for path := range paths {
-		count := strings.Count(path, "}")
-		vars[count] = append(vars[count], path)
-		if count > max {
-			max = count
-		}
-	}
-	ordered := make([]string, 0, len(paths))
-	for c := 0; c <= max; c++ {
-		if ps, ok := vars[c]; ok {
-			sort.Sort(sort.Reverse(sort.StringSlice(ps)))
-			ordered = append(ordered, ps...)
-		}
-	}
-	return ordered
-}
-
 // Magic strings that temporarily replace "{}" so net/url.Parse() works
 var blURL, brURL = strings.Repeat("-", 50), strings.Repeat("_", 50)
 


Test patch:
diff --git a/openapi3/schema_oneOf_test.go b/openapi3/schema_oneOf_test.go
index d3e689d..1a8ea81 100644
--- a/openapi3/schema_oneOf_test.go
+++ b/openapi3/schema_oneOf_test.go
@@ -86,7 +86,7 @@ func TestVisitJSON_OneOf_MissingDiscriptorProperty(t *testing.T) {
 	err = s.Components.Schemas["Animal"].Value.VisitJSON(map[string]interface{}{
 		"name": "snoopy",
 	})
-	require.EqualError(t, err, "input does not contain the discriminator property")
+	require.ErrorContains(t, err, "input does not contain the discriminator property \"$type\"\n")
 }
 
 func TestVisitJSON_OneOf_MissingDiscriptorValue(t *testing.T) {
@@ -96,7 +96,7 @@ func TestVisitJSON_OneOf_MissingDiscriptorValue(t *testing.T) {
 		"name":  "snoopy",
 		"$type": "snake",
 	})
-	require.EqualError(t, err, "input does not contain a valid discriminator value")
+	require.ErrorContains(t, err, "discriminator property \"$type\" has invalid value: \"snake\"")
 }
 
 func TestVisitJSON_OneOf_MissingField(t *testing.T) {
@@ -126,14 +126,14 @@ func TestVisitJSON_OneOf_BadDescriminatorType(t *testing.T) {
 		"scratches": true,
 		"$type":     1,
 	})
-	require.EqualError(t, err, "descriminator value is not a string")
+	require.ErrorContains(t, err, "value of discriminator property \"$type\" is not a string: 1")
 
 	err = s.Components.Schemas["Animal"].Value.VisitJSON(map[string]interface{}{
 		"name":  "snoopy",
 		"barks": true,
 		"$type": nil,
 	})
-	require.EqualError(t, err, "descriminator value is not a string")
+	require.ErrorContains(t, err, "value of discriminator property \"$type\" is not a string: null")
 }
 
 func TestVisitJSON_OneOf_Path(t *testing.T) {