internal/mcp/jsonschema: clearer validations errors

Validation errors now more clearly indicate the path from the root
schema.

- Enhance schema iteration to yield the path, and generalize it by using
  reflection.

- Store each schema's path inside the schema.

- Remove the path arg from validate: we don't need it anymore.

- Improve the test for the iterators and add one for validation
  errors.

Change-Id: Ief55133ef4ae2eac0736208a3b9f78ed69933b31
Reviewed-on: https://go-review.googlesource.com/c/tools/+/676375
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Jonathan Amsterdam <jba@google.com>

jsonschema/json_pointer.go

@@ -28,7 +28,18 @@ import (
 	"strings"
 )
 
-var jsonPointerReplacer = strings.NewReplacer("~0", "~", "~1", "/")
+var (
+	jsonPointerEscaper   = strings.NewReplacer("~", "~0", "/", "~1")
+	jsonPointerUnescaper = strings.NewReplacer("~0", "~", "~1", "/")
+)
+
+func escapeJSONPointerSegment(s string) string {
+	return jsonPointerEscaper.Replace(s)
+}
+
+func unescapeJSONPointerSegment(s string) string {
+	return jsonPointerUnescaper.Replace(s)
+}
 
 // parseJSONPointer splits a JSON Pointer into a sequence of segments. It doesn't
 // convert strings to numbers, because that depends on the traversal: a segment
