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 <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

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

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.

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()))
 	}

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())
 		}
 	}
 }