Repository navigation
fix(workflow): normalize unordered set keywords in schema comparison - #1138
Conversation
Static schema validation compares an edge's upstream output schema against the downstream input schema by canonicalizing both and testing byte equality. CanonicalSchemaJSON sorted object keys but kept arrays in source order, so `required` was compared order-sensitively. jsonschema.For builds `required` in struct-field declaration order, so two otherwise identical schemas whose fields are declared in a different order canonicalized differently and were rejected with "schema mismatch on edge ...". Sort the unordered set-valued keywords (`required` and a `type` union) when canonicalizing so equivalent schemas compare equal. Arrays for other keywords keep their order, since keywords such as prefixItems are order-significant.
jjsasha63
left a comment
jjsasha63
left a comment
There was a problem hiding this comment.
Thanks for this — I reproduced the bug on main and confirmed the fix works, including for nested structs. All three new tests fail if I swap in the pre-fix schema_canonical.go, so they are testing the right thing.
Two notes, neither of which I think needs to block.
1. enum is left out. An enum array is an unordered set too (JSON Schema 2020-12 §6.1.2), so the same mismatch survives:
{"enum":["x","y"],"type":"string"} != {"enum":["y","x"],"type":"string"}
dependentRequired values are sets of property names and are also unsorted. Since the title says "unordered set keywords" in general, could you either add enum with a test, or narrow the title and the doc comment to say only required and a type union are handled?
2. The sort keys off the name alone, so it also reaches data. unorderedArrayKeywords[k] is checked against a plain map[string]any walk, so any key named required or type holding an array gets sorted, including inside default, const and examples, where the order is real data:
Default {"type":["email","sms"]} and Default {"type":["sms","email"]}
both canonicalize to {"default":{"type":["email","sms"]},"type":"object"}
Two different schemas now compare equal. It needs a hand-written schema to trigger, since jsonschema.For never emits those keywords, so I am happy to see it as a follow-up.
Worth knowing if you take it: tracking an "am I in a schema position" flag is not enough on its own. A property can be named default, and suppressing normalization under it reintroduces the original bug:
{"properties":{"default":{"required":["b","a"]}}}
{"properties":{"default":{"required":["a","b"]}}}
You also need to treat properties, patternProperties, $defs, definitions and dependentSchemas as name-to-subschema maps. I tried this locally and it works out at roughly 45 lines.
Approving — the fix is correct for what it claims and the blast radius is one function with one caller.
…oogle#1138) Static schema validation compares an edge's upstream output schema against the downstream input schema by canonicalizing both and testing byte equality. CanonicalSchemaJSON sorted object keys but kept arrays in source order, so `required` was compared order-sensitively. jsonschema.For builds `required` in struct-field declaration order, so two otherwise identical schemas whose fields are declared in a different order canonicalized differently and were rejected with "schema mismatch on edge ...". Sort the unordered set-valued keywords (`required` and a `type` union) when canonicalizing so equivalent schemas compare equal. Arrays for other keywords keep their order, since keywords such as prefixItems are order-significant. Co-authored-by: Sasha <[email protected]>
…oogle#1138) Static schema validation compares an edge's upstream output schema against the downstream input schema by canonicalizing both and testing byte equality. CanonicalSchemaJSON sorted object keys but kept arrays in source order, so `required` was compared order-sensitively. jsonschema.For builds `required` in struct-field declaration order, so two otherwise identical schemas whose fields are declared in a different order canonicalized differently and were rejected with "schema mismatch on edge ...". Sort the unordered set-valued keywords (`required` and a `type` union) when canonicalizing so equivalent schemas compare equal. Arrays for other keywords keep their order, since keywords such as prefixItems are order-significant. Co-authored-by: Sasha <[email protected]>
Summary
Static schema validation (
validateStaticSchemas) rejects valid workflow edges when two structurally identical schemas declare their fields in a different order.workflow.Newfails withschema mismatch on edge A -> Bfor a perfectly compatible edge.No linked issue — found while reviewing the schema-validation path.
Root cause
For each typed edge,
validateStaticSchemascanonicalizes the upstreamOutputSchema()and the downstreamInputSchema()withutils.CanonicalSchemaJSONand compares the bytes.CanonicalSchemaJSONsorts object keys but leaves arrays in source order, sorequiredis compared order-sensitively.jsonschema.Forbuildsrequiredin struct-field declaration order, so equivalent schemas whose fields are declared in a different order canonicalize differently and fail validation.Fix
Sort the unordered set-valued keywords (
requiredand atypeunion) when canonicalizing, so equivalent schemas compare equal. Arrays for other keywords keep their order, since keywords likeprefixItemsare order-significant.CanonicalSchemaJSONis only consumed byschemasEqualCanonicalfor equality checks, so nothing else is affected.Tests
internal/utils:TestCanonicalSchemaJSON_RequiredOrderandTestCanonicalSchemaJSON_TypeUnionOrder— reorderedrequired/typeunions now canonicalize identically.workflow: added adiffer only in required order -> successcase toTestStaticSchemaValidation, next to the existingPropertyOrdercase. It fails onmainwithschema mismatch on edge A -> Band passes with this change.Verified locally with the CI command
go test -race -mod=readonly -count=1 -shuffle=on ./...(all packages pass), plusgo vet,gofmt, andgolangci-lint runclean.