From e3938ce68c0d48dba5d104fdb1d51b65acb0fb03 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Mon, 26 May 2025 07:49:23 -0400 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Auto-Submit: Jonathan Amsterdam --- jsonschema/json_pointer.go | 30 ++++----- jsonschema/resolve.go | 14 +++-- jsonschema/schema.go | 118 ++++++++++++++++++++++++------------ jsonschema/schema_test.go | 52 +++++++++++++--- jsonschema/validate.go | 62 +++++++------------ jsonschema/validate_test.go | 15 +++++ 6 files changed, 185 insertions(+), 106 deletions(-) diff --git a/jsonschema/json_pointer.go b/jsonschema/json_pointer.go index c174c1b..90b4a2c 100644 --- a/jsonschema/json_pointer.go +++ b/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 { - return v.FieldByIndex(index) + if sf, ok := schemaFieldMap[name]; ok { + return v.FieldByIndex(sf.Index) } return reflect.Value{} } diff --git a/jsonschema/resolve.go b/jsonschema/resolve.go index ccf747d..9014245 100644 --- a/jsonschema/resolve.go +++ b/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 diff --git a/jsonschema/schema.go b/jsonschema/schema.go index a8287af..01dce11 100644 --- a/jsonschema/schema.go +++ b/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 "" } @@ -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("", 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,43 +359,57 @@ 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 { - // Fields that contain individual schemas. A nil is valid: it just means the field isn't present. - for _, c := range []*Schema{ - s.Items, s.AdditionalItems, s.Contains, s.PropertyNames, s.AdditionalProperties, - s.If, s.Then, s.Else, s.Not, s.UnevaluatedItems, s.UnevaluatedProperties, - } { - if c != nil && !f(c) { - return false - } +func (s *Schema) everyChild(f func(*Schema, []string) bool, path []string) bool { + if s == nil { + 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} { - for _, c := range sl { - if !f(c) { + var ( + schemaType = reflect.TypeFor[*Schema]() + 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 } - } - } - // 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 + + 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 + } + } + + case schemaMapType: + // A field that is a map of schemas. Ditto about nils. + // 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 + } } } } @@ -402,11 +417,38 @@ func (s *Schema) everyChild(f func(*Schema) bool) bool { } // all wraps every in an iterator. -func (s *Schema) all() iter.Seq[*Schema] { - return func(yield func(*Schema) bool) { s.every(yield) } +func (s *Schema) all() iter.Seq2[*Schema, []string] { + return func(yield func(*Schema, []string) bool) { s.every(yield, nil) } } // children wraps everyChild in an iterator. -func (s *Schema) children() iter.Seq[*Schema] { - return func(yield func(*Schema) bool) { s.everyChild(yield) } +func (s *Schema) children() iter.Seq2[*Schema, []string] { + 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 + } } diff --git a/jsonschema/schema_test.go b/jsonschema/schema_test.go index b44df3c..665ac53 100644 --- a/jsonschema/schema_test.go +++ b/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{ - Items: &Schema{}, - }, + Type: "string", + PrefixItems: []*Schema{{Type: "int"}, {Items: &Schema{Type: "null"}}}, + Contains: &Schema{Properties: map[string]*Schema{ + "~1": {Type: "boolean"}, + "p": {}, + }}, } - want := 3 - got := 0 - s.every(func(*Schema) bool { got++; return true }) - if got != want { - t.Errorf("got %d, want %d", got, want) + + type item struct { + s *Schema + p string + } + 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("", 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("", err) + } + return string(data) +} diff --git a/jsonschema/validate.go b/jsonschema/validate.go index e146281..634417d 100644 --- a/jsonschema/validate.go +++ b/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, pathBuffer[:0]) + return st.validate(reflect.ValueOf(instance), st.rs.root, nil) } // 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, path []any) (err error) { +func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *annotations) (err error) { defer func() { if err != nil { - if p := formatPath(path); p != "" { - err = fmt.Errorf("%s: %w", p, err) - } + err = fmt.Errorf("%s: %w", schema, 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", - instance, schema.Contains) + return fmt.Errorf("contains: %s does not have an item matching %s", 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() -} diff --git a/jsonschema/validate_test.go b/jsonschema/validate_test.go index 8e1a568..d8dd4f7 100644 --- a/jsonschema/validate_test.go +++ b/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