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/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..48c2a946 --- /dev/null +++ b/internal/driftguard/equivalence_test.go @@ -0,0 +1,69 @@ +package driftguard + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "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. + +// 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. +// +// 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) { + // 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() + 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, + "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) { + left, right := &parser.Schema{}, &parser.Schema{} + 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/fields_test.go b/internal/driftguard/fields_test.go new file mode 100644 index 00000000..1c15728b --- /dev/null +++ b/internal/driftguard/fields_test.go @@ -0,0 +1,184 @@ +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`, 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: + 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..823f2e9b --- /dev/null +++ b/internal/driftguard/hash_test.go @@ -0,0 +1,128 @@ +package driftguard + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "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) { + schema := &parser.Schema{} + 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), + "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..ffcf0680 --- /dev/null +++ b/internal/driftguard/marshal_test.go @@ -0,0 +1,148 @@ +package driftguard + +import ( + "encoding/json" + "strings" + "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. + // 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", + }, +} + +// 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) { + value, _ := subject() + 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) + } + + 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) + }) + } + } +} + +// 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..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+) @@ -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,11 @@ func (h *SchemaHasher) hashSchema(hasher hash.Hash64, schema *parser.Schema) { } } + // JSON Schema identity and dialect, value semantics, and the content keywords + h.hashIdentity(hasher, schema) + h.hashValueSemantics(hasher, schema) + h.hashContentKeywords(hasher, schema) + // Contains if schema.Contains != nil { h.writeString(hasher, "contains:") @@ -283,12 +290,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 +372,79 @@ 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.writeLabeled(hasher, "v", strconv.FormatBool(schema.Vocabulary[k])) + } +} + +// 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. 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) + } + 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/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 175fb0bd..a6b76390 100644 --- a/joiner/equivalence.go +++ b/joiner/equivalence.go @@ -2,6 +2,7 @@ package joiner import ( "fmt" + "maps" "reflect" "sort" "strconv" @@ -463,6 +464,224 @@ 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 !equalExternalDocs(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 { + // 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, rightValue, 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.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") + } + 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") + } + // 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") + } + + // 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. + 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 { + 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 && + 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. +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 +925,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) @@ -1020,18 +1242,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 @@ -1041,7 +1273,15 @@ func compareAdditionalPropertiesSchemas(left, right any, path *comparePath, resu leftSchema, leftIsSchema := left.(*parser.Schema) rightSchema, rightIsSchema := right.(*parser.Schema) if leftIsSchema && rightIsSchema { - path.push("additionalProperties") + // 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() return @@ -1052,12 +1292,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() } @@ -1065,12 +1305,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() } diff --git a/joiner/equivalence_test.go b/joiner/equivalence_test.go index 1382a774..4255685a 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,182 @@ 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) + }) +} + +// 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") + }) + } +} 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)