jsonschema: represent integer keywords as ints

Some JSON Schema keywords, like minLength, are logically integers.
The JSON Schema spec allows them to be written as floating-point
values with zero fractional part, which is a reasonable choice, since
JSON doesn't distinguish the two.

We could simply validate these fields as integers in the normal course
of events (namely, validating the schema against its meta-schema),
but this CL does a little better: it uses *int struct fields for
these keywords, preventing them from ever being non-integers.
The UnmarshalJSON method still accepts integral floating-point values.
Marshaling is unaffected, and the results don't change: encoding/json
has always marshaled integral floats without the decimal part.

Other choices:
- Leave them as float64: you could then get away with
    Schema{MinLength: Ptr(1.5)}
  and though you couldn't get far with that, it's still irksome.

- json.Number: same problem as float64, plus it's even harder to write
  as a literal.

- int64: now `Ptr(1)` doesn't work (it's a *int, not a *int64).
  Anyway, these keywords all compare against len(x) in Go, which
  is of type int.

Change-Id: Ie3f08bd8a5deaf2a21344d6ab873551c011b12d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/668315
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: jba

internal/mcp/internal/jsonschema/infer.go

@@ -92,8 +92,8 @@ func typeSchema(t reflect.Type, seen map[reflect.Type]*Schema) (*Schema, error)
 			return nil, fmt.Errorf("computing element schema: %v", err)
 		}
 		if t.Kind() == reflect.Array {
-			s.MinItems = Ptr(float64(t.Len()))
-			s.MaxItems = Ptr(float64(t.Len()))
+			s.MinItems = Ptr(t.Len())
+			s.MaxItems = Ptr(t.Len())
 		}
 
 	case reflect.String:

internal/mcp/internal/jsonschema/schema.go