@@ -47,7 +58,7 @@ func parseJSONPointer(ptr string) (segments []string, err error) {
 	if strings.Contains(ptr, "~") {
 		// Undo the simple escaping rules that allow one to include a slash in a segment.
 		for i := range segments {
-			segments[i] = jsonPointerReplacer.Replace(segments[i])
+			segments[i] = unescapeJSONPointerSegment(segments[i])
 		}
 	}
 	return segments, nil
@@ -121,17 +132,6 @@ func dereferenceJSONPointer(s *Schema, sptr string) (_ *Schema, err error) {
 	return nil, fmt.Errorf("does not refer to a schema, but to a %s", v.Type())
 }
 
-// map from JSON names for fields in a Schema to their indexes in the struct.
-var schemaFields = map[string][]int{}
-
-func init() {
-	for _, f := range reflect.VisibleFields(reflect.TypeFor[Schema]()) {
-		if name, ok := jsonName(f); ok {
-			schemaFields[name] = f.Index
-		}
-	}
-}
-
 // lookupSchemaField returns the value of the field with the given name in v,
 // or the zero value if there is no such field or it is not of type Schema or *Schema.
 func lookupSchemaField(v reflect.Value, name string) reflect.Value {
@@ -143,8 +143,8 @@ func lookupSchemaField(v reflect.Value, name string) reflect.Value {
 		}
 		return v.FieldByName("Types")
 	}
-	if index := schemaFields[name]; index != nil {
+	if sf, ok := schemaFieldMap[name]; ok {
-		return v.FieldByIndex(index)
+		return v.FieldByIndex(sf.Index)
 	}
 	return reflect.Value{}
 }

jsonschema/resolve.go

@@ -118,7 +118,7 @@ func (root *Schema) check() error {
 	report := func(err error) { errs = append(errs, err) }
 
 	seen := map[*Schema]bool{}
-	for ss := range root.all() {
+	for ss, path := range root.all() {
 		if seen[ss] {
 			// The schema graph rooted at s is not a tree, but it needs to
 			// be because we assume a unique parent when we store a schema's base
@@ -127,6 +127,11 @@ func (root *Schema) check() error {
 			return fmt.Errorf("schemas rooted at %s do not form a tree (saw %s twice)", root, ss)
 		}
 		seen[ss] = true
+		if len(path) == 0 {
+			ss.path = "root"
+		} else {
+			ss.path = "/" + strings.Join(path, "/")
+		}
 		ss.checkLocal(report)
 	}
 	return errors.Join(errs...)
@@ -138,7 +143,8 @@ func (root *Schema) check() error {
 // It appends the errors it finds to errs.
 func (s *Schema) checkLocal(report func(error)) {
 	addf := func(format string, args ...any) {
-		report(fmt.Errorf("jsonschema.Schema: "+format, args...))
+		msg := fmt.Sprintf(format, args...)
+		report(fmt.Errorf("jsonschema.Schema: %s: %s", s.path, msg))
 	}
 
 	if s == nil {
@@ -165,7 +171,7 @@ func (s *Schema) checkLocal(report func(error)) {
 	if s.Pattern != "" {
 		re, err := regexp.Compile(s.Pattern)
 		if err != nil {
-			addf("pattern: %w", err)
+			addf("pattern: %v", err)
 		} else {
 			s.pattern = re
 		}
@@ -175,7 +181,7 @@ func (s *Schema) checkLocal(report func(error)) {
 		for reString, subschema := range s.PatternProperties {
 			re, err := regexp.Compile(reString)
 			if err != nil {
-				addf("patternProperties[%q]: %w", reString, err)
+				addf("patternProperties[%q]: %v", reString, err)
 				continue
 			}
 			s.patternProperties[re] = subschema

jsonschema/schema.go

@@ -11,9 +11,13 @@ import (
 	"errors"
 	"fmt"
 	"iter"
+	"maps"
 	"math"
 	"net/url"
+	"reflect"
 	"regexp"
+	"slices"
+	"strconv"
 )
 
 // A Schema is a JSON schema object.
@@ -139,6 +143,10 @@ type Schema struct {
 	//   s.base == s <=> s.uri != nil
 	uri *url.URL
 
+	// The JSON Pointer path from the root schema to here.
+	// Used in errors.
+	path string
+
 	// The schema to which Ref refers.
 	resolvedRef *Schema
 
@@ -180,7 +188,9 @@ func (s *Schema) String() string {
 	if a := cmp.Or(s.Anchor, s.DynamicAnchor); a != "" {
 		return fmt.Sprintf("%q, anchor %s", s.base.uri.String(), a)
 	}
-	// TODO: return something better, like a JSON Pointer from the base.
+	if s.path != "" {
+		return s.path
+	}
 	return "<anonymous schema>"
 }
 
@@ -192,15 +202,6 @@ func (s *Schema) ResolvedRef() *Schema {
 	return s.resolvedRef
 }
 
-// json returns the schema in json format.
-func (s *Schema) json() string {
-	data, err := json.MarshalIndent(s, "", "  ")
-	if err != nil {
-		return fmt.Sprintf("<jsonschema.Schema:%v>", err)
-	}
-	return string(data)
-}
-
 func (s *Schema) basicChecks() error {
 	if s.Type != "" && s.Types != nil {
 		return errors.New("both Type and Types are set; at most one should be")
@@ -358,55 +359,96 @@ func (ip *integer) UnmarshalJSON(data []byte) error {
 func Ptr[T any](x T) *T { return &x }
 
 // every applies f preorder to every schema under s including s.
+// The second argument to f is the path to the schema appended to the argument path.
 // It stops when f returns false.
-func (s *Schema) every(f func(*Schema) bool) bool {
+func (s *Schema) every(f func(*Schema, []string) bool, path []string) bool {
 	return s == nil ||
-		f(s) && s.everyChild(func(s *Schema) bool { return s.every(f) })
+		f(s, path) && s.everyChild(func(s *Schema, p []string) bool { return s.every(f, p) }, path)
 }
 
 // everyChild reports whether f is true for every immediate child schema of s.
+// The second argument to f is the path to the schema appended to the argument path.
 //
 // It does not call f on nil-valued fields holding individual schemas, like Contains,
 // because a nil value indicates that the field is absent.
 // It does call f on nils when they occur in slices and maps, so those invalid values
 // can be detected when the schema is validated.
-func (s *Schema) everyChild(f func(*Schema) bool) bool {
+func (s *Schema) everyChild(f func(*Schema, []string) bool, path []string) bool {
-	// Fields that contain individual schemas. A nil is valid: it just means the field isn't present.
+	if s == nil {
-	for _, c := range []*Schema{
+		return false
-		s.Items, s.AdditionalItems, s.Contains, s.PropertyNames, s.AdditionalProperties,
+	}
-		s.If, s.Then, s.Else, s.Not, s.UnevaluatedItems, s.UnevaluatedProperties,
+	var (
-	} {
+		schemaType      = reflect.TypeFor[*Schema]()
-		if c != nil && !f(c) {
+		schemaSliceType = reflect.TypeFor[[]*Schema]()
+		schemaMapType   = reflect.TypeFor[map[string]*Schema]()
+	)
+	v := reflect.ValueOf(s)
+	for _, info := range schemaFieldInfos {
+		fv := v.Elem().FieldByIndex(info.sf.Index)
+		switch info.sf.Type {
+		case schemaType:
+			// A field that contains an individual schema. A nil is valid: it just means the field isn't present.
+			c := fv.Interface().(*Schema)
+			if c != nil && !f(c, append(path, info.jsonName)) {
+				return false
+			}
+
+		case schemaSliceType:
+			slice := fv.Interface().([]*Schema)
+			// A field that contains a slice of schemas. Yield nils so we can check for their presence.
+			for i, c := range slice {
+				if !f(c, append(path, info.jsonName, strconv.Itoa(i))) {
 					return false
 				}
 			}
-	// Fields that contain slices of schemas. Yield nils so we can check for their presence.
+
-	for _, sl := range [][]*Schema{s.PrefixItems, s.AllOf, s.AnyOf, s.OneOf} {
+		case schemaMapType:
-		for _, c := range sl {
+			// A field that is a map of schemas. Ditto about nils.
-			if !f(c) {
+			// Sort keys for determinism.
+			m := fv.Interface().(map[string]*Schema)
+			for _, k := range slices.Sorted(maps.Keys(m)) {
+				if !f(m[k], append(path, info.jsonName, escapeJSONPointerSegment(k))) {
 					return false
 				}
 			}
 		}
-	// Fields that are maps of schemas. Ditto about nils.
-	for _, m := range []map[string]*Schema{
-		s.Defs, s.Definitions, s.Properties, s.PatternProperties, s.DependentSchemas,
-	} {
-		for _, c := range m {
-			if !f(c) {
-				return false
-			}
-		}
 	}
 	return true
 }
 
 // all wraps every in an iterator.
-func (s *Schema) all() iter.Seq[*Schema] {
+func (s *Schema) all() iter.Seq2[*Schema, []string] {
-	return func(yield func(*Schema) bool) { s.every(yield) }
+	return func(yield func(*Schema, []string) bool) { s.every(yield, nil) }
 }
 
 // children wraps everyChild in an iterator.
-func (s *Schema) children() iter.Seq[*Schema] {
+func (s *Schema) children() iter.Seq2[*Schema, []string] {
-	return func(yield func(*Schema) bool) { s.everyChild(yield) }
+	var pathBuffer [4]string
+	return func(yield func(*Schema, []string) bool) { s.everyChild(yield, pathBuffer[:0]) }
+}
+
+type structFieldInfo struct {
+	sf       reflect.StructField
+	jsonName string
+}
+
+var (
+	// the visible fields of Schema that have a JSON name, sorted by that name
+	schemaFieldInfos []structFieldInfo
+	// map from JSON name to field
+	schemaFieldMap = map[string]reflect.StructField{}
+)
+
+func init() {
+	for _, sf := range reflect.VisibleFields(reflect.TypeFor[Schema]()) {
+		if name, ok := jsonName(sf); ok {
+			schemaFieldInfos = append(schemaFieldInfos, structFieldInfo{sf, name})
+		}
+	}
+	slices.SortFunc(schemaFieldInfos, func(i1, i2 structFieldInfo) int {
+		return cmp.Compare(i1.jsonName, i2.jsonName)
+	})
+	for _, info := range schemaFieldInfos {
+		schemaFieldMap[info.jsonName] = info.sf
+	}
 }

jsonschema/schema_test.go

@@ -9,6 +9,8 @@ import (
 	"fmt"
 	"math"
 	"regexp"
+	"slices"
+	"strings"
 	"testing"
 )
 
@@ -112,15 +114,31 @@ func TestUnmarshalErrors(t *testing.T) {
 func TestEvery(t *testing.T) {
 	// Schema.every should visit all descendants of a schema, not just the immediate ones.
 	s := &Schema{
-		Items: &Schema{
+		Type:        "string",
-			Items: &Schema{},
+		PrefixItems: []*Schema{{Type: "int"}, {Items: &Schema{Type: "null"}}},
-		},
+		Contains: &Schema{Properties: map[string]*Schema{
+			"~1": {Type: "boolean"},
+			"p":  {},
+		}},
 	}
-	want := 3
+
-	got := 0
+	type item struct {
-	s.every(func(*Schema) bool { got++; return true })
+		s *Schema
-	if got != want {
+		p string
-		t.Errorf("got %d, want %d", got, want)
+	}
+	want := []item{
+		{s, ""},
+		{s.Contains, "contains"},
+		{s.Contains.Properties["p"], "contains/properties/p"},
+		{s.Contains.Properties["~1"], "contains/properties/~01"},
+		{s.PrefixItems[0], "prefixItems/0"},
+		{s.PrefixItems[1], "prefixItems/1"},
+		{s.PrefixItems[1].Items, "prefixItems/1/items"},
+	}
+	var got []item
+	s.every(func(s *Schema, p []string) bool { got = append(got, item{s, strings.Join(p, "/")}); return true }, nil)
+	if !slices.Equal(got, want) {
+		t.Errorf("\n got  %v\nwant %v", got, want)
 	}
 }
 
@@ -130,3 +148,21 @@ func mustUnmarshal(t *testing.T, data []byte, ptr any) {
 		t.Fatal(err)
 	}
 }
+
+// json returns the schema in json format.
+func (s *Schema) json() string {
+	data, err := json.Marshal(s)
+	if err != nil {
+		return fmt.Sprintf("<jsonschema.Schema:%v>", err)
+	}
+	return string(data)
+}
+
+// json returns the schema in json format, indented.
+func (s *Schema) jsonIndent() string {
+	data, err := json.MarshalIndent(s, "", "  ")
+	if err != nil {
+		return fmt.Sprintf("<jsonschema.Schema:%v>", err)
+	}
+	return string(data)
+}

jsonschema/validate.go

@@ -27,8 +27,7 @@ func (rs *Resolved) Validate(instance any) error {
 		return fmt.Errorf("cannot validate version %s, only %s", s, draft202012)
 	}
 	st := &state{rs: rs}
-	var pathBuffer [4]any
+	return st.validate(reflect.ValueOf(instance), st.rs.root, nil)
-	return st.validate(reflect.ValueOf(instance), st.rs.root, nil, pathBuffer[:0])
 }
 
 // state is the state of single call to ResolvedSchema.Validate.
@@ -42,13 +41,10 @@ type state struct {
 }
 
 // validate validates the reflected value of the instance.
-// It keeps track of the path within the instance for better error messages.
+func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *annotations) (err error) {
-func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *annotations, path []any) (err error) {
 	defer func() {
 		if err != nil {
-			if p := formatPath(path); p != "" {
+			err = fmt.Errorf("%s: %w", schema, err)
-				err = fmt.Errorf("%s: %w", p, err)
-			}
 		}
 	}()
 
@@ -72,7 +68,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 	if schema.Type != "" || schema.Types != nil {
 		gotType, ok := jsonType(instance)
 		if !ok {
-			return fmt.Errorf("%v of type %[1]T is not a valid JSON value", instance)
+			return fmt.Errorf("type: %v of type %[1]T is not a valid JSON value", instance)
 		}
 		if schema.Type != "" {
 			// "number" subsumes integers
@@ -163,7 +159,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 
 	// $ref: https://json-schema.org/draft/2020-12/json-schema-core#section-8.2.3.1
 	if schema.Ref != "" {
-		if err := st.validate(instance, schema.resolvedRef, &anns, path); err != nil {
+		if err := st.validate(instance, schema.resolvedRef, &anns); err != nil {
 			return err
 		}
 	}
@@ -175,7 +171,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			"DynamicRef not resolved properly")
 		if schema.resolvedDynamicRef != nil {
 			// Same as $ref.
-			if err := st.validate(instance, schema.resolvedDynamicRef, &anns, path); err != nil {
+			if err := st.validate(instance, schema.resolvedDynamicRef, &anns); err != nil {
 				return err
 			}
 		} else {
@@ -199,7 +195,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			if dynamicSchema == nil {
 				return fmt.Errorf("missing dynamic anchor %q", schema.dynamicRefAnchor)
 			}
-			if err := st.validate(instance, dynamicSchema, &anns, path); err != nil {
+			if err := st.validate(instance, dynamicSchema, &anns); err != nil {
 				return err
 			}
 		}
@@ -214,11 +210,11 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 	// If any of these fail, then validation fails, even if there is an unevaluatedXXX
 	// keyword in the schema. The spec is unclear about this, but that is the intention.
 
-	valid := func(s *Schema, anns *annotations) bool { return st.validate(instance, s, anns, path) == nil }
+	valid := func(s *Schema, anns *annotations) bool { return st.validate(instance, s, anns) == nil }
 
 	if schema.AllOf != nil {
 		for _, ss := range schema.AllOf {
-			if err := st.validate(instance, ss, &anns, path); err != nil {
+			if err := st.validate(instance, ss, &anns); err != nil {
 				return err
 			}
 		}
@@ -264,7 +260,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			ss = schema.Else
 		}
 		if ss != nil {
-			if err := st.validate(instance, ss, &anns, path); err != nil {
+			if err := st.validate(instance, ss, &anns); err != nil {
 				return err
 			}
 		}
@@ -281,7 +277,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			if i >= instance.Len() {
 				break // shorter is OK
 			}
-			if err := st.validate(instance.Index(i), ischema, nil, append(path, i)); err != nil {
+			if err := st.validate(instance.Index(i), ischema, nil); err != nil {
 				return err
 			}
 		}
@@ -289,7 +285,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 
 		if schema.Items != nil {
 			for i := len(schema.PrefixItems); i < instance.Len(); i++ {
-				if err := st.validate(instance.Index(i), schema.Items, nil, append(path, i)); err != nil {
+				if err := st.validate(instance.Index(i), schema.Items, nil); err != nil {
 					return err
 				}
 			}
@@ -300,14 +296,13 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 		nContains := 0
 		if schema.Contains != nil {
 			for i := range instance.Len() {
-				if err := st.validate(instance.Index(i), schema.Contains, nil, append(path, i)); err == nil {
+				if err := st.validate(instance.Index(i), schema.Contains, nil); err == nil {
 					nContains++
 					anns.noteIndex(i)
 				}
 			}
 			if nContains == 0 && (schema.MinContains == nil || *schema.MinContains > 0) {
-				return fmt.Errorf("contains: %s does not have an item matching %s",
+				return fmt.Errorf("contains: %s does not have an item matching %s", instance, schema.Contains)
-					instance, schema.Contains)
 			}
 		}
 
@@ -366,7 +361,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			// That includes validations by subschemas on the same instance, like allOf.
 			for i := anns.endIndex; i < instance.Len(); i++ {
 				if !anns.evaluatedIndexes[i] {
-					if err := st.validate(instance.Index(i), schema.UnevaluatedItems, nil, append(path, i)); err != nil {
+					if err := st.validate(instance.Index(i), schema.UnevaluatedItems, nil); err != nil {
 						return err
 					}
 				}
@@ -399,7 +394,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			if instance.Kind() == reflect.Struct && val.IsZero() && !schema.isRequired[prop] {
 				continue
 			}
-			if err := st.validate(val, subschema, nil, append(path, prop)); err != nil {
+			if err := st.validate(val, subschema, nil); err != nil {
 				return err
 			}
 			evalProps[prop] = true
@@ -409,7 +404,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 				// Check every matching pattern.
 				for re, schema := range schema.patternProperties {
 					if re.MatchString(prop) {
-						if err := st.validate(val, schema, nil, append(path, prop)); err != nil {
+						if err := st.validate(val, schema, nil); err != nil {
 							return err
 						}
 						evalProps[prop] = true
@@ -421,7 +416,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			// Apply to all properties not handled above.
 			for prop, val := range properties(instance) {
 				if !evalProps[prop] {
-					if err := st.validate(val, schema.AdditionalProperties, nil, append(path, prop)); err != nil {
+					if err := st.validate(val, schema.AdditionalProperties, nil); err != nil {
 						return err
 					}
 					evalProps[prop] = true
@@ -433,7 +428,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			// Note: properties unnecessarily fetches each value. We could define a propertyNames function
 			// if performance ever matters.
 			for prop := range properties(instance) {
-				if err := st.validate(reflect.ValueOf(prop), schema.PropertyNames, nil, append(path, prop)); err != nil {
+				if err := st.validate(reflect.ValueOf(prop), schema.PropertyNames, nil); err != nil {
 					return err
 				}
 			}
@@ -493,7 +488,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			for dprop, ss := range schema.DependentSchemas {
 				if hasProperty(dprop) {
 					// TODO: include dependentSchemas[dprop] in the errors.
-					err := st.validate(instance, ss, &anns, path)
+					err := st.validate(instance, ss, &anns)
 					if err != nil {
 						return err
 					}
@@ -505,7 +500,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 			// in addition to sibling keywords.
 			for prop, val := range properties(instance) {
 				if !anns.evaluatedProperties[prop] {
-					if err := st.validate(val, schema.UnevaluatedProperties, nil, append(path, prop)); err != nil {
+					if err := st.validate(val, schema.UnevaluatedProperties, nil); err != nil {
 						return err
 					}
 				}
@@ -633,18 +628,3 @@ func jsonName(f reflect.StructField) (string, bool) {
 	}
 	return f.Name, true
 }
-
-func formatPath(path []any) string {
-	var b strings.Builder
-	for i, p := range path {
-		if n, ok := p.(int); ok {
-			fmt.Fprintf(&b, "[%d]", n)
-		} else {
-			if i > 0 {
-				b.WriteByte('.')
-			}
-			fmt.Fprintf(&b, "%q", p)
-		}
-	}
-	return b.String()
-}

jsonschema/validate_test.go

@@ -78,6 +78,21 @@ func TestValidate(t *testing.T) {
 	}
 }
 
+func TestValidateErrors(t *testing.T) {
+	schema := &Schema{
+		PrefixItems: []*Schema{{Contains: &Schema{Type: "integer"}}},
+	}
+	rs, err := schema.Resolve("", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = rs.Validate([]any{[]any{"1"}})
+	want := "prefixItems/0"
+	if err == nil || !strings.Contains(err.Error(), want) {
+		t.Errorf("error:\n%s\ndoes not contain %q", err, want)
+	}
+}
+
 func TestStructInstance(t *testing.T) {
 	instance := struct {
 		I int