From 5bb0d0af0aa75d0e0bb19d6290bf2128cfc18953 Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Thu, 30 Jul 2026 18:10:02 -0700 Subject: [PATCH 1/4] fix: guard against field drift, and close the gaps the guard found A field on a parser type has to be handled in several independent places: the hand-built slow MarshalJSON path, the structural hasher, each type's equality function, and the joiner's schema comparison. Nothing connected them, so a field could be added, tested and shipped while silently vanishing from JSON documents carrying an extension, or while leaving two different schemas looking identical. internal/driftguard closes the loop. It reflects over each type, sets one field at a time, and requires the behavior under test to notice: the field survives both marshal paths, moves the hash, and makes both equality checks disagree. Asking whether the output changes rather than whether the field appears in some switch is what makes it honest. Deliberate exclusions are listed with a reason beside the check that honors them. Written first, it named the gaps rather than leaving them to be triaged by hand, and it found more than the audit did. Marshal: nine fields reached the struct tags and not the map builder, so they were dropped from any object that also carried an x- extension. Four are the OAS 3.2 features #397 recorded as already implemented ($self, components.mediaTypes, query, additionalOperations); five are JSON Schema 2020-12 keywords, so OAS 3.1 documents were affected too. Hash: thirteen structural fields were never read, including $id, $dynamicRef and $schema, which decide what a reference resolves to, and collectionFormat, which decides an array's wire format. Three list-valued fields also ran together unframed, so required ["ab"] hashed identically to ["a", "b"]; they are length-framed now, as xml and discriminator already were. Equivalence: joiner's deep comparison read 38 of Schema's 65 fields. Twenty-four structural ones are now compared, and $comment, externalDocs and deprecated join the documentation set so they follow EquivalenceDocsIgnore. For six fields the hash was silent too, so schemas differing only in collectionFormat, default, $schema, $id, $dynamicRef or deprecated were being merged outright. parser's own Equals needed no change; the guard confirms it already covered every field but the deliberately excluded StringForm. Structural hashes therefore change for most non-trivial schemas. The pending release note says so and explains why. Fixes #414 Fixes #410 --- internal/driftguard/doc.go | 19 +++ internal/driftguard/equivalence_test.go | 80 ++++++++++ internal/driftguard/fields_test.go | 179 ++++++++++++++++++++++ internal/driftguard/hash_test.go | 121 +++++++++++++++ internal/driftguard/marshal_test.go | 126 ++++++++++++++++ internal/schemautil/hash.go | 87 ++++++++++- joiner/equivalence.go | 189 ++++++++++++++++++++++++ joiner/joiner_dedupe_test.go | 99 +++++++++++++ parser/oas3_json.go | 6 + parser/paths_json.go | 2 + parser/schema_json.go | 6 + 11 files changed, 908 insertions(+), 6 deletions(-) create mode 100644 internal/driftguard/doc.go create mode 100644 internal/driftguard/equivalence_test.go create mode 100644 internal/driftguard/fields_test.go create mode 100644 internal/driftguard/hash_test.go create mode 100644 internal/driftguard/marshal_test.go diff --git a/internal/driftguard/doc.go b/internal/driftguard/doc.go new file mode 100644 index 00000000..6b4f82ff --- /dev/null +++ b/internal/driftguard/doc.go @@ -0,0 +1,19 @@ +// Package driftguard holds the tests that keep a parser struct field from being +// added in one place and forgotten in the others. +// +// A field on a parser type has to be handled in several independent places: the +// hand-built slow MarshalJSON path, the structural hasher, each type's equality +// function, and the joiner's schema comparison. Nothing connected them, so a +// field could be added, tested and shipped while silently vanishing from JSON +// documents that carried an extension, or while leaving two different schemas +// looking identical. That is issue #397, and #414 found nine more fields in the +// same state afterwards. +// +// The guards live here, in one internal package with no exported symbols, rather +// than spread across the packages they exercise. Keeping them together means the +// reflection helper they share is not a maintained API for anyone else, and the +// list of deliberate exclusions sits next to the checks that honor it. +// +// Everything is in _test.go files; this file exists only to give the package a +// clause and a home for this explanation. +package driftguard diff --git a/internal/driftguard/equivalence_test.go b/internal/driftguard/equivalence_test.go new file mode 100644 index 00000000..9f2787f6 --- /dev/null +++ b/internal/driftguard/equivalence_test.go @@ -0,0 +1,80 @@ +package driftguard + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/erraggy/oastools/joiner" + "github.com/erraggy/oastools/parser" +) + +// joiner's deep comparison is the step that verifies a hash grouping before +// deduplication merges it. A field it does not compare cannot split a false +// positive, so a hash collision becomes a merge of two different schemas. +// +// The two checks are meant to agree. [parser.Schema.Equals] compares a field or +// deliberately excludes it; the joiner should reach the same verdict for anything +// structural. This guard sets one field at a time and requires both to notice. + +// equivalenceExclusions lists fields deep comparison deliberately treats as not +// affecting equivalence, with the reason. +// +// Documentation fields are absent from this list on purpose: they are handled at +// runtime by [joiner.EquivalenceDocsInclude], which is the default and makes them +// count. Anything listed here is excluded unconditionally. +var equivalenceExclusions = map[string]string{ + // A $ref schema is compared by the definition it names, which is walked at the + // top level, so comparing the ref string here would double-report. + "Ref": "compared via the definition it names", +} + +func TestDeepComparisonReadsEveryStructuralSchemaField(t *testing.T) { + for _, f := range fieldsOf[parser.Schema]() { + t.Run(f.name, func(t *testing.T) { + if reason, skipped := equivalenceExclusions[f.name]; skipped { + t.Skipf("deliberately not compared: %s", reason) + } + + // A type and a property keep both schemas out of the empty-schema early + // return, which reports any two empty schemas as non-equivalent for + // reasons unrelated to the field under test. + base := func() *parser.Schema { + return &parser.Schema{ + Type: "object", + Properties: map[string]*parser.Schema{"p": {Type: "string"}}, + } + } + + left, right := base(), base() + if !populate(right, f) { + t.Skip("no distinctive value for this field's type") + } + + result := joiner.CompareSchemas(left, right, joiner.EquivalenceModeDeep) + assert.False(t, result.Equivalent, + "Schema.%s differs but deep comparison called the schemas equivalent; "+ + "semantic deduplication would merge them", f.name) + }) + } +} + +// TestEqualsReadsEveryStructuralSchemaField holds parser's own equality to the +// same standard, so the two cannot drift apart from each other either. +func TestEqualsReadsEveryStructuralSchemaField(t *testing.T) { + for _, f := range fieldsOf[parser.Schema]() { + t.Run(f.name, func(t *testing.T) { + if f.name == "StringForm" { + t.Skip("not a Schema field") + } + + left, right := &parser.Schema{}, &parser.Schema{} + if !populate(right, f) { + t.Skip("no distinctive value for this field's type") + } + + assert.False(t, left.Equals(right), + "Schema.%s differs but Equals reported the schemas equal", f.name) + }) + } +} diff --git a/internal/driftguard/fields_test.go b/internal/driftguard/fields_test.go new file mode 100644 index 00000000..6fc872c2 --- /dev/null +++ b/internal/driftguard/fields_test.go @@ -0,0 +1,179 @@ +package driftguard + +import ( + "reflect" + "strings" +) + +// field describes one exported field of a struct that a guard can populate. +type field struct { + // name is the Go field name, e.g. "DefaultMapping". + name string + // jsonKey is the field's json tag name, empty when tagged json:"-". + jsonKey string + // index is the field's index within the struct, for reflect.Value.Field. + index int +} + +// fieldsOf lists the exported fields of the struct type T, skipping the Extra +// extension map that every parser type carries and no guard is about. +func fieldsOf[T any]() []field { + var zero T + t := reflect.TypeOf(zero) + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + + fields := make([]field, 0, t.NumField()) + for i := range t.NumField() { + f := t.Field(i) + if !f.IsExported() || f.Name == "Extra" { + continue + } + key, _, _ := strings.Cut(f.Tag.Get("json"), ",") + if key == "-" { + key = "" + } + fields = append(fields, field{name: f.Name, jsonKey: key, index: i}) + } + return fields +} + +// populate sets one field of target to a distinctive non-zero value and reports +// whether it managed to. target must be a non-nil pointer to a struct. +// +// This is what lets a guard ask the question a source-scanning check cannot +// answer honestly: with this field set and nothing else, does the behavior under +// test actually observe it? Whether the field appears in some switch statement is +// a proxy; whether it changes the output is the thing itself. +// +// false means the field's type has no obvious distinctive value, so the caller +// skips it rather than assuming. Reporting that separately keeps a type the +// helper does not understand from passing as though it were covered. +func populate(target any, f field) bool { + v := reflect.ValueOf(target) + if v.Kind() != reflect.Pointer || v.IsNil() { + return false + } + fv := v.Elem().Field(f.index) + if !fv.CanSet() { + return false + } + + switch fv.Kind() { + case reflect.String: + fv.SetString(marker) + return true + case reflect.Bool: + fv.SetBool(true) + return true + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + fv.SetInt(7) + return true + case reflect.Float32, reflect.Float64: + fv.SetFloat(7) + return true + case reflect.Pointer: + // The pointer type, not its element: newValue allocates for a pointer and + // returns a value for a struct, and the two are not interchangeable. + fv.Set(newValue(fv.Type())) + return true + case reflect.Slice: + fv.Set(reflect.Append(reflect.MakeSlice(fv.Type(), 0, 1), newValue(fv.Type().Elem()))) + return true + case reflect.Map: + key := reflect.New(fv.Type().Key()).Elem() + if key.Kind() != reflect.String { + return false + } + key.SetString(markerKey) + m := reflect.MakeMap(fv.Type()) + m.SetMapIndex(key, newValue(fv.Type().Elem())) + fv.Set(m) + return true + case reflect.Interface: + // The schema-or-bool fields and the example and default values are declared + // `any`. A string is a legal inhabitant of every one of them. + fv.Set(reflect.ValueOf(marker)) + return true + default: + return false + } +} + +const ( + marker = "drift-guard" + markerKey = "driftKey" +) + +// newValue builds a non-zero value of t, one level deep. A nested struct is left +// zero apart from its first string field, which is enough for the value to +// serialize to something a guard can see. +func newValue(t reflect.Type) reflect.Value { + switch t.Kind() { + case reflect.Pointer: + p := reflect.New(t.Elem()) + fillFirstString(p.Elem()) + return p + case reflect.String: + return reflect.ValueOf(marker).Convert(t) + case reflect.Bool: + return reflect.ValueOf(true).Convert(t) + case reflect.Interface: + return reflect.ValueOf(marker) + case reflect.Struct: + v := reflect.New(t).Elem() + fillFirstString(v) + return v + case reflect.Slice: + return reflect.Append(reflect.MakeSlice(t, 0, 1), newValue(t.Elem())) + case reflect.Map: + m := reflect.MakeMap(t) + key := reflect.New(t.Key()).Elem() + if key.Kind() == reflect.String { + key.SetString(markerKey) + m.SetMapIndex(key, newValue(t.Elem())) + } + return m + default: + return reflect.New(t).Elem() + } +} + +// fillFirstString sets the first settable string field of a struct, skipping Ref +// so a nested value does not become a bare $ref that callers treat as an alias. +func fillFirstString(v reflect.Value) { + if v.Kind() != reflect.Struct { + return + } + for i := range v.NumField() { + f := v.Field(i) + if f.Kind() == reflect.String && f.CanSet() && v.Type().Field(i).Name != "Ref" { + f.SetString(marker) + return + } + } +} + +// reflectExtraField returns the Extra map of a parser value, or the zero Value +// when it has none. Found by reflection rather than a type switch so adding a +// type to a guard's subject list needs no second edit here. +func reflectExtraField(value any) reflect.Value { + v := reflect.ValueOf(value) + if v.Kind() != reflect.Pointer || v.IsNil() { + return reflect.Value{} + } + return v.Elem().FieldByName("Extra") +} + +// newExtension builds the specification extension that forces a type down its +// slow MarshalJSON path. Any non-empty Extra does; the key is an x- one so it is +// what ExtractExtensions would have produced. +func newExtension(t reflect.Type) reflect.Value { + m := reflect.MakeMap(t) + m.SetMapIndex( + reflect.ValueOf("x-drift-guard").Convert(t.Key()), + reflect.ValueOf(any("forces the slow path")), + ) + return m +} diff --git a/internal/driftguard/hash_test.go b/internal/driftguard/hash_test.go new file mode 100644 index 00000000..19cc0752 --- /dev/null +++ b/internal/driftguard/hash_test.go @@ -0,0 +1,121 @@ +package driftguard + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/erraggy/oastools/internal/schemautil" + "github.com/erraggy/oastools/parser" +) + +// The structural hash is what groups schemas before deduplication compares them. +// A field it does not read puts two different schemas in one bucket, and if the +// comparison misses the same field they are then merged. That is how the XML +// Object came to merge schemas serializing to different XML: neither the hash nor +// the comparison looked at it, so each gap hid the other. +// +// This guard sets one field at a time and requires the hash to move. + +// hashExclusions lists the fields the hasher deliberately ignores, with the +// reason. [schemautil.SchemaHasher] is documented as a *structural* hash, so +// anything that cannot change how a payload validates or serializes belongs +// here rather than in the hash. +var hashExclusions = map[string]string{ + "Title": "documentation, cannot change a payload", + "Description": "documentation, cannot change a payload", + "Example": "documentation, cannot change a payload", + "Examples": "documentation, cannot change a payload", + "ExternalDocs": "documentation, cannot change a payload", + "Comment": "$comment is documentation, cannot change a payload", + "Deprecated": "advisory, cannot change a payload", +} + +func TestHashReadsEveryStructuralSchemaField(t *testing.T) { + hasher := schemautil.NewSchemaHasher() + baseline := hasher.Hash(&parser.Schema{}) + + for _, f := range fieldsOf[parser.Schema]() { + t.Run(f.name, func(t *testing.T) { + if reason, skipped := hashExclusions[f.name]; skipped { + t.Skipf("deliberately not hashed: %s", reason) + } + + schema := &parser.Schema{} + if !populate(schema, f) { + t.Skip("no distinctive value for this field's type") + } + + assert.NotEqual(t, baseline, hasher.Hash(schema), + "Schema.%s is set but the structural hash did not change; "+ + "two schemas differing only in this field land in one deduplication bucket", + f.name) + }) + } +} + +// TestHashFramesAdjacentValues covers the other half of hashing correctly. +// +// SchemaHasher.writeString appends raw bytes, so a value written next to another +// with no framing runs into it and two different schemas produce one hash. A +// delimiter cannot fix this, because any sentinel byte can also occur inside a +// value; only length framing is injective. +// +// Each pair below collided at some point. They are grouped here rather than +// spread across schemautil's own tests because they are one defect, and the next +// field added to the hasher will be susceptible to it too. +func TestHashFramesAdjacentValues(t *testing.T) { + hasher := schemautil.NewSchemaHasher() + + tests := []struct { + name string + left, right *parser.Schema + }{ + { + name: "required entries run together", + left: &parser.Schema{Type: "object", Required: []string{"ab"}}, + right: &parser.Schema{Type: "object", Required: []string{"a", "b"}}, + }, + { + name: "enum values run together", + left: &parser.Schema{Type: "string", Enum: []any{"ab"}}, + right: &parser.Schema{Type: "string", Enum: []any{"a", "b"}}, + }, + { + name: "dependentRequired entries run together", + left: &parser.Schema{Type: "object", DependentRequired: map[string][]string{"a": {"bc"}}}, + right: &parser.Schema{Type: "object", DependentRequired: map[string][]string{"a": {"b", "c"}}}, + }, + { + name: "xml values run together", + left: &parser.Schema{Type: "string", XML: &parser.XML{Name: "anamespace:b"}}, + right: &parser.Schema{Type: "string", XML: &parser.XML{Name: "a", Namespace: "b"}}, + }, + { + name: "an xml value containing the framing does not forge a boundary", + left: &parser.Schema{Type: "string", XML: &parser.XML{Name: "a1:bnamespace:1:c"}}, + right: &parser.Schema{Type: "string", XML: &parser.XML{Name: "a", Namespace: "b"}}, + }, + { + name: "a discriminator mapping key runs into its value", + left: &parser.Schema{Type: "object", Discriminator: &parser.Discriminator{ + PropertyName: "kind", Mapping: map[string]string{"ab": "c"}}}, + right: &parser.Schema{Type: "object", Discriminator: &parser.Discriminator{ + PropertyName: "kind", Mapping: map[string]string{"a": "bc"}}}, + }, + { + name: "a mapping value forges the defaultMapping label", + left: &parser.Schema{Type: "object", Discriminator: &parser.Discriminator{ + PropertyName: "kind", Mapping: map[string]string{"k": "xdefaultMapping:y"}}}, + right: &parser.Schema{Type: "object", Discriminator: &parser.Discriminator{ + PropertyName: "kind", Mapping: map[string]string{"k": "x"}, DefaultMapping: "y"}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.NotEqual(t, hasher.Hash(tt.left), hasher.Hash(tt.right), + "unframed values collided; length-frame them with writeLabeled") + }) + } +} diff --git a/internal/driftguard/marshal_test.go b/internal/driftguard/marshal_test.go new file mode 100644 index 00000000..b2c46fa9 --- /dev/null +++ b/internal/driftguard/marshal_test.go @@ -0,0 +1,126 @@ +package driftguard + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/erraggy/oastools/parser" +) + +// Every parser type marshals through two paths: a fast one that hands the struct +// to encoding/json and lets the tags do the work, and a slow one that hand-builds +// a map[string]any, taken whenever Extra is non-empty because Go cannot inline a +// map the way yaml:",inline" does. +// +// Adding a field to the struct wires up the fast path only. The slow path is a +// separate list edited by hand, so a field can be added, tested and shipped while +// silently vanishing from every document that also carries an x- extension. +// +// These two guards run the same field set through both paths. A field present in +// the tags but missing from the map builder passes the fast guard and fails the +// slow one, which is exactly the shape of #397 and of the nine fields #414 found. + +// marshalExclusions lists fields the marshalers deliberately do not emit, with +// the reason. Everything else must survive both paths. +var marshalExclusions = map[string]map[string]string{ + "Discriminator": { + // Spelling rather than meaning: it records which dialect the document used, + // and is excluded from JSON, YAML and equality alike. + "StringForm": "not a specification field", + }, +} + +// marshalSubjects pairs each type carrying a hand-built MarshalJSON with a fresh +// value and its field list. A guard cannot reflect over a package, so this list +// is the one thing a new type has to be added to. +func marshalSubjects() map[string]func() (any, []field) { + return map[string]func() (any, []field){ + "OAS3Document": func() (any, []field) { return &parser.OAS3Document{}, fieldsOf[parser.OAS3Document]() }, + "OAS2Document": func() (any, []field) { return &parser.OAS2Document{}, fieldsOf[parser.OAS2Document]() }, + "Components": func() (any, []field) { return &parser.Components{}, fieldsOf[parser.Components]() }, + "PathItem": func() (any, []field) { return &parser.PathItem{}, fieldsOf[parser.PathItem]() }, + "Operation": func() (any, []field) { return &parser.Operation{}, fieldsOf[parser.Operation]() }, + "Response": func() (any, []field) { return &parser.Response{}, fieldsOf[parser.Response]() }, + "MediaType": func() (any, []field) { return &parser.MediaType{}, fieldsOf[parser.MediaType]() }, + "Example": func() (any, []field) { return &parser.Example{}, fieldsOf[parser.Example]() }, + "Encoding": func() (any, []field) { return &parser.Encoding{}, fieldsOf[parser.Encoding]() }, + "Link": func() (any, []field) { return &parser.Link{}, fieldsOf[parser.Link]() }, + "Parameter": func() (any, []field) { return &parser.Parameter{}, fieldsOf[parser.Parameter]() }, + "Header": func() (any, []field) { return &parser.Header{}, fieldsOf[parser.Header]() }, + "Items": func() (any, []field) { return &parser.Items{}, fieldsOf[parser.Items]() }, + "RequestBody": func() (any, []field) { return &parser.RequestBody{}, fieldsOf[parser.RequestBody]() }, + "Schema": func() (any, []field) { return &parser.Schema{}, fieldsOf[parser.Schema]() }, + "Discriminator": func() (any, []field) { return &parser.Discriminator{}, fieldsOf[parser.Discriminator]() }, + "XML": func() (any, []field) { return &parser.XML{}, fieldsOf[parser.XML]() }, + "Tag": func() (any, []field) { return &parser.Tag{}, fieldsOf[parser.Tag]() }, + "Server": func() (any, []field) { return &parser.Server{}, fieldsOf[parser.Server]() }, + "ServerVariable": func() (any, []field) { return &parser.ServerVariable{}, fieldsOf[parser.ServerVariable]() }, + "Info": func() (any, []field) { return &parser.Info{}, fieldsOf[parser.Info]() }, + "Contact": func() (any, []field) { return &parser.Contact{}, fieldsOf[parser.Contact]() }, + "License": func() (any, []field) { return &parser.License{}, fieldsOf[parser.License]() }, + "ExternalDocs": func() (any, []field) { return &parser.ExternalDocs{}, fieldsOf[parser.ExternalDocs]() }, + "SecurityScheme": func() (any, []field) { return &parser.SecurityScheme{}, fieldsOf[parser.SecurityScheme]() }, + "OAuthFlows": func() (any, []field) { return &parser.OAuthFlows{}, fieldsOf[parser.OAuthFlows]() }, + "OAuthFlow": func() (any, []field) { return &parser.OAuthFlow{}, fieldsOf[parser.OAuthFlow]() }, + } +} + +func TestMarshalSlowPathEmitsEveryField(t *testing.T) { + runMarshalGuard(t, true, + "%s.%s is set but the slow MarshalJSON path did not emit it; add it to that path's map builder") +} + +// TestMarshalFastPathEmitsEveryField is the control: the same fields with no +// extension present go through the struct tags, which is the path that already +// worked. A failure here means the struct tag itself is wrong. +func TestMarshalFastPathEmitsEveryField(t *testing.T) { + runMarshalGuard(t, false, + "%s.%s is set but the struct tags did not emit it") +} + +func runMarshalGuard(t *testing.T, withExtension bool, message string) { + t.Helper() + + for typeName, subject := range marshalSubjects() { + _, fields := subject() + for _, f := range fields { + t.Run(typeName+"/"+f.name, func(t *testing.T) { + if reason, skipped := marshalExclusions[typeName][f.name]; skipped { + t.Skipf("deliberately not emitted: %s", reason) + } + if f.jsonKey == "" { + t.Skip("not serialized to JSON") + } + + value, _ := subject() + if !populate(value, f) { + t.Skip("no distinctive value for this field's type") + } + if withExtension { + require.True(t, setExtension(value), + "%s has no Extra field, so the slow path cannot be reached", typeName) + } + + encoded, err := json.Marshal(value) + require.NoError(t, err) + + assert.Contains(t, string(encoded), `"`+f.jsonKey+`"`, message, typeName, f.name) + }) + } + } +} + +// setExtension puts an extension on the value so MarshalJSON takes the slow path, +// and reports whether it found somewhere to put it. Done by reflection rather +// than a type switch so a new type in marshalSubjects needs no second edit here. +func setExtension(value any) bool { + extra := reflectExtraField(value) + if !extra.IsValid() || !extra.CanSet() { + return false + } + extra.Set(newExtension(extra.Type())) + return true +} diff --git a/internal/schemautil/hash.go b/internal/schemautil/hash.go index 207ae53d..0c766cfe 100644 --- a/internal/schemautil/hash.go +++ b/internal/schemautil/hash.go @@ -80,11 +80,12 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { h.writeString(hasher, "pattern:") h.writeString(hasher, schema.Pattern) - // Enum (order matters) + // Enum (order matters). Length-framed: unframed, ["ab"] and ["a", "b"] both + // hash as "enum:ab". if len(schema.Enum) > 0 { h.writeString(hasher, "enum:") for _, v := range schema.Enum { - h.writeString(hasher, fmt.Sprintf("%v", v)) + h.writeLabeled(hasher, "v", fmt.Sprintf("%v", v)) } } @@ -94,14 +95,15 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { h.writeString(hasher, fmt.Sprintf("%v", schema.Const)) } - // Required (sort for order-independent comparison) + // Required (sort for order-independent comparison). Length-framed for the same + // reason as Enum. if len(schema.Required) > 0 { h.writeString(hasher, "required:") sorted := make([]string, len(schema.Required)) copy(sorted, schema.Required) sort.Strings(sorted) for _, r := range sorted { - h.writeString(hasher, r) + h.writeLabeled(hasher, "r", r) } } @@ -262,6 +264,10 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { } } + // JSON Schema identity, dialect, and the content keywords + h.hashIdentity(hasher, schema) + h.hashContentKeywords(hasher, schema) + // Contains if schema.Contains != nil { h.writeString(hasher, "contains:") @@ -283,12 +289,12 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { } sort.Strings(keys) for _, k := range keys { - h.writeString(hasher, k) + h.writeLabeled(hasher, "k", k) deps := make([]string, len(schema.DependentRequired[k])) copy(deps, schema.DependentRequired[k]) sort.Strings(deps) for _, d := range deps { - h.writeString(hasher, d) + h.writeLabeled(hasher, "d", d) } } } @@ -365,6 +371,75 @@ func (h *SchemaHasher) hashSchemaOrBool(hasher hash.Hash64, v any) { } } +// hashIdentity hashes the JSON Schema identity and dialect keywords. They decide +// which schema a $ref or $dynamicRef resolves to and which vocabulary validates +// it, so two schemas differing here are not interchangeable however alike their +// constraints look. +func (h *SchemaHasher) hashIdentity(hasher hash.Hash64, schema *parser.Schema) { + if schema.Schema != "" { + h.writeLabeled(hasher, "$schema", schema.Schema) + } + if schema.ID != "" { + h.writeLabeled(hasher, "$id", schema.ID) + } + if schema.Anchor != "" { + h.writeLabeled(hasher, "$anchor", schema.Anchor) + } + if schema.DynamicRef != "" { + h.writeLabeled(hasher, "$dynamicRef", schema.DynamicRef) + } + if schema.DynamicAnchor != "" { + h.writeLabeled(hasher, "$dynamicAnchor", schema.DynamicAnchor) + } + if len(schema.Vocabulary) == 0 { + return + } + h.writeString(hasher, "$vocabulary:") + keys := make([]string, 0, len(schema.Vocabulary)) + for k := range schema.Vocabulary { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + h.writeLabeled(hasher, "k", k) + h.writeString(hasher, strconv.FormatBool(schema.Vocabulary[k])) + } +} + +// hashContentKeywords hashes the value and serialization keywords: the default, the +// OAS 2.0 array format, and the JSON Schema 2020-12 unevaluated and content +// keywords, all of which participate in validation or in the wire format. +// +// default is an annotation in JSON Schema terms, but two schemas that default +// differently generate different code and fill payloads differently, so +// consolidating them is not safe. +func (h *SchemaHasher) hashContentKeywords(hasher hash.Hash64, schema *parser.Schema) { + if schema.Default != nil { + h.writeLabeled(hasher, "default", fmt.Sprintf("%v", schema.Default)) + } + if schema.CollectionFormat != "" { + h.writeLabeled(hasher, "collectionFormat", schema.CollectionFormat) + } + if schema.UnevaluatedProperties != nil { + h.writeString(hasher, "unevaluatedProperties:") + h.hashSchemaOrBool(hasher, schema.UnevaluatedProperties) + } + if schema.UnevaluatedItems != nil { + h.writeString(hasher, "unevaluatedItems:") + h.hashSchemaOrBool(hasher, schema.UnevaluatedItems) + } + if schema.ContentEncoding != "" { + h.writeLabeled(hasher, "contentEncoding", schema.ContentEncoding) + } + if schema.ContentMediaType != "" { + h.writeLabeled(hasher, "contentMediaType", schema.ContentMediaType) + } + if schema.ContentSchema != nil { + h.writeString(hasher, "contentSchema:") + h.hashSchema(hasher, schema.ContentSchema) + } +} + // hashNumericConstraints hashes numeric validation fields. func (h *SchemaHasher) hashNumericConstraints(hasher hash.Hash64, schema *parser.Schema) { if schema.Minimum != nil { diff --git a/joiner/equivalence.go b/joiner/equivalence.go index 175fb0bd..46f408db 100644 --- a/joiner/equivalence.go +++ b/joiner/equivalence.go @@ -463,6 +463,192 @@ func compareDocFields(left, right *parser.Schema, path *comparePath, result *Equ }) path.pop() } + + // $comment, externalDocs and deprecated are documentation and advisory rather + // than structural, so they belong to this set: they count by default, and are + // ignored under EquivalenceDocsIgnore along with the four above. + if left.Comment != right.Comment { + result.record(path, "$comment", left.Comment, right.Comment, "$comment mismatch") + } + if !reflect.DeepEqual(left.ExternalDocs, right.ExternalDocs) { + result.record(path, "externalDocs", left.ExternalDocs, right.ExternalDocs, "externalDocs mismatch") + } + if left.Deprecated != right.Deprecated { + result.record(path, "deprecated", left.Deprecated, right.Deprecated, "deprecated mismatch") + } +} + +// record appends one field difference at path. The existing comparisons above +// spell this out inline; new ones use this so the check is the line you read. +func (r *EquivalenceResult) record(path *comparePath, field string, left, right any, description string) { + path.push(field) + r.Differences = append(r.Differences, SchemaDifference{ + Path: path.String(), + LeftValue: left, + RightValue: right, + Description: description, + }) + path.pop() +} + +// compareSchemaMaps compares two name-keyed schema maps deeply. +func compareSchemaMaps( + field string, + left, right map[string]*parser.Schema, + path *comparePath, + result *EquivalenceResult, + visited map[pointerPair]bool, + compareDocs bool, +) { + if !equalPropertyNames(left, right) { + result.record(path, field, getPropertyNames(left), getPropertyNames(right), field+" names mismatch") + return + } + if left == nil { + return + } + path.push(field) + for name, leftValue := range left { + path.push(name) + compareDeep(leftValue, right[name], path, result, visited, compareDocs) + path.pop() + } + path.pop() +} + +// compareStructuralSchemaFields compares the Schema fields that affect what a +// document means but are not reached by the comparisons written out above. +// +// They were all unchecked until issue #410: deep comparison read 38 of the 65 +// fields, so schemas differing only in nullability, in which property +// discriminates a union, or in OAS 2.0 array serialization were reported +// equivalent and merged. internal/driftguard is what keeps this list complete. +func compareStructuralSchemaFields( + left, right *parser.Schema, + path *comparePath, + result *EquivalenceResult, + visited map[pointerPair]bool, + compareDocs bool, +) { + // JSON Schema identity and dialect: these decide what a $ref or $dynamicRef + // resolves to and which vocabulary validates it. + if left.Schema != right.Schema { + result.record(path, "$schema", left.Schema, right.Schema, "$schema mismatch") + } + if left.ID != right.ID { + result.record(path, "$id", left.ID, right.ID, "$id mismatch") + } + if left.Anchor != right.Anchor { + result.record(path, "$anchor", left.Anchor, right.Anchor, "$anchor mismatch") + } + if left.DynamicRef != right.DynamicRef { + result.record(path, "$dynamicRef", left.DynamicRef, right.DynamicRef, "$dynamicRef mismatch") + } + if left.DynamicAnchor != right.DynamicAnchor { + result.record(path, "$dynamicAnchor", left.DynamicAnchor, right.DynamicAnchor, "$dynamicAnchor mismatch") + } + if !reflect.DeepEqual(left.Vocabulary, right.Vocabulary) { + result.record(path, "$vocabulary", left.Vocabulary, right.Vocabulary, "$vocabulary mismatch") + } + + // Value and serialization semantics. + if !reflect.DeepEqual(left.Default, right.Default) { + result.record(path, "default", left.Default, right.Default, "default mismatch") + } + if left.CollectionFormat != right.CollectionFormat { + result.record(path, "collectionFormat", left.CollectionFormat, right.CollectionFormat, + "collectionFormat mismatch") + } + + // OAS flags. Merging across any of these changes what a payload may contain. + if left.Nullable != right.Nullable { + result.record(path, "nullable", left.Nullable, right.Nullable, "nullable mismatch") + } + if left.ReadOnly != right.ReadOnly { + result.record(path, "readOnly", left.ReadOnly, right.ReadOnly, "readOnly mismatch") + } + if left.WriteOnly != right.WriteOnly { + result.record(path, "writeOnly", left.WriteOnly, right.WriteOnly, "writeOnly mismatch") + } + + // Numeric and array constraints. + if !equalutil.EqualPtr(left.MultipleOf, right.MultipleOf) { + result.record(path, "multipleOf", left.MultipleOf, right.MultipleOf, "multipleOf constraint mismatch") + } + if !reflect.DeepEqual(left.ExclusiveMaximum, right.ExclusiveMaximum) { + result.record(path, "exclusiveMaximum", left.ExclusiveMaximum, right.ExclusiveMaximum, + "exclusiveMaximum constraint mismatch") + } + if !reflect.DeepEqual(left.ExclusiveMinimum, right.ExclusiveMinimum) { + result.record(path, "exclusiveMinimum", left.ExclusiveMinimum, right.ExclusiveMinimum, + "exclusiveMinimum constraint mismatch") + } + if !equalutil.EqualPtr(left.MaxContains, right.MaxContains) { + result.record(path, "maxContains", left.MaxContains, right.MaxContains, "maxContains constraint mismatch") + } + if !equalutil.EqualPtr(left.MinContains, right.MinContains) { + result.record(path, "minContains", left.MinContains, right.MinContains, "minContains constraint mismatch") + } + if !equalStringSliceMaps(left.DependentRequired, right.DependentRequired) { + result.record(path, "dependentRequired", left.DependentRequired, right.DependentRequired, + "dependentRequired mismatch") + } + + // Polymorphism. A discriminator names the property that selects a subschema, + // so two schemas discriminating differently describe different payloads. + if !equalDiscriminators(left.Discriminator, right.Discriminator) { + result.record(path, "discriminator", left.Discriminator, right.Discriminator, "discriminator mismatch") + } + + // Nested schemas. + compareAdditionalPropertiesSchemas(left.AdditionalItems, right.AdditionalItems, path, result, visited, compareDocs) + compareSchemaMaps("patternProperties", left.PatternProperties, right.PatternProperties, path, result, visited, compareDocs) + compareSchemaMaps("$defs", left.Defs, right.Defs, path, result, visited, compareDocs) + for _, c := range []struct { + name string + left, right *parser.Schema + }{ + {"if", left.If, right.If}, + {"then", left.Then, right.Then}, + {"else", left.Else, right.Else}, + } { + if (c.left == nil) != (c.right == nil) { + result.record(path, c.name, c.left != nil, c.right != nil, c.name+" presence mismatch") + continue + } + if c.left != nil { + path.push(c.name) + compareDeep(c.left, c.right, path, result, visited, compareDocs) + path.pop() + } + } +} + +// equalDiscriminators compares two Discriminator Objects. +// +// StringForm is excluded for the same reason parser's equalDiscriminator excludes +// it: it records which dialect spelled the discriminator, not what it selects. +func equalDiscriminators(left, right *parser.Discriminator) bool { + if left == nil || right == nil { + return left == right + } + return left.PropertyName == right.PropertyName && + left.DefaultMapping == right.DefaultMapping && + reflect.DeepEqual(left.Mapping, right.Mapping) +} + +// equalStringSliceMaps compares two name-keyed string-slice maps, order-independently. +func equalStringSliceMaps(left, right map[string][]string) bool { + if len(left) != len(right) { + return false + } + for k, lv := range left { + rv, ok := right[k] + if !ok || !equalStringSlices(lv, rv) { + return false + } + } + return true } // compareCommonFields compares schema fields common to both shallow and deep comparison. @@ -706,6 +892,9 @@ func compareDeep(left, right *parser.Schema, path *comparePath, result *Equivale path.pop() } + // Compare the structural fields that are not written out above (issue #410). + compareStructuralSchemaFields(left, right, path, result, visited, compareDocs) + // Compare items (array item schema) compareItemsSchemas(left.Items, right.Items, path, result, visited, compareDocs) diff --git a/joiner/joiner_dedupe_test.go b/joiner/joiner_dedupe_test.go index e3430616..da4f1a9a 100644 --- a/joiner/joiner_dedupe_test.go +++ b/joiner/joiner_dedupe_test.go @@ -1202,3 +1202,102 @@ func TestJoiner_SemanticDeduplication_MergesXMLIdenticalSchemas(t *testing.T) { "identical xml must still deduplicate") assert.Contains(t, doc.Components.Schemas, "Alpha", "alphabetically first name is canonical") } + +// TestJoiner_SemanticDeduplication_KeepsStructurallyDistinctSchemas is the +// end-to-end regression for issue #410. +// +// Deep comparison read 38 of Schema's 65 fields. For six of the rest the +// structural hash was silent too, so the schemas landed in one bucket and the +// verification step that exists to split false positives agreed they matched. +// Each pair below was merged before, and merging them changes what the document +// says: a different array serialization, a different default, a different JSON +// Schema dialect or identity. +func TestJoiner_SemanticDeduplication_KeepsStructurallyDistinctSchemas(t *testing.T) { + tests := []struct { + name string + left, right *parser.Schema + }{ + { + name: "collectionFormat", + left: &parser.Schema{Type: "array", CollectionFormat: "csv", Items: &parser.Schema{Type: "string"}}, + right: &parser.Schema{Type: "array", CollectionFormat: "pipes", Items: &parser.Schema{Type: "string"}}, + }, + { + name: "default", + left: &parser.Schema{Type: "string", Default: "a"}, + right: &parser.Schema{Type: "string", Default: "b"}, + }, + { + name: "$schema", + left: &parser.Schema{Type: "string", Schema: "https://json-schema.org/draft/2020-12/schema"}, + right: &parser.Schema{Type: "string", Schema: "http://json-schema.org/draft-07/schema#"}, + }, + { + name: "$id", + left: &parser.Schema{Type: "string", ID: "https://example.com/a"}, + right: &parser.Schema{Type: "string", ID: "https://example.com/b"}, + }, + { + name: "$dynamicRef", + left: &parser.Schema{Type: "string", DynamicRef: "#a"}, + right: &parser.Schema{Type: "string", DynamicRef: "#b"}, + }, + { + name: "deprecated", + left: &parser.Schema{Type: "string", Deprecated: true}, + right: &parser.Schema{Type: "string"}, + }, + { + name: "nullable", + left: &parser.Schema{Type: "string", Nullable: true}, + right: &parser.Schema{Type: "string"}, + }, + { + name: "discriminator", + left: &parser.Schema{ + Type: "object", + Properties: map[string]*parser.Schema{"kind": {Type: "string"}}, + Discriminator: &parser.Discriminator{PropertyName: "kind"}, + }, + right: &parser.Schema{ + Type: "object", + Properties: map[string]*parser.Schema{"kind": {Type: "string"}}, + Discriminator: &parser.Discriminator{PropertyName: "other"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + docOf := func(title, name string, schema *parser.Schema) parser.ParseResult { + return parser.ParseResult{ + Document: &parser.OAS3Document{ + OpenAPI: "3.1.0", + Info: &parser.Info{Title: title, Version: "1.0.0"}, + Paths: make(parser.Paths), + Components: &parser.Components{Schemas: map[string]*parser.Schema{name: schema}}, + OASVersion: parser.OASVersion310, + }, + Version: "3.1.0", + OASVersion: parser.OASVersion310, + SourcePath: title + ".yaml", + SourceFormat: "yaml", + } + } + + config := DefaultConfig() + config.SemanticDeduplication = true + + joined, err := New(config).JoinParsed([]parser.ParseResult{ + docOf("API 1", "Alpha", tt.left), + docOf("API 2", "Beta", tt.right), + }) + require.NoError(t, err) + + doc, ok := joined.Document.(*parser.OAS3Document) + require.True(t, ok) + assert.Len(t, doc.Components.Schemas, 2, + "schemas differing in %s must not be consolidated", tt.name) + }) + } +} diff --git a/parser/oas3_json.go b/parser/oas3_json.go index 530f6402..e80462f8 100644 --- a/parser/oas3_json.go +++ b/parser/oas3_json.go @@ -46,6 +46,9 @@ func (d *OAS3Document) MarshalJSON() ([]byte, error) { if d.ExternalDocs != nil { m["externalDocs"] = d.ExternalDocs } + if d.Self != "" { + m["$self"] = d.Self + } if d.JSONSchemaDialect != "" { m["jsonSchemaDialect"] = d.JSONSchemaDialect } @@ -109,6 +112,9 @@ func (c *Components) MarshalJSON() ([]byte, error) { if len(c.Callbacks) > 0 { m["callbacks"] = c.Callbacks } + if len(c.MediaTypes) > 0 { + m["mediaTypes"] = c.MediaTypes + } if len(c.PathItems) > 0 { m["pathItems"] = c.PathItems } diff --git a/parser/paths_json.go b/parser/paths_json.go index 56b7497a..cf6873e4 100644 --- a/parser/paths_json.go +++ b/parser/paths_json.go @@ -32,6 +32,8 @@ func (p *PathItem) MarshalJSON() ([]byte, error) { jsonhelpers.SetIfNotNil(m, "head", p.Head) jsonhelpers.SetIfNotNil(m, "patch", p.Patch) jsonhelpers.SetIfNotNil(m, "trace", p.Trace) + jsonhelpers.SetIfNotNil(m, "query", p.Query) // OAS 3.2+ + jsonhelpers.SetIfMapNotEmpty(m, "additionalOperations", p.AdditionalOperations) // OAS 3.2+ jsonhelpers.SetIfSliceNotEmpty(m, "servers", p.Servers) jsonhelpers.SetIfSliceNotEmpty(m, "parameters", p.Parameters) diff --git a/parser/schema_json.go b/parser/schema_json.go index d34d0814..7cb03e8c 100644 --- a/parser/schema_json.go +++ b/parser/schema_json.go @@ -58,6 +58,12 @@ func (s *Schema) MarshalJSON() ([]byte, error) { jsonhelpers.SetIfNotNil(m, "minProperties", s.MinProperties) jsonhelpers.SetIfNotNil(m, "dependentRequired", s.DependentRequired) jsonhelpers.SetIfNotNil(m, "dependentSchemas", s.DependentSchemas) + // JSON Schema 2020-12 unevaluated and content keywords (OAS 3.1+) + jsonhelpers.SetIfNotNil(m, "unevaluatedProperties", s.UnevaluatedProperties) + jsonhelpers.SetIfNotNil(m, "unevaluatedItems", s.UnevaluatedItems) + jsonhelpers.SetIfNotEmpty(m, "contentEncoding", s.ContentEncoding) + jsonhelpers.SetIfNotEmpty(m, "contentMediaType", s.ContentMediaType) + jsonhelpers.SetIfNotNil(m, "contentSchema", s.ContentSchema) jsonhelpers.SetIfNotNil(m, "if", s.If) jsonhelpers.SetIfNotNil(m, "then", s.Then) jsonhelpers.SetIfNotNil(m, "else", s.Else) From c1e455ca6ed7b5c7786e71ecb1d3037e613154f9 Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Thu, 30 Jul 2026 20:15:05 -0700 Subject: [PATCH 2/4] fix: name the keyword a schema-or-bool difference belongs to, and guard nil map values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #416. Two of the seven were defects in this branch. compareAdditionalPropertiesSchemas hardcodes its own path segment, so reusing it for AdditionalItems reported every item difference under additionalProperties. The comparison was right and the diagnostic lied about which keyword differed. The keyword is a parameter now, in compareSchemaOrBool, and the old function is a one-line wrapper, so a future reuse cannot misname a field either. compareSchemaMaps handed map values straight to compareDeep, which dereferences both operands with no nil guard. A map may hold a nil value for a present key — `patternProperties: {"^a": }` parses that way — and comparing one against a present schema panicked. The pre-existing Properties and DependentSchemas loops share the hazard; this fixes the two maps the new helper added. The rest are hygiene in the same spirit as the guard itself: - the marshal guard matched its key as a substring of the encoded JSON, which a nested value could satisfy. It decodes and checks for a top-level key now: it exposed nothing, but a guard that passes falsely is worth nothing. - populate panicked on a named interface field rather than reporting it uncovered. A panic aborts the whole guard, which is how the pointer bug during development reported one failure instead of nine. - the $vocabulary boolean was the one unframed write in a function whose point is injectivity, so the next value type added there would have copied it. - hashContentKeywords also hashed default and collectionFormat, which are not content keywords; hashValueSemantics now holds those two and both names are honest. - dropped an unreachable skip for Discriminator.StringForm in a guard that only ever iterates Schema fields. --- internal/driftguard/equivalence_test.go | 4 --- internal/driftguard/fields_test.go | 7 ++++- internal/driftguard/marshal_test.go | 7 ++++- internal/schemautil/hash.go | 19 ++++++++----- joiner/equivalence.go | 38 +++++++++++++++++++------ 5 files changed, 53 insertions(+), 22 deletions(-) diff --git a/internal/driftguard/equivalence_test.go b/internal/driftguard/equivalence_test.go index 9f2787f6..df3e05ba 100644 --- a/internal/driftguard/equivalence_test.go +++ b/internal/driftguard/equivalence_test.go @@ -64,10 +64,6 @@ func TestDeepComparisonReadsEveryStructuralSchemaField(t *testing.T) { func TestEqualsReadsEveryStructuralSchemaField(t *testing.T) { for _, f := range fieldsOf[parser.Schema]() { t.Run(f.name, func(t *testing.T) { - if f.name == "StringForm" { - t.Skip("not a Schema field") - } - left, right := &parser.Schema{}, &parser.Schema{} if !populate(right, f) { t.Skip("no distinctive value for this field's type") diff --git a/internal/driftguard/fields_test.go b/internal/driftguard/fields_test.go index 6fc872c2..1c15728b 100644 --- a/internal/driftguard/fields_test.go +++ b/internal/driftguard/fields_test.go @@ -93,7 +93,12 @@ func populate(target any, f field) bool { return true case reflect.Interface: // The schema-or-bool fields and the example and default values are declared - // `any`. A string is a legal inhabitant of every one of them. + // `any`, and a string inhabits every one of them. A named interface would + // not accept one, and a panic here aborts the whole guard rather than + // failing a case, so report it as uncovered instead. + if !reflect.TypeOf(marker).AssignableTo(fv.Type()) { + return false + } fv.Set(reflect.ValueOf(marker)) return true default: diff --git a/internal/driftguard/marshal_test.go b/internal/driftguard/marshal_test.go index b2c46fa9..0d9563cb 100644 --- a/internal/driftguard/marshal_test.go +++ b/internal/driftguard/marshal_test.go @@ -107,7 +107,12 @@ func runMarshalGuard(t *testing.T, withExtension bool, message string) { encoded, err := json.Marshal(value) require.NoError(t, err) - assert.Contains(t, string(encoded), `"`+f.jsonKey+`"`, message, typeName, f.name) + // Decoded rather than substring-matched: the key name can occur inside a + // nested value too, and a guard that passes falsely is worse than none. + var decoded map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + assert.Contains(t, decoded, f.jsonKey, message, typeName, f.name) }) } } diff --git a/internal/schemautil/hash.go b/internal/schemautil/hash.go index 0c766cfe..37c27c82 100644 --- a/internal/schemautil/hash.go +++ b/internal/schemautil/hash.go @@ -264,8 +264,9 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { } } - // JSON Schema identity, dialect, and the content keywords + // JSON Schema identity and dialect, value semantics, and the content keywords h.hashIdentity(hasher, schema) + h.hashValueSemantics(hasher, schema) h.hashContentKeywords(hasher, schema) // Contains @@ -402,24 +403,28 @@ func (h *SchemaHasher) hashIdentity(hasher hash.Hash64, schema *parser.Schema) { sort.Strings(keys) for _, k := range keys { h.writeLabeled(hasher, "k", k) - h.writeString(hasher, strconv.FormatBool(schema.Vocabulary[k])) + h.writeLabeled(hasher, "v", strconv.FormatBool(schema.Vocabulary[k])) } } -// hashContentKeywords hashes the value and serialization keywords: the default, the -// OAS 2.0 array format, and the JSON Schema 2020-12 unevaluated and content -// keywords, all of which participate in validation or in the wire format. +// hashValueSemantics hashes the keywords that decide what a value is when the +// payload does not say: the default, and the OAS 2.0 array serialization. // // default is an annotation in JSON Schema terms, but two schemas that default // differently generate different code and fill payloads differently, so -// consolidating them is not safe. -func (h *SchemaHasher) hashContentKeywords(hasher hash.Hash64, schema *parser.Schema) { +// consolidating them is not safe. collectionFormat decides a wire format outright. +func (h *SchemaHasher) hashValueSemantics(hasher hash.Hash64, schema *parser.Schema) { if schema.Default != nil { h.writeLabeled(hasher, "default", fmt.Sprintf("%v", schema.Default)) } if schema.CollectionFormat != "" { h.writeLabeled(hasher, "collectionFormat", schema.CollectionFormat) } +} + +// hashContentKeywords hashes the JSON Schema 2020-12 unevaluated and content +// keywords, all of which participate in validation. +func (h *SchemaHasher) hashContentKeywords(hasher hash.Hash64, schema *parser.Schema) { if schema.UnevaluatedProperties != nil { h.writeString(hasher, "unevaluatedProperties:") h.hashSchemaOrBool(hasher, schema.UnevaluatedProperties) diff --git a/joiner/equivalence.go b/joiner/equivalence.go index 46f408db..2b164881 100644 --- a/joiner/equivalence.go +++ b/joiner/equivalence.go @@ -509,8 +509,18 @@ func compareSchemaMaps( } path.push(field) for name, leftValue := range left { + // compareDeep dereferences both operands with no nil guard of its own, and a + // map may hold a nil value for a present key: `patternProperties: {"^a": }` + // parses that way. + rightValue := right[name] + if leftValue == nil || rightValue == nil { + if (leftValue == nil) != (rightValue == nil) { + result.record(path, name, leftValue != nil, rightValue != nil, field+" entry presence mismatch") + } + continue + } path.push(name) - compareDeep(leftValue, right[name], path, result, visited, compareDocs) + compareDeep(leftValue, rightValue, path, result, visited, compareDocs) path.pop() } path.pop() @@ -601,7 +611,7 @@ func compareStructuralSchemaFields( } // Nested schemas. - compareAdditionalPropertiesSchemas(left.AdditionalItems, right.AdditionalItems, path, result, visited, compareDocs) + compareSchemaOrBool("additionalItems", left.AdditionalItems, right.AdditionalItems, path, result, visited, compareDocs) compareSchemaMaps("patternProperties", left.PatternProperties, right.PatternProperties, path, result, visited, compareDocs) compareSchemaMaps("$defs", left.Defs, right.Defs, path, result, visited, compareDocs) for _, c := range []struct { @@ -1209,18 +1219,28 @@ func compareItemsSchemas(left, right any, path *comparePath, result *Equivalence } func compareAdditionalPropertiesSchemas(left, right any, path *comparePath, result *EquivalenceResult, visited map[pointerPair]bool, compareDocs bool) { + compareSchemaOrBool("additionalProperties", left, right, path, result, visited, compareDocs) +} + +// compareSchemaOrBool compares one schema-or-bool field, recording differences +// under the keyword it belongs to. +// +// The keyword is a parameter because additionalProperties and additionalItems are +// the same shape but not the same field: reusing the additionalProperties path for +// additionalItems reported an item difference under an object keyword. +func compareSchemaOrBool(field string, left, right any, path *comparePath, result *EquivalenceResult, visited map[pointerPair]bool, compareDocs bool) { // Both nil if left == nil && right == nil { return } // One nil if left == nil || right == nil { - path.push("additionalProperties") + path.push(field) result.Differences = append(result.Differences, SchemaDifference{ Path: path.String(), LeftValue: left != nil, RightValue: right != nil, - Description: "additionalProperties presence mismatch", + Description: field + " presence mismatch", }) path.pop() return @@ -1230,7 +1250,7 @@ func compareAdditionalPropertiesSchemas(left, right any, path *comparePath, resu leftSchema, leftIsSchema := left.(*parser.Schema) rightSchema, rightIsSchema := right.(*parser.Schema) if leftIsSchema && rightIsSchema { - path.push("additionalProperties") + path.push(field) compareDeep(leftSchema, rightSchema, path, result, visited, compareDocs) path.pop() return @@ -1241,12 +1261,12 @@ func compareAdditionalPropertiesSchemas(left, right any, path *comparePath, resu rightBool, rightIsBool := right.(bool) if leftIsBool && rightIsBool { if leftBool != rightBool { - path.push("additionalProperties") + path.push(field) result.Differences = append(result.Differences, SchemaDifference{ Path: path.String(), LeftValue: leftBool, RightValue: rightBool, - Description: "additionalProperties boolean value mismatch", + Description: field + " boolean value mismatch", }) path.pop() } @@ -1254,12 +1274,12 @@ func compareAdditionalPropertiesSchemas(left, right any, path *comparePath, resu } // Type mismatch - path.push("additionalProperties") + path.push(field) result.Differences = append(result.Differences, SchemaDifference{ Path: path.String(), LeftValue: fmt.Sprintf("%T", left), RightValue: fmt.Sprintf("%T", right), - Description: "additionalProperties type mismatch", + Description: field + " type mismatch", }) path.pop() } From c3558400b997cc1b099ff581991fde59e640bc6c Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Thu, 30 Jul 2026 20:52:07 -0700 Subject: [PATCH 3/4] fix: assert exclusions instead of skipping them, and attribute cross-package coverage Review follow-ups on #416. Every skipped case in the drift guard was a deliberate exclusion, which is the wrong way to express one: a skip checks nothing, so hashing Title by accident, or emitting StringForm, would have gone unnoticed while the guard kept reporting SKIP. Each is now an assertion that the exclusion holds, and a field populate cannot build a value for fails rather than skipping, so an unhandled type is visible instead of silently uncovered. Zero skips, more assertions. Converting them surfaced two things. Deep comparison never compared $ref, so two schemas pointing at different targets were reported equivalent; it compares them now and the exclusion list is empty. And StringForm does not drop a key, it re-spells the Discriminator as the OAS 2.0 bare string, so there is no object to decode: that case is checked on the raw bytes, and its recorded reason says what it actually does. Two findings from the second CodeRabbit pass, both verified by reproduction. A typed nil in a schema-or-bool field passed the interface nil checks and asserted cleanly, then panicked in compareDeep. And hashSchema returned as soon as it had written $ref, so {$ref: X, default: 1} and {$ref: X, default: 2} hashed alike, though the comparison did split them, making it a wasted comparison rather than a bad merge. Both fixed, both with the tests the previous round should have had. Coverage: go test attributes coverage per package, so the guards in internal/driftguard exercised joiner and internal/schemautil thoroughly and registered as almost none of it, which is why the patch check failed at 31%. -coverpkg=./... credits a test to the package it exercises. codecov.yml already notes the same limitation where it ignores internal/testutil. Overall coverage rises 83.7% to 85.6% because it stops discarding real coverage; the workflow needs no change, since it uploads whatever coverage.txt make check produced. --- Makefile | 8 +- internal/driftguard/equivalence_test.go | 33 +++---- internal/driftguard/hash_test.go | 19 ++-- internal/driftguard/marshal_test.go | 43 +++++--- internal/schemautil/hash.go | 8 +- internal/schemautil/hash_test.go | 41 ++++++++ joiner/equivalence.go | 11 +++ joiner/equivalence_test.go | 126 ++++++++++++++++++++++++ 8 files changed, 244 insertions(+), 45 deletions(-) diff --git a/Makefile b/Makefile index ad7e93c3..e303530b 100644 --- a/Makefile +++ b/Makefile @@ -58,13 +58,17 @@ clean: ## test: Run tests with coverage (parallel execution for speed) ## Note: Fuzz tests are skipped in regular test runs. Use 'make test-fuzz-parse' to run them separately. +## +## -coverpkg=./... credits a test to the package it exercises, not only the one it +## lives in. Go attributes coverage per-package by default, which discards +## everything a test in one package covers in another. .PHONY: test test: @echo "Running tests..." ifeq ("$(shell command -v gotestsum)", "") - go test -coverprofile=coverage.txt -covermode=atomic -timeout=5m -skip='^Fuzz' ./... + go test -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic -timeout=5m -skip='^Fuzz' ./... else - gotestsum --format testname -- -coverprofile=coverage.txt -covermode=atomic -timeout=5m -failfast -skip='^Fuzz' ./... + gotestsum --format testname -- -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic -timeout=5m -failfast -skip='^Fuzz' ./... endif ## test-quick: Run tests quickly for rapid iteration (no coverage, short mode) diff --git a/internal/driftguard/equivalence_test.go b/internal/driftguard/equivalence_test.go index df3e05ba..48c2a946 100644 --- a/internal/driftguard/equivalence_test.go +++ b/internal/driftguard/equivalence_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/erraggy/oastools/joiner" "github.com/erraggy/oastools/parser" @@ -17,25 +18,17 @@ import ( // deliberately excludes it; the joiner should reach the same verdict for anything // structural. This guard sets one field at a time and requires both to notice. -// equivalenceExclusions lists fields deep comparison deliberately treats as not -// affecting equivalence, with the reason. +// Deep comparison excludes nothing: every field of a Schema affects what a +// document means, documentation included, since [joiner.EquivalenceDocsInclude] +// is the default and decides at runtime whether the documentation fields count. // -// Documentation fields are absent from this list on purpose: they are handled at -// runtime by [joiner.EquivalenceDocsInclude], which is the default and makes them -// count. Anything listed here is excluded unconditionally. -var equivalenceExclusions = map[string]string{ - // A $ref schema is compared by the definition it names, which is walked at the - // top level, so comparing the ref string here would double-report. - "Ref": "compared via the definition it names", -} +// There is deliberately no exclusion list here. If one becomes necessary, the +// entry belongs beside an assertion that the field really is ignored, not beside +// a skip: a skipped case checks nothing and reads as though it did. func TestDeepComparisonReadsEveryStructuralSchemaField(t *testing.T) { for _, f := range fieldsOf[parser.Schema]() { t.Run(f.name, func(t *testing.T) { - if reason, skipped := equivalenceExclusions[f.name]; skipped { - t.Skipf("deliberately not compared: %s", reason) - } - // A type and a property keep both schemas out of the empty-schema early // return, which reports any two empty schemas as non-equivalent for // reasons unrelated to the field under test. @@ -47,9 +40,9 @@ func TestDeepComparisonReadsEveryStructuralSchemaField(t *testing.T) { } left, right := base(), base() - if !populate(right, f) { - t.Skip("no distinctive value for this field's type") - } + require.True(t, populate(right, f), + "populate cannot produce a value for Schema.%s; extend it rather than "+ + "leaving the field unchecked", f.name) result := joiner.CompareSchemas(left, right, joiner.EquivalenceModeDeep) assert.False(t, result.Equivalent, @@ -65,9 +58,9 @@ func TestEqualsReadsEveryStructuralSchemaField(t *testing.T) { for _, f := range fieldsOf[parser.Schema]() { t.Run(f.name, func(t *testing.T) { left, right := &parser.Schema{}, &parser.Schema{} - if !populate(right, f) { - t.Skip("no distinctive value for this field's type") - } + require.True(t, populate(right, f), + "populate cannot produce a value for Schema.%s; extend it rather than "+ + "leaving the field unchecked", f.name) assert.False(t, left.Equals(right), "Schema.%s differs but Equals reported the schemas equal", f.name) diff --git a/internal/driftguard/hash_test.go b/internal/driftguard/hash_test.go index 19cc0752..823f2e9b 100644 --- a/internal/driftguard/hash_test.go +++ b/internal/driftguard/hash_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/erraggy/oastools/internal/schemautil" "github.com/erraggy/oastools/parser" @@ -37,13 +38,19 @@ func TestHashReadsEveryStructuralSchemaField(t *testing.T) { for _, f := range fieldsOf[parser.Schema]() { t.Run(f.name, func(t *testing.T) { - if reason, skipped := hashExclusions[f.name]; skipped { - t.Skipf("deliberately not hashed: %s", reason) - } - schema := &parser.Schema{} - if !populate(schema, f) { - t.Skip("no distinctive value for this field's type") + require.True(t, populate(schema, f), + "populate cannot produce a value for Schema.%s; extend it rather than "+ + "leaving the field unchecked", f.name) + + // An excluded field is asserted to stay excluded rather than skipped. A + // skip checks nothing, so hashing Title by accident would go unnoticed; + // this way the exclusion is a claim the suite keeps honest. + if reason, excluded := hashExclusions[f.name]; excluded { + assert.Equal(t, baseline, hasher.Hash(schema), + "Schema.%s is excluded from the structural hash (%s) but setting it "+ + "changed the hash", f.name, reason) + return } assert.NotEqual(t, baseline, hasher.Hash(schema), diff --git a/internal/driftguard/marshal_test.go b/internal/driftguard/marshal_test.go index 0d9563cb..ffcf0680 100644 --- a/internal/driftguard/marshal_test.go +++ b/internal/driftguard/marshal_test.go @@ -2,6 +2,7 @@ package driftguard import ( "encoding/json" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -27,9 +28,10 @@ import ( // the reason. Everything else must survive both paths. var marshalExclusions = map[string]map[string]string{ "Discriminator": { - // Spelling rather than meaning: it records which dialect the document used, - // and is excluded from JSON, YAML and equality alike. - "StringForm": "not a specification field", + // Spelling rather than meaning: it records which dialect the document used. + // Setting it does not drop a key, it re-spells the Discriminator as the OAS + // 2.0 bare string, which is why this case is checked before decoding. + "StringForm": "selects the OAS 2.0 bare-string form", }, } @@ -88,17 +90,10 @@ func runMarshalGuard(t *testing.T, withExtension bool, message string) { _, fields := subject() for _, f := range fields { t.Run(typeName+"/"+f.name, func(t *testing.T) { - if reason, skipped := marshalExclusions[typeName][f.name]; skipped { - t.Skipf("deliberately not emitted: %s", reason) - } - if f.jsonKey == "" { - t.Skip("not serialized to JSON") - } - value, _ := subject() - if !populate(value, f) { - t.Skip("no distinctive value for this field's type") - } + require.True(t, populate(value, f), + "populate cannot produce a value for %s.%s; extend it rather than "+ + "leaving the field unchecked", typeName, f.name) if withExtension { require.True(t, setExtension(value), "%s has no Extra field, so the slow path cannot be reached", typeName) @@ -107,11 +102,33 @@ func runMarshalGuard(t *testing.T, withExtension bool, message string) { encoded, err := json.Marshal(value) require.NoError(t, err) + // An excluded field is asserted to stay excluded rather than skipped: a + // skip checks nothing, so emitting one by accident would go unnoticed. + // + // Checked on the raw bytes because an excluded field can change the shape + // of the output rather than merely drop a key. Discriminator.StringForm + // re-spells the whole object as an OAS 2.0 bare string, so there is no + // object left to decode. + if reason, excluded := marshalExclusions[typeName][f.name]; excluded { + assert.NotContains(t, string(encoded), `"`+f.name+`"`, + "%s.%s is excluded from JSON (%s) but was emitted", typeName, f.name, reason) + return + } + // Decoded rather than substring-matched: the key name can occur inside a // nested value too, and a guard that passes falsely is worse than none. var decoded map[string]json.RawMessage require.NoError(t, json.Unmarshal(encoded, &decoded)) + if f.jsonKey == "" { + // Tagged json:"-", so no spelling of the name should appear as a key. + assert.NotContains(t, decoded, f.name, + `%s.%s is tagged json:"-" but was emitted`, typeName, f.name) + assert.NotContains(t, decoded, strings.ToLower(f.name[:1])+f.name[1:], + `%s.%s is tagged json:"-" but was emitted`, typeName, f.name) + return + } + assert.Contains(t, decoded, f.jsonKey, message, typeName, f.name) }) } diff --git a/internal/schemautil/hash.go b/internal/schemautil/hash.go index 37c27c82..a4da3207 100644 --- a/internal/schemautil/hash.go +++ b/internal/schemautil/hash.go @@ -62,11 +62,11 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { h.visited[ptr] = true defer func() { h.visited[ptr] = false }() - // Hash $ref if present (schema is just a reference) + // Hash $ref if present. JSON Schema 2020-12 allows keywords alongside $ref, so + // this records the reference and keeps going: returning here made + // {$ref: X, default: 1} and {$ref: X, default: 2} hash alike. if schema.Ref != "" { - h.writeString(hasher, "$ref:") - h.writeString(hasher, schema.Ref) - return + h.writeLabeled(hasher, "$ref", schema.Ref) } // Type (handle both string and []any for OAS 3.1+) diff --git a/internal/schemautil/hash_test.go b/internal/schemautil/hash_test.go index 0ffa9c25..131e3fe4 100644 --- a/internal/schemautil/hash_test.go +++ b/internal/schemautil/hash_test.go @@ -505,3 +505,44 @@ func TestHashFramesDiscriminatorStrings(t *testing.T) { assert.Equal(t, hasher.Hash(left), hasher.Hash(right)) }) } + +// TestHashReadsRefSiblings covers the keywords a $ref schema may carry alongside +// the reference. +// +// JSON Schema 2020-12 allows siblings to $ref, unlike OAS 3.0 where they are +// ignored. Returning as soon as the reference was written made every $ref schema +// to the same target hash alike whatever else it said. +func TestHashReadsRefSiblings(t *testing.T) { + hasher := NewSchemaHasher() + const target = "#/components/schemas/X" + + plain := &parser.Schema{Ref: target} + + tests := []struct { + name string + other *parser.Schema + }{ + {"default", &parser.Schema{Ref: target, Default: "a"}}, + {"$id", &parser.Schema{Ref: target, ID: "https://example.com/a"}}, + {"$anchor", &parser.Schema{Ref: target, Anchor: "a"}}, + {"$dynamicRef", &parser.Schema{Ref: target, DynamicRef: "#a"}}, + {"contentEncoding", &parser.Schema{Ref: target, ContentEncoding: "base64"}}, + {"contentMediaType", &parser.Schema{Ref: target, ContentMediaType: "application/json"}}, + {"contentSchema", &parser.Schema{Ref: target, ContentSchema: &parser.Schema{Type: "string"}}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.NotEqual(t, hasher.Hash(plain), hasher.Hash(tt.other), + "a $ref schema carrying %s must not hash as the bare reference", tt.name) + }) + } + + t.Run("two references to the same target still hash alike", func(t *testing.T) { + assert.Equal(t, hasher.Hash(&parser.Schema{Ref: target}), hasher.Hash(&parser.Schema{Ref: target})) + }) + + t.Run("different targets differ", func(t *testing.T) { + assert.NotEqual(t, hasher.Hash(&parser.Schema{Ref: target}), hasher.Hash(&parser.Schema{Ref: target + "Y"})) + }) +} diff --git a/joiner/equivalence.go b/joiner/equivalence.go index 2b164881..af5dc887 100644 --- a/joiner/equivalence.go +++ b/joiner/equivalence.go @@ -542,6 +542,9 @@ func compareStructuralSchemaFields( ) { // JSON Schema identity and dialect: these decide what a $ref or $dynamicRef // resolves to and which vocabulary validates it. + if left.Ref != right.Ref { + result.record(path, "$ref", left.Ref, right.Ref, "$ref target mismatch") + } if left.Schema != right.Schema { result.record(path, "$schema", left.Schema, right.Schema, "$schema mismatch") } @@ -1250,6 +1253,14 @@ func compareSchemaOrBool(field string, left, right any, path *comparePath, resul leftSchema, leftIsSchema := left.(*parser.Schema) rightSchema, rightIsSchema := right.(*parser.Schema) if leftIsSchema && rightIsSchema { + // A typed nil passes the interface nil checks above and asserts cleanly, so + // the pointer itself has to be tested before compareDeep dereferences it. + if leftSchema == nil || rightSchema == nil { + if leftSchema != rightSchema { + result.record(path, field, leftSchema != nil, rightSchema != nil, field+" presence mismatch") + } + return + } path.push(field) compareDeep(leftSchema, rightSchema, path, result, visited, compareDocs) path.pop() diff --git a/joiner/equivalence_test.go b/joiner/equivalence_test.go index 1382a774..4d53a48a 100644 --- a/joiner/equivalence_test.go +++ b/joiner/equivalence_test.go @@ -1,6 +1,7 @@ package joiner import ( + "strings" "testing" "github.com/erraggy/oastools/parser" @@ -1453,3 +1454,128 @@ func TestCompareSchemas_XMLEquivalent(t *testing.T) { assert.True(t, CompareSchemas(plainLeft, plainRight, EquivalenceModeDeep).Equivalent) }) } + +// TestCompareSchemas_NilNestedSchemas covers the nil values a schema map or a +// schema-or-bool field can legitimately hold. +// +// compareDeep dereferences both operands with no nil guard of its own, so every +// nested walk has to keep nils away from it. `patternProperties: {"^a": }` parses +// to a nil entry, and a caller can store a typed nil `(*parser.Schema)(nil)` in +// additionalItems, which passes an interface nil check and asserts cleanly. +func TestCompareSchemas_NilNestedSchemas(t *testing.T) { + base := func() map[string]*parser.Schema { + return map[string]*parser.Schema{"p": {Type: "string"}} + } + + tests := []struct { + name string + left, right *parser.Schema + equivalent bool + wantPath string + }{ + { + name: "nil patternProperties entry against a present one", + left: &parser.Schema{Type: "object", Properties: base(), PatternProperties: map[string]*parser.Schema{"^a": nil}}, + right: &parser.Schema{Type: "object", Properties: base(), PatternProperties: map[string]*parser.Schema{"^a": {Type: "string"}}}, + equivalent: false, + wantPath: "patternProperties.^a", + }, + { + name: "nil $defs entry against a present one", + left: &parser.Schema{Type: "object", Properties: base(), Defs: map[string]*parser.Schema{"A": nil}}, + right: &parser.Schema{Type: "object", Properties: base(), Defs: map[string]*parser.Schema{"A": {Type: "string"}}}, + equivalent: false, + wantPath: "$defs.A", + }, + { + name: "both entries nil", + left: &parser.Schema{Type: "object", Properties: base(), Defs: map[string]*parser.Schema{"A": nil}}, + right: &parser.Schema{Type: "object", Properties: base(), Defs: map[string]*parser.Schema{"A": nil}}, + equivalent: true, + }, + { + name: "typed-nil additionalItems against a present schema", + left: &parser.Schema{Type: "array", Properties: base(), AdditionalItems: (*parser.Schema)(nil)}, + right: &parser.Schema{Type: "array", Properties: base(), AdditionalItems: &parser.Schema{Type: "string"}}, + equivalent: false, + wantPath: "additionalItems", + }, + { + name: "typed-nil additionalProperties against a present schema", + left: &parser.Schema{Type: "object", Properties: base(), AdditionalProperties: (*parser.Schema)(nil)}, + right: &parser.Schema{Type: "object", Properties: base(), AdditionalProperties: &parser.Schema{Type: "string"}}, + equivalent: false, + wantPath: "additionalProperties", + }, + { + name: "both additionalItems typed-nil", + left: &parser.Schema{Type: "array", Properties: base(), AdditionalItems: (*parser.Schema)(nil)}, + right: &parser.Schema{Type: "array", Properties: base(), AdditionalItems: (*parser.Schema)(nil)}, + equivalent: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := CompareSchemas(tt.left, tt.right, EquivalenceModeDeep) + + assert.Equal(t, tt.equivalent, result.Equivalent) + if tt.wantPath == "" { + return + } + var paths []string + for _, d := range result.Differences { + paths = append(paths, d.Path) + } + assert.Contains(t, paths, tt.wantPath, + "the difference should be reported at the field it belongs to") + }) + } +} + +// TestCompareSchemas_AdditionalItemsNamesItsOwnKeyword pins the keyword a +// schema-or-bool difference is reported under. +// +// additionalItems and additionalProperties share a comparison, which used to +// hardcode the additionalProperties path, so an item difference was labelled as +// an object one. The comparison was right and only the diagnostic was wrong, +// which is why no equivalence assertion caught it. +func TestCompareSchemas_AdditionalItemsNamesItsOwnKeyword(t *testing.T) { + props := map[string]*parser.Schema{"p": {Type: "string"}} + left := &parser.Schema{Type: "array", Properties: props, AdditionalItems: &parser.Schema{Type: "string"}} + right := &parser.Schema{Type: "array", Properties: props, AdditionalItems: &parser.Schema{Type: "integer"}} + + result := CompareSchemas(left, right, EquivalenceModeDeep) + + require.False(t, result.Equivalent) + require.NotEmpty(t, result.Differences) + for _, d := range result.Differences { + assert.True(t, strings.HasPrefix(d.Path, "additionalItems"), + "expected the difference under additionalItems, got %q", d.Path) + } +} + +// TestCompareSchemas_SchemaOrBoolMismatch covers the remaining shapes a +// schema-or-bool field can take, since additionalItems and additionalProperties +// accept either and a document may change which. +func TestCompareSchemas_SchemaOrBoolMismatch(t *testing.T) { + props := map[string]*parser.Schema{"p": {Type: "string"}} + + t.Run("boolean values differ", func(t *testing.T) { + left := &parser.Schema{Type: "object", Properties: props, AdditionalProperties: true} + right := &parser.Schema{Type: "object", Properties: props, AdditionalProperties: false} + assert.False(t, CompareSchemas(left, right, EquivalenceModeDeep).Equivalent) + }) + + t.Run("a schema against a boolean", func(t *testing.T) { + left := &parser.Schema{Type: "object", Properties: props, AdditionalProperties: &parser.Schema{Type: "string"}} + right := &parser.Schema{Type: "object", Properties: props, AdditionalProperties: false} + assert.False(t, CompareSchemas(left, right, EquivalenceModeDeep).Equivalent) + }) + + t.Run("present against absent", func(t *testing.T) { + left := &parser.Schema{Type: "object", Properties: props, AdditionalProperties: true} + right := &parser.Schema{Type: "object", Properties: props} + assert.False(t, CompareSchemas(left, right, EquivalenceModeDeep).Equivalent) + }) +} From 07789f689c962f87fd8ca7e948e9c3a4a888bbd1 Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Thu, 30 Jul 2026 21:43:43 -0700 Subject: [PATCH 4/4] fix(joiner): use maps.Equal so nil and empty maps compare as parser does parser documents "Nil and empty maps are considered equal" and uses maps.Equal throughout. The comparisons added in this branch used reflect.DeepEqual, which splits those two, so the packages contradicted each other about the same pair of schemas: a discriminator declaring `mapping: {}` differed from one declaring no mapping under joiner and matched under parser. $vocabulary and the discriminator mapping now use maps.Equal, and externalDocs is compared field by field rather than by DeepEqual on the pointer, whose comparison of Extra had the same split. Extra keeps maps.EqualFunc with reflect.DeepEqual rather than maps.Equal, which is the pairing parser uses: specification extensions hold arbitrary JSON, and maps.Equal compares values with ==, which panics on a slice. The duplication behind all of this is filed as #418, along with a second divergence it exposed: parser compares `required` order-sensitively while joiner and the hasher both treat it as a set. --- joiner/equivalence.go | 26 +++++++++++++++--- joiner/equivalence_test.go | 54 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/joiner/equivalence.go b/joiner/equivalence.go index af5dc887..a6b76390 100644 --- a/joiner/equivalence.go +++ b/joiner/equivalence.go @@ -2,6 +2,7 @@ package joiner import ( "fmt" + "maps" "reflect" "sort" "strconv" @@ -470,7 +471,7 @@ func compareDocFields(left, right *parser.Schema, path *comparePath, result *Equ if left.Comment != right.Comment { result.record(path, "$comment", left.Comment, right.Comment, "$comment mismatch") } - if !reflect.DeepEqual(left.ExternalDocs, right.ExternalDocs) { + if !equalExternalDocs(left.ExternalDocs, right.ExternalDocs) { result.record(path, "externalDocs", left.ExternalDocs, right.ExternalDocs, "externalDocs mismatch") } if left.Deprecated != right.Deprecated { @@ -560,7 +561,10 @@ func compareStructuralSchemaFields( if left.DynamicAnchor != right.DynamicAnchor { result.record(path, "$dynamicAnchor", left.DynamicAnchor, right.DynamicAnchor, "$dynamicAnchor mismatch") } - if !reflect.DeepEqual(left.Vocabulary, right.Vocabulary) { + // maps.Equal, not reflect.DeepEqual: this package and parser both treat a nil + // map and an empty one as equal, and DeepEqual splits them, which made a schema + // declaring `$vocabulary: {}` differ from one declaring none. + if !maps.Equal(left.Vocabulary, right.Vocabulary) { result.record(path, "$vocabulary", left.Vocabulary, right.Vocabulary, "$vocabulary mismatch") } @@ -647,7 +651,23 @@ func equalDiscriminators(left, right *parser.Discriminator) bool { } return left.PropertyName == right.PropertyName && left.DefaultMapping == right.DefaultMapping && - reflect.DeepEqual(left.Mapping, right.Mapping) + maps.Equal(left.Mapping, right.Mapping) +} + +// equalExternalDocs compares two External Documentation Objects. +// +// Mirrors parser's equalExternalDocs field by field rather than reaching for +// reflect.DeepEqual, whose comparison of the Extra map would split a nil map from +// an empty one and disagree with parser about the same pair. +func equalExternalDocs(left, right *parser.ExternalDocs) bool { + if left == nil || right == nil { + return left == right + } + return left.Description == right.Description && + left.URL == right.URL && + // EqualFunc with DeepEqual, not maps.Equal: Extra holds arbitrary JSON, and + // == on a slice value panics. Same pairing parser's equalMapStringAny uses. + maps.EqualFunc(left.Extra, right.Extra, reflect.DeepEqual) } // equalStringSliceMaps compares two name-keyed string-slice maps, order-independently. diff --git a/joiner/equivalence_test.go b/joiner/equivalence_test.go index 4d53a48a..4255685a 100644 --- a/joiner/equivalence_test.go +++ b/joiner/equivalence_test.go @@ -1579,3 +1579,57 @@ func TestCompareSchemas_SchemaOrBoolMismatch(t *testing.T) { assert.False(t, CompareSchemas(left, right, EquivalenceModeDeep).Equivalent) }) } + +// TestCompareSchemas_NilAndEmptyMapsAgreeWithParser pins deep comparison to the +// convention parser's equality already documents: "Nil and empty maps are +// considered equal." +// +// reflect.DeepEqual splits those two, so using it for a typed map made this +// package disagree with parser about the same pair of schemas. The last case +// covers why the fix is maps.EqualFunc rather than maps.Equal for Extra: +// specification extensions hold arbitrary JSON, and == on a slice value panics. +func TestCompareSchemas_NilAndEmptyMapsAgreeWithParser(t *testing.T) { + base := func() map[string]*parser.Schema { + return map[string]*parser.Schema{"p": {Type: "string"}} + } + + tests := []struct { + name string + left, right *parser.Schema + }{ + { + name: "$vocabulary absent against declared empty", + left: &parser.Schema{Type: "object", Properties: base()}, + right: &parser.Schema{Type: "object", Properties: base(), Vocabulary: map[string]bool{}}, + }, + { + name: "discriminator mapping absent against declared empty", + left: &parser.Schema{Type: "object", Properties: base(), + Discriminator: &parser.Discriminator{PropertyName: "kind"}}, + right: &parser.Schema{Type: "object", Properties: base(), + Discriminator: &parser.Discriminator{PropertyName: "kind", Mapping: map[string]string{}}}, + }, + { + name: "dependentRequired absent against declared empty", + left: &parser.Schema{Type: "object", Properties: base()}, + right: &parser.Schema{Type: "object", Properties: base(), + DependentRequired: map[string][]string{}}, + }, + { + name: "externalDocs extensions holding an uncomparable value", + left: &parser.Schema{Type: "object", Properties: base(), + ExternalDocs: &parser.ExternalDocs{URL: "https://example.com", Extra: map[string]any{"x-a": []any{1, 2}}}}, + right: &parser.Schema{Type: "object", Properties: base(), + ExternalDocs: &parser.ExternalDocs{URL: "https://example.com", Extra: map[string]any{"x-a": []any{1, 2}}}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.True(t, CompareSchemas(tt.left, tt.right, EquivalenceModeDeep).Equivalent, + "deep comparison should treat these as equivalent") + assert.True(t, tt.left.Equals(tt.right), + "parser agrees, and the two must not diverge") + }) + } +}