From 78c71e928bd0c23e10fa6bcdec3d4fd641dfd016 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 10:20:48 +0200 Subject: [PATCH 1/4] fix: report the same findings, in the same order, on every run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several checks walk a map looking for fault: definitions, paths, status codes, response headers, properties. Go randomises map iteration, so the same document could be reported differently from one run to the next — in which order the findings came out, and, where a check stops on the first fault it meets, which of two equally faulty definitions was named at all. Those walks now take their keys in sorted order. "First" therefore means first in definition-name (or path, or status-code) order. How many findings a document yields, and when a check stops, are unchanged. The circular-ancestry check in validateDuplicatePropertyNames returned on the first offender whatever the options said, so a second circular definition could never be heard from. It now returns only when ContinueOnErrors is false, and otherwise moves on to the next definition without descending into the loop it just found. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 31 ++++--- deterministic_test.go | 194 ++++++++++++++++++++++++++++++++++++++++++ example_validator.go | 31 ++++--- helpers.go | 10 ++- object_validator.go | 15 ++-- schema_props.go | 2 +- sorted.go | 39 +++++++++ spec.go | 65 +++++++++----- spec_ref_warnings.go | 2 +- 9 files changed, 335 insertions(+), 54 deletions(-) create mode 100644 deterministic_test.go create mode 100644 sorted.go diff --git a/default_validator.go b/default_validator.go index 010a10f..8ea9a1e 100644 --- a/default_validator.go +++ b/default_validator.go @@ -76,8 +76,11 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { res := validatorPools.results.Borrow() // will redeem when merged s := d.SpecValidator - for method, pathItem := range s.expandedAnalyzer().Operations() { - for path, op := range pathItem { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pathItem := operations[method] + for _, path := range sortedKeys(pathItem) { + op := pathItem[path] // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.Default != nil && param.Required { @@ -130,8 +133,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { } // Same constraint on regular Responses if op.Responses.StatusCodeResponses != nil { // Safeguard - for code, r := range op.Responses.StatusCodeResponses { - res.Merge(d.validateDefaultInResponse(&r, "response", path, method, code, op.ID)) //#nosec + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + r := op.Responses.StatusCodeResponses[code] + res.Merge(d.validateDefaultInResponse(&r, "response", path, method, code, op.ID)) } } } else if op.ID != "" { @@ -143,8 +147,10 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if s.spec.Spec().Definitions != nil { // Safeguard // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() - for nm, sch := range s.spec.Spec().Definitions { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec + definitions := s.spec.Spec().Definitions + for _, nm := range sortedKeys(definitions) { + sch := definitions[nm] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) } } return res @@ -163,7 +169,8 @@ func (d *defaultValidator) validateDefaultInResponse( responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) if response.Headers != nil { // Safeguard - for nm, h := range response.Headers { + for _, nm := range sortedKeys(response.Headers) { + h := response.Headers[nm] // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() @@ -244,11 +251,13 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegm // NOTE: we keep validating values, even though additionalItems is not supported by Swagger 2.0 (and 3.0 as well) res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } - for propName, prop := range schema.Properties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.Properties) { + prop := schema.Properties[propName] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) } - for propName, prop := range schema.PatternProperties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.PatternProperties) { + prop := schema.PatternProperties[propName] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) diff --git a/deterministic_test.go b/deterministic_test.go new file mode 100644 index 0000000..6d0a20d --- /dev/null +++ b/deterministic_test.go @@ -0,0 +1,194 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "path/filepath" + "testing" + + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// repeats is how many times a document is validated when checking that its +// report does not move. Go randomises map iteration per range statement, so a +// two-key map picks the same starting bucket often enough that a handful of +// runs would not notice; a few dozen make an unstable report near-certain to +// show itself. +const repeats = 50 + +// twoFaultyDefinitionsFixture holds one fault in each of two definitions: Pet +// requires a property it never declares (an error), Tag marks one both required +// and readOnly (a warning). +// +// Walking definitions in map order, the check stops on the first fault it meets, +// so whether Tag was heard from at all used to depend on where the map started. +const twoFaultyDefinitionsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/pets": {"get": {"operationId": "getPets", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/Pet"}}}}} + }, + "definitions": { + "Pet": {"type": "object", "required": ["notDeclared"], "properties": {"name": {"type": "string"}}}, + "Tag": {"type": "object", "required": ["readOnlyToo"], "properties": {"readOnlyToo": {"type": "string", "readOnly": true}}} + } +}` + +// twoCircularDefinitionsFixture holds two definitions that are each their own +// ancestor. Only one circular ancestry is reported, and which one it is used to +// be drawn from the map iteration order. +const twoCircularDefinitionsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/a": {"get": {"operationId": "getA", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/A"}}}}}, + "/b": {"get": {"operationId": "getB", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/B"}}}}} + }, + "definitions": { + "A": {"type": "object", "allOf": [{"$ref": "#/definitions/A"}]}, + "B": {"type": "object", "allOf": [{"$ref": "#/definitions/B"}]} + } +}` + +func TestDeterministic_RequiredDefinitions(t *testing.T) { + t.Parallel() + + t.Run("stopping on the first fault always stops on the same one", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoFaultyDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = false + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/Pet/required/0") + }) + + t.Run("reporting everything reports both definitions", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoFaultyDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = true + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/Pet/required/0") + assert.SliceContainsT(t, reports[0], "WARN /definitions/Tag/required/0") + }) +} + +func TestDeterministic_CircularAncestry(t *testing.T) { + t.Parallel() + + t.Run("the definition named is the first in name order", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoCircularDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = false + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/A") + }) + + t.Run("reporting everything reports both circular definitions", func(t *testing.T) { + t.Parallel() + + // the check used to return on the first circular ancestry whatever the + // options said, so the second definition could never be heard from + reports := repeatedReports(t, twoCircularDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = true + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/A") + assert.SliceContainsT(t, reports[0], "ERR /definitions/B") + }) +} + +// TestDeterministic_FullReport guards the whole sweep rather than the two known +// sites: a document exercising most checks must produce the very same report, +// in the very same order, on every run. +func TestDeterministic_FullReport(t *testing.T) { + t.Parallel() + + // a whole report is a much finer probe than a single finding: every map a + // check walks contributes to it, so a handful of runs suffice + const runs = 20 + + for _, fixture := range []string{ + filepath.Join("fixtures", "validation", "fixture-1231.yaml"), + filepath.Join("fixtures", "validation", "fixture-additional-items-invalid-values.yaml"), + filepath.Join("fixtures", "validation", "fixture-342.yaml"), + } { + t.Run(filepath.Base(fixture), func(t *testing.T) { + t.Parallel() + + reports := make([][]string, 0, runs) + for range runs { + doc, err := loads.Spec(fixture) + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + reports = append(reports, report(res)) + } + + require.NotEmpty(t, reports[0], "expected the fixture to yield findings") + assertSameReport(t, reports) + }) + } +} + +// repeatedReports validates the same document [repeats] times and returns one +// report per run, each in the order the checks emitted it. +func repeatedReports(t *testing.T, raw string, configure func(*SpecValidator)) [][]string { + t.Helper() + + reports := make([][]string, 0, repeats) + for range repeats { + // reloading each time so that no state carried by the document or its + // analyzer can smooth over an unstable walk + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + configure(validator) + res, _ := validator.Validate(doc) + reports = append(reports, report(res)) + } + + return reports +} + +// report renders the located findings of a result as one line each, so that two +// runs can be compared on both what they found and where they said it was. +func report(res *Result) []string { + lines := make([]string, 0, len(res.Errors)+len(res.Warnings)) + for _, located := range res.LocatedErrors() { + lines = append(lines, "ERR "+located.Pointer) + } + for _, located := range res.LocatedWarnings() { + lines = append(lines, "WARN "+located.Pointer) + } + + return lines +} + +func assertSameReport(t *testing.T, reports [][]string) { + t.Helper() + + require.NotEmpty(t, reports) + for i, got := range reports[1:] { + assert.Equal(t, reports[0], got, "run %d reported differently from run 0", i+1) + } +} diff --git a/example_validator.go b/example_validator.go index 1f830a6..9c64c73 100644 --- a/example_validator.go +++ b/example_validator.go @@ -66,8 +66,11 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { res := validatorPools.results.Borrow() s := ex.SpecValidator - for method, pathItem := range s.expandedAnalyzer().Operations() { - for path, op := range pathItem { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pathItem := operations[method] + for _, path := range sortedKeys(pathItem) { + op := pathItem[path] // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { @@ -120,8 +123,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { } // Same constraint on regular Responses if op.Responses.StatusCodeResponses != nil { // Safeguard - for code, r := range op.Responses.StatusCodeResponses { - res.Merge(ex.validateExampleInResponse(&r, "response", path, method, code, op.ID)) //#nosec + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + r := op.Responses.StatusCodeResponses[code] + res.Merge(ex.validateExampleInResponse(&r, "response", path, method, code, op.ID)) } } } else if op.ID != "" { @@ -133,8 +137,10 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if s.spec.Spec().Definitions != nil { // Safeguard // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() - for nm, sch := range s.spec.Spec().Definitions { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec + definitions := s.spec.Spec().Definitions + for _, nm := range sortedKeys(definitions) { + sch := definitions[nm] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) } } return res @@ -153,7 +159,8 @@ func (ex *exampleValidator) validateExampleInResponse( responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) if response.Headers != nil { // Safeguard - for nm, h := range response.Headers { + for _, nm := range sortedKeys(response.Headers) { + h := response.Headers[nm] // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() @@ -253,11 +260,13 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSeg // NOTE: we keep validating values, even though additionalItems is unsupported in Swagger 2.0 (and 3.0 as well) res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } - for propName, prop := range schema.Properties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.Properties) { + prop := schema.Properties[propName] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) } - for propName, prop := range schema.PatternProperties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.PatternProperties) { + prop := schema.PatternProperties[propName] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) diff --git a/helpers.go b/helpers.go index b8f331e..303c7da 100644 --- a/helpers.go +++ b/helpers.go @@ -293,15 +293,19 @@ func (h *paramHelper) safeExpandedParamsFor(path, method, operationID string, re // remove params with invalid expansion from Slice operation.Parameters = resolvedParams - for _, ppr := range s.expandedAnalyzer().SafeParamsFor(method, path, + // the analyzer keys parameters by name and location: walk those keys in + // order, so that findings about an operation's parameters come out the + // same way on every run + safeParams := s.expandedAnalyzer().SafeParamsFor(method, path, func(_ spec.Parameter, err error) bool { // since params have already been expanded, there are few causes for error res.addErrorsAt(operationPath(path, method), someParametersBrokenMsg(path, method, operationID)) // original error from analyzer res.addErrorsAt(operationPath(path, method), err) return true - }) { - params = append(params, ppr) + }) + for _, k := range sortedKeys(safeParams) { + params = append(params, safeParams[k]) } } return diff --git a/object_validator.go b/object_validator.go index 2d14557..4f1dd15 100644 --- a/object_validator.go +++ b/object_validator.go @@ -103,7 +103,8 @@ func (o *objectValidator) Validate(data any) *Result { // Check patternProperties // NOTE: it looks like we have done that twice in many cases - for key, value := range val { + for _, key := range sortedKeys(val) { + value := val[key] _, regularProperty := o.Properties[key] matched, _, patterns := o.validatePatternProperty(key, value, res) // applies to regular properties as well if regularProperty || !matched { @@ -214,7 +215,7 @@ func (o *objectValidator) precheck(res *Result, val map[string]any) { } func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res *Result) { - for k := range val { + for _, k := range sortedKeys(val) { if k == "$schema" || k == "id" { // special properties "$schema" and "id" are ignored continue @@ -263,7 +264,8 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res continue } - for headerKey, headerBody := range headers { + for _, headerKey := range sortedKeys(headers) { + headerBody := headers[headerKey] if headerBody == nil { continue } @@ -296,7 +298,8 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res } func (o *objectValidator) validateAdditionalProperties(val map[string]any, res *Result) { - for key, value := range val { + for _, key := range sortedKeys(val) { + value := val[key] _, regularProperty := o.Properties[key] if regularProperty { continue @@ -333,7 +336,7 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu validatorPools.schemas.Redeem(pSchema) }() - for pName := range o.Properties { + for _, pName := range sortedKeys(o.Properties) { *pSchema = o.Properties[pName] rName := o.Path.child(pName) @@ -392,7 +395,7 @@ func (o *objectValidator) validatePatternProperty(key string, value any, result validatorPools.schemas.Redeem(schema) }() - for k := range o.PatternProperties { + for _, k := range sortedKeys(o.PatternProperties) { re, err := compileRegexp(k) if err != nil { continue diff --git a/schema_props.go b/schema_props.go index 83a3735..d7ecb14 100644 --- a/schema_props.go +++ b/schema_props.go @@ -279,7 +279,7 @@ func (s *schemaPropsValidator) validateNot(data any, mainResult *Result) { func (s *schemaPropsValidator) validateDependencies(data any, mainResult *Result) { val := data.(map[string]any) //nolint:forcetypeassert // caller guarantees map[string]any - for key := range val { + for _, key := range sortedKeys(val) { dep, ok := s.Dependencies[key] if !ok { continue diff --git a/sorted.go b/sorted.go new file mode 100644 index 0000000..829a042 --- /dev/null +++ b/sorted.go @@ -0,0 +1,39 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "cmp" + "maps" + "slices" + "strings" + + "github.com/go-openapi/spec" +) + +// sortedKeys returns the keys of a map in ascending order. +// +// Findings are reported in the order the checks walk the document, and several +// of them walk a map: definitions, paths, response codes, headers, properties. +// Go randomises map iteration, so the same bytes validated twice would list +// their findings in a different order — and where a check stops on the first +// fault it meets, would name a different offender altogether. +// +// Walking keys in sorted order makes both defined: findings come out in +// definition-name (or path, or status-code) order, on every run. +func sortedKeys[K cmp.Ordered, V any](m map[K]V) []K { + return slices.Sorted(maps.Keys(m)) +} + +// sortedRefs orders references by the location they point at. +// +// The analyzer gathers references in a map, so the slice it hands back comes +// out in a different order on every run. See [sortedKeys]. +func sortedRefs(refs []spec.Ref) []spec.Ref { + slices.SortFunc(refs, func(a, b spec.Ref) int { + return strings.Compare(a.String(), b.String()) + }) + + return refs +} diff --git a/spec.go b/spec.go index f2ced4c..19305fa 100644 --- a/spec.go +++ b/spec.go @@ -193,7 +193,7 @@ func (s *SpecValidator) validateNonEmptyPathParamNames() *Result { return res } - for k := range s.spec.Spec().Paths.Paths { + for _, k := range sortedKeys(s.spec.Spec().Paths.Paths) { if strings.Contains(k, "{}") { res.addErrorsAt(newPathSegments(swaggerPaths, k), emptyPathParameterMsg(k)) } @@ -219,8 +219,8 @@ func (s *SpecValidator) validateDuplicateOperationIDs() *Result { known[v]++ } } - for k, v := range known { - if v > 1 { + for _, k := range sortedKeys(known) { + if v := known[k]; v > 1 { res.AddErrors(nonUniqueOperationIDMsg(k, v)) } } @@ -235,7 +235,9 @@ type dupProp struct { func (s *SpecValidator) validateDuplicatePropertyNames() *Result { // definition can't declare a property that's already defined by one of its ancestors res := validatorPools.results.Borrow() - for k, sch := range s.spec.Spec().Definitions { + definitions := s.spec.Spec().Definitions + for _, k := range sortedKeys(definitions) { + sch := definitions[k] if len(sch.AllOf) == 0 { continue } @@ -250,7 +252,14 @@ func (s *SpecValidator) validateDuplicatePropertyNames() *Result { } if len(ancs) > 0 { res.addErrorsAt(newPathSegments(swaggerDefinitions, k), circularAncestryDefinitionMsg(k, ancs)) - return res + if !s.Options.ContinueOnErrors { + return res + } + + // the ancestry loops back on itself: searching it for duplicate + // property names would not terminate, so this definition stops here + // and the next one is examined. + continue } knowns := make(map[string]struct{}) @@ -307,7 +316,7 @@ func (s *SpecValidator) validateSchemaPropertyNames(nm string, sch spec.Schema, return dups, res } - for k := range schc.Properties { + for _, k := range sortedKeys(schc.Properties) { _, ok := knowns[k] if ok { dups = append(dups, dupProp{Name: k, Definition: schn}) @@ -373,8 +382,11 @@ func (s *SpecValidator) validateItems() *Result { // validate parameter, items, schema and response objects for presence of item if type is array res := validatorPools.results.Borrow() - for method, pi := range s.analyzer.Operations() { - for path, op := range pi { + operations := s.analyzer.Operations() + for _, method := range sortedKeys(operations) { + pi := operations[method] + for _, path := range sortedKeys(pi) { + op := pi[path] for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.TypeName() == arrayType && param.ItemsTypeName() == "" { @@ -411,8 +423,11 @@ func (s *SpecValidator) validateItems() *Result { responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) } if op.Responses.StatusCodeResponses != nil { - for code, v := range op.Responses.StatusCodeResponses { - responses = append(responses, codedResponse{code: strconv.Itoa(code), resp: v}) + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + responses = append(responses, codedResponse{ + code: strconv.Itoa(code), + resp: op.Responses.StatusCodeResponses[code], + }) } } } @@ -420,8 +435,8 @@ func (s *SpecValidator) validateItems() *Result { for _, resp := range responses { at := responsePath(path, method, resp.code) // Response headers with array - for hn, hv := range resp.resp.Headers { - if hv.TypeName() == arrayType && hv.ItemsTypeName() == "" { + for _, hn := range sortedKeys(resp.resp.Headers) { + if hv := resp.resp.Headers[hn]; hv.TypeName() == arrayType && hv.ItemsTypeName() == "" { res.addErrorsAt(at.children(swaggerHeaders, hn), arrayInHeaderRequiresItemsMsg(hn, op.ID)) } } @@ -514,7 +529,7 @@ func (s *SpecValidator) validateReferencedParameters() *Result { return nil } result := validatorPools.results.Borrow() - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedParamMsg(k)) } return result @@ -538,10 +553,12 @@ func (s *SpecValidator) validateReferencedResponses() *Result { if len(expected) == 0 { return nil } + result := validatorPools.results.Borrow() - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedResponseMsg(k)) } + return result } @@ -565,7 +582,7 @@ func (s *SpecValidator) validateReferencedDefinitions() *Result { } result := new(Result) - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedDefinitionMsg(k)) } return result @@ -575,8 +592,11 @@ func (s *SpecValidator) validateRequiredDefinitions() *Result { // Each property listed in the required array must be defined in the properties of the model res := validatorPools.results.Borrow() + definitions := s.spec.Spec().Definitions + DEFINITIONS: - for d, schema := range s.spec.Spec().Definitions { + for _, d := range sortedKeys(definitions) { + schema := definitions[d] if schema.Required != nil { // Safeguard definitionAt := newPathSegments(swaggerDefinitions, d) for i, pn := range schema.Required { @@ -621,14 +641,14 @@ func (s *SpecValidator) validateRequiredProperties( // NOTE: patternProperties are not supported in swagger. Even though, we continue validation here // We check all defined patterns: if one regexp is invalid, croaks an error - for pp, pv := range v.PatternProperties { + for _, pp := range sortedKeys(v.PatternProperties) { re, err := compileRegexp(pp) if err != nil { res.addErrorsAt(schemaAt, invalidPatternMsg(pp, in)) } else if re.MatchString(path) { patternMatch = true if !propertyMatch { - isReadOnly = pv.ReadOnly + isReadOnly = v.PatternProperties[pp].ReadOnly } } } @@ -676,9 +696,12 @@ func (s *SpecValidator) validateParameters() *Result { // - path param must be required res := validatorPools.results.Borrow() rexGarbledPathSegment := mustCompileRegexp(`.*[{}\s]+.*`) - for method, pi := range s.expandedAnalyzer().Operations() { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pi := operations[method] methodPaths := make(map[string]map[string]string) - for path, op := range pi { + for _, path := range sortedKeys(pi) { + op := pi[path] if s.Options.StrictPathParamUniqueness { pathToAdd := pathHelp.stripParametersInPath(path) @@ -817,7 +840,7 @@ func (s *SpecValidator) validateParameters() *Result { func (s *SpecValidator) validateReferencesValid() *Result { // each reference must point to a valid object res := validatorPools.results.Borrow() - for _, r := range s.analyzer.AllRefs() { + for _, r := range sortedRefs(s.analyzer.AllRefs()) { if !r.IsValidURI(s.spec.SpecFilePath()) { // Safeguard - spec should always yield a valid URI res.addErrorsAt(s.refLocations.at(r.String()), invalidRefMsg(r.String())) } diff --git a/spec_ref_warnings.go b/spec_ref_warnings.go index 3c8608f..d56cb17 100644 --- a/spec_ref_warnings.go +++ b/spec_ref_warnings.go @@ -39,7 +39,7 @@ func (s *SpecValidator) validateDubiousRefs() *Result { baseDir, hasBase := s.localBaseDir() remoteHosts := make(map[string]struct{}) - for _, r := range s.analyzer.AllRefs() { + for _, r := range sortedRefs(s.analyzer.AllRefs()) { u := r.GetURL() if u == nil { // Safeguard: a valid spec always yields parseable refs continue From 62ecb2e8ac32732666c26ab28457c441c9b7fd74 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 12:41:54 +0200 Subject: [PATCH 2/4] fix: point every finding at a node the document contains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Located.Pointer is documented as a JSON pointer into the validated document, and in four families of finding it addressed nothing. Values checked by a borrowed validator. The parameter and header validators are the ones a generated client uses at runtime, where the name of the parameter is all a caller has, so they locate a finding by that name. Spec validation borrows them to check a default or an x-example, where the name addresses nothing: those findings now sit on the value's own node. A body parameter is entered through its schema, as a response already was. The path template. A parameter missing from a path template was reported under the variable name rather than the path holding it. Findings that knew their site but did not say it. A duplicate operationId is counted over the operations rather than a flat list of identifiers, so it can name the first operation declaring it. An unresolved reference is located at the first local $ref that addresses nothing, and the separate message raised while expanding a response carries that response. Faults reached through a $ref. Checks walk an expanded document, so a finding below a reference came out with a pointer descending into a node holding nothing but "$ref". Such a pointer is now followed to what the reference leads to, as many times as it takes; one that stops at the reference itself is left where it is. Three smaller gaps close with them: a document holding no paths is reported at the root rather than under /paths, the format validator records the location it holds, and a parameter too broken to be identified keeps its name in the message while the pointer stops on the array that holds it — pathToken gains a cosmetic kind, the converse of structural, for that. Finally, a location is trimmed to the deepest node the document holds. This runs once every check has had its say and only ever shortens, which is what makes the pointer always resolve. Over the fixture corpus, findings whose pointer addresses nothing go from 132 to 0, and exact locations from 1955 to 2161 of 2213. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 9 +- example_validator.go | 9 +- formats.go | 2 +- helpers.go | 5 +- param_locations.go | 6 +- param_locations_test.go | 8 +- path.go | 31 ++++- pointer_conformance_test.go | 262 ++++++++++++++++++++++++++++++++++++ pointer_resolution_test.go | 254 ++++++++++++++++++++++++++++++++++ ref_redirects.go | 71 ++++++++++ ref_redirects_test.go | 97 +++++++++++++ resolvable.go | 70 ++++++++++ resolvable_test.go | 85 ++++++++++++ result.go | 37 +++++ spec.go | 96 +++++++++++-- 15 files changed, 1014 insertions(+), 28 deletions(-) create mode 100644 pointer_conformance_test.go create mode 100644 pointer_resolution_test.go create mode 100644 ref_redirects.go create mode 100644 ref_redirects_test.go create mode 100644 resolvable.go create mode 100644 resolvable_test.go diff --git a/default_validator.go b/default_validator.go index 8ea9a1e..5902cee 100644 --- a/default_validator.go +++ b/default_validator.go @@ -95,6 +95,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Default != nil && param.Schema == nil { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, d.schemaOptions).Validate(param.Default) //#nosec + red.relocate(s.parameterPath(path, method, param.In, param.Name).child(jsonDefault)) if red.HasErrorsOrWarnings() { res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -116,7 +117,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate default value against schema - red := d.validateDefaultValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name).structuralChild(jsonSchema), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -161,13 +162,12 @@ func (d *defaultValidator) validateDefaultInResponse( ) *Result { s := d.SpecValidator - response, res := responseHelp.expandResponseRef(resp, path, s) + responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) + response, res := responseHelp.expandResponseRef(resp, path, responsePath(path, method, responseCodeAsStr), s) if !res.IsValid() { return res } - responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) - if response.Headers != nil { // Safeguard for _, nm := range sortedKeys(response.Headers) { h := response.Headers[nm] @@ -176,6 +176,7 @@ func (d *defaultValidator) validateDefaultInResponse( if h.Default != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, d.schemaOptions).Validate(h.Default) //#nosec + red.relocate(responseHeaderPath(path, method, responseCodeAsStr, nm).child(jsonDefault)) if red.HasErrorsOrWarnings() { res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.Merge(red) diff --git a/example_validator.go b/example_validator.go index 9c64c73..0e193d5 100644 --- a/example_validator.go +++ b/example_validator.go @@ -85,6 +85,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Example != nil && param.Schema == nil { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, ex.schemaOptions).Validate(param.Example) //#nosec + red.relocate(s.parameterPath(path, method, param.In, param.Name).child(swaggerExample)) if red.HasErrorsOrWarnings() { res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.MergeAsWarnings(red) @@ -106,7 +107,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate example value against schema - red := ex.validateExampleValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name).structuralChild(jsonSchema), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -151,13 +152,12 @@ func (ex *exampleValidator) validateExampleInResponse( ) *Result { s := ex.SpecValidator - response, res := responseHelp.expandResponseRef(resp, path, s) + responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) + response, res := responseHelp.expandResponseRef(resp, path, responsePath(path, method, responseCodeAsStr), s) if !res.IsValid() { // Safeguard return res } - responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) - if response.Headers != nil { // Safeguard for _, nm := range sortedKeys(response.Headers) { h := response.Headers[nm] @@ -166,6 +166,7 @@ func (ex *exampleValidator) validateExampleInResponse( if h.Example != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, ex.schemaOptions).Validate(h.Example) //#nosec + red.relocate(responseHeaderPath(path, method, responseCodeAsStr, nm).child(swaggerExample)) if red.HasErrorsOrWarnings() { res.addWarningsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.MergeAsWarnings(red) diff --git a/formats.go b/formats.go index bb58f79..23359e3 100644 --- a/formats.go +++ b/formats.go @@ -78,7 +78,7 @@ func (f *formatValidator) Validate(val any) *Result { } if err := FormatOf(f.Path.dotted(), f.In, f.Format, str, f.KnownFormats); err != nil { - result.AddErrors(err) + result.addErrorsAt(f.Path, err) } return result diff --git a/helpers.go b/helpers.go index 303c7da..0d8dea8 100644 --- a/helpers.go +++ b/helpers.go @@ -52,6 +52,7 @@ const ( swaggerResponses = "responses" swaggerParameters = "parameters" swaggerHeaders = "headers" + swaggerOperationID = "operationId" jsonMimeApplicationJSON = "application/json" ) @@ -365,7 +366,7 @@ type responseHelper struct { func (r *responseHelper) expandResponseRef( response *spec.Response, - path string, s *SpecValidator, + path string, at pathSegments, s *SpecValidator, ) (*spec.Response, *Result) { // Ensure response is expanded var err error @@ -378,7 +379,7 @@ func (r *responseHelper) expandResponseRef( } if err != nil { // Safeguard // NOTE: we may enter here when the whole response is an unresolved $ref. - errorHelp.addPointerError(res, err, response.Ref.String(), path) + errorHelp.addPointerErrorAt(res, at, err, response.Ref.String(), path) return nil, res } diff --git a/param_locations.go b/param_locations.go index e8b4b22..703d6ca 100644 --- a/param_locations.go +++ b/param_locations.go @@ -59,8 +59,8 @@ func newParamLocations(sp *spec.Swagger) paramLocations { // // A parameter the operation does not declare itself may come from its path // item. When neither knows it, which happens for a parameter too broken to be -// identified, the name stands in for the index: the location no longer -// resolves, but it still says what it is about. +// identified, the pointer stops on the array holding it and the name is kept +// for the message alone: an index no one could work out would be a guess. func (l paramLocations) at(path, method, in, name string) pathSegments { if found, isDeclared := l[paramKey{path: path, method: methodToken(method), in: in, name: name}]; isDeclared { return found @@ -70,7 +70,7 @@ func (l paramLocations) at(path, method, in, name string) pathSegments { return found } - return operationPath(path, method).children(swaggerParameters, name) + return operationPath(path, method).child(swaggerParameters).cosmeticChild(name) } func (l paramLocations) collect(sp *spec.Swagger, key paramKey, at pathSegments, params []spec.Parameter) { diff --git a/param_locations_test.go b/param_locations_test.go index 2f7971a..b308190 100644 --- a/param_locations_test.go +++ b/param_locations_test.go @@ -108,11 +108,11 @@ func TestParamLocations_UnknownParameterKeepsItsName(t *testing.T) { locations := paramLocationsOf(t) - // a parameter too broken to be identified has no index to point at, so - // the name stands in: the location no longer resolves, but a message - // built from it still says what it is about + // a parameter too broken to be identified has no index to point at, so the + // pointer stops on the array that holds it — a node the document does have + // — while the message still says which parameter it is about unknown := locations.at("/pets", methodGet, inQuery, "neverDeclared") - assert.EqualT(t, "/paths/~1pets/get/parameters/neverDeclared", unknown.pointer()) + assert.EqualT(t, "/paths/~1pets/get/parameters", unknown.pointer()) assert.EqualT(t, "paths./pets.get.parameters.neverDeclared", unknown.dotted()) } diff --git a/path.go b/path.go index 7bbe2ec..940fc4b 100644 --- a/path.go +++ b/path.go @@ -40,6 +40,12 @@ type pathToken struct { // of its members, say. It is part of the pointer and absent from the // dotted form. structural bool + + // cosmetic is the converse: a token messages name but that the document + // does not address, such as a parameter too broken to be found by name in + // the array holding it. It is part of the dotted form and absent from the + // pointer, which then stops at the deepest node the document does contain. + cosmetic bool } // readable renders a token the way a message should spell it. @@ -92,20 +98,37 @@ func (p pathSegments) structuralChild(token string) pathSegments { return p.appendToken(pathToken{token: token, structural: true}) } +// cosmeticChild returns a location that messages spell as a member named token, +// while the pointer stays on p. +func (p pathSegments) cosmeticChild(token string) pathSegments { + return p.appendToken(pathToken{token: token, cosmetic: true}) +} + func (p pathSegments) appendToken(token pathToken) pathSegments { child := make(pathSegments, len(p)+1) copy(child, p) - child[len(p)] = token + child[len(p)] = p.inherit(token) return child } +// inherit passes down what a parent token says about addressability: nothing +// below a token the document does not address is addressable either, so the +// pointer has to stop at the same place. +func (p pathSegments) inherit(token pathToken) pathToken { + if len(p) > 0 && p[len(p)-1].cosmetic { + token.cosmetic = true + } + + return token +} + // children returns the location of a chain of named members below p. func (p pathSegments) children(tokens ...string) pathSegments { child := make(pathSegments, len(p)+len(tokens)) copy(child, p) for i, token := range tokens { - child[len(p)+i] = pathToken{token: token} + child[len(p)+i] = child[:len(p)+i].inherit(pathToken{token: token}) } return child @@ -234,6 +257,10 @@ func (p pathSegments) pointer() string { var w strings.Builder for _, token := range p { + if token.cosmetic { + continue + } + w.WriteByte('/') w.WriteString(jsonpointer.Escape(token.token)) } diff --git a/pointer_conformance_test.go b/pointer_conformance_test.go new file mode 100644 index 0000000..1285675 --- /dev/null +++ b/pointer_conformance_test.go @@ -0,0 +1,262 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" +) + +// conformanceCase is one minimal document with one fault, and the node a reader +// has to go to in order to amend it. +type conformanceCase struct { + name string + doc string + // says which finding of the document the case is about + message string + pointer string +} + +// The grid below pins the location of one finding per kind of fault, over the +// whole shape of a Swagger document: the root, the info block, an operation, a +// parameter wherever it may be declared, a response, a definition. +// +// It is a conformance matrix rather than a set of regression tests. Each row +// works today; the point is that none of them may quietly stop working, since +// a location is only ever as useful as it is stable. +var conformanceCases = []conformanceCase{ + { + name: "a document with no info block", + doc: `{"swagger":"2.0","paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok"}}}}}}`, + message: "info in body is required", + pointer: "", + }, + { + name: "a document with no paths", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}}`, + message: "paths in body is required", + pointer: "", + }, + { + name: "an info block with no title", + doc: `{"swagger":"2.0","info":{"version":"1"},"paths":{}}`, + message: "info.title in body is required", + pointer: "/info", + }, + { + name: "a license with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1","license":{"url":"http://x"}},"paths":{}}`, + message: "info.license.name in body is required", + pointer: "/info/license", + }, + { + name: "external documentation with no url", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{},"externalDocs":{"description":"d"}}`, + message: "externalDocs.url in body is required", + pointer: "/externalDocs", + }, + { + name: "a tag with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{},"tags":[{"description":"d"}]}`, + message: "tags.0.name in body is required", + pointer: "/tags/0", + }, + { + name: "a response with no description", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{}}}}}}`, + message: "paths./a.get.responses.200.description in body is required", + pointer: "/paths/~1a/get/responses/200", + }, + { + name: "a shared response with no description", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"$ref":"#/responses/shared"}}}}}, + "responses":{"shared":{}}}`, + message: "responses.shared.description in body is required", + pointer: "/responses/shared", + }, + { + name: "an operation with no responses", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a"}}}}`, + message: "paths./a.get.responses in body is required", + pointer: "/paths/~1a/get", + }, + { + name: "a body parameter with no schema", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a","parameters":[{"name":"p","in":"body"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "invalid definition for parameter p in body in operation \"a\"", + pointer: "/paths/~1a/post/parameters/0", + }, + { + name: "a parameter with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"in":"query","type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "paths./a.get.parameters.0.name in body is required", + pointer: firstParam, + }, + { + name: "a shared parameter with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"$ref":"#/parameters/shared"}], + "responses":{"200":{"description":"ok"}}}}}, + "parameters":{"shared":{"name":"s","in":"query"}}}`, + message: "parameters.shared.type in body is required", + pointer: "/parameters/shared", + }, + { + name: "a path-item parameter with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"parameters":[{"name":"s","in":"query"}], + "get":{"operationId":"a","responses":{"200":{"description":"ok"}}}}}}`, + message: "paths./a.parameters.0.type in body is required", + pointer: "/paths/~1a/parameters/0", + }, + { + name: "an array parameter with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"name":"s","in":"query","type":"array"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "param \"s\" for \"a\" is a collection without an element type (array requires item definition)", + pointer: firstParam, + }, + { + name: "a response header with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "headers":{"X":{"format":"int32"}}}}}}}}`, + message: "paths./a.get.responses.200.headers.X.type in body is required", + pointer: "/paths/~1a/get/responses/200/headers/X", + }, + { + name: "an array response schema with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"type":"array"}}}}}}}`, + message: "response body for \"a\" is a collection without an element type (array requires items definition)", + pointer: "/paths/~1a/get/responses/200/schema", + }, + { + name: "an array definition property with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","properties":{"list":{"type":"array"}}}}}`, + message: "items in definitions.A.properties.list is required", + pointer: "/definitions/A/properties/list", + }, + { + name: "a security definition with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{}, + "securityDefinitions":{"k":{"name":"api_key","in":"header"}}}`, + message: "securityDefinitions.k.type in body is required", + pointer: "/securityDefinitions/k", + }, + { + name: "a required entry naming an undeclared property", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}`, + message: `"ghost" is present in required but not defined as property in definition "A"`, + pointer: "/definitions/A/required/0", + }, + { + name: "a path parameter absent from the path template", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"id","in":"path","required":true,"type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `path param "id" is not present in path "/a"`, + pointer: "/paths/~1a", + }, + { + name: "more than one body parameter", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a","parameters":[ + {"name":"one","in":"body","schema":{"type":"string"}}, + {"name":"two","in":"body","schema":{"type":"string"}}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `operation "a" has more than 1 body param: ["one" "two"]`, + pointer: "/paths/~1a/post", + }, + { + name: "a nested array parameter with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"s","in":"query","type":"array","items":{"type":"array"}}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "items in paths./a.get.parameters.s.items is required", + pointer: "/paths/~1a/get/parameters/0/items", + }, + { + name: "an array additionalProperties with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","additionalProperties":{"type":"array"}}}}`, + message: "items in definitions.A.additionalProperties is required", + pointer: "/definitions/A/additionalProperties", + }, + { + name: "an array property of an allOf member with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"allOf":[{"type":"object","properties":{"l":{"type":"array"}}}]}}}`, + message: "items in definitions.A.allOf.0.properties.l is required", + pointer: "/definitions/A/allOf/0/properties/l", + }, + { + name: "a property declared twice through allOf", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/Child"}}}}}}, + "definitions":{ + "Base":{"type":"object","properties":{"dup":{"type":"string"}}}, + "Child":{"allOf":[{"$ref":"#/definitions/Base"},{"type":"object","properties":{"dup":{"type":"string"}}}]}}}`, + message: `definition "Child" contains duplicate properties: [Child.dup]`, + pointer: "/definitions/Child", + }, + { + name: "a parameter declared twice in an operation", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[ + {"name":"s","in":"query","type":"string"}, + {"name":"s","in":"query","type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `duplicate parameter name "s" for "query" in operation "a"`, + // the second declaration is the offending one + pointer: "/paths/~1a/get/parameters/1", + }, +} + +func TestPointerConformance(t *testing.T) { + t.Parallel() + + for _, testCase := range conformanceCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, testCase.doc) + + matched := false + for message, pointer := range found { + if message != testCase.message { + continue + } + + matched = true + assert.EqualT(t, testCase.pointer, pointer, "for %q", message) + } + + assert.TrueT(t, matched, "expected a finding containing %q, got %v", testCase.message, found) + }) + } +} diff --git a/pointer_resolution_test.go b/pointer_resolution_test.go new file mode 100644 index 0000000..098c7ad --- /dev/null +++ b/pointer_resolution_test.go @@ -0,0 +1,254 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/go-openapi/jsonpointer" + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// The tests below pin down that a reported location addresses a node the +// document contains, and which node that is. +// +// They exist because a check walks an expanded, model-level view of a +// specification: parameters merged in from a path item, schemata reached +// through a $ref, values validated by the runtime validators a generated client +// uses. Each of those used to produce a pointer that walked off the document. + +// resolvedPointers pairs every finding of a document with the pointer reported +// for it, having first verified that the pointer addresses something. +type resolvedPointers map[string]string + +func TestPointerResolution_ValueBelowItsHolder(t *testing.T) { + t.Parallel() + + t.Run("a simple parameter default is located on the default", func(t *testing.T) { + t.Parallel() + + // the parameter is named zzz so that a pointer built from the name + // rather than from the document is unmistakable + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"zzz","in":"query","type":"integer","default":"nope"}], + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/parameters/0/default", + found[`zzz in query must be of type integer: "string"`]) + assert.EqualT(t, "/paths/~1a/get/parameters/0", + found["default value for zzz in query does not validate its schema"]) + }) + + t.Run("a response header default is located on the default", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "headers":{"X-Count":{"type":"integer","default":"nope"}}}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/responses/200/headers/X-Count/default", + found[`X-Count in header must be of type integer: "string"`]) + }) + + t.Run("a body parameter is entered through its schema", func(t *testing.T) { + t.Parallel() + + // the response side gained this token first; a body parameter holds its + // schema under the same member and needs the same one + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a", + "parameters":[{"name":"payload","in":"body", + "schema":{"type":"object","properties":{"n":{"type":"integer","default":"nope"}}}}], + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/post/parameters/0/schema/properties/n/default", + found[`paths./a.post.parameters.payload.n.default in body must be of type integer: "string"`]) + }) +} + +func TestPointerResolution_PathTemplate(t *testing.T) { + t.Parallel() + + // the variable name used to stand where the path key belongs, unescaped: + // ghostvar makes it plain which token was taken + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/deep/nested/{ghostvar}":{"get":{"operationId":"a", + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1deep~1nested~1{ghostvar}", + found[`path param "{ghostvar}" has no parameter definition`]) +} + +func TestPointerResolution_FindingsWithADefiniteSite(t *testing.T) { + t.Parallel() + + t.Run("a duplicate operationId names the first operation using it", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"dup","responses":{"200":{"description":"ok"}}}}, + "/b":{"get":{"operationId":"dup","responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/operationId", found[`"dup" is defined 2 times`]) + }) + + t.Run("an unresolvable reference is located where it is declared", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/Missing"}}}}}}}`) + + // expansion reports the whole document in one message, so the finding + // is given the site of the reference it could not follow + require.NotEmpty(t, found) + for message, pointer := range found { + if strings.Contains(message, "could not be resolved") { + assert.EqualT(t, "/paths/~1a/get/responses/200/schema", pointer) + } + } + }) +} + +func TestPointerResolution_ThroughARef(t *testing.T) { + t.Parallel() + + // the response is a bare $ref, so nothing exists below it: the location has + // to be followed to what the document does hold, hopping twice on the way + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"$ref":"#/responses/shared"}}}}}, + "responses":{"shared":{"description":"ok","schema":{"$ref":"#/definitions/A"}}}, + "definitions":{"A":{"type":"object","default":{"n":"not-an-object"}, + "properties":{"n":{"type":"object"}}}}}`) + + assert.EqualT(t, "/definitions/A/default/n", + found[`paths./a.get.responses.200.default.n in body must be of type object: "string"`]) + assert.EqualT(t, "/paths/~1a/get/responses/200", + found[`in operation "a", default value in response 200 does not validate its schema`]) +} + +// TestPointerResolution_EveryFixture is the standing guarantee: no document in +// the corpus may report a location that addresses nothing. +func TestPointerResolution_EveryFixture(t *testing.T) { + t.Parallel() + + var files []string + for _, pattern := range []string{ + filepath.Join("fixtures", "validation", "*.json"), + filepath.Join("fixtures", "validation", "*.yaml"), + filepath.Join("fixtures", "bugs", "*", "*.json"), + filepath.Join("fixtures", "bugs", "*", "*.yaml"), + filepath.Join("fixtures", "go-swagger", "*", "*", "*.json"), + filepath.Join("fixtures", "go-swagger", "*", "*", "*.yaml"), + filepath.Join("fixtures", "petstore", "*.json"), + } { + matched, err := filepath.Glob(pattern) + require.NoError(t, err) + files = append(files, matched...) + } + require.NotEmpty(t, files) + + var specs, findings int + for _, file := range files { + doc, err := loads.Spec(file) + if err != nil { + // a fixture that does not even load says nothing about locations + continue + } + + var document any + if err := json.Unmarshal(doc.Raw(), &document); err != nil { + continue + } + + func() { + // a handful of fixtures are deliberately degenerate + defer func() { _ = recover() }() + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + if res == nil { + return + } + specs++ + + for _, located := range append(res.LocatedErrors(), res.LocatedWarnings()...) { + findings++ + assert.TrueT(t, addresses(document, located.Pointer), + "%s: %q addresses nothing (%v)", filepath.Base(file), located.Pointer, located.Err) + } + }() + } + + t.Logf("%d specifications, %d findings", specs, findings) + require.NotZero(t, findings) +} + +// locatedFindings validates a document and returns its findings by message, +// asserting first that every pointer addresses a node the document holds. +func locatedFindings(t *testing.T, raw string) resolvedPointers { + t.Helper() + + var document any + require.NoError(t, json.Unmarshal([]byte(raw), &document)) + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + require.NotNil(t, res) + + found := make(resolvedPointers) + for _, located := range append(res.LocatedErrors(), res.LocatedWarnings()...) { + assert.TrueT(t, addresses(document, located.Pointer), + "%q addresses nothing (%v)", located.Pointer, located.Err) + found[located.Err.Error()] = located.Pointer + } + + return found +} + +// addresses walks a JSON pointer over a decoded document, independently of the +// code that produced it. +func addresses(document any, pointer string) bool { + if pointer == "" { + return true // the whole document + } + if !strings.HasPrefix(pointer, "/") { + return false + } + + node := document + for token := range strings.SplitSeq(pointer[1:], "/") { + switch held := node.(type) { + case map[string]any: + member, isHeld := held[jsonpointer.Unescape(token)] + if !isHeld { + return false + } + node = member + case []any: + index, err := strconv.Atoi(token) + if err != nil || index < 0 || index >= len(held) { + return false + } + node = held[index] + default: + return false + } + } + + return true +} diff --git a/ref_redirects.go b/ref_redirects.go new file mode 100644 index 0000000..95fe96c --- /dev/null +++ b/ref_redirects.go @@ -0,0 +1,71 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/analysis" +) + +// maxRefHops bounds how many $ref a pointer may be followed through, so that a +// document referring to itself cannot spin here. +const maxRefHops = 10 + +// refRedirects maps the location of a $ref to the location it points at, for +// the local references of a document. +// +// Checks walk the expanded document, so a finding below a $ref comes out with a +// pointer that descends into a node the authored document does not contain: a +// bare "$ref" member has nothing under it. Following the reference turns such a +// pointer back into one the document addresses. +type refRedirects map[string]string + +func newRefRedirects(analyzer *analysis.Spec) refRedirects { + redirects := make(refRedirects) + for location, ref := range analyzer.AllRefsByLocation() { + target := ref.String() + if !strings.HasPrefix(target, "#/") { + // only a local reference has a location in this document + continue + } + + redirects[strings.TrimPrefix(location, "#")] = strings.TrimPrefix(target, "#") + } + + return redirects +} + +// through rewrites a pointer that descends below a $ref. +// +// A pointer that stops at the $ref itself is left alone: that node exists, and +// it is where a reader has to go to amend the reference. +func (r refRedirects) through(pointer string) string { + if len(r) == 0 { + return pointer + } + + for range maxRefHops { + prefix, rest, ok := r.crossing(pointer) + if !ok { + return pointer + } + + pointer = prefix + rest + } + + return pointer +} + +// crossing finds the longest prefix of pointer that holds a $ref, and returns +// the location that reference points at together with what is left below it. +func (r refRedirects) crossing(pointer string) (target, rest string, ok bool) { + for at := strings.LastIndex(pointer, "/"); at > 0; at = strings.LastIndex(pointer[:at], "/") { + if target, isRef := r[pointer[:at]]; isRef { + return target, pointer[at:], true + } + } + + return "", "", false +} diff --git a/ref_redirects_test.go b/ref_redirects_test.go new file mode 100644 index 0000000..ddb7474 --- /dev/null +++ b/ref_redirects_test.go @@ -0,0 +1,97 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/analysis" + "github.com/go-openapi/loads" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +const chainedRefsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/a": {"get": {"operationId": "a", + "responses": {"200": {"$ref": "#/responses/shared"}}}}, + "/loop": {"get": {"operationId": "loop", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/Recursive"}}}}} + }, + "responses": {"shared": {"description": "ok", "schema": {"$ref": "#/definitions/A"}}}, + "definitions": { + "A": {"type": "object", "properties": {"n": {"type": "string"}}}, + "Recursive": {"type": "object", "properties": {"self": {"$ref": "#/definitions/Recursive"}}} + } +}` + +func redirectsOf(t *testing.T) refRedirects { + t.Helper() + + doc, err := loads.Analyzed(json.RawMessage(chainedRefsFixture), "") + require.NoError(t, err) + + return newRefRedirects(analysis.New(doc.Spec())) +} + +func TestRefRedirects_FollowsToWhatTheDocumentHolds(t *testing.T) { + t.Parallel() + + redirects := redirectsOf(t) + + t.Run("a pointer below a $ref is followed, as many times as it takes", func(t *testing.T) { + t.Parallel() + + // the response is a $ref to a shared response whose schema is itself a + // $ref: two hops before the pointer lands on something written down + assert.EqualT(t, "/definitions/A/properties/n", + redirects.through("/paths/~1a/get/responses/200/schema/properties/n")) + }) + + t.Run("a pointer that stops on the $ref is left alone", func(t *testing.T) { + t.Parallel() + + // that node exists, and it is where a reader goes to amend the reference + assert.EqualT(t, "/paths/~1a/get/responses/200", + redirects.through("/paths/~1a/get/responses/200")) + }) + + t.Run("a pointer that crosses no $ref is left alone", func(t *testing.T) { + t.Parallel() + + assert.EqualT(t, "/definitions/A/properties/n", + redirects.through("/definitions/A/properties/n")) + assert.EqualT(t, "", redirects.through("")) + }) + + t.Run("a definition referring to itself does not spin", func(t *testing.T) { + t.Parallel() + + // each hop shortens the pointer by "properties/self", so this bottoms + // out; the hop bound is what guarantees it whatever the document says + assert.EqualT(t, "/definitions/Recursive/properties/self", + redirects.through("/definitions/Recursive/properties/self/properties/self")) + }) +} + +func TestRefRedirects_IgnoresRemoteReferences(t *testing.T) { + t.Parallel() + + doc, err := loads.Analyzed(json.RawMessage(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {"/a": {"get": {"operationId": "a", + "responses": {"200": {"description": "ok", "schema": {"$ref": "elsewhere.json#/definitions/A"}}}}}} +}`), "") + require.NoError(t, err) + + // nothing in this document says what a remote reference leads to, so the + // pointer is left as it stands and trimming has the last word + redirects := newRefRedirects(analysis.New(doc.Spec())) + assert.EqualT(t, "/paths/~1a/get/responses/200/schema/properties/n", + redirects.through("/paths/~1a/get/responses/200/schema/properties/n")) +} diff --git a/resolvable.go b/resolvable.go new file mode 100644 index 0000000..bbd35d3 --- /dev/null +++ b/resolvable.go @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strconv" + "strings" + + "github.com/go-openapi/jsonpointer" +) + +// resolvable trims a pointer down to the deepest node the document holds. +// +// Checks walk an expanded, model-level view of a specification, which holds +// members the document itself never wrote: a parameter merged in from a path +// item, a member of a schema reached through a $ref. A pointer built along the +// way may therefore end on something a reader cannot go to. +// +// Trimming is the last word on a location, applied once every check has had its +// say: it only ever shortens, so a pointer that already addressed a node comes +// back untouched, and one that did not still says as much as it truthfully can. +// This is what makes [Located.Pointer] always resolve. +func (s *SpecValidator) resolvable(pointer string) string { + if pointer == "" || s.document == nil { + return pointer + } + + node := s.document + for at := 0; at < len(pointer); { + end := strings.IndexByte(pointer[at+1:], '/') + token := pointer[at+1:] + if end >= 0 { + token = pointer[at+1 : at+1+end] + } + + member, isHeld := memberOf(node, jsonpointer.Unescape(token)) + if !isHeld { + return pointer[:at] + } + node = member + + if end < 0 { + break + } + at += end + 1 + } + + return pointer +} + +// memberOf returns the member a reference token addresses in a decoded JSON +// node, and whether the node holds one at all. +func memberOf(node any, token string) (any, bool) { + switch held := node.(type) { + case map[string]any: + member, isHeld := held[token] + + return member, isHeld + case []any: + index, err := strconv.Atoi(token) + if err != nil || index < 0 || index >= len(held) { + return nil, false + } + + return held[index], true + default: + return nil, false + } +} diff --git a/resolvable_test.go b/resolvable_test.go new file mode 100644 index 0000000..687319f --- /dev/null +++ b/resolvable_test.go @@ -0,0 +1,85 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +const ( + firstParam = "/paths/~1a/get/parameters/0" + firstParamName = firstParam + "/name" +) + +func validatorHoldingDocument(t *testing.T, raw string) *SpecValidator { + t.Helper() + + var document any + require.NoError(t, json.Unmarshal([]byte(raw), &document)) + + return &SpecValidator{document: document} +} + +func TestResolvable_TrimsToWhatTheDocumentHolds(t *testing.T) { + t.Parallel() + + s := validatorHoldingDocument(t, `{ + "paths": {"/a": {"get": {"parameters": [{"name": "p", "in": "query"}]}}}, + "definitions": {"a~b": {"type": "object"}} +}`) + + for _, testCase := range []struct { + name string + pointer string + want string + }{ + {"a pointer that addresses a node is untouched", firstParamName, firstParamName}, + {"a member the node does not hold is cut off", firstParam + "/type", firstParam}, + {"everything below the first miss goes too", firstParam + "/schema/properties/n", firstParam}, + {"an index past the end of an array is cut off", "/paths/~1a/get/parameters/1", "/paths/~1a/get/parameters"}, + {"a member of a scalar is cut off", firstParamName + "/deeper", firstParamName}, + {"an escaped token is unescaped before the lookup", "/definitions/a~0b/type", "/definitions/a~0b/type"}, + {"a miss at the first token leaves the root", "/nowhere/at/all", ""}, + {"the root is the root", "", ""}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + assert.EqualT(t, testCase.want, s.resolvable(testCase.pointer)) + }) + } +} + +func TestResolvable_WithoutADocumentSaysNothing(t *testing.T) { + t.Parallel() + + // nothing is known about what holds what, so no location may be shortened + s := new(SpecValidator) + assert.EqualT(t, "/paths/~1a", s.resolvable("/paths/~1a")) +} + +func TestPathSegments_CosmeticTokensStayOutOfThePointer(t *testing.T) { + t.Parallel() + + at := newPathSegments(swaggerPaths, "/a", "get").child(swaggerParameters).cosmeticChild("broken") + + assert.EqualT(t, "/paths/~1a/get/parameters", at.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken", at.dotted()) + + t.Run("nothing below a cosmetic token is addressable either", func(t *testing.T) { + t.Parallel() + + below := at.child(jsonType) + assert.EqualT(t, "/paths/~1a/get/parameters", below.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken.type", below.dotted()) + + deeper := at.children(jsonSchema, jsonProperties) + assert.EqualT(t, "/paths/~1a/get/parameters", deeper.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken.schema.properties", deeper.dotted()) + }) +} diff --git a/result.go b/result.go index a4834b4..9b874a7 100644 --- a/result.go +++ b/result.go @@ -402,6 +402,43 @@ func (r *Result) addLocatedWarnings(pointer string, warnings ...error) { } } +// relocate rewrites every location this result recorded. +// +// The parameter and header validators are the ones a generated client uses at +// runtime, so they locate a finding by the name of the parameter or header it +// concerns: a name is all the caller has. When spec validation borrows them to +// check a default or an example, that name addresses nothing in the document, +// and the value's own node is the best location available for everything the +// borrowed validator found. +func (r *Result) relocate(at pathSegments) { + if r == nil { + return + } + + pointer := at.pointer() + r.errorLocations = fillLocations(r.errorLocations[:0], len(r.Errors), pointer) + r.warningLocations = fillLocations(r.warningLocations[:0], len(r.Warnings), pointer) +} + +// fillLocations records the same location for a whole run of findings. +func fillLocations(locations []string, count int, pointer string) []string { + for range count { + locations = append(locations, pointer) + } + + return locations +} + +// redirect rewrites every location this result recorded with the given mapping. +func (r *Result) redirect(through func(string) string) { + for i, pointer := range r.errorLocations { + r.errorLocations[i] = through(pointer) + } + for i, pointer := range r.warningLocations { + r.warningLocations[i] = through(pointer) + } +} + // carryErrors adds errors from another result as errors, one by one, so that // each keeps the location that result recorded for it. func (r *Result) carryErrors(errs []error, locations []string) { diff --git a/spec.go b/spec.go index 19305fa..6d5b333 100644 --- a/spec.go +++ b/spec.go @@ -54,7 +54,9 @@ type SpecValidator struct { analyzer *analysis.Spec expanded *loads.Document refLocations refLocations + refRedirects refRedirects paramLocations paramLocations + document any // the document as decoded, to tell what it holds KnownFormats strfmt.Registry Options Opts // validation options schemaOptions *SchemaValidatorOptions @@ -107,6 +109,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // where each operation declares its parameters: the document addresses // them by index, and expansion loses that s.paramLocations = newParamLocations(sd.Spec()) + // where each $ref leads: checks walk the expanded document, and a finding + // below a $ref has to be brought back to a node the document contains + s.refRedirects = newRefRedirects(s.analyzer) // Raw spec unmarshalling errors var obj any @@ -115,8 +120,13 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // So this one is just a paranoid check on the behavior of the spec package panic(InvalidDocumentError) } + s.document = obj defer func() { + // bring findings reached through a $ref back onto the document, then + // hold every location to what the document actually addresses + errs.redirect(s.refRedirects.through) + errs.redirect(s.resolvable) // errs holds all errors and warnings, // warnings only warnings errs.MergeAsWarnings(warnings) @@ -180,8 +190,9 @@ func (s *SpecValidator) SetContinueOnErrors(c bool) { func (s *SpecValidator) validateNonEmptyPathParamNames() *Result { res := validatorPools.results.Borrow() if s.spec.Spec().Paths == nil { - // There is no Paths object: error - res.addErrorsAt(newPathSegments(swaggerPaths), noValidPathMsg()) + // There is no Paths object: the document itself is what lacks it, so + // there is no node below it to point at + res.addErrorsAt(rootPath(), noValidPathMsg()) return res } @@ -213,20 +224,54 @@ func (s *SpecValidator) validateDuplicateOperationIDs() *Result { analyzer = s.analyzer } res := validatorPools.results.Borrow() + + // the message says how many times an identifier is used, so the count is + // what it needs; a reader needs somewhere to go, so the first operation to + // declare the identifier is remembered along with it known := make(map[string]int) - for _, v := range analyzer.OperationIDs() { - if v != "" { - known[v]++ + declaredAt := make(map[string]pathSegments) + operations := analyzer.Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + id := operationIdentity(method, path, op) + known[id]++ + if _, isKnown := declaredAt[id]; !isKnown { + declaredAt[id] = operationIDPath(path, method, op) + } } } + for _, k := range sortedKeys(known) { if v := known[k]; v > 1 { - res.AddErrors(nonUniqueOperationIDMsg(k, v)) + res.addErrorsAt(declaredAt[k], nonUniqueOperationIDMsg(k, v)) } } return res } +// operationIdentity names an operation the way the analyzer does: by its +// operationId, or by method and path when it declares none. +func operationIdentity(method, path string, op *spec.Operation) string { + if op == nil || op.ID == "" { + return strings.ToUpper(method) + " " + path + } + + return op.ID +} + +// operationIDPath locates the operationId of an operation, or the operation +// itself when it declares none. +func operationIDPath(path, method string, op *spec.Operation) pathSegments { + at := operationPath(path, method) + if op == nil || op.ID == "" { + return at + } + + return at.child(swaggerOperationID) +} + type dupProp struct { Name string Definition string @@ -485,7 +530,7 @@ func (s *SpecValidator) validatePathParamPresence(path string, fromPath, fromOpe } } if !matched { - res.addErrorsAt(newPathSegments(swaggerPaths, l), noParameterInPathMsg(l)) + res.addErrorsAt(newPathSegments(swaggerPaths, path), noParameterInPathMsg(l)) } } @@ -854,13 +899,48 @@ func (s *SpecValidator) validateReferencesValid() *Result { // is set, this is a no-op: loads falls back to the document's own loader. exp, err := s.spec.Expanded(s.schemaOptions.expandOptions("")) if err != nil { - res.AddErrors(unresolvedReferencesMsg(err)) + res.addErrorsAt(s.firstUnresolvableRef(), unresolvedReferencesMsg(err)) } s.expanded = exp } return res } +// firstUnresolvableRef locates the declaration of the first local $ref, in +// document order, that points at a node the document does not hold. +// +// Expansion reports the whole document in a single message, naming only the +// reference it happened to trip on, so the finding has no location of its own. +// A document usually has one broken reference; when it has several, this is the +// first one a reader would meet. +func (s *SpecValidator) firstUnresolvableRef() pathSegments { + first := rootPath() + found := false + + for _, r := range s.analyzer.AllRefs() { + value := r.String() + if !strings.HasPrefix(value, "#/") { + // a remote reference cannot be checked against the document alone + continue + } + + pointer, err := jsonpointer.New(strings.TrimPrefix(value, "#")) + if err != nil { + continue + } + if _, _, err := pointer.Get(s.document); err == nil { + continue + } + + at := s.refLocations.at(value) + if !found || at.pointer() < first.pointer() { + first, found = at, true + } + } + + return first +} + func (s *SpecValidator) checkUniqueParams(path, method string, op *spec.Operation) *Result { // Check for duplicate parameters declaration in param section. // Each parameter should have a unique `name` and `type` combination From 7cdf3d90d2de2f47f95abc3af706fe33aae5e2f0 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 13:14:52 +0200 Subject: [PATCH 3/4] feat: check required entries of nested schemas, not only of a definition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A definition is not the only place a document says an object must hold a property: so does the schema of a property, of an array item, of an additionalProperties. Each is a self-contained object definition, and a required entry naming something it never declares is the same modelling slip wherever it sits. Only the top of a definition was checked, so the slip went unreported one level down. The check now walks the schemas a definition holds inline, and locates each finding on the offending entry of the required array it walked into, e.g. /definitions/A/properties/inner/required/0. A finding about such a schema names it by the way down to it — schema "A.inner" — rather than naming the definition holding it, which would send a reader to the wrong place. A definition still names itself, in the words it always used. Two boundaries keep the walk honest. A schema written as a $ref is left alone: it is checked where it is defined, and following it would report the same slip twice and would not terminate on a recursive definition. And inside allOf, anyOf, oneOf or not, a member is a fragment of a constraint rather than a complete definition — its required entries speak of the instance the whole composition describes, are legitimately met by a sibling member or by no declaration at all, and are enforced when data is validated. Their own member schemas are still walked. That second boundary also fixes a false positive that predates the walk: a definition requiring a property contributed by one of its allOf members, or by a base definition it refers to, was reported as requiring something it does not declare. Over the fixture corpus, which includes the Kubernetes and Bitbucket APIs, this reports neither more nor less than before: the specifications that do carry nested required entries declare them correctly. Signed-off-by: Frederic BIDON --- .gitignore | 1 + .golangci.yml | 2 + helpers.go | 3 + required_walk.go | 179 ++++++++++++++++++++++++++++++++++ required_walk_test.go | 218 ++++++++++++++++++++++++++++++++++++++++++ result.go | 16 ++-- spec.go | 36 +++---- spec_messages.go | 7 ++ 8 files changed, 431 insertions(+), 31 deletions(-) create mode 100644 required_walk.go create mode 100644 required_walk_test.go diff --git a/.gitignore b/.gitignore index d8f4186..fbb78de 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ .idea .env .mcp.json +.worktrees diff --git a/.golangci.yml b/.golangci.yml index 4d6b36e..b8875d7 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,6 +7,8 @@ linters: - gochecknoglobals - gochecknoinits - godox + - gomodguard + - gomodguard_v2 - exhaustruct - ireturn - nlreturn diff --git a/helpers.go b/helpers.go index 0d8dea8..62deb97 100644 --- a/helpers.go +++ b/helpers.go @@ -44,6 +44,9 @@ const ( jsonDefault = "default" jsonAllOf = "allOf" + jsonAnyOf = "anyOf" + jsonOneOf = "oneOf" + jsonNot = "not" jsonAdditionalItems = "additionalItems" jsonAdditionalProperties = "additionalProperties" diff --git a/required_walk.go b/required_walk.go new file mode 100644 index 0000000..b3fd3cd --- /dev/null +++ b/required_walk.go @@ -0,0 +1,179 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/errors" + "github.com/go-openapi/spec" +) + +// schemaIdentity is how a message refers to the schema a required entry +// belongs to. +// +// A definition is named on its own, the way it always has been. A schema the +// definition holds is named by the way down to it, relative to the definition: +// "A.inner" rather than "A", which would send a reader to the wrong place. +type schemaIdentity struct { + name string + nested bool +} + +// identify names the schema a location leads to. +func identify(at pathSegments) schemaIdentity { + const definitionDepth = 2 // "definitions", then the name of one + + return schemaIdentity{ + name: strings.TrimPrefix(at.dotted(), swaggerDefinitions+"."), + nested: len(at) > definitionDepth, + } +} + +func (i schemaIdentity) requiredButNotDefined(property string) errors.Error { + if i.nested { + return requiredButNotDefinedInSchemaMsg(property, i.name) + } + + return requiredButNotDefinedMsg(property, i.name) +} + +// maxCompositionHops bounds how far the search for a declared property follows +// allOf members and the local $ref they may be written as. +const maxCompositionHops = 20 + +// walkRequired checks the required entries of a schema, then those of every +// schema it holds inline. +// +// A definition is not the only place a document says an object must hold a +// property: so does the schema of a property, of an array item, of an +// additionalProperties. Each of those is a self-contained object definition, +// and a required entry naming something it never declares is the same slip +// wherever it sits. +// +// A schema written as a $ref is left alone: it is checked where it is defined, +// and following it here would report the same slip twice and, for a recursive +// definition, would not terminate. +// +// It reports whether the walk should carry on, which is how the caller stops on +// the first fault unless it was asked for everything. +func (s *SpecValidator) walkRequired(at pathSegments, v *spec.Schema, res *Result) bool { + if v == nil || v.Ref.String() != "" { + return true + } + + for i, pn := range v.Required { + // the offending entry of the required array, not the schema holding + // it: that is what a reader has to go and amend + red := s.validateRequiredProperties(pn, identify(at), at, at.child(jsonRequired).item(i), v) + // NOTE: capture validity before merging: Merge may redeem `red` to the + // pool (wantsRedeemOnMerge), after which reading it races with a + // concurrent BorrowResult().cleared() in another goroutine. + isValid := red.IsValid() + res.Merge(red) + if !isValid && !s.Options.ContinueOnErrors { + return false + } + } + + return s.walkInlineSchemas(at, v, res) +} + +// walkInlineSchemas descends into every schema a schema holds, without checking +// the required entries of a composition member. +// +// Inside allOf, anyOf, oneOf or not, a member is a fragment of a constraint +// rather than a complete definition: its required entries speak of the instance +// the whole composition describes, and are legitimately met by a sibling member +// or by no declaration at all. Those are honoured when data is validated, and +// saying anything about them here would be wrong. Their own members are still +// walked, because a property schema nested in one of them is a definition like +// any other. +func (s *SpecValidator) walkInlineSchemas(at pathSegments, v *spec.Schema, res *Result) bool { + for _, name := range sortedKeys(v.Properties) { + held := v.Properties[name] + if !s.walkRequired(at.structuralChild(jsonProperties).child(name), &held, res) { + return false + } + } + + for _, pattern := range sortedKeys(v.PatternProperties) { + held := v.PatternProperties[pattern] + if !s.walkRequired(at.structuralChild(jsonPatternProperties).child(pattern), &held, res) { + return false + } + } + + if v.Items != nil { + if v.Items.Schema != nil && !s.walkRequired(at.child(jsonItems), v.Items.Schema, res) { + return false + } + for i := range v.Items.Schemas { + if !s.walkRequired(at.child(jsonItems).item(i), &v.Items.Schemas[i], res) { + return false + } + } + } + + if v.AdditionalProperties != nil && v.AdditionalProperties.Schema != nil && + !s.walkRequired(at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema, res) { + return false + } + + // a composition member is walked for the schemas it holds, never for its + // own required entries + for _, composition := range []struct { + keyword string + members []spec.Schema + }{ + {jsonAllOf, v.AllOf}, + {jsonAnyOf, v.AnyOf}, + {jsonOneOf, v.OneOf}, + } { + for i := range composition.members { + if !s.walkInlineSchemas(at.child(composition.keyword).item(i), &composition.members[i], res) { + return false + } + } + } + + if v.Not != nil { + return s.walkInlineSchemas(at.child(jsonNot), v.Not, res) + } + + return true +} + +// declaresProperty reports whether a schema, or any schema composed into it by +// allOf, declares the named property, and whether that declaration is readOnly. +// +// An allOf member may be written as a $ref, which is followed here: a property +// contributed by a base definition is declared just as plainly as one written +// in place. +func (s *SpecValidator) declaresProperty(v *spec.Schema, name string, hops int) (readOnly, declared bool) { + if v == nil || hops <= 0 { + return false, false + } + + if held, ok := v.Properties[name]; ok { + return held.ReadOnly, true + } + + for i := range v.AllOf { + member := &v.AllOf[i] + if member.Ref.String() != "" { + resolved, err := s.resolveRef(&member.Ref) + if err != nil { + continue + } + member = resolved + } + + if readOnly, ok := s.declaresProperty(member, name, hops-1); ok { + return readOnly, true + } + } + + return false, false +} diff --git a/required_walk_test.go b/required_walk_test.go new file mode 100644 index 0000000..1809b97 --- /dev/null +++ b/required_walk_test.go @@ -0,0 +1,218 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" +) + +// definitionsDoc wraps a definitions block into the smallest document that +// reaches it, so that a case reads as the definitions it is about. +func definitionsDoc(definitions string) string { + return `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":` + definitions + `}` +} + +// A required entry naming a property the schema never declares is a modelling +// slip wherever the schema sits, not only at the top of a definition. +func TestRequiredWalk_DescendsIntoNestedSchemas(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + // the message names the schema by the way down to it, so that it does + // not send a reader to the definition holding it + schema string + pointer string + }{ + { + name: "the schema of a property", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}}`, + schema: "A.inner", + pointer: "/definitions/A/properties/inner/required/0", + }, + { + name: "the schema of an array item", + definitions: `{"A":{"type":"object","properties":{ + "list":{"type":"array","items":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}}}`, + schema: "A.list.items", + pointer: "/definitions/A/properties/list/items/required/0", + }, + { + name: "the schema of additionalProperties", + definitions: `{"A":{"type":"object","additionalProperties": + {"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}`, + schema: "A.additionalProperties", + pointer: "/definitions/A/additionalProperties/required/0", + }, + { + name: "a property schema nested in an allOf member", + definitions: `{"A":{"allOf":[{"type":"object","properties":{ + "inner":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}]}}`, + schema: "A.allOf.0.inner", + pointer: "/definitions/A/allOf/0/properties/inner/required/0", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(testCase.definitions)) + assert.EqualT(t, testCase.pointer, + found[`"ghost" is present in required but not defined as property in schema "`+testCase.schema+`"`]) + }) + } +} + +// Inside a composition, a required entry speaks of the instance the whole +// composition describes: it is legitimately met by a sibling member, or by no +// declaration at all, and data validation is what enforces it. +func TestRequiredWalk_LeavesCompositionAlone(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + }{ + { + name: "allOf members each requiring what the other does not declare", + definitions: `{"A":{"allOf":[ + {"type":"object","required":["a"]}, + {"type":"object","required":["b"]}]}}`, + }, + { + name: "a required entry declared by a sibling allOf member", + definitions: `{"A":{"allOf":[ + {"type":"object","required":["a"]}, + {"type":"object","properties":{"a":{"type":"string"}}}]}}`, + }, + { + name: "a oneOf member requiring what it does not declare", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"oneOf":[{"type":"object","required":["a"]},{"type":"object","required":["b"]}]}}}}`, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + for message := range locatedFindings(t, definitionsDoc(testCase.definitions)) { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + } +} + +// A property contributed by a base definition is declared just as plainly as +// one written in place. +func TestRequiredWalk_CountsPropertiesContributedByAllOf(t *testing.T) { + t.Parallel() + + t.Run("declared by an allOf member written in place", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["a"], + "allOf":[{"type":"object","properties":{"a":{"type":"string"}}}]}}`)) + + for message := range found { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + + t.Run("declared by an allOf member written as a $ref", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{ + "Base":{"type":"object","properties":{"a":{"type":"string"}}}, + "A":{"type":"object","required":["a"],"allOf":[{"$ref":"#/definitions/Base"}]}}`)) + + for message := range found { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + + t.Run("still reported when nothing in the composition declares it", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{ + "Base":{"type":"object","properties":{"other":{"type":"string"}}}, + "A":{"type":"object","required":["a"],"allOf":[{"$ref":"#/definitions/Base"}]}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"a" is present in required but not defined as property in definition "A"`]) + }) +} + +func TestRequiredWalk_QuietWhenTheSchemaIsSound(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + }{ + { + name: "the nested property is declared", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["real"],"properties":{"real":{"type":"string"}}}}}}`, + }, + { + name: "the nested schema takes any property", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["anything"],"additionalProperties":true}}}}`, + }, + { + name: "the nested schema is a $ref, checked where it is defined", + definitions: `{ + "Inner":{"type":"object","required":["real"],"properties":{"real":{"type":"string"}}}, + "A":{"type":"object","properties":{"inner":{"$ref":"#/definitions/Inner"}}}}`, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + for message := range locatedFindings(t, definitionsDoc(testCase.definitions)) { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + } +} + +// A definition names itself, exactly as it always has. +func TestRequiredWalk_ADefinitionStillNamesItself(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["ghost"], + "properties":{"real":{"type":"string"}}}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"ghost" is present in required but not defined as property in definition "A"`]) +} + +// A recursive definition is reached through a $ref, which the walk does not +// follow, so it cannot spin — and the slip is still reported once, where the +// definition writes it. +func TestRequiredWalk_ReportsARecursiveDefinitionOnce(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["ghost"], + "properties":{"self":{"$ref":"#/definitions/A"}}}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"ghost" is present in required but not defined as property in definition "A"`]) +} + +// The readOnly warning follows the entry it is about, wherever that sits. +func TestRequiredWalk_ReadOnlyWarningIsLocatedToo(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["ro"],"properties":{"ro":{"type":"string","readOnly":true}}}}}}`)) + + assert.EqualT(t, "/definitions/A/properties/inner/required/0", + found[`Required property ro in "A.inner" should not be marked as both required and readOnly`]) +} diff --git a/result.go b/result.go index 9b874a7..5684774 100644 --- a/result.go +++ b/result.go @@ -362,6 +362,14 @@ func (r *Result) AsError() error { return errors.CompositeValidationError(r.Errors...) } +// Reset clears this result so it may be reused, keeping allocated capacity. +// +// It implements the hook the pool calls when a result is borrowed and when it +// is redeemed. Calling it on a result still in use loses its findings. +func (r *Result) Reset() { + _ = r.cleared() +} + // addErrorsAt adds errors located at the given path. func (r *Result) addErrorsAt(at pathSegments, errors ...error) { r.addLocatedErrors(at.pointer(), errors...) @@ -622,14 +630,6 @@ func (r *Result) keepRelevantErrors() *Result { return strippedResult } -// Reset clears this result so it may be reused, keeping allocated capacity. -// -// It implements the hook the pool calls when a result is borrowed and when it -// is redeemed. Calling it on a result still in use loses its findings. -func (r *Result) Reset() { - _ = r.cleared() -} - func (r *Result) cleared() *Result { // clear the Result to be reusable. Keep allocated capacity. r.Errors = r.Errors[:0] diff --git a/spec.go b/spec.go index 6d5b333..e5d16b6 100644 --- a/spec.go +++ b/spec.go @@ -642,35 +642,25 @@ func (s *SpecValidator) validateRequiredDefinitions() *Result { DEFINITIONS: for _, d := range sortedKeys(definitions) { schema := definitions[d] - if schema.Required != nil { // Safeguard - definitionAt := newPathSegments(swaggerDefinitions, d) - for i, pn := range schema.Required { - // the offending entry of the required array, not the definition - // holding it: that is what a reader has to go and amend - requiredAt := definitionAt.child(jsonRequired).item(i) - red := s.validateRequiredProperties(pn, d, definitionAt, requiredAt, &schema) //#nosec - // NOTE: capture validity before merging: Merge may redeem `red` to the - // pool (wantsRedeemOnMerge), after which reading it races with a concurrent - // BorrowResult().cleared() in another goroutine sharing the global pool. - isValid := red.IsValid() - res.Merge(red) - if !isValid && !s.Options.ContinueOnErrors { - break DEFINITIONS // there is an error, let's stop that bleeding - } - } + red := validatorPools.results.Borrow() + keepGoing := s.walkRequired(newPathSegments(swaggerDefinitions, d), &schema, red) //#nosec + res.Merge(red) + if !keepGoing { + break DEFINITIONS // there is an error, let's stop that bleeding } } return res } -// validateRequiredProperties checks one entry of a definition's required array. +// validateRequiredProperties checks one entry of a required array. // // schemaAt locates the schema being searched for the property, which moves as // the search descends into additionalProperties. requiredAt locates the entry // of the required array that started it, and stays put. func (s *SpecValidator) validateRequiredProperties( - path, in string, schemaAt, requiredAt pathSegments, v *spec.Schema, + path string, of schemaIdentity, schemaAt, requiredAt pathSegments, v *spec.Schema, ) *Result { + in := of.name // Takes care of recursive property definitions, which may be nested in additionalProperties schemas res := validatorPools.results.Borrow() propertyMatch := false @@ -678,10 +668,10 @@ func (s *SpecValidator) validateRequiredProperties( additionalPropertiesMatch := false isReadOnly := false - // Regular properties - if _, ok := v.Properties[path]; ok { + // Regular properties, including those a base definition contributes + if readOnly, declared := s.declaresProperty(v, path, maxCompositionHops); declared { propertyMatch = true - isReadOnly = v.Properties[path].ReadOnly + isReadOnly = readOnly } // NOTE: patternProperties are not supported in swagger. Even though, we continue validation here @@ -706,7 +696,7 @@ func (s *SpecValidator) validateRequiredProperties( // additionalProperties as schema are upported in swagger // recursively validates additionalProperties schema // Proposal for enhancement: anyOf, allOf, oneOf like in schemaPropsValidator - red := s.validateRequiredProperties(path, in, schemaAt.child(jsonAdditionalProperties), requiredAt, v.AdditionalProperties.Schema) + red := s.validateRequiredProperties(path, of, schemaAt.child(jsonAdditionalProperties), requiredAt, v.AdditionalProperties.Schema) if red.IsValid() { additionalPropertiesMatch = true if !propertyMatch && !patternMatch { @@ -719,7 +709,7 @@ func (s *SpecValidator) validateRequiredProperties( } if !propertyMatch && !patternMatch && !additionalPropertiesMatch { - res.addErrorsAt(requiredAt, requiredButNotDefinedMsg(path, in)) + res.addErrorsAt(requiredAt, of.requiredButNotDefined(path)) } if isReadOnly { diff --git a/spec_messages.go b/spec_messages.go index eeb8a86..0a0739a 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -132,6 +132,9 @@ const ( // RequiredButNotDefinedError ... RequiredButNotDefinedError = "%q is present in required but not defined as property in definition %q" + // RequiredButNotDefinedInSchemaError is the same slip, in a schema a definition holds rather than + // in the definition itself. + RequiredButNotDefinedInSchemaError = "%q is present in required but not defined as property in schema %q" // SomeParametersBrokenError indicates that some parameters could not be resolved, which might result in partial checks to be carried on. SomeParametersBrokenError = "some parameters definitions are broken in %q.%s. Cannot carry on full checks on parameters for operation %s" @@ -260,6 +263,10 @@ func requiredButNotDefinedMsg(path, definition string) errors.Error { return errors.New(errors.CompositeErrorCode, RequiredButNotDefinedError, path, definition) } +func requiredButNotDefinedInSchemaMsg(path, schema string) errors.Error { + return errors.New(errors.CompositeErrorCode, RequiredButNotDefinedInSchemaError, path, schema) +} + func pathParamGarbledMsg(path, param string) errors.Error { return errors.New(errors.CompositeErrorCode, PathParamGarbledWarning, path, param) } From 2bf3c27ce032479d022c1d577581afe435755e9d Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 14:49:09 +0200 Subject: [PATCH 4/4] ci: enable poolsdebug on separate test lane No -race with that flag, as the extra resource consumption with pool debug breaks CI. Signed-off-by: Frederic BIDON --- .github/workflows/go-test.yml | 2 - .github/workflows/integration-test.yml | 53 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/integration-test.yml diff --git a/.github/workflows/go-test.yml b/.github/workflows/go-test.yml index 1722ebc..ec67258 100644 --- a/.github/workflows/go-test.yml +++ b/.github/workflows/go-test.yml @@ -14,6 +14,4 @@ on: jobs: test: uses: go-openapi/ci-workflows/.github/workflows/go-test.yml@befbc52f24215555cb601b0d824b2987c1d49d0b # v0.4.3 - with: - extra-flags: '-tags poolsdebug' secrets: inherit diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml new file mode 100644 index 0000000..bffa4e5 --- /dev/null +++ b/.github/workflows/integration-test.yml @@ -0,0 +1,53 @@ +name: integration test + +permissions: + pull-requests: read + contents: read + +on: + push: + branches: + - master + + pull_request: + +jobs: + test: + name: integration (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest] + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: stable + check-latest: true + cache: true + cache-dependency-path: '**/go.sum' + + - name: Run integration tests + run: > + go test + -tags poolsdebug + -count 1 + -timeout 30m + + # Single gate job for branch protection rules + integration-test: + name: integration test + needs: test + if: always() + runs-on: ubuntu-latest + steps: + - name: Check matrix results + run: | + if [ "${{ needs.test.result }}" != "success" ]; then + echo "Matrix jobs failed or were cancelled" + exit 1 + fi