Jonathan Amsterdam (Gerrit)

unread,

May 23, 2025, 4:37:37 PMMay 23

to Alan Donovan, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Jonathan Amsterdam has uploaded the change for review

Jonathan Amsterdam would like Alan Donovan to review this change.

Commit message

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

Change diff

diff --git a/internal/mcp/jsonschema/validate.go b/internal/mcp/jsonschema/validate.go
index 47738dc..448fba1 100644
--- a/internal/mcp/jsonschema/validate.go
+++ b/internal/mcp/jsonschema/validate.go
@@ -271,6 +271,7 @@
 	}
 
 	// 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,20 @@
 		// 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.
+			// QUADRATIC(jba): we could walk the required list for every property.
+			if instance.Kind() == reflect.Struct && val.IsZero() && !slices.Contains(schema.Required, prop) {
+				continue
+			}
+			if err := st.validate(val, subschema, nil, append(path, prop)); err != nil {
 				return err
 			}
 			evalProps[prop] = true
@@ -433,13 +441,17 @@
 		}
 
 		// 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.Required)
+		}
 		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 +569,25 @@
 	}
 }
 
-// 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, required []string) (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() || slices.Contains(required, prop) {
+				min++
+			}
+		}
+		return min, len(sp)
 	default:
 		panic(fmt.Sprintf("properties: bad value: %s of kind %s", v, v.Kind()))
 	}
diff --git a/internal/mcp/jsonschema/validate_test.go b/internal/mcp/jsonschema/validate_test.go
index 1b96a0b..fde1959 100644
--- a/internal/mcp/jsonschema/validate_test.go
+++ b/internal/mcp/jsonschema/validate_test.go
@@ -82,30 +82,92 @@
 	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 but wanted failure; schema = %s", tt.s.json())
+		} else if err != nil && tt.want {
+			t.Errorf("failed but wanted success\nerror %v\nschema = %s", err, tt.s.json())
 		}
 	}
 }

Change information

Files:

M internal/mcp/jsonschema/validate.go
M internal/mcp/jsonschema/validate_test.go

Change size: M

Delta: 2 files changed, 111 insertions(+), 26 deletions(-)

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

May 23, 2025, 4:55:47 PMMay 23

to Jonathan Amsterdam, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Attention needed from Jonathan Amsterdam

Alan Donovan added 3 comments

File internal/mcp/jsonschema/validate.go

Line 399, Patchset 1 (Latest): // QUADRATIC(jba): we could walk the required list for every property.

Alan Donovan . unresolved

Have we not learned? ;-) (Context: CL 675736)

Can we build the set of required properties during schema validation? It looks like we also want it at L586.

File internal/mcp/jsonschema/validate_test.go

Line 168, Patchset 1 (Latest): t.Errorf("succeeded but wanted failure; schema = %s", tt.s.json())

Alan Donovan . unresolved

"Resolve succeeded unexpectedly\nschema = %s"?

Line 170, Patchset 1 (Latest): t.Errorf("failed but wanted success\nerror %v\nschema = %s", err, tt.s.json())

Alan Donovan . unresolved

"Resolve: %v\nschema = %s"

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Amsterdam

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Jonathan Amsterdam (Gerrit)

unread,

May 23, 2025, 6:34:39 PMMay 23

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Jonathan Amsterdam uploaded new patchset

Jonathan Amsterdam uploaded patch set #2 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Jonathan Amsterdam (Gerrit)

unread,

May 23, 2025, 6:34:40 PMMay 23

to goph...@pubsubhelper.golang.org, Go LUCI, Alan Donovan, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Jonathan Amsterdam added 3 comments

File internal/mcp/jsonschema/validate.go

Line 399, Patchset 1: // QUADRATIC(jba): we could walk the required list for every property.

Alan Donovan . resolved

Have we not learned? ;-) (Context: CL 675736)
Can we build the set of required properties during schema validation? It looks like we also want it at L586.

Jonathan Amsterdam

Done, s/validation/resolution/

File internal/mcp/jsonschema/validate_test.go

Line 168, Patchset 1: t.Errorf("succeeded but wanted failure; schema = %s", tt.s.json())

Alan Donovan . resolved

"Resolve succeeded unexpectedly\nschema = %s"?

Jonathan Amsterdam

Done

Line 170, Patchset 1: t.Errorf("failed but wanted success\nerror %v\nschema = %s", err, tt.s.json())

Alan Donovan . resolved

"Resolve: %v\nschema = %s"

Jonathan Amsterdam

Done

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

May 23, 2025, 10:07:53 PMMay 23

to Jonathan Amsterdam, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Attention needed from Jonathan Amsterdam

Alan Donovan voted and added 1 comment

Votes added by Alan Donovan

Code-Review

+2

1 comment

File internal/mcp/jsonschema/validate.go

Line 444, Patchset 2 (Latest): if schema.MinProperties != nil || schema.MaxProperties != nil {

Alan Donovan . resolved

Min and max property constraints are a strange concept. I can't see how one would use it.

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Amsterdam

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Jonathan Amsterdam (Gerrit)

unread,

May 24, 2025, 7:08:28 AMMay 24

to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Alan Donovan, Go LUCI, golang-co...@googlegroups.com

Jonathan Amsterdam submitted the change

Change information

Commit message:

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 <adon...@google.com>

LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Files:

M internal/mcp/jsonschema/resolve.go
M internal/mcp/jsonschema/schema.go
M internal/mcp/jsonschema/validate.go
M internal/mcp/jsonschema/validate_test.go

Change size: M

Delta: 4 files changed, 122 insertions(+), 26 deletions(-)

Branch: refs/heads/master

Submit Requirements:

Code-Review: +2 by Alan Donovan
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Jonathan Amsterdam (Gerrit)

unread,

May 24, 2025, 11:02:01 AMMay 24

to goph...@pubsubhelper.golang.org, Alan Donovan, Go LUCI, golang-co...@googlegroups.com

Jonathan Amsterdam added 1 comment

File internal/mcp/jsonschema/validate.go

Line 444, Patchset 2: if schema.MinProperties != nil || schema.MaxProperties != nil {

Alan Donovan . resolved

Min and max property constraints are a strange concept. I can't see how one would use it.

Jonathan Amsterdam

Agreed.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review

No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[tools] internal/mcp: treat zero struct fields generously

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam has uploaded the change for review

Commit message

Change diff

Change information

Related details

Alan Donovan (Gerrit)

Alan Donovan added 3 comments

Related details

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam uploaded new patchset

Related details

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam added 3 comments

Related details

Alan Donovan (Gerrit)

Alan Donovan voted and added 1 comment

Votes added by Alan Donovan

1 comment

Related details

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam submitted the change

Change information

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam added 1 comment

Related details