@@ -8,6 +8,7 @@ package jsonschema
 
 import (
 	"bytes"
+	"cmp"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -67,25 +68,25 @@ type Schema struct {
 	Maximum          *float64 `json:"maximum,omitempty"`
 	ExclusiveMinimum *float64 `json:"exclusiveMinimum,omitempty"`
 	ExclusiveMaximum *float64 `json:"exclusiveMaximum,omitempty"`
-	MinLength        *float64 `json:"minLength,omitempty"`
-	MaxLength        *float64 `json:"maxLength,omitempty"`
+	MinLength        *int     `json:"minLength,omitempty"`
+	MaxLength        *int     `json:"maxLength,omitempty"`
 	Pattern          string   `json:"pattern,omitempty"`
 
 	// arrays
 	PrefixItems      []*Schema `json:"prefixItems,omitempty"`
 	Items            *Schema   `json:"items,omitempty"`
-	MinItems         *float64  `json:"minItems,omitempty"`
-	MaxItems         *float64  `json:"maxItems,omitempty"`
+	MinItems         *int      `json:"minItems,omitempty"`
+	MaxItems         *int      `json:"maxItems,omitempty"`
 	AdditionalItems  *Schema   `json:"additionalItems,omitempty"`
 	UniqueItems      bool      `json:"uniqueItems,omitempty"`
 	Contains         *Schema   `json:"contains,omitempty"`
-	MinContains      *float64  `json:"minContains,omitempty"`
-	MaxContains      *float64  `json:"maxContains,omitempty"`
+	MinContains      *int      `json:"minContains,omitempty"` // *int, not int: default is 1, not 0
+	MaxContains      *int      `json:"maxContains,omitempty"`
 	UnevaluatedItems *Schema   `json:"unevaluatedItems,omitempty"`
 
 	// objects
-	MinProperties         *float64            `json:"minProperties,omitempty"`
-	MaxProperties         *float64            `json:"maxProperties,omitempty"`
+	MinProperties         *int                `json:"minProperties,omitempty"`
+	MaxProperties         *int                `json:"maxProperties,omitempty"`
 	Required              []string            `json:"required,omitempty"`
 	DependentRequired     map[string][]string `json:"dependentRequired,omitempty"`
 	Properties            map[string]*Schema  `json:"properties,omitempty"`
@@ -174,8 +175,17 @@ func (s *Schema) UnmarshalJSON(data []byte) error {
 	}
 
 	ms := struct {
-		Type  json.RawMessage `json:"type,omitempty"`
-		Const json.RawMessage `json:"const,omitempty"`
+		Type          json.RawMessage `json:"type,omitempty"`
+		Const         json.RawMessage `json:"const,omitempty"`
+		MinLength     *float64        `json:"minLength,omitempty"`
+		MaxLength     *float64        `json:"maxLength,omitempty"`
+		MinItems      *float64        `json:"minItems,omitempty"`
+		MaxItems      *float64        `json:"maxItems,omitempty"`
+		MinProperties *float64        `json:"minProperties,omitempty"`
+		MaxProperties *float64        `json:"maxProperties,omitempty"`
+		MinContains   *float64        `json:"minContains,omitempty"`
+		MaxContains   *float64        `json:"maxContains,omitempty"`
+
 		*schemaWithoutMethods
 	}{
 		schemaWithoutMethods: (*schemaWithoutMethods)(s),
@@ -192,12 +202,13 @@ func (s *Schema) UnmarshalJSON(data []byte) error {
 		case '[':
 			err = json.Unmarshal(ms.Type, &s.Types)
 		default:
-			err = fmt.Errorf("invalid type: %q", ms.Type)
+			err = fmt.Errorf(`invalid value for "type": %q`, ms.Type)
 		}
 	}
 	if err != nil {
 		return err
 	}
+
 	// Setting Const to a pointer to null will marshal properly, but won't unmarshal:
 	// the *any is set to nil, not a pointer to nil.
 	if len(ms.Const) > 0 {
@@ -207,6 +218,34 @@ func (s *Schema) UnmarshalJSON(data []byte) error {
 			return err
 		}
 	}
+
+	// Store integer properties as ints.
+	setInt := func(name string, dst **int, src *float64) error {
+		if src == nil {
+			return nil
+		}
+		i := int(*src)
+		if float64(i) != *src {
+			return fmt.Errorf("%s: %f is not an int", name, *src)
+		}
+		*dst = &i
+		return nil
+	}
+
+	err = cmp.Or(
+		setInt("minLength", &s.MinLength, ms.MinLength),
+		setInt("maxLength", &s.MaxLength, ms.MaxLength),
+		setInt("minItems", &s.MinItems, ms.MinItems),
+		setInt("maxItems", &s.MaxItems, ms.MaxItems),
+		setInt("minProperties", &s.MinProperties, ms.MinProperties),
+		setInt("maxProperties", &s.MaxProperties, ms.MaxProperties),
+		setInt("minContains", &s.MinContains, ms.MinContains),
+		setInt("maxContains", &s.MaxContains, ms.MaxContains),
+	)
+	if err != nil {
+		return err
+	}
+
 	return nil
 }
 

internal/mcp/internal/jsonschema/schema_test.go

@@ -6,14 +6,16 @@ package jsonschema
 
 import (
 	"encoding/json"
+	"regexp"
 	"testing"
 )
 
-func TestMarshal(t *testing.T) {
+func TestGoRoundTrip(t *testing.T) {
+	// Verify that Go representations round-trip.
 	for _, s := range []*Schema{
 		{Type: "null"},
 		{Types: []string{"null", "number"}},
-		{Type: "string", MinLength: Ptr(20.0)},
+		{Type: "string", MinLength: Ptr(20)},
 		{Minimum: Ptr(20.0)},
 		{Items: &Schema{Type: "integer"}},
 		{Const: Ptr(any(0))},
@@ -38,3 +40,69 @@ func TestMarshal(t *testing.T) {
 		}
 	}
 }
+
+func TestJSONRoundTrip(t *testing.T) {
+	// Verify that JSON texts for schemas marshal into equivalent forms.
+	// We don't expect everything to round-trip perfectly. For example, "true" and "false"
+	// will turn into their object equivalents.
+	// But most things should.
+	// Some of these cases test Schema.{UnM,M}arshalJSON.
+	// Most of others follow from the behavior of encoding/json, but they are still
+	// valuable as regression tests of this package's behavior.
+	for _, tt := range []struct {
+		in, want string
+	}{
+		{`true`, `{}`}, // boolean schemas become object schemas
+		{`false`, `{"not":{}}`},
+		{`{"type":"", "enum":null}`, `{}`}, // empty fields are omitted
+		{`{"minimum":1}`, `{"minimum":1}`},
+		{`{"minimum":1.0}`, `{"minimum":1}`},     // floating-point integers lose their fractional part
+		{`{"minLength":1.0}`, `{"minLength":1}`}, // some floats are unmarshaled into ints, but you can't tell
+		{
+			// map keys are sorted
+			`{"$vocabulary":{"b":true, "a":false}}`,
+			`{"$vocabulary":{"a":false,"b":true}}`,
+		},
+		{`{"unk":0}`, `{}`}, // unknown fields are dropped, unfortunately
+	} {
+		var s Schema
+		if err := json.Unmarshal([]byte(tt.in), &s); err != nil {
+			t.Fatal(err)
+		}
+		data, err := json.Marshal(s)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got := string(data); got != tt.want {
+			t.Errorf("%s:\ngot  %s\nwant %s", tt.in, got, tt.want)
+		}
+	}
+}
+
+func TestUnmarshalErrors(t *testing.T) {
+	for _, tt := range []struct {
+		in   string
+		want string // error must match this regexp
+	}{
+		{`1`, "cannot unmarshal number"},
+		{`{"type":1}`, `invalid value for "type"`},
+		{`{"minLength":1.5}`, `minLength:.*not an int`},
+		{`{"maxLength":1.5}`, `maxLength:.*not an int`},
+		{`{"minItems":1.5}`, `minItems:.*not an int`},
+		{`{"maxItems":1.5}`, `maxItems:.*not an int`},
+		{`{"minProperties":1.5}`, `minProperties:.*not an int`},
+		{`{"maxProperties":1.5}`, `maxProperties:.*not an int`},
+		{`{"minContains":1.5}`, `minContains:.*not an int`},
+		{`{"maxContains":1.5}`, `maxContains:.*not an int`},
+	} {
+		var s Schema
+		err := json.Unmarshal([]byte(tt.in), &s)
+		if err == nil {
+			t.Fatalf("%s: no error but expected one", tt.in)
+		}
+		if !regexp.MustCompile(tt.want).MatchString(err.Error()) {
+			t.Errorf("%s: error %q does not match %q", tt.in, err, tt.want)
+		}
+
+	}
+}

internal/mcp/internal/jsonschema/validate.go

@@ -145,12 +145,12 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 		str := instance.String()
 		n := utf8.RuneCountInString(str)
 		if schema.MinLength != nil {
-			if m := int(*schema.MinLength); n < m {
+			if m := *schema.MinLength; n < m {
 				return fmt.Errorf("minLength: %q contains %d Unicode code points, fewer than %d", str, n, m)
 			}
 		}
 		if schema.MaxLength != nil {
-			if m := int(*schema.MaxLength); n > m {
+			if m := *schema.MaxLength; n > m {
 				return fmt.Errorf("maxLength: %q contains %d Unicode code points, more than %d", str, n, m)
 			}
 		}
@@ -268,7 +268,7 @@ func (st *state) validate(instance reflect.Value, schema *Schema, callerAnns *an
 					anns.noteIndex(i)
 				}
 			}
-			if nContains == 0 && (schema.MinContains == nil || int(*schema.MinContains) > 0) {
+			if nContains == 0 && (schema.MinContains == nil || *schema.MinContains > 0) {
 				return fmt.Errorf("contains: %s does not have an item matching %s",
 					instance, schema.Contains)
 			}
@@ -277,23 +277,23 @@ 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.4
 		// TODO(jba): check that these next four keywords' values are integers.
 		if schema.MinContains != nil && schema.Contains != nil {
-			if m := int(*schema.MinContains); nContains < m {
+			if m := *schema.MinContains; nContains < m {
 				return fmt.Errorf("minContains: contains validated %d items, less than %d", nContains, m)
 			}
 		}
 		if schema.MaxContains != nil && schema.Contains != nil {
-			if m := int(*schema.MaxContains); nContains > m {
+			if m := *schema.MaxContains; nContains > m {
 				return fmt.Errorf("maxContains: contains validated %d items, greater than %d", nContains, m)
 			}
 		}
 		if schema.MinItems != nil {
-			if m := int(*schema.MinItems); instance.Len() < m {
+			if m := *schema.MinItems; instance.Len() < m {
 				return fmt.Errorf("minItems: array length %d is less than %d", instance.Len(), m)
 			}
 		}
 		if schema.MaxItems != nil {
-			if m := int(*schema.MaxItems); instance.Len() > m {
-				return fmt.Errorf("minItems: array length %d is greater than %d", instance.Len(), m)
+			if m := *schema.MaxItems; instance.Len() > m {
+				return fmt.Errorf("maxItems: array length %d is greater than %d", instance.Len(), m)
 			}
 		}
 		if schema.UniqueItems {
@@ -385,12 +385,12 @@ 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
 		if schema.MinProperties != nil {
-			if n, m := instance.Len(), int(*schema.MinProperties); n < m {
+			if n, m := instance.Len(), *schema.MinProperties; n < m {
 				return fmt.Errorf("minProperties: object has %d properties, less than %d", n, m)
 			}
 		}
 		if schema.MaxProperties != nil {
-			if n, m := instance.Len(), int(*schema.MaxProperties); n > m {
+			if n, m := instance.Len(), *schema.MaxProperties; n > m {
 				return fmt.Errorf("maxProperties: object has %d properties, greater than %d", n, m)
 			}
 		}