From a08732fd0a32f22b04202f4952d02938503d85de Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Fri, 23 May 2025 15:41:07 -0400 Subject: [PATCH] internal/mcp: treat zero struct fields generously Validating a struct is ambiguous, because a zero field could be considered missing or present. Interpret a zero optional struct field in whichever way results in success. Change-Id: I6a9474e6f6558d2a8522bc5ba0451967367e467d Reviewed-on: https://go-review.googlesource.com/c/tools/+/675956 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- jsonschema/resolve.go | 9 ++++ jsonschema/schema.go | 3 ++ jsonschema/validate.go | 38 +++++++++++--- jsonschema/validate_test.go | 98 ++++++++++++++++++++++++++++++------- 4 files changed, 122 insertions(+), 26 deletions(-) diff --git a/jsonschema/resolve.go b/jsonschema/resolve.go index 8f88121..ccf747d 100644 --- a/jsonschema/resolve.go +++ b/jsonschema/resolve.go @@ -181,6 +181,15 @@ func (s *Schema) checkLocal(report func(error)) { s.patternProperties[re] = subschema } } + + // Build a set of required properties, to avoid quadratic behavior when validating + // a struct. + if len(s.Required) > 0 { + s.isRequired = map[string]bool{} + for _, r := range s.Required { + s.isRequired[r] = true + } + } } // resolveURIs resolves the ids and anchors in all the schemas of root, relative diff --git a/jsonschema/schema.go b/jsonschema/schema.go index a55a41d..a8287af 100644 --- a/jsonschema/schema.go +++ b/jsonschema/schema.go @@ -155,6 +155,9 @@ type Schema struct { // compiled regexps pattern *regexp.Regexp patternProperties map[*regexp.Regexp]*Schema + + // the set of required properties + isRequired map[string]bool } // falseSchema returns a new Schema tree that fails to validate any value. diff --git a/jsonschema/validate.go b/jsonschema/validate.go index c8cbe30..75c4911 100644 --- a/jsonschema/validate.go +++ b/jsonschema/validate.go @@ -271,6 +271,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an } // arrays + // TODO(jba): consider arrays of structs. if instance.Kind() == reflect.Array || instance.Kind() == reflect.Slice { // https://json-schema.org/draft/2020-12/json-schema-core#section-10.3.1 // This validate call doesn't collect annotations for the items of the instance; they are separate @@ -386,13 +387,19 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an // If we used anns here, then we'd be including properties evaluated in subschemas // from allOf, etc., which additionalProperties shouldn't observe. evalProps := map[string]bool{} - for prop, schema := range schema.Properties { + for prop, subschema := range schema.Properties { val := property(instance, prop) if !val.IsValid() { // It's OK if the instance doesn't have the property. continue } - if err := st.validate(val, schema, nil, append(path, prop)); err != nil { + // If the instance is a struct and an optional property has the zero + // value, then we could interpret it as present or missing. Be generous: + // assume it's missing, and thus always validates successfully. + if instance.Kind() == reflect.Struct && val.IsZero() && !schema.isRequired[prop] { + continue + } + if err := st.validate(val, subschema, nil, append(path, prop)); err != nil { return err } evalProps[prop] = true @@ -433,13 +440,17 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an } // https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-validation-01#section-6.5 + var min, max int + if schema.MinProperties != nil || schema.MaxProperties != nil { + min, max = numPropertiesBounds(instance, schema.isRequired) + } if schema.MinProperties != nil { - if n, m := numProperties(instance), *schema.MinProperties; n < m { + if n, m := max, *schema.MinProperties; n < m { return fmt.Errorf("minProperties: object has %d properties, less than %d", n, m) } } if schema.MaxProperties != nil { - if n, m := numProperties(instance), *schema.MaxProperties; n > m { + if n, m := min, *schema.MaxProperties; n > m { return fmt.Errorf("maxProperties: object has %d properties, greater than %d", n, m) } } @@ -557,14 +568,25 @@ func properties(v reflect.Value) iter.Seq2[string, reflect.Value] { } } -// numProperties returns the number of v's properties. +// numPropertiesBounds returns bounds on the number of v's properties. // v must be a map or a struct. -func numProperties(v reflect.Value) int { +// If v is a map, both bounds are the map's size. +// If v is a struct, the max is the number of struct properties. +// But since we don't know whether a zero value indicates a missing optional property +// or not, be generous and use the number of non-zero properties as the min. +func numPropertiesBounds(v reflect.Value, isRequired map[string]bool) (int, int) { switch v.Kind() { case reflect.Map: - return v.Len() + return v.Len(), v.Len() case reflect.Struct: - return len(structPropertiesOf(v.Type())) + sp := structPropertiesOf(v.Type()) + min := 0 + for prop, index := range sp { + if !v.FieldByIndex(index).IsZero() || isRequired[prop] { + min++ + } + } + return min, len(sp) default: panic(fmt.Sprintf("properties: bad value: %s of kind %s", v, v.Kind())) } diff --git a/jsonschema/validate_test.go b/jsonschema/validate_test.go index 66208a2..fca4258 100644 --- a/jsonschema/validate_test.go +++ b/jsonschema/validate_test.go @@ -81,30 +81,92 @@ func TestStructInstance(t *testing.T) { instance := struct { I int B bool `json:"b"` - u int - }{1, true, 0} + P *int // either missing or nil + u int // unexported: not a property + }{1, true, nil, 0} - // The instance fails for all of these schemas, demonstrating that it - // was processed correctly. - for _, schema := range []*Schema{ - {MinProperties: Ptr(3)}, - {MaxProperties: Ptr(1)}, - {Required: []string{"i"}}, // the name is "I" - {Required: []string{"B"}}, // the name is "b" - {PropertyNames: &Schema{MinLength: Ptr(2)}}, - {Properties: map[string]*Schema{"b": {Type: "number"}}}, - {Required: []string{"I"}, AdditionalProperties: falseSchema()}, - {DependentRequired: map[string][]string{"b": {"u"}}}, - {DependentSchemas: map[string]*Schema{"b": falseSchema()}}, - {UnevaluatedProperties: falseSchema()}, + for _, tt := range []struct { + s Schema + want bool + }{ + { + Schema{MinProperties: Ptr(4)}, + false, + }, + { + Schema{MinProperties: Ptr(3)}, + true, // P interpreted as present + }, + { + Schema{MaxProperties: Ptr(1)}, + false, + }, + { + Schema{MaxProperties: Ptr(2)}, + true, // P interpreted as absent + }, + { + Schema{Required: []string{"i"}}, // the name is "I" + false, + }, + { + Schema{Required: []string{"B"}}, // the name is "b" + false, + }, + { + Schema{PropertyNames: &Schema{MinLength: Ptr(2)}}, + false, + }, + { + Schema{Properties: map[string]*Schema{"b": {Type: "boolean"}}}, + true, + }, + { + Schema{Properties: map[string]*Schema{"b": {Type: "number"}}}, + false, + }, + { + Schema{Required: []string{"I"}}, + true, + }, + { + Schema{Required: []string{"I", "P"}}, + true, // P interpreted as present + }, + { + Schema{Required: []string{"I", "P"}, Properties: map[string]*Schema{"P": {Type: "number"}}}, + false, // P interpreted as present, but not a number + }, + { + Schema{Required: []string{"I"}, Properties: map[string]*Schema{"P": {Type: "number"}}}, + true, // P not required, so interpreted as absent + }, + { + Schema{Required: []string{"I"}, AdditionalProperties: falseSchema()}, + false, + }, + { + Schema{DependentRequired: map[string][]string{"b": {"u"}}}, + false, + }, + { + Schema{DependentSchemas: map[string]*Schema{"b": falseSchema()}}, + false, + }, + { + Schema{UnevaluatedProperties: falseSchema()}, + false, + }, } { - res, err := schema.Resolve("", nil) + res, err := tt.s.Resolve("", nil) if err != nil { t.Fatal(err) } err = res.Validate(instance) - if err == nil { - t.Errorf("succeeded but wanted failure; schema = %s", schema.json()) + if err == nil && !tt.want { + t.Errorf("succeeded unexpectedly\nschema = %s", tt.s.json()) + } else if err != nil && tt.want { + t.Errorf("Validate: %v\nschema = %s", err, tt.s.json()) } } }