From ffd33821234fdd4eff41ace357388a0aab73ed07 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sat, 8 Aug 2026 15:47:49 +0200 Subject: [PATCH 1/8] fix: correct the locations reported by validation errors Locations were assembled by string concatenation, which lost information and produced notations no consumer could parse. They are now built from a list of JSON pointer reference tokens and rendered on demand, so a separator can never be confused with a token that contains one. Locations that change: - array items carry their index again: "a.b" becomes "a.0.b". The index was set on the schema validator after its sub-validators had been built, so it never reached the validator raising the error; - "items[0]" and "allOf[0]" become "items.0" and "allOf.0"; - the "default" and "example" keywords are no longer appended twice; - a response is located by its operation rather than by a bare status code: "200.name.default" becomes "paths./pets/{id}.get.responses.200.name.default"; - a required property missing at the root loses its leading separator: ".paths in body is required" becomes "paths in body is required". The predicates telling a schema apart from plain data (isProperties, isDefault, isExample) and the recursion guard on visited schemas walk tokens instead of splitting on dots, so they no longer match mid-token. Trailing indices are trimmed before the marker lookup: an element of an example is example data, and schema-only checks stay off it. Error names keep the dotted rendering, so messages returned by generated servers are unchanged apart from the locations listed above. The JSON pointer rendering is available internally; no API exposes it yet. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 77 ++++++-------- error_location_test.go | 213 +++++++++++++++++++++++++++++++++++++++ example_validator.go | 54 +++++----- formats.go | 14 +-- formats_test.go | 4 +- helpers.go | 21 ++++ items_validator_test.go | 12 +-- object_validator.go | 69 ++++++------- object_validator_test.go | 47 +++++---- path.go | 161 +++++++++++++++++++++++++++++ path_test.go | 158 +++++++++++++++++++++++++++++ schema.go | 58 ++++++++--- schema_props.go | 28 ++--- schema_props_test.go | 16 +-- slice_validator.go | 29 +++--- slice_validator_test.go | 6 +- spec.go | 4 +- spec_test.go | 40 ++++---- type.go | 18 ++-- type_test.go | 2 +- validator.go | 131 ++++++++++++------------ validator_test.go | 12 +-- 22 files changed, 878 insertions(+), 296 deletions(-) create mode 100644 error_location_test.go create mode 100644 path.go create mode 100644 path_test.go diff --git a/default_validator.go b/default_validator.go index ebcd807..baca95d 100644 --- a/default_validator.go +++ b/default_validator.go @@ -4,9 +4,6 @@ package validate import ( - "fmt" - "strings" - "github.com/go-openapi/spec" ) @@ -44,28 +41,16 @@ func (d *defaultValidator) resetVisited() { } } -func isVisited(path string, visitedSchemas map[string]struct{}) bool { - _, found := visitedSchemas[path] +func isVisited(path pathSegments, visitedSchemas map[string]struct{}) bool { + _, found := visitedSchemas[path.pointer()] if found { return true } - // search for overlapping paths - var ( - parent string - suffix string - ) - const backtrackFromEnd = 2 - for i := len(path) - backtrackFromEnd; i >= 0; i-- { - r := path[i] - if r != '.' { - continue - } - - parent = path[0:i] - suffix = path[i+1:] - - if strings.HasSuffix(parent, suffix) { + // search for overlapping paths: a trailing run of tokens that already + // appears at the end of what leads to it means we are going in circles. + for i := 1; i < len(path); i++ { + if path[:i].hasSuffix(path[i:]) { return true } } @@ -74,12 +59,12 @@ func isVisited(path string, visitedSchemas map[string]struct{}) bool { } // beingVisited asserts a schema is being visited. -func (d *defaultValidator) beingVisited(path string) { - d.visitedSchemas[path] = struct{}{} +func (d *defaultValidator) beingVisited(path pathSegments) { + d.visitedSchemas[path.pointer()] = struct{}{} } // isVisited tells if a path has already been visited. -func (d *defaultValidator) isVisited(path string) bool { +func (d *defaultValidator) isVisited(path pathSegments) bool { return isVisited(path, d.visitedSchemas) } @@ -117,7 +102,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := d.validateDefaultValueItemsAgainstSchema(param.Name, param.In, ¶m, param.Items) //#nosec + red := d.validateDefaultValueItemsAgainstSchema(newPathSegments(param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { res.AddErrors(defaultValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -128,7 +113,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate default value against schema - red := d.validateDefaultValueSchemaAgainstSchema(param.Name, param.In, param.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(newPathSegments(param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -141,12 +126,12 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if op.Responses != nil { if op.Responses.Default != nil { // Same constraint on default Response - res.Merge(d.validateDefaultInResponse(op.Responses.Default, jsonDefault, path, 0, op.ID)) + res.Merge(d.validateDefaultInResponse(op.Responses.Default, jsonDefault, path, method, 0, op.ID)) } // 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, code, op.ID)) //#nosec + res.Merge(d.validateDefaultInResponse(&r, "response", path, method, code, op.ID)) //#nosec } } } else if op.ID != "" { @@ -159,13 +144,15 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() for nm, sch := range s.spec.Spec().Definitions { - res.Merge(d.validateDefaultValueSchemaAgainstSchema("definitions."+nm, "body", &sch)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec } } return res } -func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, responseType, path string, responseCode int, operationID string) *Result { +func (d *defaultValidator) validateDefaultInResponse( + resp *spec.Response, responseType, path, method string, responseCode int, operationID string, +) *Result { s := d.SpecValidator response, res := responseHelp.expandResponseRef(resp, path, s) @@ -192,7 +179,7 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon // Headers have inline definition, like params if h.Items != nil { - red := d.validateDefaultValueItemsAgainstSchema(nm, "header", &h, h.Items) //#nosec + red := d.validateDefaultValueItemsAgainstSchema(newPathSegments(nm), "header", &h, h.Items) //#nosec if red.HasErrorsOrWarnings() { res.AddErrors(defaultValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) res.Merge(red) @@ -212,7 +199,7 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() - red := d.validateDefaultValueSchemaAgainstSchema(responseCodeAsStr, "response", response.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost res.AddErrors(defaultValueInDoesNotValidateMsg(operationID, responseName)) @@ -224,7 +211,7 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon return res } -func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in string, schema *spec.Schema) *Result { +func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegments, in string, schema *spec.Schema) *Result { if schema == nil || d.isVisited(path) { // Avoids recursing if we are already done with that check return nil @@ -235,39 +222,39 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in stri if schema.Default != nil { res.Merge( - newSchemaValidator(schema, s.spec.Spec(), path+".default", s.KnownFormats, d.schemaOptions).Validate(schema.Default), + newSchemaValidator(schema, s.spec.Spec(), path.child(jsonDefault), s.KnownFormats, d.schemaOptions).Validate(schema.Default), ) } if schema.Items != nil { if schema.Items.Schema != nil { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path+".items.default", in, schema.Items.Schema)) + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonItems), in, schema.Items.Schema)) } // Multiple schemas in items if schema.Items.Schemas != nil { // Safeguard for i, sch := range schema.Items.Schemas { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(fmt.Sprintf("%s.items[%d].default", path, i), in, &sch)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonItems).item(i), in, &sch)) //#nosec } } } if _, err := compileRegexp(schema.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path, in, schema.Pattern)) + res.AddErrors(invalidPatternInMsg(path.dotted(), in, schema.Pattern)) } if schema.AdditionalItems != nil && schema.AdditionalItems.Schema != nil { // 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+".additionalItems", in, schema.AdditionalItems.Schema)) + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } for propName, prop := range schema.Properties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path+"."+propName, in, &prop)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec } for propName, prop := range schema.PatternProperties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path+"."+propName, in, &prop)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path+".additionalProperties", in, schema.AdditionalProperties.Schema)) + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) } if schema.AllOf != nil { for i, aoSch := range schema.AllOf { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(fmt.Sprintf("%s.allOf[%d]", path, i), in, &aoSch)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAllOf).item(i), in, &aoSch)) //#nosec } } return res @@ -275,7 +262,7 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in stri // NOTE: Temporary duplicated code. Need to refactor with examples -func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path, in string, root any, items *spec.Items) *Result { +func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path pathSegments, in string, root any, items *spec.Items) *Result { res := pools.poolOfResults.BorrowResult() s := d.SpecValidator if items != nil { @@ -285,10 +272,10 @@ func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path, in strin ) } if items.Items != nil { - res.Merge(d.validateDefaultValueItemsAgainstSchema(path+"[0].default", in, root, items.Items)) + res.Merge(d.validateDefaultValueItemsAgainstSchema(path.item(0), in, root, items.Items)) } if _, err := compileRegexp(items.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path, in, items.Pattern)) + res.AddErrors(invalidPatternInMsg(path.dotted(), in, items.Pattern)) } } return res diff --git a/error_location_test.go b/error_location_test.go new file mode 100644 index 0000000..e21e7a8 --- /dev/null +++ b/error_location_test.go @@ -0,0 +1,213 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/loads" + "github.com/go-openapi/spec" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +const ( + nameProp = "name" + dummyProp = "dummy" +) + +// The tests below pin down where a validation error says it happened. +// +// They exist because locations used to be assembled by string concatenation, +// which lost array indices altogether, mixed dotted and bracketed notations, +// and duplicated keyword suffixes. + +func TestErrorLocation_ArrayItemsCarryTheirIndex(t *testing.T) { + t.Parallel() + + // the item index used to be dropped: the sub-validators that report the + // error were built before the index was known. + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{ + "type": "object", + "properties": { + "friends": { + "type": "array", + "items": { + "type": "object", + "properties": {"name": {"type": "string"}}, + "required": ["age"] + } + } + } + }`), schema)) + + data := map[string]any{ + "friends": []any{ + map[string]any{nameProp: "ok", "age": 1}, + map[string]any{nameProp: 42}, + }, + } + + res := NewSchemaValidator(schema, nil, "", strfmt.Default).Validate(data) + require.False(t, res.IsValid()) + + messages := errorMessages(res) + assert.SliceContainsT(t, messages, "friends.1.name in body must be of type string: \"integer\"") + assert.SliceContainsT(t, messages, "friends.1.age in body is required") + + for _, msg := range messages { + assert.StringNotContainsT(t, msg, "friends.name", "expected no location to elide the item index") + } +} + +func TestErrorLocation_TupleItemsCarryTheirIndex(t *testing.T) { + t.Parallel() + + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{ + "type": "array", + "items": [{"type": "string"}, {"type": "integer"}] + }`), schema)) + + res := NewSchemaValidator(schema, nil, "", strfmt.Default).Validate([]any{1, "two"}) + require.False(t, res.IsValid()) + + messages := errorMessages(res) + assert.SliceContainsT(t, messages, "0 in body must be of type string: \"integer\"") + assert.SliceContainsT(t, messages, "1 in body must be of type integer: \"string\"") +} + +func TestErrorLocation_RootRequiredHasNoLeadingSeparator(t *testing.T) { + t.Parallel() + + // a missing property at the root used to be reported as ".swagger", + // because the empty root was concatenated with a separator. + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{ + "type": "object", + "required": ["swagger"], + "properties": {"swagger": {"type": "string"}} + }`), schema)) + + res := NewSchemaValidator(schema, nil, "", strfmt.Default).Validate(map[string]any{}) + require.False(t, res.IsValid()) + require.Len(t, res.Errors, 1) + + assert.EqualError(t, res.Errors[0], "swagger in body is required") +} + +func TestErrorLocation_SpecDefaultsAndExamples(t *testing.T) { + t.Parallel() + + const raw = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/pets/{id}": { + "get": { + "operationId": "getPet", + "parameters": [ + {"name": "id", "in": "path", "required": true, "type": "string"} + ], + "responses": { + "200": { + "description": "ok", + "schema": {"$ref": "#/definitions/Pet"}, + "examples": { + "application/json": {"friends": [{"name": 7}]} + } + } + } + } + } + }, + "definitions": { + "Pet": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "friends": {"type": "array", "items": {"$ref": "#/definitions/Pet"}}, + "tuple": { + "type": "array", + "items": [{"type": "string", "default": 1}, {"type": "integer", "default": "x"}] + } + } + } + } + }` + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + res, _ := NewSpecValidator(doc.Schema(), strfmt.Default).Validate(doc) + require.False(t, res.IsValid()) + + errs := errorMessages(res) + warns := warningMessages(res) + + t.Run("tuple item defaults use an index token, not a bracket", func(t *testing.T) { + assert.SliceContainsT(t, errs, + "definitions.Pet.tuple.items.0.default in body must be of type string: \"number\"") + assert.SliceContainsT(t, errs, + "definitions.Pet.tuple.items.1.default in body must be of type integer: \"string\"") + + for _, msg := range errs { + assert.StringNotContainsT(t, msg, "items[", "expected no bracketed index notation") + } + }) + + t.Run("the default keyword is not appended twice", func(t *testing.T) { + for _, msg := range append(errs, warns...) { + assert.StringNotContainsT(t, msg, ".default.default") + assert.StringNotContainsT(t, msg, ".example.example") + } + }) + + t.Run("an example inside an array carries the item index", func(t *testing.T) { + assert.SliceContainsT(t, warns, + "paths./pets/{id}.get.responses.200.examples.friends.0.name in body must be of type string: \"number\"") + }) + + t.Run("a response is located by its operation, not by the bare status code", func(t *testing.T) { + for _, msg := range append(errs, warns...) { + assert.NotEqualT(t, "200 in response has invalid pattern", msg) + } + assert.SliceContainsT(t, warns, + "paths./pets/{id}.get.responses.200.examples.friends.0.name in body must be of type string: \"number\"") + }) + + t.Run("the method is spelled as the document spells it", func(t *testing.T) { + for _, msg := range append(errs, warns...) { + assert.StringNotContainsT(t, msg, ".GET.", "expected the lower-case path item key") + } + }) +} + +func TestErrorLocation_ExampleItemsSkipSchemaOnlyChecks(t *testing.T) { + t.Parallel() + + // restoring the item index moved example data from "x.example" to + // "x.example.0", which must still count as being inside an example: + // the array-must-have-items check does not apply to plain data. + validator := newObjectValidator( + newPathSegments("itemsparam", swaggerExample, "0"), + "body", nil, nil, nil, nil, nil, nil, nil, nil, + &SchemaValidatorOptions{EnableObjectArrayTypeCheck: true, EnableArrayMustHaveItemsCheck: true}, + ) + + res := validator.Validate(map[string]any{jsonItems: dummyProp}) + assert.Empty(t, res.Errors, "expected schema-only checks to be skipped inside an example") +} + +func errorMessages(res *Result) []string { + messages := make([]string, 0, len(res.Errors)) + for _, err := range res.Errors { + messages = append(messages, err.Error()) + } + + return messages +} diff --git a/example_validator.go b/example_validator.go index eb6b5ee..713d8bb 100644 --- a/example_validator.go +++ b/example_validator.go @@ -4,8 +4,6 @@ package validate import ( - "fmt" - "github.com/go-openapi/spec" ) @@ -50,12 +48,12 @@ func (ex *exampleValidator) resetVisited() { } // beingVisited asserts a schema is being visited. -func (ex *exampleValidator) beingVisited(path string) { - ex.visitedSchemas[path] = struct{}{} +func (ex *exampleValidator) beingVisited(path pathSegments) { + ex.visitedSchemas[path.pointer()] = struct{}{} } // isVisited tells if a path has already been visited. -func (ex *exampleValidator) isVisited(path string) bool { +func (ex *exampleValidator) isVisited(path pathSegments) bool { return isVisited(path, ex.visitedSchemas) } @@ -94,7 +92,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := ex.validateExampleValueItemsAgainstSchema(param.Name, param.In, ¶m, param.Items) //#nosec + red := ex.validateExampleValueItemsAgainstSchema(newPathSegments(param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { res.AddWarnings(exampleValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -105,7 +103,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate example value against schema - red := ex.validateExampleValueSchemaAgainstSchema(param.Name, param.In, param.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(newPathSegments(param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -118,12 +116,12 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if op.Responses != nil { if op.Responses.Default != nil { // Same constraint on default Response - res.Merge(ex.validateExampleInResponse(op.Responses.Default, jsonDefault, path, 0, op.ID)) + res.Merge(ex.validateExampleInResponse(op.Responses.Default, jsonDefault, path, method, 0, op.ID)) } // 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, code, op.ID)) //#nosec + res.Merge(ex.validateExampleInResponse(&r, "response", path, method, code, op.ID)) //#nosec } } } else if op.ID != "" { @@ -136,13 +134,15 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() for nm, sch := range s.spec.Spec().Definitions { - res.Merge(ex.validateExampleValueSchemaAgainstSchema("definitions."+nm, "body", &sch)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec } } return res } -func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, responseType, path string, responseCode int, operationID string) *Result { +func (ex *exampleValidator) validateExampleInResponse( + resp *spec.Response, responseType, path, method string, responseCode int, operationID string, +) *Result { s := ex.SpecValidator response, res := responseHelp.expandResponseRef(resp, path, s) @@ -169,7 +169,7 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo // Headers have inline definition, like params if h.Items != nil { - red := ex.validateExampleValueItemsAgainstSchema(nm, "header", &h, h.Items) //#nosec + red := ex.validateExampleValueItemsAgainstSchema(newPathSegments(nm), "header", &h, h.Items) //#nosec if red.HasErrorsOrWarnings() { res.AddWarnings(exampleValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) res.MergeAsWarnings(red) @@ -189,7 +189,7 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() - red := ex.validateExampleValueSchemaAgainstSchema(responseCodeAsStr, "response", response.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost res.AddWarnings(exampleValueInDoesNotValidateMsg(operationID, responseName)) @@ -203,7 +203,7 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo if response.Schema != nil { if example, ok := response.Examples["application/json"]; ok { res.MergeAsWarnings( - newSchemaValidator(response.Schema, s.spec.Spec(), path+".examples", s.KnownFormats, s.schemaOptions).Validate(example), + newSchemaValidator(response.Schema, s.spec.Spec(), responsePath(path, method, responseCodeAsStr).child(swaggerExamples), s.KnownFormats, s.schemaOptions).Validate(example), ) } else { // Proposal for enhancement: validate other media types too @@ -216,7 +216,7 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo return res } -func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in string, schema *spec.Schema) *Result { +func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSegments, in string, schema *spec.Schema) *Result { if schema == nil || ex.isVisited(path) { // Avoids recursing if we are already done with that check return nil @@ -227,39 +227,39 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in str if schema.Example != nil { res.MergeAsWarnings( - newSchemaValidator(schema, s.spec.Spec(), path+".example", s.KnownFormats, ex.schemaOptions).Validate(schema.Example), + newSchemaValidator(schema, s.spec.Spec(), path.child(swaggerExample), s.KnownFormats, ex.schemaOptions).Validate(schema.Example), ) } if schema.Items != nil { if schema.Items.Schema != nil { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path+".items.example", in, schema.Items.Schema)) + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonItems), in, schema.Items.Schema)) } // Multiple schemas in items if schema.Items.Schemas != nil { // Safeguard for i, sch := range schema.Items.Schemas { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(fmt.Sprintf("%s.items[%d].example", path, i), in, &sch)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonItems).item(i), in, &sch)) //#nosec } } } if _, err := compileRegexp(schema.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path, in, schema.Pattern)) + res.AddErrors(invalidPatternInMsg(path.dotted(), in, schema.Pattern)) } if schema.AdditionalItems != nil && schema.AdditionalItems.Schema != nil { // NOTE: we keep validating values, even though additionalItems is unsupported in Swagger 2.0 (and 3.0 as well) - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path+".additionalItems", in, schema.AdditionalItems.Schema)) + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } for propName, prop := range schema.Properties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path+"."+propName, in, &prop)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec } for propName, prop := range schema.PatternProperties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path+"."+propName, in, &prop)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path+".additionalProperties", in, schema.AdditionalProperties.Schema)) + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) } if schema.AllOf != nil { for i, aoSch := range schema.AllOf { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(fmt.Sprintf("%s.allOf[%d]", path, i), in, &aoSch)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAllOf).item(i), in, &aoSch)) //#nosec } } return res @@ -268,7 +268,7 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in str // NOTE: Temporary duplicated code. Need to refactor with examples // -func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path, in string, root any, items *spec.Items) *Result { +func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path pathSegments, in string, root any, items *spec.Items) *Result { res := pools.poolOfResults.BorrowResult() s := ex.SpecValidator if items != nil { @@ -278,10 +278,10 @@ func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path, in stri ) } if items.Items != nil { - res.Merge(ex.validateExampleValueItemsAgainstSchema(path+"[0].example", in, root, items.Items)) + res.Merge(ex.validateExampleValueItemsAgainstSchema(path.item(0), in, root, items.Items)) } if _, err := compileRegexp(items.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path, in, items.Pattern)) + res.AddErrors(invalidPatternInMsg(path.dotted(), in, items.Pattern)) } } diff --git a/formats.go b/formats.go index eab2615..a050b39 100644 --- a/formats.go +++ b/formats.go @@ -11,14 +11,14 @@ import ( ) type formatValidator struct { - Path string + Path pathSegments In string Format string KnownFormats strfmt.Registry Options *SchemaValidatorOptions } -func newFormatValidator(path, in, format string, formats strfmt.Registry, opts *SchemaValidatorOptions) *formatValidator { +func newFormatValidator(path pathSegments, in, format string, formats strfmt.Registry, opts *SchemaValidatorOptions) *formatValidator { if opts == nil { opts = new(SchemaValidatorOptions) } @@ -39,10 +39,6 @@ func newFormatValidator(path, in, format string, formats strfmt.Registry, opts * return f } -func (f *formatValidator) SetPath(path string) { - f.Path = path -} - func (f *formatValidator) Applies(source any, kind reflect.Kind) bool { if source == nil || f.KnownFormats == nil { return false @@ -81,13 +77,17 @@ func (f *formatValidator) Validate(val any) *Result { return result } - if err := FormatOf(f.Path, f.In, f.Format, str, f.KnownFormats); err != nil { + if err := FormatOf(f.Path.dotted(), f.In, f.Format, str, f.KnownFormats); err != nil { result.AddErrors(err) } return result } +func (f *formatValidator) setPath(path pathSegments) { + f.Path = path +} + func (f *formatValidator) redeem() { pools.poolOfFormatValidators.RedeemValidator(f) } diff --git a/formats_test.go b/formats_test.go index 3af44c1..b21dad7 100644 --- a/formats_test.go +++ b/formats_test.go @@ -17,9 +17,9 @@ import ( func TestFormatValidator_EdgeCases(t *testing.T) { // Apply v := newFormatValidator( - "", "", "", strfmt.Default, nil, + nil, "", "", strfmt.Default, nil, ) - v.SetPath("a.b.c") + v.setPath(newPathSegments("a", "b", "c")) // formatValidator applies to: Items, Parameter,Schema diff --git a/helpers.go b/helpers.go index 7cc254e..34b5d17 100644 --- a/helpers.go +++ b/helpers.go @@ -38,8 +38,29 @@ const ( jsonType = "type" // jsonSchema = "schema". jsonDefault = "default" + + jsonAllOf = "allOf" + jsonAdditionalItems = "additionalItems" + jsonAdditionalProperties = "additionalProperties" + + swaggerPaths = "paths" + swaggerDefinitions = "definitions" + swaggerResponses = "responses" + swaggerParameters = "parameters" ) +// responsePath locates a response of an operation in the spec document. +func responsePath(path, method, responseCode string) pathSegments { + return newPathSegments(swaggerPaths, path, methodToken(method), swaggerResponses, responseCode) +} + +// methodToken normalizes an HTTP method into the key under which the operation +// is found in the document: the analyzer hands them over in upper case, but a +// path item spells them in lower case. +func methodToken(method string) string { + return strings.ToLower(method) +} + const ( stringFormatDate = "date" stringFormatDateTime = "date-time" diff --git a/items_validator_test.go b/items_validator_test.go index d8abd3d..56e95d1 100644 --- a/items_validator_test.go +++ b/items_validator_test.go @@ -106,7 +106,7 @@ func TestNumberItemsValidation(t *testing.T) { items.Typed("integer", "int32") parent := spec.QueryParam("factors").CollectionOf(items, "") path := fmt.Sprintf("factors.%d", i) - validator := newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator := newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) // MultipleOf err := validator.Validate(i, v[0]) @@ -125,7 +125,7 @@ func TestNumberItemsValidation(t *testing.T) { // ExclusiveMaximum items.ExclusiveMaximum = true // requires a new items validator because this is set a creation time - validator = newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator = newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) err = validator.Validate(i, v[1]) assert.TrueT(t, err.HasErrors()) require.NotEmpty(t, err.Errors) @@ -142,7 +142,7 @@ func TestNumberItemsValidation(t *testing.T) { // ExclusiveMinimum items.ExclusiveMinimum = true // requires a new items validator because this is set a creation time - validator = newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator = newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) err = validator.Validate(i, v[3]) assert.TrueT(t, err.HasErrors()) require.NotEmpty(t, err.Errors) @@ -165,7 +165,7 @@ func TestStringItemsValidation(t *testing.T) { items.WithEnum("aaa", "bbb", "ccc") parent := spec.QueryParam("tags").CollectionOf(items, "") path := parent.Name + ".1" - validator := newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator := newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) // required data := "" @@ -212,7 +212,7 @@ func TestArrayItemsValidation(t *testing.T) { items.WithEnum("aaa", "bbb", "ccc") parent := spec.QueryParam("tags").CollectionOf(items, "") path := parent.Name + ".1" - validator := newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator := newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) // MinItems data := []string{} @@ -242,7 +242,7 @@ func TestArrayItemsValidation(t *testing.T) { // Items strItems := spec.NewItems().WithMinLength(3).WithMaxLength(5).WithPattern(`^[a-z]+$`).Typed(stringType, "") items = spec.NewItems().CollectionOf(strItems, "").WithMinItems(1).WithMaxItems(5).UniqueValues() - validator = newItemsValidator(parent.Name, parent.In, items, parent, strfmt.Default, nil) + validator = newItemsValidator(newPathSegments(parent.Name), parent.In, items, parent, strfmt.Default, nil) data = []string{"aa", "bbb", "ccc"} err = validator.Validate(1, data) diff --git a/object_validator.go b/object_validator.go index e651b3f..432b371 100644 --- a/object_validator.go +++ b/object_validator.go @@ -4,7 +4,6 @@ package validate import ( - "fmt" "reflect" "strings" @@ -14,7 +13,7 @@ import ( ) type objectValidator struct { - Path string + Path pathSegments In string MaxProperties *int64 MinProperties *int64 @@ -25,10 +24,9 @@ type objectValidator struct { Root any KnownFormats strfmt.Registry Options *SchemaValidatorOptions - splitPath []string } -func newObjectValidator(path, in string, +func newObjectValidator(path pathSegments, in string, maxProperties, minProperties *int64, required []string, properties spec.SchemaProperties, additionalProperties *spec.SchemaOrBool, patternProperties spec.SchemaProperties, root any, formats strfmt.Registry, opts *SchemaValidatorOptions, @@ -55,7 +53,6 @@ func newObjectValidator(path, in string, v.Root = root v.KnownFormats = formats v.Options = opts - v.splitPath = strings.Split(v.Path, ".") return v } @@ -72,16 +69,16 @@ func (o *objectValidator) Validate(data any) *Result { var ok bool val, ok = data.(map[string]any) if !ok { - return errorHelp.sErr(invalidObjectMsg(o.Path, o.In), o.Options.recycleResult) + return errorHelp.sErr(invalidObjectMsg(o.Path.dotted(), o.In), o.Options.recycleResult) } } numKeys := int64(len(val)) if o.MinProperties != nil && numKeys < *o.MinProperties { - return errorHelp.sErr(errors.TooFewProperties(o.Path, o.In, *o.MinProperties), o.Options.recycleResult) + return errorHelp.sErr(errors.TooFewProperties(o.Path.dotted(), o.In, *o.MinProperties), o.Options.recycleResult) } if o.MaxProperties != nil && numKeys > *o.MaxProperties { - return errorHelp.sErr(errors.TooManyProperties(o.Path, o.In, *o.MaxProperties), o.Options.recycleResult) + return errorHelp.sErr(errors.TooManyProperties(o.Path.dotted(), o.In, *o.MaxProperties), o.Options.recycleResult) } var res *Result @@ -115,7 +112,7 @@ func (o *objectValidator) Validate(data any) *Result { for _, pName := range patterns { if v, ok := o.PatternProperties[pName]; ok { - r := newSchemaValidator(&v, o.Root, o.Path+"."+key, o.KnownFormats, o.Options).Validate(value) + r := newSchemaValidator(&v, o.Root, o.Path.child(key), o.KnownFormats, o.Options).Validate(value) res.mergeForField(data.(map[string]any), key, r) //nolint:forcetypeassert // data is always map[string]any at this point } } @@ -124,11 +121,6 @@ func (o *objectValidator) Validate(data any) *Result { return res } -func (o *objectValidator) SetPath(path string) { - o.Path = path - o.splitPath = strings.Split(path, ".") -} - func (o *objectValidator) Applies(source any, kind reflect.Kind) bool { // NOTE: this should also work for structs // there is a problem in the type validator where it will be unhappy about null values @@ -137,19 +129,29 @@ func (o *objectValidator) Applies(source any, kind reflect.Kind) bool { return isSchema && (kind == reflect.Map || kind == reflect.Struct) } +// The three predicates below tell what kind of content the validated object +// is, so that schema-only checks are not run against plain data. +// +// Array indices are trimmed first: an element of an example is example data +// just as much as the example itself. + func (o *objectValidator) isProperties() bool { - p := o.splitPath - return len(p) > 1 && p[len(p)-1] == jsonProperties && p[len(p)-2] != jsonProperties + p := o.Path.trimIndexes() + + return p.last() == jsonProperties && p.beforeLast() != jsonProperties } func (o *objectValidator) isDefault() bool { - p := o.splitPath - return len(p) > 1 && p[len(p)-1] == jsonDefault && p[len(p)-2] != jsonDefault + p := o.Path.trimIndexes() + + return p.last() == jsonDefault && p.beforeLast() != jsonDefault } func (o *objectValidator) isExample() bool { - p := o.splitPath - return len(p) > 1 && (p[len(p)-1] == swaggerExample || p[len(p)-1] == swaggerExamples) && p[len(p)-2] != swaggerExample + p := o.Path.trimIndexes() + last := p.last() + + return (last == swaggerExample || last == swaggerExamples) && p.beforeLast() != swaggerExample } func (o *objectValidator) checkArrayMustHaveItems(res *Result, val map[string]any) { @@ -174,7 +176,7 @@ func (o *objectValidator) checkArrayMustHaveItems(res *Result, val map[string]an return } - res.AddErrors(errors.Required(jsonItems, o.Path, item)) + res.AddErrors(errors.Required(jsonItems, o.Path.dotted(), item)) } func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string]any) { @@ -194,11 +196,11 @@ func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string] t, typeFound := val[jsonType] if !typeFound { // there is no type - res.AddErrors(errors.Required(jsonType, o.Path, t)) + res.AddErrors(errors.Required(jsonType, o.Path.dotted(), t)) } if tpe, isString := t.(string); !isString || tpe != arrayType { - res.AddErrors(errors.InvalidType(o.Path, o.In, arrayType, nil)) + res.AddErrors(errors.InvalidType(o.Path.dotted(), o.In, arrayType, nil)) } } @@ -238,7 +240,7 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res continue } - res.AddErrors(errors.PropertyNotAllowed(o.Path, o.In, k)) + res.AddErrors(errors.PropertyNotAllowed(o.Path.dotted(), o.In, k)) // BUG(fredbi): This section should move to a part dedicated to spec validation as // it will conflict with regular schemas where a property "headers" is defined. @@ -282,7 +284,7 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res } msg := strings.Join([]string{", one may not use $ref=\":", refString, "\""}, "") - res.AddErrors(refNotAllowedInHeaderMsg(o.Path, headerKey, msg)) + res.AddErrors(refNotAllowedInHeaderMsg(o.Path.dotted(), headerKey, msg)) /* case "$ref": if val[k] != nil { @@ -315,7 +317,7 @@ func (o *objectValidator) validateAdditionalProperties(val map[string]any, res * // Cases: properties which are not regular properties and have not been matched by the PatternProperties validator // AdditionalProperties as Schema - r := newSchemaValidator(o.AdditionalProperties.Schema, o.Root, o.Path+"."+key, o.KnownFormats, o.Options).Validate(value) + r := newSchemaValidator(o.AdditionalProperties.Schema, o.Root, o.Path.child(key), o.KnownFormats, o.Options).Validate(value) res.mergeForField(val, key, r) } // Valid cases: additionalProperties: true or undefined @@ -333,12 +335,7 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu for pName := range o.Properties { *pSchema = o.Properties[pName] - var rName string - if o.Path == "" { - rName = pName - } else { - rName = o.Path + "." + pName - } + rName := o.Path.child(pName) // Recursively validates each property against its schema v, ok := val[pName] @@ -374,7 +371,7 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu continue } - res.AddErrors(errors.Required(fmt.Sprintf("%s.%s", o.Path, k), o.In, v)) + res.AddErrors(errors.Required(o.Path.child(k).dotted(), o.In, v)) } } @@ -407,7 +404,7 @@ func (o *objectValidator) validatePatternProperty(key string, value any, result *schema = o.PatternProperties[k] patterns = append(patterns, k) matched = true - validator := newSchemaValidator(schema, o.Root, fmt.Sprintf("%s.%s", o.Path, key), o.KnownFormats, o.Options) + validator := newSchemaValidator(schema, o.Root, o.Path.child(key), o.KnownFormats, o.Options) res := validator.Validate(value) result.Merge(res) @@ -416,6 +413,10 @@ func (o *objectValidator) validatePatternProperty(key string, value any, result return matched, succeededOnce, patterns } +func (o *objectValidator) setPath(path pathSegments) { + o.Path = path +} + func (o *objectValidator) redeem() { pools.poolOfObjectValidators.RedeemValidator(o) } diff --git a/object_validator_test.go b/object_validator_test.go index be31db8..86af1ee 100644 --- a/object_validator_test.go +++ b/object_validator_test.go @@ -27,6 +27,9 @@ import ( "github.com/go-openapi/testify/v2/require" ) +// wantedProp is the name of the required property exercised by these tests. +const wantedProp = "wanted" + func itemsFixture() map[string]any { return map[string]any{ "type": "array", @@ -51,7 +54,7 @@ func expectOnlyInvalid(t *testing.T, ov EntityValidator, dataValid, dataInvalid } func TestItemsMustBeTypeArray(t *testing.T) { - ov := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + ov := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) dataValid := itemsFixture() dataInvalid := map[string]any{ "type": "object", @@ -64,7 +67,7 @@ func TestItemsMustBeTypeArray(t *testing.T) { } func TestItemsMustHaveType(t *testing.T) { - ov := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + ov := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) dataValid := itemsFixture() dataInvalid := map[string]any{ "items": "dummy", @@ -76,7 +79,7 @@ func TestItemsMustHaveType(t *testing.T) { } func TestTypeArrayMustHaveItems(t *testing.T) { - ov := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + ov := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) dataValid := itemsFixture() dataInvalid := map[string]any{ "type": "array", @@ -92,13 +95,13 @@ func TestTypeArrayMustHaveItems(t *testing.T) { // to simulate with specs // (this one is a trivial, just to check all methods are filled). func TestObjectValidator_EdgeCases(t *testing.T) { - s := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) - s.SetPath("path") - assert.EqualT(t, "path", s.Path) + s := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + s.setPath(newPathSegments("path")) + assert.EqualT(t, "path", s.Path.dotted()) } func TestObjectValidatorApply(t *testing.T) { - s := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + s := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) require.TrueT(t, s.Applies(&spec.Schema{}, reflect.Map)) require.FalseT(t, s.Applies(&spec.Response{}, reflect.Map)) require.FalseT(t, s.Applies(&struct{}{}, reflect.Map)) @@ -127,7 +130,7 @@ func TestObjectValidatorPatternProperties(t *testing.T) { } t.Run("should ignore invalid regexp in pattern properties", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, nil, patternWithValid, nil, nil, nil) + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, nil, patternWithValid, nil, nil, nil) res := s.Validate(map[string]any{"valid": "test_string"}) require.NotNil(t, res) @@ -135,7 +138,7 @@ func TestObjectValidatorPatternProperties(t *testing.T) { }) t.Run("should report forbidden property when invalid regexp in pattern properties", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, nil, patternGarbled, nil, nil, nil) + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, nil, patternGarbled, nil, nil, nil) res := s.Validate(map[string]any{"valid": "test_string"}) require.NotNil(t, res) @@ -143,7 +146,7 @@ func TestObjectValidatorPatternProperties(t *testing.T) { }) t.Run("should ignore invalid regexp in pattern properties of additional properties", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, &spec.SchemaOrBool{ + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, &spec.SchemaOrBool{ Schema: &spec.Schema{}, Allows: false, }, patternWithValid, nil, nil, nil) @@ -154,7 +157,7 @@ func TestObjectValidatorPatternProperties(t *testing.T) { }) t.Run("should report forbidden property when invalid regexp in pattern properties of additional properties", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, &spec.SchemaOrBool{ + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, &spec.SchemaOrBool{ Schema: &spec.Schema{}, Allows: false, }, patternGarbled, nil, nil, nil) @@ -168,7 +171,7 @@ func TestObjectValidatorPatternProperties(t *testing.T) { func TestObjectValidatorNilData(t *testing.T) { t.Run("object Validate should NOT panic on nil data", func(t *testing.T) { - s := newObjectValidator("", "", nil, nil, nil, nil, nil, nil, nil, nil, nil) + s := newObjectValidator(nil, "", nil, nil, nil, nil, nil, nil, nil, nil, nil) require.NotPanics(t, func() { _ = s.Validate(nil) }) @@ -179,35 +182,35 @@ func TestObjectValidatorNilData(t *testing.T) { }) t.Run("object Validate should validate required on nil data", func(t *testing.T) { - s := newObjectValidator("", "", nil, nil, []string{"wanted"}, nil, nil, nil, nil, nil, nil) + s := newObjectValidator(nil, "", nil, nil, []string{wantedProp}, nil, nil, nil, nil, nil, nil) res := s.Validate(nil) require.NotNil(t, res) require.NotEmpty(t, res.Errors) }) t.Run("object Validate should NOT panic on unexpected input", func(t *testing.T) { - s := newObjectValidator("", "", nil, nil, []string{"wanted"}, nil, nil, nil, nil, nil, nil) - res := s.Validate(map[string]string{"wanted": "not expected"}) + s := newObjectValidator(nil, "", nil, nil, []string{wantedProp}, nil, nil, nil, nil, nil, nil) + res := s.Validate(map[string]string{wantedProp: "not expected"}) require.NotNil(t, res) require.Len(t, res.Errors, 1) require.ErrorContains(t, res.Errors[0], "expected an object") }) t.Run("object Validate should NOT panic on nil input (with array type check)", func(t *testing.T) { - s := newObjectValidator("", "", nil, nil, []string{"wanted"}, nil, nil, nil, nil, nil, &SchemaValidatorOptions{ + s := newObjectValidator(nil, "", nil, nil, []string{wantedProp}, nil, nil, nil, nil, nil, &SchemaValidatorOptions{ EnableArrayMustHaveItemsCheck: true, EnableObjectArrayTypeCheck: true, }) res := s.Validate(nil) require.NotNil(t, res) require.Len(t, res.Errors, 1) - require.ErrorContains(t, res.Errors[0], "wanted is required") + require.ErrorContains(t, res.Errors[0], wantedProp+" is required") }) } func TestObjectValidatorWithHeaderProperty(t *testing.T) { t.Run("should report extra information about forbidden $ref in this context", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, &spec.SchemaOrBool{ + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, &spec.SchemaOrBool{ Schema: &spec.Schema{}, Allows: false, }, nil, nil, nil, nil) @@ -234,7 +237,7 @@ func TestObjectValidatorWithHeaderProperty(t *testing.T) { }) t.Run("should NOT report extra information when header is not detected", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, nil, nil, &spec.SchemaOrBool{ + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, nil, nil, &spec.SchemaOrBool{ Schema: &spec.Schema{}, Allows: false, }, nil, nil, nil, nil) @@ -314,10 +317,10 @@ func TestObjectValidatorWithDefault(t *testing.T) { root interface{}, formats strfmt.Registry, opts *SchemaValidatorOptions) *objectValidator { */ t.Run("should accept required populated with a default", func(t *testing.T) { - s := newObjectValidator("test", "body", nil, nil, - []string{"wanted"}, + s := newObjectValidator(newPathSegments("test"), "body", nil, nil, + []string{wantedProp}, spec.SchemaProperties{ - "wanted": spec.Schema{ + wantedProp: spec.Schema{ SchemaProps: spec.SchemaProps{ Default: "default_value", }, diff --git a/path.go b/path.go new file mode 100644 index 0000000..f37c846 --- /dev/null +++ b/path.go @@ -0,0 +1,161 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strconv" + "strings" + + "github.com/go-openapi/jsonpointer" +) + +// pathSegments is the location of a validated value inside a document, +// held as an ordered list of unescaped JSON pointer reference tokens. +// +// Validators build a location by appending tokens as they descend into +// properties and array items, then render it only when they report an error. +// Keeping the tokens apart until then is what makes it possible to produce a +// valid [RFC 6901] JSON pointer: a token is escaped when it is rendered, and +// the separator can never be confused with a token that contains one. +// +// The zero value is the location of the document root. +// +// [RFC 6901]: https://datatracker.ietf.org/doc/html/rfc6901 +type pathSegments []string + +// newPathSegments builds a location from a list of unescaped tokens. +func newPathSegments(tokens ...string) pathSegments { + if len(tokens) == 0 { + return nil + } + + return pathSegments(tokens) +} + +// rootPath is the location of the document root. +func rootPath() pathSegments { return nil } + +// String implements [fmt.Stringer] with the legacy dotted notation, so that a +// location interpolated into a message reads as it always has. +func (p pathSegments) String() string { return p.dotted() } + +// child returns the location of a named member of the value at p. +// +// The receiver is never modified: sibling children may be derived from the +// same parent without aliasing one another. +func (p pathSegments) child(token string) pathSegments { + child := make(pathSegments, len(p)+1) + copy(child, p) + child[len(p)] = token + + return child +} + +// 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) + copy(child[len(p):], tokens) + + return child +} + +// item returns the location of the index'th element of the array at p. +func (p pathSegments) item(index int) pathSegments { + return p.child(strconv.Itoa(index)) +} + +// isEmpty tells if p locates the document root. +func (p pathSegments) isEmpty() bool { return len(p) == 0 } + +// last returns the trailing token, or an empty string at the document root. +func (p pathSegments) last() string { + if len(p) == 0 { + return "" + } + + return p[len(p)-1] +} + +// beforeLast returns the token before the trailing one, or an empty string +// when p holds fewer than two tokens. +func (p pathSegments) beforeLast() string { + const beforeLast = 2 + if len(p) < beforeLast { + return "" + } + + return p[len(p)-beforeLast] +} + +// trimIndexes returns p without its trailing array index tokens. +// +// It answers "what is this value inside of", disregarding how deep into an +// array it sits: the items of an example are still an example. +func (p pathSegments) trimIndexes() pathSegments { + end := len(p) + for end > 0 && isIndexToken(p[end-1]) { + end-- + } + + return p[:end] +} + +// isIndexToken tells if a token addresses an array element rather than a member. +func isIndexToken(token string) bool { + if token == "" { + return false + } + + for _, r := range token { + if r < '0' || r > '9' { + return false + } + } + + return true +} + +// hasSuffix tells if p ends with the given sequence of tokens. +func (p pathSegments) hasSuffix(suffix pathSegments) bool { + if len(suffix) > len(p) { + return false + } + + offset := len(p) - len(suffix) + for i, token := range suffix { + if p[offset+i] != token { + return false + } + } + + return true +} + +// dotted renders the location in the legacy dot-separated notation, e.g. +// "definitions.Pet.friends.0.name". +// +// Tokens are emitted verbatim: a token containing a dot is indistinguishable +// from a separator. This notation is kept because it is what surfaces as the +// name of a validation error, and API consumers of go-swagger servers see it. +// Use [pathSegments.pointer] whenever the location needs to be unambiguous. +func (p pathSegments) dotted() string { + return strings.Join(p, ".") +} + +// pointer renders the location as an RFC 6901 JSON pointer, e.g. +// "/definitions/Pet/friends/0/name". The document root renders as "". +func (p pathSegments) pointer() string { + if len(p) == 0 { + return "" + } + + var w strings.Builder + for _, token := range p { + w.WriteByte('/') + w.WriteString(jsonpointer.Escape(token)) + } + + return w.String() +} diff --git a/path_test.go b/path_test.go new file mode 100644 index 0000000..6c2bd81 --- /dev/null +++ b/path_test.go @@ -0,0 +1,158 @@ +// 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" + "github.com/go-openapi/testify/v2/require" +) + +func TestPathSegmentsRendering(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path pathSegments + dotted string + pointer string + }{ + { + name: "document root", + path: rootPath(), + dotted: "", + pointer: "", + }, + { + name: "single token", + path: rootPath().child("definitions"), + dotted: "definitions", + pointer: "/definitions", + }, + { + name: "nested properties", + path: newPathSegments("definitions", "Pet", "name"), + dotted: "definitions.Pet.name", + pointer: "/definitions/Pet/name", + }, + { + name: "array item", + path: newPathSegments("friends").item(0).child("name"), + dotted: "friends.0.name", + pointer: "/friends/0/name", + }, + { + name: "token holding the dotted separator", + path: newPathSegments("a.b", "c"), + dotted: "a.b.c", // ambiguous, which is the whole point of the pointer form + pointer: "/a.b/c", + }, + { + name: "token needing RFC 6901 escaping", + path: newPathSegments("n~x/y", "0"), + dotted: "n~x/y.0", + pointer: "/n~0x~1y/0", + }, + { + name: "templated swagger path", + path: newPathSegments("paths", "/pets/{id}", "get"), + dotted: "paths./pets/{id}.get", + pointer: "/paths/~1pets~1{id}/get", + }, + { + name: "empty token", + path: newPathSegments("properties", ""), + dotted: "properties.", + pointer: "/properties/", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.dotted, tt.path.dotted()) + assert.Equal(t, tt.dotted, tt.path.String()) + assert.Equal(t, tt.pointer, tt.path.pointer()) + }) + } +} + +func TestPathSegmentsAppendIsCopyOnWrite(t *testing.T) { + t.Parallel() + + // a parent spawning several children is the common case: each child must + // get its own backing array, or siblings overwrite one another. + parent := newPathSegments("definitions", "Pet") + require.Equal(t, 2, len(parent)) + + first := parent.child("name") + second := parent.child("age") + third := parent.item(3) + + assert.Equal(t, "definitions.Pet", parent.dotted()) + assert.Equal(t, "definitions.Pet.name", first.dotted()) + assert.Equal(t, "definitions.Pet.age", second.dotted()) + assert.Equal(t, "definitions.Pet.3", third.dotted()) +} + +func TestPathSegmentsChildren(t *testing.T) { + t.Parallel() + + parent := newPathSegments("paths") + + assert.Equal(t, "paths./pets.get.responses", parent.children("/pets", "get", "responses").dotted()) + assert.Equal(t, "/paths/~1pets/get/responses", parent.children("/pets", "get", "responses").pointer()) + assert.Equal(t, "paths", parent.children().dotted()) + assert.Equal(t, "paths", parent.dotted(), "expected the parent to be left alone") +} + +func TestPathSegmentsInspection(t *testing.T) { + t.Parallel() + + t.Run("with an empty path", func(t *testing.T) { + t.Parallel() + + empty := rootPath() + assert.True(t, empty.isEmpty()) + assert.Empty(t, empty.last()) + assert.Empty(t, empty.beforeLast()) + }) + + t.Run("with a single token", func(t *testing.T) { + t.Parallel() + + single := newPathSegments("properties") + assert.False(t, single.isEmpty()) + assert.Equal(t, "properties", single.last()) + assert.Empty(t, single.beforeLast(), "expected no token before the only one") + }) + + t.Run("with several tokens", func(t *testing.T) { + t.Parallel() + + path := newPathSegments("definitions", "Pet", "properties") + assert.Equal(t, "properties", path.last()) + assert.Equal(t, "Pet", path.beforeLast()) + }) +} + +func TestPathSegmentsHasSuffix(t *testing.T) { + t.Parallel() + + path := newPathSegments("definitions", "Pet", "friends", "Pet") + + assert.True(t, path.hasSuffix(newPathSegments("Pet"))) + assert.True(t, path.hasSuffix(newPathSegments("friends", "Pet"))) + assert.True(t, path.hasSuffix(path)) + assert.True(t, path.hasSuffix(rootPath()), "expected the empty suffix to always match") + + assert.False(t, path.hasSuffix(newPathSegments("friends"))) + assert.False(t, path.hasSuffix(newPathSegments("Dog"))) + assert.False(t, path.hasSuffix(newPathSegments("definitions", "Pet", "friends", "Pet", "name"))) + + // tokens are compared whole: no mid-token match like the dotted string used to allow + assert.False(t, newPathSegments("abc").hasSuffix(newPathSegments("bc"))) +} diff --git a/schema.go b/schema.go index 706b7f5..26e47df 100644 --- a/schema.go +++ b/schema.go @@ -15,7 +15,17 @@ import ( // SchemaValidator validates data against a JSON schema. type SchemaValidator struct { - Path string + // Path is the location of the validated value, in the legacy dot-separated + // notation. It is what surfaces as the name of a validation error. + // + // Deprecated: a dotted path is ambiguous whenever a property name contains + // a dot. Prefer the JSON pointer rendering of the same location. + Path string + + // path is the same location, kept as JSON pointer reference tokens so that + // children may be derived from it unambiguously. + path pathSegments + in string Schema *spec.Schema validators [8]valueValidator @@ -51,10 +61,23 @@ func NewSchemaValidator(schema *spec.Schema, rootSchema any, root string, format o(opts) } - return newSchemaValidator(schema, rootSchema, root, formats, opts) + return newSchemaValidator(schema, rootSchema, rootPathFromString(root), formats, opts) +} + +// rootPathFromString interprets the root path of the exported constructors. +// +// The caller hands over an opaque string, so there is no telling which of its +// dots are separators and which belong to a name: it is taken as a single +// reference token. +func rootPathFromString(root string) pathSegments { + if root == "" { + return rootPath() + } + + return newPathSegments(root) } -func newSchemaValidator(schema *spec.Schema, rootSchema any, root string, formats strfmt.Registry, opts *SchemaValidatorOptions) *SchemaValidator { +func newSchemaValidator(schema *spec.Schema, rootSchema any, root pathSegments, formats strfmt.Registry, opts *SchemaValidatorOptions) *SchemaValidator { if schema == nil { return nil } @@ -82,7 +105,8 @@ func newSchemaValidator(schema *spec.Schema, rootSchema any, root string, format s = new(SchemaValidator) } - s.Path = root + s.path = root + s.Path = root.dotted() s.in = "body" s.Schema = schema s.Root = rootSchema @@ -104,8 +128,11 @@ func newSchemaValidator(schema *spec.Schema, rootSchema any, root string, format } // SetPath sets the path for this schema validator. +// +// Note that the sub-validators are built when the validator is created, so +// this only affects errors reported by this validator, not by its children. func (s *SchemaValidator) SetPath(path string) { - s.Path = path + s.setPath(rootPathFromString(path)) } // Applies returns true when this schema validator applies. @@ -235,7 +262,7 @@ func (s *SchemaValidator) Validate(data any) *Result { func (s *SchemaValidator) typeValidator() valueValidator { return newTypeValidator( - s.Path, + s.path, s.in, s.Schema.Type, s.Schema.Nullable, @@ -246,7 +273,7 @@ func (s *SchemaValidator) typeValidator() valueValidator { func (s *SchemaValidator) commonValidator() valueValidator { return newBasicCommonValidator( - s.Path, + s.path, s.in, s.Schema.Default, s.Schema.Enum, @@ -256,7 +283,7 @@ func (s *SchemaValidator) commonValidator() valueValidator { func (s *SchemaValidator) sliceValidator() valueValidator { return newSliceValidator( - s.Path, + s.path, s.in, s.Schema.MaxItems, s.Schema.MinItems, @@ -271,7 +298,7 @@ func (s *SchemaValidator) sliceValidator() valueValidator { func (s *SchemaValidator) numberValidator() valueValidator { return newNumberValidator( - s.Path, + s.path, s.in, s.Schema.Default, s.Schema.MultipleOf, @@ -287,7 +314,7 @@ func (s *SchemaValidator) numberValidator() valueValidator { func (s *SchemaValidator) stringValidator() valueValidator { return newStringValidator( - s.Path, + s.path, s.in, nil, false, @@ -301,7 +328,7 @@ func (s *SchemaValidator) stringValidator() valueValidator { func (s *SchemaValidator) formatValidator() valueValidator { return newFormatValidator( - s.Path, + s.path, s.in, s.Schema.Format, s.KnownFormats, @@ -312,14 +339,14 @@ func (s *SchemaValidator) formatValidator() valueValidator { func (s *SchemaValidator) schemaPropsValidator() valueValidator { sch := s.Schema return newSchemaPropsValidator( - s.Path, s.in, sch.AllOf, sch.OneOf, sch.AnyOf, sch.Not, sch.Dependencies, s.Root, s.KnownFormats, + s.path, s.in, sch.AllOf, sch.OneOf, sch.AnyOf, sch.Not, sch.Dependencies, s.Root, s.KnownFormats, s.Options, ) } func (s *SchemaValidator) objectValidator() valueValidator { return newObjectValidator( - s.Path, + s.path, s.in, s.Schema.MaxProperties, s.Schema.MinProperties, @@ -333,6 +360,11 @@ func (s *SchemaValidator) objectValidator() valueValidator { ) } +func (s *SchemaValidator) setPath(path pathSegments) { + s.path = path + s.Path = path.dotted() +} + func (s *SchemaValidator) redeem() { pools.poolOfSchemaValidators.RedeemValidator(s) } diff --git a/schema_props.go b/schema_props.go index 2c4354d..a9e2897 100644 --- a/schema_props.go +++ b/schema_props.go @@ -12,7 +12,7 @@ import ( ) type schemaPropsValidator struct { - Path string + Path pathSegments In string AllOf []spec.Schema OneOf []spec.Schema @@ -28,12 +28,8 @@ type schemaPropsValidator struct { Options *SchemaValidatorOptions } -func (s *schemaPropsValidator) SetPath(path string) { - s.Path = path -} - func newSchemaPropsValidator( - path string, in string, allOf, oneOf, anyOf []spec.Schema, not *spec.Schema, deps spec.Dependencies, root any, formats strfmt.Registry, + path pathSegments, in string, allOf, oneOf, anyOf []spec.Schema, not *spec.Schema, deps spec.Dependencies, root any, formats strfmt.Registry, opts *SchemaValidatorOptions, ) *schemaPropsValidator { if opts == nil { @@ -178,7 +174,7 @@ func (s *schemaPropsValidator) validateAnyOf(data any, mainResult, keepResultAny } } - mainResult.AddErrors(mustValidateAtLeastOneSchemaMsg(s.Path)) + mainResult.AddErrors(mustValidateAtLeastOneSchemaMsg(s.Path.dotted())) mainResult.Merge(bestFailures) } @@ -224,7 +220,7 @@ func (s *schemaPropsValidator) validateOneOf(data any, mainResult, keepResultOne switch validated { case 0: - mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path, "Found none valid")) + mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path.dotted(), "Found none valid")) mainResult.Merge(bestFailures) // firstSucess necessarily nil case 1: @@ -233,7 +229,7 @@ func (s *schemaPropsValidator) validateOneOf(data any, mainResult, keepResultOne pools.poolOfResults.RedeemResult(bestFailures) } default: - mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path, fmt.Sprintf("Found %d valid alternatives", validated))) + mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path.dotted(), fmt.Sprintf("Found %d valid alternatives", validated))) mainResult.Merge(bestFailures) if firstSuccess != nil && firstSuccess.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(firstSuccess) @@ -260,10 +256,10 @@ func (s *schemaPropsValidator) validateAllOf(data any, mainResult, keepResultAll switch validated { case 0: - mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path, ". None validated")) + mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path.dotted(), ". None validated")) case len(s.allOfValidators): default: - mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path, "")) + mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path.dotted(), "")) } } @@ -274,7 +270,7 @@ func (s *schemaPropsValidator) validateNot(data any, mainResult *Result) { } // We keep inner IMPORTANT! errors no matter what MatchCount tells us if result.IsValid() { - mainResult.AddErrors(mustNotValidatechemaMsg(s.Path)) + mainResult.AddErrors(mustNotValidatechemaMsg(s.Path.dotted())) } if result.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(result) // this result is ditched @@ -291,7 +287,7 @@ func (s *schemaPropsValidator) validateDependencies(data any, mainResult *Result if dep.Schema != nil { mainResult.Merge( - newSchemaValidator(dep.Schema, s.Root, s.Path+"."+key, s.KnownFormats, s.Options).Validate(data), + newSchemaValidator(dep.Schema, s.Root, s.Path.child(key), s.KnownFormats, s.Options).Validate(data), ) continue } @@ -299,13 +295,17 @@ func (s *schemaPropsValidator) validateDependencies(data any, mainResult *Result if len(dep.Property) > 0 { for _, depKey := range dep.Property { if _, ok := val[depKey]; !ok { - mainResult.AddErrors(hasADependencyMsg(s.Path, depKey)) + mainResult.AddErrors(hasADependencyMsg(s.Path.dotted(), depKey)) } } } } } +func (s *schemaPropsValidator) setPath(path pathSegments) { + s.Path = path +} + func (s *schemaPropsValidator) redeem() { pools.poolOfSchemaPropsValidators.RedeemValidator(s) } diff --git a/schema_props_test.go b/schema_props_test.go index 3a08add..bd8a203 100644 --- a/schema_props_test.go +++ b/schema_props_test.go @@ -18,15 +18,15 @@ import ( func TestSchemaPropsValidator_EdgeCases(t *testing.T) { t.Run("should validate props against empty validator", func(t *testing.T) { s := newSchemaPropsValidator( - "", "", nil, nil, nil, nil, nil, nil, strfmt.Default, nil) - s.SetPath("path") - assert.EqualT(t, "path", s.Path) + nil, "", nil, nil, nil, nil, nil, nil, strfmt.Default, nil) + s.setPath(newPathSegments("path")) + assert.EqualT(t, "path", s.Path.dotted()) }) t.Run("with allOf", func(t *testing.T) { makeValidator := func() EntityValidator { return newSchemaPropsValidator( - "path", "body", + newPathSegments("path"), "body", []spec.Schema{ *spec.StringProperty(), *spec.StrFmtProperty("date"), @@ -64,7 +64,7 @@ func TestSchemaPropsValidator_EdgeCases(t *testing.T) { t.Run("with oneOf", func(t *testing.T) { makeValidator := func() EntityValidator { return newSchemaPropsValidator( - "path", "body", + newPathSegments("path"), "body", nil, []spec.Schema{ *spec.Int64Property(), @@ -94,7 +94,7 @@ func TestSchemaPropsValidator_EdgeCases(t *testing.T) { t.Run("with anyOf", func(t *testing.T) { makeValidator := func() EntityValidator { return newSchemaPropsValidator( - "path", "body", + newPathSegments("path"), "body", nil, nil, []spec.Schema{ @@ -125,7 +125,7 @@ func TestSchemaPropsValidator_EdgeCases(t *testing.T) { t.Run("with not", func(t *testing.T) { makeValidator := func() EntityValidator { return newSchemaPropsValidator( - "path", "body", + newPathSegments("path"), "body", nil, nil, nil, @@ -182,7 +182,7 @@ func TestSchemaPropsValidator_EdgeCases(t *testing.T) { }, }, nil, - "root", + newPathSegments("root"), strfmt.Default, &SchemaValidatorOptions{recycleValidators: true}) } diff --git a/slice_validator.go b/slice_validator.go index 8f49d13..db0d989 100644 --- a/slice_validator.go +++ b/slice_validator.go @@ -4,7 +4,6 @@ package validate import ( - "fmt" "reflect" "github.com/go-openapi/spec" @@ -12,7 +11,7 @@ import ( ) type schemaSliceValidator struct { - Path string + Path pathSegments In string MaxItems *int64 MinItems *int64 @@ -24,7 +23,7 @@ type schemaSliceValidator struct { Options *SchemaValidatorOptions } -func newSliceValidator(path, in string, +func newSliceValidator(path pathSegments, in string, maxItems, minItems *int64, uniqueItems bool, additionalItems *spec.SchemaOrBool, items *spec.SchemaOrArray, root any, formats strfmt.Registry, opts *SchemaValidatorOptions, @@ -54,10 +53,6 @@ func newSliceValidator(path, in string, return v } -func (s *schemaSliceValidator) SetPath(path string) { - s.Path = path -} - func (s *schemaSliceValidator) Applies(source any, kind reflect.Kind) bool { _, ok := source.(*spec.Schema) r := ok && kind == reflect.Slice @@ -85,8 +80,10 @@ func (s *schemaSliceValidator) Validate(data any) *Result { if s.Items != nil && s.Items.Schema != nil { for i := range size { - validator := newSchemaValidator(s.Items.Schema, s.Root, s.Path, s.KnownFormats, s.Options) - validator.SetPath(fmt.Sprintf("%s.%d", s.Path, i)) + // the index has to reach the constructor: the sub-validators that + // report the error are built there, and setting the path afterwards + // would leave them located on the array rather than on the item. + validator := newSchemaValidator(s.Items.Schema, s.Root, s.Path.item(i), s.KnownFormats, s.Options) value := val.Index(i) result.mergeForSlice(val, i, validator.Validate(value.Interface())) } @@ -100,7 +97,7 @@ func (s *schemaSliceValidator) Validate(data any) *Result { break } - validator := newSchemaValidator(&s.Items.Schemas[i], s.Root, fmt.Sprintf("%s.%d", s.Path, i), s.KnownFormats, s.Options) + validator := newSchemaValidator(&s.Items.Schemas[i], s.Root, s.Path.item(i), s.KnownFormats, s.Options) result.mergeForSlice(val, i, validator.Validate(val.Index(i).Interface())) } } @@ -110,24 +107,24 @@ func (s *schemaSliceValidator) Validate(data any) *Result { } if s.AdditionalItems.Schema != nil { for i := itemsSize; i < size-itemsSize+1; i++ { - validator := newSchemaValidator(s.AdditionalItems.Schema, s.Root, fmt.Sprintf("%s.%d", s.Path, i), s.KnownFormats, s.Options) + validator := newSchemaValidator(s.AdditionalItems.Schema, s.Root, s.Path.item(i), s.KnownFormats, s.Options) result.mergeForSlice(val, i, validator.Validate(val.Index(i).Interface())) } } } if s.MinItems != nil { - if err := MinItems(s.Path, s.In, int64(size), *s.MinItems); err != nil { + if err := MinItems(s.Path.dotted(), s.In, int64(size), *s.MinItems); err != nil { result.AddErrors(err) } } if s.MaxItems != nil { - if err := MaxItems(s.Path, s.In, int64(size), *s.MaxItems); err != nil { + if err := MaxItems(s.Path.dotted(), s.In, int64(size), *s.MaxItems); err != nil { result.AddErrors(err) } } if s.UniqueItems { - if err := UniqueItems(s.Path, s.In, val.Interface()); err != nil { + if err := UniqueItems(s.Path.dotted(), s.In, val.Interface()); err != nil { result.AddErrors(err) } } @@ -135,6 +132,10 @@ func (s *schemaSliceValidator) Validate(data any) *Result { return result } +func (s *schemaSliceValidator) setPath(path pathSegments) { + s.Path = path +} + func (s *schemaSliceValidator) redeem() { pools.poolOfSliceValidators.RedeemValidator(s) } diff --git a/slice_validator_test.go b/slice_validator_test.go index 7fbedd9..6db2954 100644 --- a/slice_validator_test.go +++ b/slice_validator_test.go @@ -13,9 +13,9 @@ import ( // to simulate with specs // (this one is a trivial, just to check all methods are filled). func TestSliceValidator_EdgeCases(t *testing.T) { - s := newSliceValidator("", "", nil, nil, false, nil, nil, nil, nil, nil) - s.SetPath("path") - assert.EqualT(t, "path", s.Path) + s := newSliceValidator(nil, "", nil, nil, false, nil, nil, nil, nil, nil) + s.setPath(newPathSegments("path")) + assert.EqualT(t, "path", s.Path.dotted()) r := s.Validate(nil) assert.NotNil(t, r) diff --git a/spec.go b/spec.go index d6a61ea..3a786cb 100644 --- a/spec.go +++ b/spec.go @@ -115,7 +115,7 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { }() // Swagger schema validator - schv := newSchemaValidator(s.schema, nil, "", s.KnownFormats, s.schemaOptions) + schv := newSchemaValidator(s.schema, nil, rootPath(), s.KnownFormats, s.schemaOptions) errs.Merge(schv.Validate(obj)) // error - // There may be a point in continuing to try and determine more accurate errors if !s.Options.ContinueOnErrors && errs.HasErrors() { @@ -697,7 +697,7 @@ func (s *SpecValidator) validateParameters() *Result { for _, pr := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { // An expanded parameter must validate the Parameter schema (an unexpanded $ref always passes high-level schema validation) - schv := newSchemaValidator(¶mSchema, s.schema, fmt.Sprintf("%s.%s.parameters.%s", path, method, pr.Name), s.KnownFormats, s.schemaOptions) + schv := newSchemaValidator(¶mSchema, s.schema, newPathSegments(swaggerPaths, path, methodToken(method), swaggerParameters, pr.Name), s.KnownFormats, s.schemaOptions) var obj any if err := jsonutils.FromDynamicJSON(pr, &obj); err != nil { res.AddErrors(err) diff --git a/spec_test.go b/spec_test.go index 43c20f6..1e9e2f5 100644 --- a/spec_test.go +++ b/spec_test.go @@ -107,7 +107,7 @@ func TestSpec_Issue52(t *testing.T) { res := schemaValidator.Validate(&sch) assert.FalseT(t, res.IsValid()) require.NotEmpty(t, res.Errors) - require.EqualError(t, res.Errors[0], ".paths in body is required") + require.EqualError(t, res.Errors[0], "paths in body is required") // as swagger spec: path is set to nil // Here, validation stops as paths is initialized to empty @@ -116,7 +116,7 @@ func TestSpec_Issue52(t *testing.T) { verifiedErrors := verifiedTestErrors(res) assert.Len(t, verifiedErrors, 2, "Unexpected number of error messages returned") - assert.SliceContainsT(t, verifiedErrors, ".paths in body is required") + assert.SliceContainsT(t, verifiedErrors, "paths in body is required") assert.SliceContainsT(t, verifiedErrors, "spec has no valid path defined") } @@ -132,13 +132,13 @@ func TestSpec_Issue53(t *testing.T) { res := schemaValidator.Validate(&sch) assert.FalseT(t, res.IsValid()) require.NotEmpty(t, res.Errors) - require.EqualError(t, res.Errors[0], ".swagger in body is required") + require.EqualError(t, res.Errors[0], "swagger in body is required") // as swagger despec res, _ = loadAndValidate(t, fp, false) require.FalseT(t, res.IsValid()) require.NotEmpty(t, res.Errors) - require.EqualError(t, res.Errors[0], ".swagger in body is required") + require.EqualError(t, res.Errors[0], "swagger in body is required") } func TestSpec_Issue62(t *testing.T) { @@ -226,14 +226,13 @@ func TestSpec_Issue18(t *testing.T) { case strings.Contains(path, "paramItems.json"): assert.SliceContainsT(t, verifiedErrors, "body param \"user\" for \"\" has invalid items pattern: \")<-- bad pattern\"") - // Updated message: from "user.items in body has invalid pattern: \")<-- bad pattern\"" to: assert.SliceContainsT(t, verifiedErrors, "default value for user in body does not validate its schema") - assert.SliceContainsT(t, verifiedErrors, "user.items.default in body has invalid pattern: \")<-- bad pattern\"") + assert.SliceContainsT(t, verifiedErrors, "user.items in body has invalid pattern: \")<-- bad pattern\"") case strings.Contains(path, "parameters.json"): assert.SliceContainsT(t, verifiedErrors, "operation \"\" has invalid pattern in param \"userId\": \")<-- bad pattern\"") case strings.Contains(path, "schema.json"): - // NOTE: strange that the text does not say response "200"... - assert.SliceContainsT(t, verifiedErrors, "200 in response has invalid pattern: \")<-- bad pattern\"") + assert.SliceContainsT(t, verifiedErrors, + "paths./foo.get.responses.200 in response has invalid pattern: \")<-- bad pattern\"") default: t.Logf("Returned error messages: %v", verifiedErrors) t.Fatal("fixture not tested. Please add assertions for messages") @@ -460,10 +459,10 @@ func TestSpec_ValidateParameters(t *testing.T) { res := validator.validateParameters() require.Len(t, res.Errors, 2) assert.StringContainsT(t, res.Errors[0].Error(), - `"/pets.POST.parameters.pet" must validate one and only one schema (oneOf). Found none valid`, + `"paths./pets.post.parameters.pet" must validate one and only one schema (oneOf). Found none valid`, ) assert.StringContainsT(t, res.Errors[1].Error(), - `/pets.POST.parameters.pet.schema.anyOf in body is a forbidden property`, + `paths./pets.post.parameters.pet.schema.anyOf in body is a forbidden property`, ) }) t.Run("with loads.Spec", func(t *testing.T) { @@ -561,16 +560,16 @@ func TestSpec_ValidateParameters(t *testing.T) { err = Spec(doc, strfmt.Default) require.Error(t, err) require.ErrorContains(t, err, - `/deposits.GET.parameters..enum in body is a forbidden property`, + `paths./deposits.get.parameters..enum in body is a forbidden property`, ) require.ErrorContains(t, err, - `deposits.GET.parameters..type in body is a forbidden property`, + `paths./deposits.get.parameters..type in body is a forbidden property`, ) require.ErrorContains(t, err, - `/deposits.GET.parameters..name in body is required`, + `paths./deposits.get.parameters..name in body is required`, ) require.ErrorContains(t, err, - `/deposits.GET.parameters..in in body is required`, + `paths./deposits.get.parameters..in in body is required`, ) }) @@ -606,18 +605,24 @@ func TestSpec_ValidateParameters(t *testing.T) { require.NoError(t, err) errs, warns := NewSpecValidator(doc.Schema(), strfmt.Default).Validate(doc) - require.Len(t, errs.Errors, 3) + + // the fixture declares "type: [zilk, zork]": both entries are invalid and + // each is reported on its own index. + require.Len(t, errs.Errors, 4) require.Empty(t, warns.Errors) - var found1, found2, found3 int + const oneOfTypes = ` in body should be one of [array boolean integer null number object string]` + var found1, found2, found3, found4 int for _, err := range errs.Errors { switch { case strings.Contains(err.Error(), `definitions.WrongSchema.descriptions in body is a forbidden property`): found1++ case strings.Contains(err.Error(), `"definitions.WrongSchema.type" must validate at least one schema (anyOf)`): found2++ - case strings.Contains(err.Error(), `definitions.WrongSchema.type in body should be one of [array boolean integer null number object string]`): + case strings.Contains(err.Error(), `definitions.WrongSchema.type.0`+oneOfTypes): found3++ + case strings.Contains(err.Error(), `definitions.WrongSchema.type.1`+oneOfTypes): + found4++ } } @@ -625,6 +630,7 @@ func TestSpec_ValidateParameters(t *testing.T) { require.EqualT(t, 1, found1) require.EqualT(t, 1, found2) require.EqualT(t, 1, found3) + require.EqualT(t, 1, found4) }) }) }) diff --git a/type.go b/type.go index d29574c..1b23f6a 100644 --- a/type.go +++ b/type.go @@ -15,7 +15,7 @@ import ( ) type typeValidator struct { - Path string + Path pathSegments In string Type spec.StringOrArray Nullable bool @@ -23,7 +23,7 @@ type typeValidator struct { Options *SchemaValidatorOptions } -func newTypeValidator(path, in string, typ spec.StringOrArray, nullable bool, format string, opts *SchemaValidatorOptions) *typeValidator { +func newTypeValidator(path pathSegments, in string, typ spec.StringOrArray, nullable bool, format string, opts *SchemaValidatorOptions) *typeValidator { if opts == nil { opts = new(SchemaValidatorOptions) } @@ -45,10 +45,6 @@ func newTypeValidator(path, in string, typ spec.StringOrArray, nullable bool, fo return t } -func (t *typeValidator) SetPath(path string) { - t.Path = path -} - func (t *typeValidator) Applies(source any, _ reflect.Kind) bool { // typeValidator applies to Schema, Parameter and Header objects switch source.(type) { @@ -72,7 +68,7 @@ func (t *typeValidator) Validate(data any) *Result { if data == nil { // nil or zero value for the passed structure require Type: null if len(t.Type) > 0 && !t.Type.Contains(nullType) && !t.Nullable { // NOTE: if a property is not required it also passes this - return errorHelp.sErr(errors.InvalidType(t.Path, t.In, strings.Join(t.Type, ","), nullType), t.Options.recycleResult) + return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), nullType), t.Options.recycleResult) } return emptyResult @@ -98,7 +94,7 @@ func (t *typeValidator) Validate(data any) *Result { !isFloatInt && !isIntFloat && !isLowerInt && !isLowerFloat if formatMismatch { // NOTE: test case - return errorHelp.sErr(errors.InvalidType(t.Path, t.In, t.Format, format), t.Options.recycleResult) + return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, t.Format, format), t.Options.recycleResult) } if !t.Type.Contains(numberType) && !t.Type.Contains(integerType) && t.Format != "" && (kind == reflect.String || kind == reflect.Slice) { @@ -106,7 +102,7 @@ func (t *typeValidator) Validate(data any) *Result { } if !t.Type.Contains(schType) && !isFloatInt && !isIntFloat { - return errorHelp.sErr(errors.InvalidType(t.Path, t.In, strings.Join(t.Type, ","), schType), t.Options.recycleResult) + return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), schType), t.Options.recycleResult) } return emptyResult @@ -201,6 +197,10 @@ func (t *typeValidator) schemaInfoForType(data any) (string, string) { return "", "" } +func (t *typeValidator) setPath(path pathSegments) { + t.Path = path +} + func (t *typeValidator) redeem() { pools.poolOfTypeValidators.RedeemValidator(t) } diff --git a/type_test.go b/type_test.go index 6f77813..84f4876 100644 --- a/type_test.go +++ b/type_test.go @@ -256,7 +256,7 @@ func TestType_schemaInfoForType(t *testing.T) { } v := newTypeValidator( - "", "", nil, false, "", nil, + nil, "", nil, false, "", nil, ) t.Run("should not apply", func(t *testing.T) { diff --git a/validator.go b/validator.go index e7aebc5..b8c4e75 100644 --- a/validator.go +++ b/validator.go @@ -4,7 +4,6 @@ package validate import ( - "fmt" "reflect" "github.com/go-openapi/errors" @@ -18,7 +17,7 @@ type EntityValidator interface { } type valueValidator interface { - SetPath(path string) + setPath(path pathSegments) Applies(source any, kind reflect.Kind) bool Validate(data any) *Result } @@ -26,14 +25,14 @@ type valueValidator interface { type itemsValidator struct { items *spec.Items root any - path string + path pathSegments in string validators [6]valueValidator KnownFormats strfmt.Registry Options *SchemaValidatorOptions } -func newItemsValidator(path, in string, items *spec.Items, root any, formats strfmt.Registry, opts *SchemaValidatorOptions) *itemsValidator { +func newItemsValidator(path pathSegments, in string, items *spec.Items, root any, formats strfmt.Registry, opts *SchemaValidatorOptions) *itemsValidator { if opts == nil { opts = new(SchemaValidatorOptions) } @@ -79,7 +78,7 @@ func (i *itemsValidator) Validate(index int, data any) *Result { result = new(Result) } - path := fmt.Sprintf("%s.%d", i.path, index) + path := i.path.item(index) for idx, validator := range i.validators { if !validator.Applies(i.root, kind) { @@ -97,7 +96,7 @@ func (i *itemsValidator) Validate(index int, data any) *Result { continue } - validator.SetPath(path) + validator.setPath(path) err := validator.Validate(data) if i.Options.recycleValidators { i.validators[idx] = nil // prevents further (unsafe) usage @@ -130,7 +129,7 @@ func (i *itemsValidator) typeValidator() valueValidator { func (i *itemsValidator) commonValidator() valueValidator { return newBasicCommonValidator( - "", + nil, // located by the item index, set on each Validate call i.in, i.items.Default, i.items.Enum, @@ -140,7 +139,7 @@ func (i *itemsValidator) commonValidator() valueValidator { func (i *itemsValidator) sliceValidator() valueValidator { return newBasicSliceValidator( - "", + nil, // located by the item index, set on each Validate call i.in, i.items.Default, i.items.MaxItems, @@ -155,7 +154,7 @@ func (i *itemsValidator) sliceValidator() valueValidator { func (i *itemsValidator) numberValidator() valueValidator { return newNumberValidator( - "", + nil, // located by the item index, set on each Validate call i.in, i.items.Default, i.items.MultipleOf, @@ -171,7 +170,7 @@ func (i *itemsValidator) numberValidator() valueValidator { func (i *itemsValidator) stringValidator() valueValidator { return newStringValidator( - "", + nil, // located by the item index, set on each Validate call i.in, i.items.Default, false, // Required @@ -185,7 +184,7 @@ func (i *itemsValidator) stringValidator() valueValidator { func (i *itemsValidator) formatValidator() valueValidator { return newFormatValidator( - "", + nil, // located by the item index, set on each Validate call i.in, i.items.Format, i.KnownFormats, @@ -213,14 +212,14 @@ func (i *itemsValidator) redeemChildren() { } type basicCommonValidator struct { - Path string + Path pathSegments In string Default any Enum []any Options *SchemaValidatorOptions } -func newBasicCommonValidator(path, in string, def any, enum []any, opts *SchemaValidatorOptions) *basicCommonValidator { +func newBasicCommonValidator(path pathSegments, in string, def any, enum []any, opts *SchemaValidatorOptions) *basicCommonValidator { if opts == nil { opts = new(SchemaValidatorOptions) } @@ -241,10 +240,6 @@ func newBasicCommonValidator(path, in string, def any, enum []any, opts *SchemaV return b } -func (b *basicCommonValidator) SetPath(path string) { - b.Path = path -} - func (b *basicCommonValidator) Applies(source any, _ reflect.Kind) bool { switch source.(type) { case *spec.Parameter, *spec.Schema, *spec.Header: @@ -279,7 +274,11 @@ func (b *basicCommonValidator) Validate(data any) (res *Result) { } } - return errorHelp.sErr(errors.EnumFail(b.Path, b.In, data, b.Enum), b.Options.recycleResult) + return errorHelp.sErr(errors.EnumFail(b.Path.dotted(), b.In, data, b.Enum), b.Options.recycleResult) +} + +func (b *basicCommonValidator) setPath(path pathSegments) { + b.Path = path } func (b *basicCommonValidator) redeem() { @@ -323,7 +322,7 @@ func newHeaderValidator(name string, header *spec.Header, formats strfmt.Registr p.Options = opts p.validators = [6]valueValidator{ newTypeValidator( - name, + newPathSegments(name), "header", spec.StringOrArray([]string{header.Type}), header.Nullable, @@ -397,7 +396,7 @@ func (p *HeaderValidator) Validate(data any) *Result { func (p *HeaderValidator) commonValidator() valueValidator { return newBasicCommonValidator( - p.name, + newPathSegments(p.name), "response", p.header.Default, p.header.Enum, @@ -407,7 +406,7 @@ func (p *HeaderValidator) commonValidator() valueValidator { func (p *HeaderValidator) sliceValidator() valueValidator { return newBasicSliceValidator( - p.name, + newPathSegments(p.name), "response", p.header.Default, p.header.MaxItems, @@ -422,7 +421,7 @@ func (p *HeaderValidator) sliceValidator() valueValidator { func (p *HeaderValidator) numberValidator() valueValidator { return newNumberValidator( - p.name, + newPathSegments(p.name), "response", p.header.Default, p.header.MultipleOf, @@ -438,7 +437,7 @@ func (p *HeaderValidator) numberValidator() valueValidator { func (p *HeaderValidator) stringValidator() valueValidator { return newStringValidator( - p.name, + newPathSegments(p.name), "response", p.header.Default, true, @@ -452,7 +451,7 @@ func (p *HeaderValidator) stringValidator() valueValidator { func (p *HeaderValidator) formatValidator() valueValidator { return newFormatValidator( - p.name, + newPathSegments(p.name), "response", p.header.Format, p.KnownFormats, @@ -514,7 +513,7 @@ func newParamValidator(param *spec.Parameter, formats strfmt.Registry, opts *Sch p.Options = opts p.validators = [6]valueValidator{ newTypeValidator( - param.Name, + newPathSegments(param.Name), param.In, spec.StringOrArray([]string{param.Type}), param.Nullable, @@ -589,7 +588,7 @@ func (p *ParamValidator) Validate(data any) *Result { func (p *ParamValidator) commonValidator() valueValidator { return newBasicCommonValidator( - p.param.Name, + newPathSegments(p.param.Name), p.param.In, p.param.Default, p.param.Enum, @@ -599,7 +598,7 @@ func (p *ParamValidator) commonValidator() valueValidator { func (p *ParamValidator) sliceValidator() valueValidator { return newBasicSliceValidator( - p.param.Name, + newPathSegments(p.param.Name), p.param.In, p.param.Default, p.param.MaxItems, @@ -614,7 +613,7 @@ func (p *ParamValidator) sliceValidator() valueValidator { func (p *ParamValidator) numberValidator() valueValidator { return newNumberValidator( - p.param.Name, + newPathSegments(p.param.Name), p.param.In, p.param.Default, p.param.MultipleOf, @@ -630,7 +629,7 @@ func (p *ParamValidator) numberValidator() valueValidator { func (p *ParamValidator) stringValidator() valueValidator { return newStringValidator( - p.param.Name, + newPathSegments(p.param.Name), p.param.In, p.param.Default, p.param.Required, @@ -644,7 +643,7 @@ func (p *ParamValidator) stringValidator() valueValidator { func (p *ParamValidator) formatValidator() valueValidator { return newFormatValidator( - p.param.Name, + newPathSegments(p.param.Name), p.param.In, p.param.Format, p.KnownFormats, @@ -672,7 +671,7 @@ func (p *ParamValidator) redeemChildren() { } type basicSliceValidator struct { - Path string + Path pathSegments In string Default any MaxItems *int64 @@ -685,7 +684,7 @@ type basicSliceValidator struct { } func newBasicSliceValidator( - path, in string, + path pathSegments, in string, def any, maxItems, minItems *int64, uniqueItems bool, items *spec.Items, source any, formats strfmt.Registry, opts *SchemaValidatorOptions, @@ -715,10 +714,6 @@ func newBasicSliceValidator( return s } -func (s *basicSliceValidator) SetPath(path string) { - s.Path = path -} - func (s *basicSliceValidator) Applies(source any, kind reflect.Kind) bool { switch source.(type) { case *spec.Parameter, *spec.Items, *spec.Header: @@ -738,19 +733,19 @@ func (s *basicSliceValidator) Validate(data any) *Result { size := int64(val.Len()) if s.MinItems != nil { - if err := MinItems(s.Path, s.In, size, *s.MinItems); err != nil { + if err := MinItems(s.Path.dotted(), s.In, size, *s.MinItems); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } if s.MaxItems != nil { - if err := MaxItems(s.Path, s.In, size, *s.MaxItems); err != nil { + if err := MaxItems(s.Path.dotted(), s.In, size, *s.MaxItems); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } if s.UniqueItems { - if err := UniqueItems(s.Path, s.In, data); err != nil { + if err := UniqueItems(s.Path.dotted(), s.In, data); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } @@ -775,12 +770,16 @@ func (s *basicSliceValidator) Validate(data any) *Result { return nil } +func (s *basicSliceValidator) setPath(path pathSegments) { + s.Path = path +} + func (s *basicSliceValidator) redeem() { pools.poolOfBasicSliceValidators.RedeemValidator(s) } type numberValidator struct { - Path string + Path pathSegments In string Default any MultipleOf *float64 @@ -795,7 +794,7 @@ type numberValidator struct { } func newNumberValidator( - path, in string, def any, + path pathSegments, in string, def any, multipleOf, maximum *float64, exclusiveMaximum bool, minimum *float64, exclusiveMinimum bool, typ, format string, opts *SchemaValidatorOptions, @@ -826,10 +825,6 @@ func newNumberValidator( return n } -func (n *numberValidator) SetPath(path string) { - n.Path = path -} - func (n *numberValidator) Applies(source any, kind reflect.Kind) bool { switch source.(type) { case *spec.Parameter, *spec.Schema, *spec.Items, *spec.Header: @@ -881,21 +876,21 @@ func (n *numberValidator) Validate(val any) *Result { data := valueHelp.asFloat64(val) // Is the provided value within the range of the specified numeric type and format? - res.AddErrors(IsValueValidAgainstRange(val, n.Type, n.Format, "Checked", n.Path)) + res.AddErrors(IsValueValidAgainstRange(val, n.Type, n.Format, "Checked", n.Path.dotted())) if n.MultipleOf != nil { resMultiple = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMultiple.AddErrors(IsValueValidAgainstRange(*n.MultipleOf, n.Type, n.Format, "MultipleOf", n.Path)) + resMultiple.AddErrors(IsValueValidAgainstRange(*n.MultipleOf, n.Type, n.Format, "MultipleOf", n.Path.dotted())) if resMultiple.IsValid() { // Constraint validated with compatible types - if err := MultipleOfNativeType(n.Path, n.In, val, *n.MultipleOf); err != nil { + if err := MultipleOfNativeType(n.Path.dotted(), n.In, val, *n.MultipleOf); err != nil { resMultiple.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number - if err := MultipleOf(n.Path, n.In, data, *n.MultipleOf); err != nil { + if err := MultipleOf(n.Path.dotted(), n.In, data, *n.MultipleOf); err != nil { resMultiple.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } @@ -905,15 +900,15 @@ func (n *numberValidator) Validate(val any) *Result { resMaximum = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMaximum.AddErrors(IsValueValidAgainstRange(*n.Maximum, n.Type, n.Format, "Maximum boundary", n.Path)) + resMaximum.AddErrors(IsValueValidAgainstRange(*n.Maximum, n.Type, n.Format, "Maximum boundary", n.Path.dotted())) if resMaximum.IsValid() { // Constraint validated with compatible types - if err := MaximumNativeType(n.Path, n.In, val, *n.Maximum, n.ExclusiveMaximum); err != nil { + if err := MaximumNativeType(n.Path.dotted(), n.In, val, *n.Maximum, n.ExclusiveMaximum); err != nil { resMaximum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number - if err := Maximum(n.Path, n.In, data, *n.Maximum, n.ExclusiveMaximum); err != nil { + if err := Maximum(n.Path.dotted(), n.In, data, *n.Maximum, n.ExclusiveMaximum); err != nil { resMaximum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } @@ -923,15 +918,15 @@ func (n *numberValidator) Validate(val any) *Result { resMinimum = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMinimum.AddErrors(IsValueValidAgainstRange(*n.Minimum, n.Type, n.Format, "Minimum boundary", n.Path)) + resMinimum.AddErrors(IsValueValidAgainstRange(*n.Minimum, n.Type, n.Format, "Minimum boundary", n.Path.dotted())) if resMinimum.IsValid() { // Constraint validated with compatible types - if err := MinimumNativeType(n.Path, n.In, val, *n.Minimum, n.ExclusiveMinimum); err != nil { + if err := MinimumNativeType(n.Path.dotted(), n.In, val, *n.Minimum, n.ExclusiveMinimum); err != nil { resMinimum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number - if err := Minimum(n.Path, n.In, data, *n.Minimum, n.ExclusiveMinimum); err != nil { + if err := Minimum(n.Path.dotted(), n.In, data, *n.Minimum, n.ExclusiveMinimum); err != nil { resMinimum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) } } @@ -942,12 +937,16 @@ func (n *numberValidator) Validate(val any) *Result { return res } +func (n *numberValidator) setPath(path pathSegments) { + n.Path = path +} + func (n *numberValidator) redeem() { pools.poolOfNumberValidators.RedeemValidator(n) } type stringValidator struct { - Path string + Path pathSegments In string Default any Required bool @@ -959,7 +958,7 @@ type stringValidator struct { } func newStringValidator( - path, in string, + path pathSegments, in string, def any, required, allowEmpty bool, maxLength, minLength *int64, pattern string, opts *SchemaValidatorOptions, ) *stringValidator { @@ -987,10 +986,6 @@ func newStringValidator( return s } -func (s *stringValidator) SetPath(path string) { - s.Path = path -} - func (s *stringValidator) Applies(source any, kind reflect.Kind) bool { switch source.(type) { case *spec.Parameter, *spec.Schema, *spec.Items, *spec.Header: @@ -1009,35 +1004,39 @@ func (s *stringValidator) Validate(val any) *Result { data, ok := val.(string) if !ok { - return errorHelp.sErr(errors.InvalidType(s.Path, s.In, stringType, val), s.Options.recycleResult) + return errorHelp.sErr(errors.InvalidType(s.Path.dotted(), s.In, stringType, val), s.Options.recycleResult) } if s.Required && !s.AllowEmptyValue && (s.Default == nil || s.Default == "") { - if err := RequiredString(s.Path, s.In, data); err != nil { + if err := RequiredString(s.Path.dotted(), s.In, data); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } if s.MaxLength != nil { - if err := MaxLength(s.Path, s.In, data, *s.MaxLength); err != nil { + if err := MaxLength(s.Path.dotted(), s.In, data, *s.MaxLength); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } if s.MinLength != nil { - if err := MinLength(s.Path, s.In, data, *s.MinLength); err != nil { + if err := MinLength(s.Path.dotted(), s.In, data, *s.MinLength); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } if s.Pattern != "" { - if err := Pattern(s.Path, s.In, data, s.Pattern); err != nil { + if err := Pattern(s.Path.dotted(), s.In, data, s.Pattern); err != nil { return errorHelp.sErr(err, s.Options.recycleResult) } } return nil } +func (s *stringValidator) setPath(path pathSegments) { + s.Path = path +} + func (s *stringValidator) redeem() { pools.poolOfStringValidators.RedeemValidator(s) } diff --git a/validator_test.go b/validator_test.go index ff2738a..1d85741 100644 --- a/validator_test.go +++ b/validator_test.go @@ -66,7 +66,7 @@ func TestNumberValidator_EdgeCases(t *testing.T) { maximum := float64(math.MaxInt32 + 1) v := newNumberValidator( - "path", + newPathSegments("path"), "in", nil, nil, @@ -129,7 +129,7 @@ func TestStringValidator_EdgeCases(t *testing.T) { // Apply v := newStringValidator( - "", "", nil, false, false, nil, nil, "", nil, + nil, "", nil, false, false, nil, nil, "", nil, ) // stringValidator applies to: Parameter,Schema,Items,Header @@ -160,7 +160,7 @@ func TestBasicCommonValidator_EdgeCases(t *testing.T) { // Apply v := newBasicCommonValidator( - "", "", + nil, "", nil, []any{"a", nil, 3}, nil, ) @@ -190,7 +190,7 @@ func TestBasicCommonValidator_EdgeCases(t *testing.T) { t.Run("shoud validate empty Enum", func(t *testing.T) { ev := newBasicCommonValidator( - "", "", + nil, "", nil, nil, nil, ) res := ev.Validate("a") @@ -213,7 +213,7 @@ func testCommonApply(t *testing.T, v *basicCommonValidator, sources []any) { func TestBasicSliceValidator_EdgeCases(t *testing.T) { t.Run("should Apply", func(t *testing.T) { v := newBasicSliceValidator( - "", "", + nil, "", nil, nil, nil, false, nil, nil, strfmt.Default, nil, ) @@ -233,7 +233,7 @@ func TestBasicSliceValidator_EdgeCases(t *testing.T) { t.Run("with recycling", func(t *testing.T) { v := newBasicSliceValidator( - "", "", + nil, "", nil, nil, nil, false, nil, nil, strfmt.Default, &SchemaValidatorOptions{recycleValidators: true}, ) From 4c5522394dc866bc2a9c1ecb977a21cd1fc9e8f8 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sat, 8 Aug 2026 17:28:35 +0200 Subject: [PATCH 2/8] feat: report the JSON pointer of each validation error Result now knows where every error and warning it holds came from, and reports it as an RFC 6901 JSON pointer into the validated document: for _, e := range result.LocatedErrors() { fmt.Printf("%s: %v\n", e.Pointer, e.Err) } /definitions/Pet/name/default: ... must be of type string: "number" /paths/~1pets~1{id}/get/responses/200/examples/friends/0/name: ... Errors and Warnings keep their contents and their types, so nothing downstream has to change and no error value is wrapped. A pointer is empty when the check that failed has no single location to name, such as a duplicate operation id or a document that could not be read at all. Locations are held in slices kept in step with the error slices, and carried through merges, through the relevance filter and through the result pool. They are recorded by the validators themselves, so a location is as precise as the validator that reported the failure: the document-wide schema pass emits true document pointers, while a check that only knows a parameter by name reports the name. Two spec messages move as a result, both now naming the operation they belong to rather than the parameter or header alone: - "user.items in body has invalid pattern" becomes "paths./foo.get.parameters.user.items in body has invalid pattern"; - "X-Foo in header has invalid pattern" becomes "paths./foo.get.responses.default.headers.X-Foo in header has invalid pattern". Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 28 ++--- example_validator.go | 30 ++--- helpers.go | 87 ++++++++++++--- object_validator.go | 18 +-- result.go | 189 ++++++++++++++++++++++++++------ result_location_test.go | 236 ++++++++++++++++++++++++++++++++++++++++ schema.go | 6 +- schema_props.go | 14 +-- slice_validator.go | 8 +- spec.go | 101 +++++++++-------- spec_test.go | 6 +- type.go | 6 +- validator.go | 38 +++---- 13 files changed, 592 insertions(+), 175 deletions(-) create mode 100644 result_location_test.go diff --git a/default_validator.go b/default_validator.go index baca95d..a570d5b 100644 --- a/default_validator.go +++ b/default_validator.go @@ -81,7 +81,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.Default != nil && param.Required { - res.AddWarnings(requiredHasDefaultMsg(param.Name, param.In)) + res.addWarningsAt(parameterPath(path, method, param.Name), requiredHasDefaultMsg(param.Name, param.In)) } // reset explored schemas to get depth-first recursive-proof exploration @@ -93,7 +93,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, d.schemaOptions).Validate(param.Default) //#nosec if red.HasErrorsOrWarnings() { - res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -102,9 +102,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := d.validateDefaultValueItemsAgainstSchema(newPathSegments(param.Name), param.In, ¶m, param.Items) //#nosec + red := d.validateDefaultValueItemsAgainstSchema(parameterPath(path, method, param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { - res.AddErrors(defaultValueItemsDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -113,9 +113,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate default value against schema - red := d.validateDefaultValueSchemaAgainstSchema(newPathSegments(param.Name), param.In, param.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(parameterPath(path, method, param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { - res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -136,7 +136,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { } } else if op.ID != "" { // Empty op.ID means there is no meaningful operation: no need to report a specific message - res.AddErrors(noValidResponseMsg(op.ID)) + res.addErrorsAt(operationPath(path, method), noValidResponseMsg(op.ID)) } } } @@ -170,7 +170,7 @@ func (d *defaultValidator) validateDefaultInResponse( if h.Default != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, d.schemaOptions).Validate(h.Default) //#nosec if red.HasErrorsOrWarnings() { - res.AddErrors(defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) + res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -179,9 +179,9 @@ func (d *defaultValidator) validateDefaultInResponse( // Headers have inline definition, like params if h.Items != nil { - red := d.validateDefaultValueItemsAgainstSchema(newPathSegments(nm), "header", &h, h.Items) //#nosec + red := d.validateDefaultValueItemsAgainstSchema(responseHeaderPath(path, method, responseCodeAsStr, nm), "header", &h, h.Items) //#nosec if red.HasErrorsOrWarnings() { - res.AddErrors(defaultValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) + res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), defaultValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -189,7 +189,7 @@ func (d *defaultValidator) validateDefaultInResponse( } if _, err := compileRegexp(h.Pattern); err != nil { - res.AddErrors(invalidPatternInHeaderMsg(operationID, nm, responseName, h.Pattern, err)) + res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), invalidPatternInHeaderMsg(operationID, nm, responseName, h.Pattern, err)) } // Headers don't have schema @@ -202,7 +202,7 @@ func (d *defaultValidator) validateDefaultInResponse( red := d.validateDefaultValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost - res.AddErrors(defaultValueInDoesNotValidateMsg(operationID, responseName)) + res.addErrorsAt(responsePath(path, method, responseCodeAsStr), defaultValueInDoesNotValidateMsg(operationID, responseName)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -237,7 +237,7 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegm } } if _, err := compileRegexp(schema.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path.dotted(), in, schema.Pattern)) + res.addErrorsAt(path, invalidPatternInMsg(path.dotted(), in, schema.Pattern)) } if schema.AdditionalItems != nil && schema.AdditionalItems.Schema != nil { // NOTE: we keep validating values, even though additionalItems is not supported by Swagger 2.0 (and 3.0 as well) @@ -275,7 +275,7 @@ func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path pathSegme res.Merge(d.validateDefaultValueItemsAgainstSchema(path.item(0), in, root, items.Items)) } if _, err := compileRegexp(items.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path.dotted(), in, items.Pattern)) + res.addErrorsAt(path, invalidPatternInMsg(path.dotted(), in, items.Pattern)) } } return res diff --git a/example_validator.go b/example_validator.go index 713d8bb..f6a1d5d 100644 --- a/example_validator.go +++ b/example_validator.go @@ -83,7 +83,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, ex.schemaOptions).Validate(param.Example) //#nosec if red.HasErrorsOrWarnings() { - res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.MergeAsWarnings(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -92,9 +92,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := ex.validateExampleValueItemsAgainstSchema(newPathSegments(param.Name), param.In, ¶m, param.Items) //#nosec + red := ex.validateExampleValueItemsAgainstSchema(parameterPath(path, method, param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { - res.AddWarnings(exampleValueItemsDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -103,9 +103,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate example value against schema - red := ex.validateExampleValueSchemaAgainstSchema(newPathSegments(param.Name), param.In, param.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(parameterPath(path, method, param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { - res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -126,7 +126,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { } } else if op.ID != "" { // Empty op.ID means there is no meaningful operation: no need to report a specific message - res.AddErrors(noValidResponseMsg(op.ID)) + res.addErrorsAt(operationPath(path, method), noValidResponseMsg(op.ID)) } } } @@ -160,7 +160,7 @@ func (ex *exampleValidator) validateExampleInResponse( if h.Example != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, ex.schemaOptions).Validate(h.Example) //#nosec if red.HasErrorsOrWarnings() { - res.AddWarnings(exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) + res.addWarningsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.MergeAsWarnings(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -169,9 +169,9 @@ func (ex *exampleValidator) validateExampleInResponse( // Headers have inline definition, like params if h.Items != nil { - red := ex.validateExampleValueItemsAgainstSchema(newPathSegments(nm), "header", &h, h.Items) //#nosec + red := ex.validateExampleValueItemsAgainstSchema(responseHeaderPath(path, method, responseCodeAsStr, nm), "header", &h, h.Items) //#nosec if red.HasErrorsOrWarnings() { - res.AddWarnings(exampleValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) + res.addWarningsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), exampleValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName)) res.MergeAsWarnings(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -179,7 +179,7 @@ func (ex *exampleValidator) validateExampleInResponse( } if _, err := compileRegexp(h.Pattern); err != nil { - res.AddErrors(invalidPatternInHeaderMsg(operationID, nm, responseName, h.Pattern, err)) + res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), invalidPatternInHeaderMsg(operationID, nm, responseName, h.Pattern, err)) } // Headers don't have schema @@ -192,7 +192,7 @@ func (ex *exampleValidator) validateExampleInResponse( red := ex.validateExampleValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost - res.AddWarnings(exampleValueInDoesNotValidateMsg(operationID, responseName)) + res.addWarningsAt(responsePath(path, method, responseCodeAsStr), exampleValueInDoesNotValidateMsg(operationID, responseName)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -207,10 +207,10 @@ func (ex *exampleValidator) validateExampleInResponse( ) } else { // Proposal for enhancement: validate other media types too - res.AddWarnings(examplesMimeNotSupportedMsg(operationID, responseName)) + res.addWarningsAt(responsePath(path, method, responseCodeAsStr).child(swaggerExamples), examplesMimeNotSupportedMsg(operationID, responseName)) } } else { - res.AddWarnings(examplesWithoutSchemaMsg(operationID, responseName)) + res.addWarningsAt(responsePath(path, method, responseCodeAsStr).child(swaggerExamples), examplesWithoutSchemaMsg(operationID, responseName)) } } return res @@ -242,7 +242,7 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSeg } } if _, err := compileRegexp(schema.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path.dotted(), in, schema.Pattern)) + res.addErrorsAt(path, invalidPatternInMsg(path.dotted(), in, schema.Pattern)) } if schema.AdditionalItems != nil && schema.AdditionalItems.Schema != nil { // NOTE: we keep validating values, even though additionalItems is unsupported in Swagger 2.0 (and 3.0 as well) @@ -281,7 +281,7 @@ func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path pathSegm res.Merge(ex.validateExampleValueItemsAgainstSchema(path.item(0), in, root, items.Items)) } if _, err := compileRegexp(items.Pattern); err != nil { - res.AddErrors(invalidPatternInMsg(path.dotted(), in, items.Pattern)) + res.addErrorsAt(path, invalidPatternInMsg(path.dotted(), in, items.Pattern)) } } diff --git a/helpers.go b/helpers.go index 34b5d17..8226afe 100644 --- a/helpers.go +++ b/helpers.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/go-openapi/errors" + "github.com/go-openapi/jsonpointer" "github.com/go-openapi/spec" ) @@ -36,8 +37,8 @@ const ( jsonProperties = "properties" jsonItems = "items" jsonType = "type" - // jsonSchema = "schema". - jsonDefault = "default" + jsonSchema = "schema" + jsonDefault = "default" jsonAllOf = "allOf" jsonAdditionalItems = "additionalItems" @@ -47,11 +48,31 @@ const ( swaggerDefinitions = "definitions" swaggerResponses = "responses" swaggerParameters = "parameters" + swaggerHeaders = "headers" ) +// operationPath locates an operation in the spec document. +func operationPath(path, method string) pathSegments { + return newPathSegments(swaggerPaths, path, methodToken(method)) +} + +// parameterPath locates a parameter of an operation. +// +// Parameters are held in an array, so a name is not how the document addresses +// them. It is used all the same: it is what a reader recognizes, and resolving +// the index would mean carrying it down every recursion. +func parameterPath(path, method, name string) pathSegments { + return operationPath(path, method).children(swaggerParameters, name) +} + // responsePath locates a response of an operation in the spec document. func responsePath(path, method, responseCode string) pathSegments { - return newPathSegments(swaggerPaths, path, methodToken(method), swaggerResponses, responseCode) + return operationPath(path, method).children(swaggerResponses, responseCode) +} + +// responseHeaderPath locates a header declared by a response. +func responseHeaderPath(path, method, responseCode, header string) pathSegments { + return responsePath(path, method, responseCode).children(swaggerHeaders, header) } // methodToken normalizes an HTTP method into the key under which the operation @@ -61,6 +82,25 @@ func methodToken(method string) string { return strings.ToLower(method) } +// localRefPath turns a local JSON reference such as "#/definitions/Pet" into +// the location of what it points to. +// +// It yields the document root for anything that does not address a local +// fragment, a remote reference in particular. +func localRefPath(ref string) pathSegments { + rest, isLocal := strings.CutPrefix(ref, "#/") + if !isLocal { + return rootPath() + } + + tokens := strings.Split(rest, "/") + for i, token := range tokens { + tokens[i] = jsonpointer.Unescape(token) + } + + return newPathSegments(tokens...) +} + const ( stringFormatDate = "date" stringFormatDateTime = "date-time" @@ -112,24 +152,33 @@ type errorHelper struct { } func (h *errorHelper) sErr(err errors.Error, recycle bool) *Result { - // Builds a Result from standard errors.Error + return h.sErrAt(nil, err, recycle) +} + +// sErrAt builds a Result from a standard errors.Error reported at a known location. +func (h *errorHelper) sErrAt(at pathSegments, err errors.Error, recycle bool) *Result { var result *Result if recycle { result = pools.poolOfResults.BorrowResult() } else { result = new(Result) } - result.Errors = []error{err} + result.addErrorsAt(at, err) return result } func (h *errorHelper) addPointerError(res *Result, err error, ref string, fromPath string) *Result { - // Provides more context on error messages - // reported by the jsoinpointer package by altering the passed Result + return h.addPointerErrorAt(res, nil, err, ref, fromPath) +} + +// addPointerErrorAt provides more context on error messages reported by the +// jsonpointer package, by altering the passed Result. +func (h *errorHelper) addPointerErrorAt(res *Result, at pathSegments, err error, ref string, fromPath string) *Result { if err != nil { - res.AddErrors(cannotResolveRefMsg(fromPath, ref, err)) + res.addErrorsAt(at, cannotResolveRefMsg(fromPath, ref, err)) } + return res } @@ -246,9 +295,9 @@ func (h *paramHelper) safeExpandedParamsFor(path, method, operationID string, re for _, ppr := range s.expandedAnalyzer().SafeParamsFor(method, path, func(_ spec.Parameter, err error) bool { // since params have already been expanded, there are few causes for error - res.AddErrors(someParametersBrokenMsg(path, method, operationID)) + res.addErrorsAt(operationPath(path, method), someParametersBrokenMsg(path, method, operationID)) // original error from analyzer - res.AddErrors(err) + res.addErrorsAt(operationPath(path, method), err) return true }) { params = append(params, ppr) @@ -270,14 +319,16 @@ func (h *paramHelper) resolveParam(path, method, operationID string, param *spec if err != nil { // Safeguard // NOTE: we may enter here when the whole parameter is an unresolved $ref refPath := strings.Join([]string{"\"" + path + "\"", method}, ".") - errorHelp.addPointerError(res, err, param.Ref.String(), refPath) + errorHelp.addPointerErrorAt(res, parameterPath(path, method, param.Name), err, param.Ref.String(), refPath) return nil, res } - res.Merge(h.checkExpandedParam(param, param.Name, param.In, operationID, isRef)) + res.Merge(h.checkExpandedParam(param, param.Name, param.In, operationID, parameterPath(path, method, param.Name), isRef)) return param, res } -func (h *paramHelper) checkExpandedParam(pr *spec.Parameter, path, in, operation string, isRef bool) *Result { +func (h *paramHelper) checkExpandedParam( + pr *spec.Parameter, path, in, operation string, at pathSegments, isRef bool, +) *Result { // Secure parameter structure after $ref resolution res := new(Result) simpleZero := spec.SimpleSchema{} @@ -288,17 +339,17 @@ func (h *paramHelper) checkExpandedParam(pr *spec.Parameter, path, in, operation // Most likely, a $ref with a sibling is an unwanted situation: in itself this is a warning... // but we detect it because of the following error: // schema took over Parameter for an unexplained reason - res.AddWarnings(refShouldNotHaveSiblingsMsg(path, operation)) + res.addWarningsAt(at, refShouldNotHaveSiblingsMsg(path, operation)) } - res.AddErrors(invalidParameterDefinitionMsg(path, in, operation)) + res.addErrorsAt(at, invalidParameterDefinitionMsg(path, in, operation)) case pr.In != swaggerBody && pr.Schema != nil: if isRef { - res.AddWarnings(refShouldNotHaveSiblingsMsg(path, operation)) + res.addWarningsAt(at, refShouldNotHaveSiblingsMsg(path, operation)) } - res.AddErrors(invalidParameterDefinitionAsSchemaMsg(path, in, operation)) + res.addErrorsAt(at, invalidParameterDefinitionAsSchemaMsg(path, in, operation)) case (pr.In == swaggerBody && pr.Schema == nil) || (pr.In != swaggerBody && pr.SimpleSchema == simpleZero): // Other unexpected mishaps - res.AddErrors(invalidParameterDefinitionMsg(path, in, operation)) + res.addErrorsAt(at, invalidParameterDefinitionMsg(path, in, operation)) } return res } diff --git a/object_validator.go b/object_validator.go index 432b371..d67e44b 100644 --- a/object_validator.go +++ b/object_validator.go @@ -69,16 +69,16 @@ func (o *objectValidator) Validate(data any) *Result { var ok bool val, ok = data.(map[string]any) if !ok { - return errorHelp.sErr(invalidObjectMsg(o.Path.dotted(), o.In), o.Options.recycleResult) + return errorHelp.sErrAt(o.Path, invalidObjectMsg(o.Path.dotted(), o.In), o.Options.recycleResult) } } numKeys := int64(len(val)) if o.MinProperties != nil && numKeys < *o.MinProperties { - return errorHelp.sErr(errors.TooFewProperties(o.Path.dotted(), o.In, *o.MinProperties), o.Options.recycleResult) + return errorHelp.sErrAt(o.Path, errors.TooFewProperties(o.Path.dotted(), o.In, *o.MinProperties), o.Options.recycleResult) } if o.MaxProperties != nil && numKeys > *o.MaxProperties { - return errorHelp.sErr(errors.TooManyProperties(o.Path.dotted(), o.In, *o.MaxProperties), o.Options.recycleResult) + return errorHelp.sErrAt(o.Path, errors.TooManyProperties(o.Path.dotted(), o.In, *o.MaxProperties), o.Options.recycleResult) } var res *Result @@ -176,7 +176,7 @@ func (o *objectValidator) checkArrayMustHaveItems(res *Result, val map[string]an return } - res.AddErrors(errors.Required(jsonItems, o.Path.dotted(), item)) + res.addErrorsAt(o.Path.child(jsonItems), errors.Required(jsonItems, o.Path.dotted(), item)) } func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string]any) { @@ -196,11 +196,11 @@ func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string] t, typeFound := val[jsonType] if !typeFound { // there is no type - res.AddErrors(errors.Required(jsonType, o.Path.dotted(), t)) + res.addErrorsAt(o.Path.child(jsonType), errors.Required(jsonType, o.Path.dotted(), t)) } if tpe, isString := t.(string); !isString || tpe != arrayType { - res.AddErrors(errors.InvalidType(o.Path.dotted(), o.In, arrayType, nil)) + res.addErrorsAt(o.Path, errors.InvalidType(o.Path.dotted(), o.In, arrayType, nil)) } } @@ -240,7 +240,7 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res continue } - res.AddErrors(errors.PropertyNotAllowed(o.Path.dotted(), o.In, k)) + res.addErrorsAt(o.Path.child(k), errors.PropertyNotAllowed(o.Path.dotted(), o.In, k)) // BUG(fredbi): This section should move to a part dedicated to spec validation as // it will conflict with regular schemas where a property "headers" is defined. @@ -284,7 +284,7 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res } msg := strings.Join([]string{", one may not use $ref=\":", refString, "\""}, "") - res.AddErrors(refNotAllowedInHeaderMsg(o.Path.dotted(), headerKey, msg)) + res.addErrorsAt(o.Path, refNotAllowedInHeaderMsg(o.Path.dotted(), headerKey, msg)) /* case "$ref": if val[k] != nil { @@ -371,7 +371,7 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu continue } - res.AddErrors(errors.Required(o.Path.child(k).dotted(), o.In, v)) + res.addErrorsAt(o.Path.child(k), errors.Required(o.Path.child(k).dotted(), o.In, v)) } } diff --git a/result.go b/result.go index ede9455..5219b31 100644 --- a/result.go +++ b/result.go @@ -14,6 +14,17 @@ import ( var emptyResult = &Result{MatchCount: 1} +// Located pairs a validation error with the location of the value that caused it. +type Located struct { + // Err is the reported error or warning. + Err error + + // Pointer locates the offending value as an RFC 6901 JSON pointer, + // relative to the validated document. It is empty when the producer of + // the error did not know where it happened. + Pointer string +} + // Result represents a validation result set, composed of // errors and warnings. // @@ -25,12 +36,17 @@ var emptyResult = &Result{MatchCount: 1} // schema validation. Results from the validation branch // with most matches get eventually selected. // -// Proposal for enhancement: keep path of key originating the error. +// Use [Result.LocatedErrors] to know where each error happened. type Result struct { Errors []error Warnings []error MatchCount int + // errorLocations[i] locates Errors[i], and likewise for warnings. Kept + // aligned by the add methods; see [Result.LocatedErrors]. + errorLocations []string + warningLocations []string + // the object data data any @@ -173,8 +189,8 @@ func (r *Result) MergeAsErrors(others ...*Result) *Result { for _, other := range others { if other != nil { r.resetCaches() - r.AddErrors(other.Errors...) - r.AddErrors(other.Warnings...) + r.carryErrors(other.Errors, other.errorLocations) + r.carryErrors(other.Warnings, other.warningLocations) r.MatchCount += other.MatchCount if other.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(other) @@ -191,8 +207,8 @@ func (r *Result) MergeAsWarnings(others ...*Result) *Result { for _, other := range others { if other != nil { r.resetCaches() - r.AddWarnings(other.Errors...) - r.AddWarnings(other.Warnings...) + r.carryWarnings(other.Errors, other.errorLocations) + r.carryWarnings(other.Warnings, other.warningLocations) r.MatchCount += other.MatchCount if other.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(other) @@ -207,39 +223,79 @@ func (r *Result) MergeAsWarnings(others ...*Result) *Result { // Since the same check may be passed several times while exploring the // spec structure (via $ref, ...) reported messages are kept // unique. +// +// Errors added this way carry no location. Validators use [Result.addErrorsAt] +// so that [Result.LocatedErrors] can tell where the failure happened. func (r *Result) AddErrors(errors ...error) { - for _, e := range errors { - found := false - if e != nil { - for _, isReported := range r.Errors { - if e.Error() == isReported.Error() { - found = true - break - } - } - if !found { - r.Errors = append(r.Errors, e) - } - } - } + r.addLocatedErrors("", errors...) } // AddWarnings adds warnings to this validation result (if not already reported). func (r *Result) AddWarnings(warnings ...error) { - for _, e := range warnings { - found := false - if e != nil { - for _, isReported := range r.Warnings { - if e.Error() == isReported.Error() { - found = true - break - } - } - if !found { - r.Warnings = append(r.Warnings, e) - } + r.addLocatedWarnings("", warnings...) +} + +// isReportedError tells if the same message is already part of a collection. +func isReportedError(reported []error, e error) bool { + msg := e.Error() + for _, isReported := range reported { + if msg == isReported.Error() { + return true } } + + return false +} + +// locationAt reads a location out of a slice that may be shorter than the +// errors it describes. +func locationAt(locations []string, i int) string { + if i < len(locations) { + return locations[i] + } + + return "" +} + +// appendLocation records the location of the error that has just been appended, +// keeping the location slice aligned with the error slice it describes. +// +// Errors may reach a Result without going through the methods here (a caller +// assigning Errors directly, say), so the slice is padded rather than assumed +// to be in step. +func appendLocation(locations []string, upTo int, pointer string) []string { + for len(locations) < upTo-1 { + locations = append(locations, "") + } + + return append(locations, pointer) +} + +// LocatedErrors returns the reported errors, each paired with the JSON pointer +// of the value that caused it. +// +// The pointer is empty whenever the location is unknown, so callers should +// treat it as a hint and keep using the error message as the primary report. +func (r *Result) LocatedErrors() []Located { + return locate(r.Errors, r.errorLocations) +} + +// LocatedWarnings returns the reported warnings, each paired with the JSON +// pointer of the value that caused it. +func (r *Result) LocatedWarnings() []Located { + return locate(r.Warnings, r.warningLocations) +} + +func locate(errs []error, locations []string) []Located { + located := make([]Located, len(errs)) + for i, err := range errs { + located[i] = Located{Err: err} + if i < len(locations) { + located[i].Pointer = locations[i] + } + } + + return located } // IsValid returns true when this result is valid. @@ -298,6 +354,61 @@ func (r *Result) AsError() error { return errors.CompositeValidationError(r.Errors...) } +// addErrorsAt adds errors located at the given path. +func (r *Result) addErrorsAt(at pathSegments, errors ...error) { + r.addLocatedErrors(at.pointer(), errors...) +} + +// addWarningsAt adds warnings located at the given path. +func (r *Result) addWarningsAt(at pathSegments, warnings ...error) { + r.addLocatedWarnings(at.pointer(), warnings...) +} + +func (r *Result) addLocatedErrors(pointer string, errors ...error) { + for _, e := range errors { + if e == nil { + continue + } + + if isReportedError(r.Errors, e) { + continue + } + + r.Errors = append(r.Errors, e) + r.errorLocations = appendLocation(r.errorLocations, len(r.Errors), pointer) + } +} + +func (r *Result) addLocatedWarnings(pointer string, warnings ...error) { + for _, e := range warnings { + if e == nil { + continue + } + + if isReportedError(r.Warnings, e) { + continue + } + + r.Warnings = append(r.Warnings, e) + r.warningLocations = appendLocation(r.warningLocations, len(r.Warnings), 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) { + for i, e := range errs { + r.addLocatedErrors(locationAt(locations, i), e) + } +} + +// carryWarnings adds errors from another result as warnings, keeping locations. +func (r *Result) carryWarnings(errs []error, locations []string) { + for i, e := range errs { + r.addLocatedWarnings(locationAt(locations, i), e) + } +} + func (r *Result) resetCaches() { r.cachedFieldSchemata = nil r.cachedItemSchemata = nil @@ -391,8 +502,8 @@ func (r *Result) addSliceSchemata(slice reflect.Value, i int, schema *spec.Schem // mergeWithoutRootSchemata merges other into r, ignoring the rootObject schemata. func (r *Result) mergeWithoutRootSchemata(other *Result) { r.resetCaches() - r.AddErrors(other.Errors...) - r.AddWarnings(other.Warnings...) + r.carryErrors(other.Errors, other.errorLocations) + r.carryWarnings(other.Warnings, other.warningLocations) r.MatchCount += other.MatchCount if other.fieldSchemata != nil { @@ -438,15 +549,19 @@ func (r *Result) keepRelevantErrors() *Result { // codes would require to change a lot here. So, for the moment, let's go with // placeholders. strippedErrors := []error{} - for _, e := range r.Errors { + strippedErrorLocations := []string{} + for i, e := range r.Errors { if isImportant(e) { strippedErrors = append(strippedErrors, stripImportantTag(e)) + strippedErrorLocations = append(strippedErrorLocations, locationAt(r.errorLocations, i)) } } strippedWarnings := []error{} - for _, e := range r.Warnings { + strippedWarningLocations := []string{} + for i, e := range r.Warnings { if isImportant(e) { strippedWarnings = append(strippedWarnings, stripImportantTag(e)) + strippedWarningLocations = append(strippedWarningLocations, locationAt(r.warningLocations, i)) } } var strippedResult *Result @@ -456,14 +571,18 @@ func (r *Result) keepRelevantErrors() *Result { strippedResult = new(Result) } strippedResult.Errors = strippedErrors + strippedResult.errorLocations = strippedErrorLocations strippedResult.Warnings = strippedWarnings + strippedResult.warningLocations = strippedWarningLocations return strippedResult } func (r *Result) cleared() *Result { // clear the Result to be reusable. Keep allocated capacity. r.Errors = r.Errors[:0] + r.errorLocations = r.errorLocations[:0] r.Warnings = r.Warnings[:0] + r.warningLocations = r.warningLocations[:0] r.MatchCount = 0 r.data = nil r.rootObjectSchemata.one = nil diff --git a/result_location_test.go b/result_location_test.go new file mode 100644 index 0000000..0c4e50a --- /dev/null +++ b/result_location_test.go @@ -0,0 +1,236 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/loads" + "github.com/go-openapi/spec" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// Locations are kept in a slice parallel to the errors. The tests below guard +// that invariant across the operations that reshuffle a Result, and check the +// pointers actually resolve into the validated document. + +func TestResultLocations_AlignedAcrossMerges(t *testing.T) { + t.Parallel() + + first := new(Result) + first.addErrorsAt(newPathSegments("a", "0"), errOne) + first.AddErrors(errAnother) // no location + + second := new(Result) + second.addErrorsAt(newPathSegments("b"), errNew) + second.addWarningsAt(newPathSegments("c"), errOneWarning) + + first.Merge(second) + + located := first.LocatedErrors() + require.Len(t, located, 3) + assert.EqualT(t, "/a/0", located[0].Pointer) + assert.EqualT(t, errOne, located[0].Err) + assert.Empty(t, located[1].Pointer, "expected AddErrors to leave the location unknown") + assert.EqualT(t, errAnother, located[1].Err) + assert.EqualT(t, "/b", located[2].Pointer) + assert.EqualT(t, errNew, located[2].Err) + + warnings := first.LocatedWarnings() + require.Len(t, warnings, 1) + assert.EqualT(t, "/c", warnings[0].Pointer) +} + +func TestResultLocations_SurviveMergeAsWarnings(t *testing.T) { + t.Parallel() + + source := new(Result) + source.addErrorsAt(newPathSegments("a"), errOne) + source.addWarningsAt(newPathSegments("b"), errOneWarning) + + target := new(Result) + target.MergeAsWarnings(source) + + assert.Empty(t, target.Errors) + located := target.LocatedWarnings() + require.Len(t, located, 2) + assert.EqualT(t, "/a", located[0].Pointer) + assert.EqualT(t, "/b", located[1].Pointer) +} + +func TestResultLocations_SurviveMergeAsErrors(t *testing.T) { + t.Parallel() + + source := new(Result) + source.addErrorsAt(newPathSegments("a"), errOne) + source.addWarningsAt(newPathSegments("b"), errOneWarning) + + target := new(Result) + target.MergeAsErrors(source) + + located := target.LocatedErrors() + require.Len(t, located, 2) + assert.EqualT(t, "/a", located[0].Pointer) + assert.EqualT(t, "/b", located[1].Pointer) +} + +func TestResultLocations_DedupeKeepsTheFirstLocation(t *testing.T) { + t.Parallel() + + // AddErrors drops a message that is already reported; the location slice + // must not gain an entry for the error that was dropped. + res := new(Result) + res.addErrorsAt(newPathSegments("a"), errOne) + res.addErrorsAt(newPathSegments("b"), errOne) + + require.Len(t, res.Errors, 1) + located := res.LocatedErrors() + require.Len(t, located, 1) + assert.EqualT(t, "/a", located[0].Pointer) +} + +func TestResultLocations_ClearedOnRecycle(t *testing.T) { + t.Parallel() + + res := pools.poolOfResults.BorrowResult() + res.addErrorsAt(newPathSegments("a"), errOne) + require.Len(t, res.LocatedErrors(), 1) + + res = res.cleared() + assert.Empty(t, res.Errors) + assert.Empty(t, res.LocatedErrors(), "expected a recycled result to leak no location") +} + +func TestResultLocations_KeepRelevantErrors(t *testing.T) { + t.Parallel() + + res := new(Result) + res.addErrorsAt(newPathSegments("dropped"), errOne) + res.addErrorsAt(newPathSegments("kept"), errImportant) + + stripped := res.keepRelevantErrors() + located := stripped.LocatedErrors() + require.Len(t, located, 1) + assert.EqualT(t, "/kept", located[0].Pointer) +} + +func TestResultLocations_UnknownWhenNeverRecorded(t *testing.T) { + t.Parallel() + + res := new(Result) + res.AddErrors(errOne, errAnother) + + for _, located := range res.LocatedErrors() { + assert.Empty(t, located.Pointer) + } +} + +func TestResultLocations_SchemaValidation(t *testing.T) { + t.Parallel() + + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{ + "type": "object", + "properties": { + "friends": { + "type": "array", + "items": {"type": "object", "properties": {"name": {"type": "string"}}, "required": ["age"]} + }, + "n~x/y": {"type": "string", "maxLength": 2} + } + }`), schema)) + + data := map[string]any{ + "friends": []any{map[string]any{nameProp: 42}}, + "n~x/y": "far too long", + } + + res := NewSchemaValidator(schema, nil, "", strfmt.Default).Validate(data) + require.False(t, res.IsValid()) + + pointers := make([]string, 0, len(res.Errors)) + for _, located := range res.LocatedErrors() { + pointers = append(pointers, located.Pointer) + } + + assert.SliceContainsT(t, pointers, "/friends/0/name") + assert.SliceContainsT(t, pointers, "/friends/0/age") + assert.SliceContainsT(t, pointers, "/n~0x~1y", "expected the token to be escaped") +} + +func TestResultLocations_SpecValidation(t *testing.T) { + t.Parallel() + + const raw = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/pets/{id}": { + "get": { + "operationId": "getPet", + "parameters": [{"name": "id", "in": "path", "required": true, "type": "string"}], + "responses": { + "200": { + "description": "ok", + "schema": {"$ref": "#/definitions/Pet"}, + "examples": {"application/json": {"friends": [{"name": 7}]}} + } + } + } + } + }, + "definitions": { + "Pet": { + "type": "object", + "properties": { + "name": {"type": "string", "default": 42}, + "friends": {"type": "array", "items": {"$ref": "#/definitions/Pet"}} + } + }, + "Unused": {"type": "object"} + } + }` + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + res, warns := NewSpecValidator(doc.Schema(), strfmt.Default).Validate(doc) + require.False(t, res.IsValid()) + + t.Run("a default is located in the definition that declares it", func(t *testing.T) { + assert.SliceContainsT(t, pointersOf(res.LocatedErrors()), "/definitions/Pet/name/default") + }) + + t.Run("an example is located under its response", func(t *testing.T) { + assert.SliceContainsT(t, pointersOf(warns.LocatedErrors()), + "/paths/~1pets~1{id}/get/responses/200/examples/friends/0/name") + }) + + t.Run("an unused definition is located", func(t *testing.T) { + assert.SliceContainsT(t, pointersOf(warns.LocatedErrors()), "/definitions/Unused") + }) + + t.Run("every reported location is a valid JSON pointer", func(t *testing.T) { + for _, located := range append(res.LocatedErrors(), warns.LocatedErrors()...) { + if located.Pointer == "" { + continue + } + + assert.EqualT(t, uint8('/'), located.Pointer[0], + "expected a pointer to start with a separator, got %q", located.Pointer) + } + }) +} + +func pointersOf(located []Located) []string { + pointers := make([]string, 0, len(located)) + for _, l := range located { + pointers = append(pointers, l.Pointer) + } + + return pointers +} diff --git a/schema.go b/schema.go index 26e47df..2dfa39a 100644 --- a/schema.go +++ b/schema.go @@ -196,7 +196,7 @@ func (s *SchemaValidator) Validate(data any) *Result { // to map[string]interface{}. var dd any if err := jsonutils.FromDynamicJSON(data, &dd); err != nil { - result.AddErrors(err) + result.addErrorsAt(s.path, err) result.Inc() return result @@ -212,7 +212,7 @@ func (s *SchemaValidator) Validate(data any) *Result { if s.Schema.Type.Contains(integerType) { // avoid lossy conversion in, erri := num.Int64() if erri != nil { - result.AddErrors(invalidTypeConversionMsg(s.Path, erri)) + result.addErrorsAt(s.path, invalidTypeConversionMsg(s.Path, erri)) result.Inc() return result @@ -221,7 +221,7 @@ func (s *SchemaValidator) Validate(data any) *Result { } else { nf, errf := num.Float64() if errf != nil { - result.AddErrors(invalidTypeConversionMsg(s.Path, errf)) + result.addErrorsAt(s.path, invalidTypeConversionMsg(s.Path, errf)) result.Inc() return result diff --git a/schema_props.go b/schema_props.go index a9e2897..08a2030 100644 --- a/schema_props.go +++ b/schema_props.go @@ -174,7 +174,7 @@ func (s *schemaPropsValidator) validateAnyOf(data any, mainResult, keepResultAny } } - mainResult.AddErrors(mustValidateAtLeastOneSchemaMsg(s.Path.dotted())) + mainResult.addErrorsAt(s.Path, mustValidateAtLeastOneSchemaMsg(s.Path.dotted())) mainResult.Merge(bestFailures) } @@ -220,7 +220,7 @@ func (s *schemaPropsValidator) validateOneOf(data any, mainResult, keepResultOne switch validated { case 0: - mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path.dotted(), "Found none valid")) + mainResult.addErrorsAt(s.Path, mustValidateOnlyOneSchemaMsg(s.Path.dotted(), "Found none valid")) mainResult.Merge(bestFailures) // firstSucess necessarily nil case 1: @@ -229,7 +229,7 @@ func (s *schemaPropsValidator) validateOneOf(data any, mainResult, keepResultOne pools.poolOfResults.RedeemResult(bestFailures) } default: - mainResult.AddErrors(mustValidateOnlyOneSchemaMsg(s.Path.dotted(), fmt.Sprintf("Found %d valid alternatives", validated))) + mainResult.addErrorsAt(s.Path, mustValidateOnlyOneSchemaMsg(s.Path.dotted(), fmt.Sprintf("Found %d valid alternatives", validated))) mainResult.Merge(bestFailures) if firstSuccess != nil && firstSuccess.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(firstSuccess) @@ -256,10 +256,10 @@ func (s *schemaPropsValidator) validateAllOf(data any, mainResult, keepResultAll switch validated { case 0: - mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path.dotted(), ". None validated")) + mainResult.addErrorsAt(s.Path, mustValidateAllSchemasMsg(s.Path.dotted(), ". None validated")) case len(s.allOfValidators): default: - mainResult.AddErrors(mustValidateAllSchemasMsg(s.Path.dotted(), "")) + mainResult.addErrorsAt(s.Path, mustValidateAllSchemasMsg(s.Path.dotted(), "")) } } @@ -270,7 +270,7 @@ func (s *schemaPropsValidator) validateNot(data any, mainResult *Result) { } // We keep inner IMPORTANT! errors no matter what MatchCount tells us if result.IsValid() { - mainResult.AddErrors(mustNotValidatechemaMsg(s.Path.dotted())) + mainResult.addErrorsAt(s.Path, mustNotValidatechemaMsg(s.Path.dotted())) } if result.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(result) // this result is ditched @@ -295,7 +295,7 @@ func (s *schemaPropsValidator) validateDependencies(data any, mainResult *Result if len(dep.Property) > 0 { for _, depKey := range dep.Property { if _, ok := val[depKey]; !ok { - mainResult.AddErrors(hasADependencyMsg(s.Path.dotted(), depKey)) + mainResult.addErrorsAt(s.Path, hasADependencyMsg(s.Path.dotted(), depKey)) } } } diff --git a/slice_validator.go b/slice_validator.go index db0d989..3445dad 100644 --- a/slice_validator.go +++ b/slice_validator.go @@ -103,7 +103,7 @@ func (s *schemaSliceValidator) Validate(data any) *Result { } if s.AdditionalItems != nil && itemsSize < size { if s.Items != nil && len(s.Items.Schemas) > 0 && !s.AdditionalItems.Allows { - result.AddErrors(arrayDoesNotAllowAdditionalItemsMsg()) + result.addErrorsAt(s.Path, arrayDoesNotAllowAdditionalItemsMsg()) } if s.AdditionalItems.Schema != nil { for i := itemsSize; i < size-itemsSize+1; i++ { @@ -115,17 +115,17 @@ func (s *schemaSliceValidator) Validate(data any) *Result { if s.MinItems != nil { if err := MinItems(s.Path.dotted(), s.In, int64(size), *s.MinItems); err != nil { - result.AddErrors(err) + result.addErrorsAt(s.Path, err) } } if s.MaxItems != nil { if err := MaxItems(s.Path.dotted(), s.In, int64(size), *s.MaxItems); err != nil { - result.AddErrors(err) + result.addErrorsAt(s.Path, err) } } if s.UniqueItems { if err := UniqueItems(s.Path.dotted(), s.In, val.Interface()); err != nil { - result.AddErrors(err) + result.addErrorsAt(s.Path, err) } } result.Inc() diff --git a/spec.go b/spec.go index 3a786cb..1171499 100644 --- a/spec.go +++ b/spec.go @@ -10,6 +10,7 @@ import ( "fmt" "slices" "sort" + "strconv" "strings" "github.com/go-openapi/analysis" @@ -111,7 +112,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // errs holds all errors and warnings, // warnings only warnings errs.MergeAsWarnings(warnings) - warnings.AddErrors(errs.Warnings...) + // reported as errors of the warnings-only result, but keeping the + // location each was recorded with + warnings.carryErrors(errs.Warnings, errs.warningLocations) }() // Swagger schema validator @@ -170,21 +173,21 @@ func (s *SpecValidator) validateNonEmptyPathParamNames() *Result { res := pools.poolOfResults.BorrowResult() if s.spec.Spec().Paths == nil { // There is no Paths object: error - res.AddErrors(noValidPathMsg()) + res.addErrorsAt(newPathSegments(swaggerPaths), noValidPathMsg()) return res } if s.spec.Spec().Paths.Paths == nil { // Paths may be empty: warning - res.AddWarnings(noValidPathMsg()) + res.addWarningsAt(newPathSegments(swaggerPaths), noValidPathMsg()) return res } for k := range s.spec.Spec().Paths.Paths { if strings.Contains(k, "{}") { - res.AddErrors(emptyPathParameterMsg(k)) + res.addErrorsAt(newPathSegments(swaggerPaths, k), emptyPathParameterMsg(k)) } } @@ -238,7 +241,7 @@ func (s *SpecValidator) validateDuplicatePropertyNames() *Result { res.Merge(rec) } if len(ancs) > 0 { - res.AddErrors(circularAncestryDefinitionMsg(k, ancs)) + res.addErrorsAt(newPathSegments(swaggerDefinitions, k), circularAncestryDefinitionMsg(k, ancs)) return res } @@ -252,7 +255,7 @@ func (s *SpecValidator) validateDuplicatePropertyNames() *Result { for _, v := range dups { pns = append(pns, v.Definition+"."+v.Name) } - res.AddErrors(duplicatePropertiesMsg(k, pns)) + res.addErrorsAt(newPathSegments(swaggerDefinitions, k), duplicatePropertiesMsg(k, pns)) } } @@ -367,7 +370,7 @@ func (s *SpecValidator) validateItems() *Result { for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.TypeName() == arrayType && param.ItemsTypeName() == "" { - res.AddErrors(arrayInParamRequiresItemsMsg(param.Name, op.ID)) + res.addErrorsAt(parameterPath(path, method, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) continue } if param.In != swaggerBody { @@ -375,7 +378,7 @@ func (s *SpecValidator) validateItems() *Result { items := param.Items for items.TypeName() == arrayType { if items.ItemsTypeName() == "" { - res.AddErrors(arrayInParamRequiresItemsMsg(param.Name, op.ID)) + res.addErrorsAt(parameterPath(path, method, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) break } items = items.Items @@ -384,32 +387,38 @@ func (s *SpecValidator) validateItems() *Result { } else { // In: body if param.Schema != nil { - res.Merge(s.validateSchemaItems(*param.Schema, fmt.Sprintf("body param %q", param.Name), op.ID)) + res.Merge(s.validateSchemaItems(*param.Schema, parameterPath(path, method, param.Name).child(jsonSchema), + fmt.Sprintf("body param %q", param.Name), op.ID)) } } } - var responses []spec.Response + type codedResponse struct { + code string + resp spec.Response + } + var responses []codedResponse if op.Responses != nil { if op.Responses.Default != nil { - responses = append(responses, *op.Responses.Default) + responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) } if op.Responses.StatusCodeResponses != nil { - for _, v := range op.Responses.StatusCodeResponses { - responses = append(responses, v) + for code, v := range op.Responses.StatusCodeResponses { + responses = append(responses, codedResponse{code: strconv.Itoa(code), resp: v}) } } } for _, resp := range responses { + at := responsePath(path, method, resp.code) // Response headers with array - for hn, hv := range resp.Headers { + for hn, hv := range resp.resp.Headers { if hv.TypeName() == arrayType && hv.ItemsTypeName() == "" { - res.AddErrors(arrayInHeaderRequiresItemsMsg(hn, op.ID)) + res.addErrorsAt(at.children(swaggerHeaders, hn), arrayInHeaderRequiresItemsMsg(hn, op.ID)) } } - if resp.Schema != nil { - res.Merge(s.validateSchemaItems(*resp.Schema, "response body", op.ID)) + if resp.resp.Schema != nil { + res.Merge(s.validateSchemaItems(*resp.resp.Schema, at.child(jsonSchema), "response body", op.ID)) } } } @@ -418,24 +427,24 @@ func (s *SpecValidator) validateItems() *Result { } // Verifies constraints on array type. -func (s *SpecValidator) validateSchemaItems(schema spec.Schema, prefix, opID string) *Result { +func (s *SpecValidator) validateSchemaItems(schema spec.Schema, at pathSegments, prefix, opID string) *Result { res := pools.poolOfResults.BorrowResult() if !schema.Type.Contains(arrayType) { return res } if schema.Items == nil || schema.Items.Len() == 0 { - res.AddErrors(arrayRequiresItemsMsg(prefix, opID)) + res.addErrorsAt(at, arrayRequiresItemsMsg(prefix, opID)) return res } if schema.Items.Schema != nil { schema = *schema.Items.Schema if _, err := compileRegexp(schema.Pattern); err != nil { - res.AddErrors(invalidItemsPatternMsg(prefix, opID, schema.Pattern)) + res.addErrorsAt(at, invalidItemsPatternMsg(prefix, opID, schema.Pattern)) } - res.Merge(s.validateSchemaItems(schema, prefix, opID)) + res.Merge(s.validateSchemaItems(schema, at.child(jsonItems), prefix, opID)) } return res } @@ -453,7 +462,7 @@ func (s *SpecValidator) validatePathParamPresence(path string, fromPath, fromOpe } } if !matched { - res.AddErrors(noParameterInPathMsg(l)) + res.addErrorsAt(newPathSegments(swaggerPaths, l), noParameterInPathMsg(l)) } } @@ -463,7 +472,7 @@ func (s *SpecValidator) validatePathParamPresence(path string, fromPath, fromOpe matched = true } if !matched { - res.AddErrors(pathParamNotInPathMsg(path, p)) + res.addErrorsAt(newPathSegments(swaggerPaths, path), pathParamNotInPathMsg(path, p)) } } @@ -498,7 +507,7 @@ func (s *SpecValidator) validateReferencedParameters() *Result { } result := pools.poolOfResults.BorrowResult() for k := range expected { - result.AddWarnings(unusedParamMsg(k)) + result.addWarningsAt(localRefPath(k), unusedParamMsg(k)) } return result } @@ -523,7 +532,7 @@ func (s *SpecValidator) validateReferencedResponses() *Result { } result := pools.poolOfResults.BorrowResult() for k := range expected { - result.AddWarnings(unusedResponseMsg(k)) + result.addWarningsAt(localRefPath(k), unusedResponseMsg(k)) } return result } @@ -549,7 +558,7 @@ func (s *SpecValidator) validateReferencedDefinitions() *Result { result := new(Result) for k := range expected { - result.AddWarnings(unusedDefinitionMsg(k)) + result.addWarningsAt(localRefPath(k), unusedDefinitionMsg(k)) } return result } @@ -562,7 +571,7 @@ DEFINITIONS: for d, schema := range s.spec.Spec().Definitions { if schema.Required != nil { // Safeguard for _, pn := range schema.Required { - red := s.validateRequiredProperties(pn, d, &schema) //#nosec + red := s.validateRequiredProperties(pn, d, newPathSegments(swaggerDefinitions, d), &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. @@ -577,7 +586,7 @@ DEFINITIONS: return res } -func (s *SpecValidator) validateRequiredProperties(path, in string, v *spec.Schema) *Result { +func (s *SpecValidator) validateRequiredProperties(path, in string, at pathSegments, v *spec.Schema) *Result { // Takes care of recursive property definitions, which may be nested in additionalProperties schemas res := pools.poolOfResults.BorrowResult() propertyMatch := false @@ -596,7 +605,7 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, v *spec.Sche for pp, pv := range v.PatternProperties { re, err := compileRegexp(pp) if err != nil { - res.AddErrors(invalidPatternMsg(pp, in)) + res.addErrorsAt(at, invalidPatternMsg(pp, in)) } else if re.MatchString(path) { patternMatch = true if !propertyMatch { @@ -613,7 +622,7 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, v *spec.Sche // 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, v.AdditionalProperties.Schema) + red := s.validateRequiredProperties(path, in, at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema) if red.IsValid() { additionalPropertiesMatch = true if !propertyMatch && !patternMatch { @@ -626,11 +635,11 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, v *spec.Sche } if !propertyMatch && !patternMatch && !additionalPropertiesMatch { - res.AddErrors(requiredButNotDefinedMsg(path, in)) + res.addErrorsAt(at, requiredButNotDefinedMsg(path, in)) } if isReadOnly { - res.AddWarnings(readOnlyAndRequiredMsg(in, path)) + res.addWarningsAt(at, readOnlyAndRequiredMsg(in, path)) } return res } @@ -656,16 +665,16 @@ func (s *SpecValidator) validateParameters() *Result { // Warn on garbled path afer param stripping if rexGarbledPathSegment.MatchString(pathToAdd) { - res.AddWarnings(pathStrippedParamGarbledMsg(pathToAdd)) + res.addWarningsAt(newPathSegments(swaggerPaths, path), pathStrippedParamGarbledMsg(pathToAdd)) } // Check uniqueness of stripped paths if _, found := methodPaths[method][pathToAdd]; found { // Sort names for stable, testable output if strings.Compare(path, methodPaths[method][pathToAdd]) < 0 { - res.AddErrors(pathOverlapMsg(path, methodPaths[method][pathToAdd])) + res.addErrorsAt(newPathSegments(swaggerPaths, path), pathOverlapMsg(path, methodPaths[method][pathToAdd])) } else { - res.AddErrors(pathOverlapMsg(methodPaths[method][pathToAdd], path)) + res.addErrorsAt(newPathSegments(swaggerPaths, path), pathOverlapMsg(methodPaths[method][pathToAdd], path)) } } else { if _, found := methodPaths[method]; !found { @@ -700,7 +709,7 @@ func (s *SpecValidator) validateParameters() *Result { schv := newSchemaValidator(¶mSchema, s.schema, newPathSegments(swaggerPaths, path, methodToken(method), swaggerParameters, pr.Name), s.KnownFormats, s.schemaOptions) var obj any if err := jsonutils.FromDynamicJSON(pr, &obj); err != nil { - res.AddErrors(err) + res.addErrorsAt(parameterPath(path, method, pr.Name), err) return res } @@ -709,7 +718,7 @@ func (s *SpecValidator) validateParameters() *Result { // Validate pattern regexp for parameters with a Pattern property if _, err := compileRegexp(pr.Pattern); err != nil { - res.AddErrors(invalidPatternInParamMsg(op.ID, pr.Name, pr.Pattern)) + res.addErrorsAt(parameterPath(path, method, pr.Name), invalidPatternInParamMsg(op.ID, pr.Name, pr.Pattern)) } // There must be at most one parameter in body: list them all @@ -722,7 +731,7 @@ func (s *SpecValidator) validateParameters() *Result { paramNames = append(paramNames, pr.Name) // Path declared in path must have the required: true property if !pr.Required { - res.AddErrors(pathParamRequiredMsg(op.ID, pr.Name)) + res.addErrorsAt(parameterPath(path, method, pr.Name), pathParamRequiredMsg(op.ID, pr.Name)) } } @@ -733,31 +742,31 @@ func (s *SpecValidator) validateParameters() *Result { if pr.Type != numberType && pr.Type != integerType && (pr.Maximum != nil || pr.Minimum != nil || pr.MultipleOf != nil) { // A non-numeric parameter has validation keywords for numeric instances (number and integer) - res.AddWarnings(parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } if pr.Type != stringType && // A non-string parameter has validation keywords for strings (pr.MaxLength != nil || pr.MinLength != nil || pr.Pattern != "") { - res.AddWarnings(parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } if pr.Type != arrayType && // A non-array parameter has validation keywords for arrays (pr.MaxItems != nil || pr.MinItems != nil || pr.UniqueItems) { - res.AddWarnings(parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } } // In:formData and In:body are mutually exclusive if hasBody && hasForm { - res.AddErrors(bothFormDataAndBodyMsg(op.ID)) + res.addErrorsAt(operationPath(path, method), bothFormDataAndBodyMsg(op.ID)) } // There must be at most one body param // Accurately report situations when more than 1 body param is declared (possibly unnamed) if len(bodyParams) > 1 { sort.Strings(bodyParams) - res.AddErrors(multipleBodyParamMsg(op.ID, bodyParams)) + res.addErrorsAt(operationPath(path, method), multipleBodyParamMsg(op.ID, bodyParams)) } // Check uniqueness of parameters in path @@ -765,7 +774,7 @@ func (s *SpecValidator) validateParameters() *Result { for i, p := range paramsInPath { for j, q := range paramsInPath { if p == q && i > j { - res.AddErrors(pathParamNotUniqueMsg(path, p, q)) + res.addErrorsAt(newPathSegments(swaggerPaths, path), pathParamNotUniqueMsg(path, p, q)) break } } @@ -775,7 +784,7 @@ func (s *SpecValidator) validateParameters() *Result { rexGarbledParam := mustCompileRegexp(`{.*[{}\s]+.*}`) for _, p := range paramsInPath { if rexGarbledParam.MatchString(p) { - res.AddWarnings(pathParamGarbledMsg(path, p)) + res.addWarningsAt(newPathSegments(swaggerPaths, path), pathParamGarbledMsg(path, p)) } } @@ -830,7 +839,7 @@ func (s *SpecValidator) checkUniqueParams(path, method string, op *spec.Operatio key := fmt.Sprintf("%s#%s", pr.In, pr.Name) if _, ok = pnames[key]; ok { - res.AddErrors(duplicateParamNameMsg(pr.In, pr.Name, op.ID)) + res.addErrorsAt(parameterPath(path, method, pr.Name), duplicateParamNameMsg(pr.In, pr.Name, op.ID)) } pnames[key] = struct{}{} } diff --git a/spec_test.go b/spec_test.go index 1e9e2f5..4f382f9 100644 --- a/spec_test.go +++ b/spec_test.go @@ -215,7 +215,8 @@ func TestSpec_Issue18(t *testing.T) { verifiedErrors := verifiedTestErrors(res) switch { case strings.Contains(path, "headerItems.json"): - assert.SliceContainsT(t, verifiedErrors, "X-Foo in header has invalid pattern: \")<-- bad pattern\"") + assert.SliceContainsT(t, verifiedErrors, + "paths./foo.get.responses.default.headers.X-Foo in header has invalid pattern: \")<-- bad pattern\"") case strings.Contains(path, "headers.json"): const badPatternSuffix = ` has invalid pattern ")<-- bad pattern": error parsing regexp: unexpected ): ` + "`)<-- bad pattern`" @@ -227,7 +228,8 @@ func TestSpec_Issue18(t *testing.T) { case strings.Contains(path, "paramItems.json"): assert.SliceContainsT(t, verifiedErrors, "body param \"user\" for \"\" has invalid items pattern: \")<-- bad pattern\"") assert.SliceContainsT(t, verifiedErrors, "default value for user in body does not validate its schema") - assert.SliceContainsT(t, verifiedErrors, "user.items in body has invalid pattern: \")<-- bad pattern\"") + assert.SliceContainsT(t, verifiedErrors, + "paths./foo.get.parameters.user.items in body has invalid pattern: \")<-- bad pattern\"") case strings.Contains(path, "parameters.json"): assert.SliceContainsT(t, verifiedErrors, "operation \"\" has invalid pattern in param \"userId\": \")<-- bad pattern\"") case strings.Contains(path, "schema.json"): diff --git a/type.go b/type.go index 1b23f6a..9baa3d6 100644 --- a/type.go +++ b/type.go @@ -68,7 +68,7 @@ func (t *typeValidator) Validate(data any) *Result { if data == nil { // nil or zero value for the passed structure require Type: null if len(t.Type) > 0 && !t.Type.Contains(nullType) && !t.Nullable { // NOTE: if a property is not required it also passes this - return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), nullType), t.Options.recycleResult) + return errorHelp.sErrAt(t.Path, errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), nullType), t.Options.recycleResult) } return emptyResult @@ -94,7 +94,7 @@ func (t *typeValidator) Validate(data any) *Result { !isFloatInt && !isIntFloat && !isLowerInt && !isLowerFloat if formatMismatch { // NOTE: test case - return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, t.Format, format), t.Options.recycleResult) + return errorHelp.sErrAt(t.Path, errors.InvalidType(t.Path.dotted(), t.In, t.Format, format), t.Options.recycleResult) } if !t.Type.Contains(numberType) && !t.Type.Contains(integerType) && t.Format != "" && (kind == reflect.String || kind == reflect.Slice) { @@ -102,7 +102,7 @@ func (t *typeValidator) Validate(data any) *Result { } if !t.Type.Contains(schType) && !isFloatInt && !isIntFloat { - return errorHelp.sErr(errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), schType), t.Options.recycleResult) + return errorHelp.sErrAt(t.Path, errors.InvalidType(t.Path.dotted(), t.In, strings.Join(t.Type, ","), schType), t.Options.recycleResult) } return emptyResult diff --git a/validator.go b/validator.go index b8c4e75..46a32ca 100644 --- a/validator.go +++ b/validator.go @@ -274,7 +274,7 @@ func (b *basicCommonValidator) Validate(data any) (res *Result) { } } - return errorHelp.sErr(errors.EnumFail(b.Path.dotted(), b.In, data, b.Enum), b.Options.recycleResult) + return errorHelp.sErrAt(b.Path, errors.EnumFail(b.Path.dotted(), b.In, data, b.Enum), b.Options.recycleResult) } func (b *basicCommonValidator) setPath(path pathSegments) { @@ -734,19 +734,19 @@ func (s *basicSliceValidator) Validate(data any) *Result { size := int64(val.Len()) if s.MinItems != nil { if err := MinItems(s.Path.dotted(), s.In, size, *s.MinItems); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } if s.MaxItems != nil { if err := MaxItems(s.Path.dotted(), s.In, size, *s.MaxItems); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } if s.UniqueItems { if err := UniqueItems(s.Path.dotted(), s.In, data); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } @@ -876,22 +876,22 @@ func (n *numberValidator) Validate(val any) *Result { data := valueHelp.asFloat64(val) // Is the provided value within the range of the specified numeric type and format? - res.AddErrors(IsValueValidAgainstRange(val, n.Type, n.Format, "Checked", n.Path.dotted())) + res.addErrorsAt(n.Path, IsValueValidAgainstRange(val, n.Type, n.Format, "Checked", n.Path.dotted())) if n.MultipleOf != nil { resMultiple = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMultiple.AddErrors(IsValueValidAgainstRange(*n.MultipleOf, n.Type, n.Format, "MultipleOf", n.Path.dotted())) + resMultiple.addErrorsAt(n.Path, IsValueValidAgainstRange(*n.MultipleOf, n.Type, n.Format, "MultipleOf", n.Path.dotted())) if resMultiple.IsValid() { // Constraint validated with compatible types if err := MultipleOfNativeType(n.Path.dotted(), n.In, val, *n.MultipleOf); err != nil { - resMultiple.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMultiple.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number if err := MultipleOf(n.Path.dotted(), n.In, data, *n.MultipleOf); err != nil { - resMultiple.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMultiple.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } } @@ -900,16 +900,16 @@ func (n *numberValidator) Validate(val any) *Result { resMaximum = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMaximum.AddErrors(IsValueValidAgainstRange(*n.Maximum, n.Type, n.Format, "Maximum boundary", n.Path.dotted())) + resMaximum.addErrorsAt(n.Path, IsValueValidAgainstRange(*n.Maximum, n.Type, n.Format, "Maximum boundary", n.Path.dotted())) if resMaximum.IsValid() { // Constraint validated with compatible types if err := MaximumNativeType(n.Path.dotted(), n.In, val, *n.Maximum, n.ExclusiveMaximum); err != nil { - resMaximum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMaximum.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number if err := Maximum(n.Path.dotted(), n.In, data, *n.Maximum, n.ExclusiveMaximum); err != nil { - resMaximum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMaximum.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } } @@ -918,16 +918,16 @@ func (n *numberValidator) Validate(val any) *Result { resMinimum = pools.poolOfResults.BorrowResult() // Is the constraint specifier within the range of the specific numeric type and format? - resMinimum.AddErrors(IsValueValidAgainstRange(*n.Minimum, n.Type, n.Format, "Minimum boundary", n.Path.dotted())) + resMinimum.addErrorsAt(n.Path, IsValueValidAgainstRange(*n.Minimum, n.Type, n.Format, "Minimum boundary", n.Path.dotted())) if resMinimum.IsValid() { // Constraint validated with compatible types if err := MinimumNativeType(n.Path.dotted(), n.In, val, *n.Minimum, n.ExclusiveMinimum); err != nil { - resMinimum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMinimum.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } else { // Constraint nevertheless validated, converted as general number if err := Minimum(n.Path.dotted(), n.In, data, *n.Minimum, n.ExclusiveMinimum); err != nil { - resMinimum.Merge(errorHelp.sErr(err, n.Options.recycleResult)) + resMinimum.Merge(errorHelp.sErrAt(n.Path, err, n.Options.recycleResult)) } } } @@ -1004,30 +1004,30 @@ func (s *stringValidator) Validate(val any) *Result { data, ok := val.(string) if !ok { - return errorHelp.sErr(errors.InvalidType(s.Path.dotted(), s.In, stringType, val), s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, errors.InvalidType(s.Path.dotted(), s.In, stringType, val), s.Options.recycleResult) } if s.Required && !s.AllowEmptyValue && (s.Default == nil || s.Default == "") { if err := RequiredString(s.Path.dotted(), s.In, data); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } if s.MaxLength != nil { if err := MaxLength(s.Path.dotted(), s.In, data, *s.MaxLength); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } if s.MinLength != nil { if err := MinLength(s.Path.dotted(), s.In, data, *s.MinLength); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } if s.Pattern != "" { if err := Pattern(s.Path.dotted(), s.In, data, s.Pattern); err != nil { - return errorHelp.sErr(err, s.Options.recycleResult) + return errorHelp.sErrAt(s.Path, err, s.Options.recycleResult) } } return nil From 97d6cca06030c5b1c2d1dda639fb8fcf04598aa7 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sat, 8 Aug 2026 18:08:15 +0200 Subject: [PATCH 3/8] feat: locate the $ref diagnostics in the document An invalid or dubious $ref now reports where it was declared: /definitions/Bad: invalid ref "file:///etc/passwd" Both checks iterate the analyzer's references, which are handed over as values only: its index is keyed by document location, but neither AllRefs nor AllReferences exposes the keys. The locations are recovered instead by walking the raw document for "$ref" members, which needs no interpretation of what a reference points to, and keeps the index in step with the unexpanded spec the two checks are meant to inspect. The index is best effort. Example subtrees are skipped, since a "$ref" member there is plain data rather than a declaration, and a reference declared more than once keeps the smallest pointer so the answer does not depend on map iteration order. A miss costs a misleading or empty pointer, never a wrong verdict: nothing else is decided from it. Diagnostics that no single location can describe still report an empty pointer, a spread of remote hosts and a duplicate operation id among them. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- helpers.go | 1 + ref_locations.go | 70 +++++++++++++++ ref_locations_test.go | 173 ++++++++++++++++++++++++++++++++++++++ spec.go | 6 +- spec_ref_warnings.go | 2 +- spec_ref_warnings_test.go | 6 ++ 6 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 ref_locations.go create mode 100644 ref_locations_test.go diff --git a/helpers.go b/helpers.go index 8226afe..b815443 100644 --- a/helpers.go +++ b/helpers.go @@ -38,6 +38,7 @@ const ( jsonItems = "items" jsonType = "type" jsonSchema = "schema" + jsonRef = "$ref" jsonDefault = "default" jsonAllOf = "allOf" diff --git a/ref_locations.go b/ref_locations.go new file mode 100644 index 0000000..7e7a91a --- /dev/null +++ b/ref_locations.go @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +// refLocations indexes the $ref values declared by a raw document, telling +// where each one sits. +// +// The analyzer keeps the same index — its reference map is keyed by document +// location — but hands over only the values, so the locations are recovered +// here. Finding them does not require interpreting a reference: it is enough +// to spot the "$ref" members while walking the document. +// +// The result is best effort. A reference may be declared in several places, +// and only one location is kept; example values are skipped, but any other +// data that happens to hold a "$ref" string could still be mistaken for a +// declaration. A wrong answer costs a misleading pointer, never a wrong +// verdict, since nothing else is decided from it. +type refLocations map[string]pathSegments + +// newRefLocations indexes every reference declared by a raw document. +func newRefLocations(document any) refLocations { + locations := make(refLocations) + locations.collect(document, rootPath()) + + return locations +} + +// at returns where a reference is declared, or the document root when the +// reference was not found. +func (l refLocations) at(ref string) pathSegments { + return l[ref] +} + +func (l refLocations) collect(node any, at pathSegments) { + switch typed := node.(type) { + case map[string]any: + for key, value := range typed { + if key == jsonRef { + if ref, isString := value.(string); isString && ref != "" { + l.keep(ref, at) + } + + continue + } + + if key == swaggerExample || key == swaggerExamples { + // plain data: a "$ref" down there declares nothing + continue + } + + l.collect(value, at.child(key)) + } + case []any: + for i, value := range typed { + l.collect(value, at.item(i)) + } + } +} + +// keep records a location for a reference, settling ties by the smallest +// pointer so that the answer does not depend on map iteration order. +func (l refLocations) keep(ref string, at pathSegments) { + known, isKnown := l[ref] + if isKnown && known.pointer() <= at.pointer() { + return + } + + l[ref] = at +} diff --git a/ref_locations_test.go b/ref_locations_test.go new file mode 100644 index 0000000..f02e6ba --- /dev/null +++ b/ref_locations_test.go @@ -0,0 +1,173 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "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" +) + +func indexRefs(t *testing.T, doc string) refLocations { + t.Helper() + + var raw any + require.NoError(t, json.Unmarshal([]byte(doc), &raw)) + + return newRefLocations(raw) +} + +func TestRefLocations_FindsDeclarations(t *testing.T) { + t.Parallel() + + locations := indexRefs(t, `{ + "paths": { + "/pets": { + "get": { + "parameters": [ + {"name": "id", "in": "path"}, + {"$ref": "#/parameters/tagsParam"} + ], + "responses": {"200": {"schema": {"$ref": "#/definitions/Pet"}}} + } + } + }, + "definitions": { + "Pet": {"properties": {"owner": {"$ref": "#/definitions/Owner"}}}, + "Remote": {"$ref": "https://elsewhere.example/schema.json"} + } + }`) + + assert.EqualT(t, "/paths/~1pets/get/parameters/1", locations.at("#/parameters/tagsParam").pointer()) + assert.EqualT(t, "/paths/~1pets/get/responses/200/schema", locations.at("#/definitions/Pet").pointer()) + assert.EqualT(t, "/definitions/Pet/properties/owner", locations.at("#/definitions/Owner").pointer()) + assert.EqualT(t, "/definitions/Remote", locations.at("https://elsewhere.example/schema.json").pointer()) +} + +func TestRefLocations_UnknownReferenceIsTheRoot(t *testing.T) { + t.Parallel() + + locations := indexRefs(t, `{"definitions": {"Pet": {"type": "object"}}}`) + + assert.True(t, locations.at("#/definitions/Nope").isEmpty()) + assert.EqualT(t, "", locations.at("#/definitions/Nope").pointer()) +} + +func TestRefLocations_SkipsExampleData(t *testing.T) { + t.Parallel() + + // an example may legitimately hold a "$ref" member: it declares nothing + locations := indexRefs(t, `{ + "definitions": { + "Pet": { + "example": {"$ref": "#/definitions/NotAReference"}, + "examples": {"application/json": {"$ref": "#/definitions/NotAReferenceEither"}} + } + } + }`) + + assert.True(t, locations.at("#/definitions/NotAReference").isEmpty()) + assert.True(t, locations.at("#/definitions/NotAReferenceEither").isEmpty()) +} + +func TestRefLocations_TieBreakIsStable(t *testing.T) { + t.Parallel() + + // the same reference declared twice: whichever is kept, it must be the + // same one on every run, since maps are walked in random order + const doc = `{ + "definitions": { + "Zebra": {"$ref": "#/definitions/Pet"}, + "Ant": {"$ref": "#/definitions/Pet"} + } + }` + + first := indexRefs(t, doc).at("#/definitions/Pet").pointer() + for range 20 { + assert.EqualT(t, first, indexRefs(t, doc).at("#/definitions/Pet").pointer()) + } + assert.EqualT(t, "/definitions/Ant", first) +} + +func TestRefLocations_IgnoresNonStringAndEmptyRefs(t *testing.T) { + t.Parallel() + + locations := indexRefs(t, `{ + "definitions": { + "A": {"$ref": ""}, + "B": {"$ref": {"not": "a reference"}}, + "C": {"properties": {"$ref": {"type": "string"}}} + } + }`) + + assert.Empty(t, locations) +} + +func TestValidateDubiousRefs_LocatesTheReference(t *testing.T) { + t.Parallel() + + doc := `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {}, + "definitions": { + "A": {"$ref": "file:///etc/passwd"} + } + }` + + res := dubiousValidatorFromJSON(t, doc).validateDubiousRefs() + located := res.LocatedWarnings() + require.Len(t, located, 1) + + assert.StringContainsT(t, located[0].Err.Error(), "escapes the spec's base path") + assert.EqualT(t, "/definitions/A", located[0].Pointer) +} + +func TestRefLocations_ReportedThroughSpecValidation(t *testing.T) { + t.Parallel() + + // end to end: refLocations is built by Validate, so an invalid $ref + // reported to a caller of the public API carries where it was declared. + const raw = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {"/pets": {"get": {"operationId": "g", "responses": {"200": {"description": "ok"}}}}}, + "definitions": {"Bad": {"$ref": "file:///etc/passwd"}} + }` + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + res, _ := NewSpecValidator(doc.Schema(), strfmt.Default).Validate(doc) + require.False(t, res.IsValid()) + + located := res.LocatedErrors() + require.Len(t, located, 1) + assert.StringContainsT(t, located[0].Err.Error(), "invalid ref") + assert.EqualT(t, "/definitions/Bad", located[0].Pointer) +} + +func TestValidateDubiousRefs_HostSpreadHasNoSingleLocation(t *testing.T) { + t.Parallel() + + // the warning is about the set of hosts, not about one reference + doc := `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {}, + "definitions": { + "A": {"$ref": "http://host-one.example/a.json"}, + "B": {"$ref": "https://host-two.example/b.json"} + } + }` + + res := dubiousValidatorFromJSON(t, doc).validateDubiousRefs() + located := res.LocatedWarnings() + require.Len(t, located, 1) + assert.Empty(t, located[0].Pointer) +} diff --git a/spec.go b/spec.go index 1171499..ebc48e3 100644 --- a/spec.go +++ b/spec.go @@ -53,6 +53,7 @@ type SpecValidator struct { spec *loads.Document analyzer *analysis.Spec expanded *loads.Document + refLocations refLocations KnownFormats strfmt.Registry Options Opts // validation options schemaOptions *SchemaValidatorOptions @@ -107,6 +108,9 @@ 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) } + // where each $ref sits, as authored: refs are reported against the + // unexpanded document, before expansion flattens them away + s.refLocations = newRefLocations(obj) defer func() { // errs holds all errors and warnings, @@ -800,7 +804,7 @@ func (s *SpecValidator) validateReferencesValid() *Result { res := pools.poolOfResults.BorrowResult() for _, r := range s.analyzer.AllRefs() { if !r.IsValidURI(s.spec.SpecFilePath()) { // Safeguard - spec should always yield a valid URI - res.AddErrors(invalidRefMsg(r.String())) + res.addErrorsAt(s.refLocations.at(r.String()), invalidRefMsg(r.String())) } } if !res.HasErrors() { diff --git a/spec_ref_warnings.go b/spec_ref_warnings.go index 49c7231..a499a64 100644 --- a/spec_ref_warnings.go +++ b/spec_ref_warnings.go @@ -48,7 +48,7 @@ func (s *SpecValidator) validateDubiousRefs() *Result { // Rule 1: absolute local reference escaping the base path. if refPath, isLocalAbs := absoluteLocalRefPath(r, u); isLocalAbs { if !hasBase || !isBeneathBase(refPath, baseDir) { - res.AddWarnings(dubiousAbsoluteRefMsg(r.String())) + res.addWarningsAt(s.refLocations.at(r.String()), dubiousAbsoluteRefMsg(r.String())) } continue } diff --git a/spec_ref_warnings_test.go b/spec_ref_warnings_test.go index f013c49..db91c19 100644 --- a/spec_ref_warnings_test.go +++ b/spec_ref_warnings_test.go @@ -148,6 +148,12 @@ func dubiousValidatorFromJSON(t *testing.T, doc string) *SpecValidator { s := NewSpecValidator(d.Schema(), strfmt.Default) s.spec = d s.analyzer = analysis.New(d.Spec()) + + // as Validate does, so that warnings can say where a $ref sits + var raw any + require.NoError(t, json.Unmarshal(d.Raw(), &raw)) + s.refLocations = newRefLocations(raw) + return s } From db6c34d8e374c08928b75c55141878aee789322e Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sat, 8 Aug 2026 20:01:09 +0200 Subject: [PATCH 4/8] fix: do not read $ref declarations out of default values A default is a value, so a "$ref" member sitting in one declares nothing and must not be taken for the location of a reference, the same way example values are already skipped. The exception is "default" under "responses", which names a response rather than holding a value, and is commonly a $ref to a shared response. That one keeps being indexed, both at the document level and under an operation. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- ref_locations.go | 18 +++++++++++++++++- ref_locations_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/ref_locations.go b/ref_locations.go index 7e7a91a..f1c64a1 100644 --- a/ref_locations.go +++ b/ref_locations.go @@ -44,7 +44,7 @@ func (l refLocations) collect(node any, at pathSegments) { continue } - if key == swaggerExample || key == swaggerExamples { + if isValueMember(key, at) { // plain data: a "$ref" down there declares nothing continue } @@ -58,6 +58,22 @@ func (l refLocations) collect(node any, at pathSegments) { } } +// isValueMember tells if a member holds plain data rather than schemas. +// +// Examples and defaults are values, so a "$ref" member inside them declares +// nothing. The exception is "default" under "responses", which names a +// response rather than holding a value, and may legitimately be a $ref. +func isValueMember(key string, at pathSegments) bool { + switch key { + case swaggerExample, swaggerExamples: + return true + case jsonDefault: + return at.last() != swaggerResponses + default: + return false + } +} + // keep records a location for a reference, settling ties by the smallest // pointer so that the answer does not depend on map iteration order. func (l refLocations) keep(ref string, at pathSegments) { diff --git a/ref_locations_test.go b/ref_locations_test.go index f02e6ba..410d910 100644 --- a/ref_locations_test.go +++ b/ref_locations_test.go @@ -75,6 +75,40 @@ func TestRefLocations_SkipsExampleData(t *testing.T) { assert.True(t, locations.at("#/definitions/NotAReferenceEither").isEmpty()) } +func TestRefLocations_SkipsDefaultValuesButNotDefaultResponses(t *testing.T) { + t.Parallel() + + locations := indexRefs(t, `{ + "responses": { + "default": {"$ref": "#/responses/sharedError"} + }, + "paths": { + "/pets": { + "get": { + "responses": { + "default": {"$ref": "#/responses/operationError"} + } + } + } + }, + "definitions": { + "Pet": { + "default": {"$ref": "#/definitions/NotAReference"} + } + } + }`) + + t.Run("a default response is a declaration", func(t *testing.T) { + assert.EqualT(t, "/responses/default", locations.at("#/responses/sharedError").pointer()) + assert.EqualT(t, "/paths/~1pets/get/responses/default", + locations.at("#/responses/operationError").pointer()) + }) + + t.Run("a default value is not", func(t *testing.T) { + assert.True(t, locations.at("#/definitions/NotAReference").isEmpty()) + }) +} + func TestRefLocations_TieBreakIsStable(t *testing.T) { t.Parallel() From 324f4345c102032addf35796823840f6a8ed62a0 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sat, 8 Aug 2026 20:52:50 +0200 Subject: [PATCH 5/8] refact: take $ref locations from the analyzer analysis v0.26.0 exposes AllRefsByLocation, so the reference index no longer has to be rebuilt here by walking the raw document. Inverting the analyzer's map replaces that walk, halving the file. The guesswork goes with it. Deciding whether a "$ref" member was a declaration or a value meant knowing example and default subtrees hold data, with an exception for a default that names a response; the analyzer indexes declarations only, so none of that has to be restated. Locations and the two diagnostics now read the same index, which makes every reference they can report one the index knows where to find. A $ref sitting directly on a shared parameter or shared response is indexed by neither, so nothing that used to be located has stopped being so. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- go.mod | 4 +- go.sum | 8 +-- ref_locations.go | 103 ++++++++++++-------------------------- ref_locations_test.go | 91 +++++++++++++++++++-------------- spec.go | 6 +-- spec_ref_warnings_test.go | 4 +- 6 files changed, 97 insertions(+), 119 deletions(-) diff --git a/go.mod b/go.mod index 74fe533..230ce38 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,7 @@ module github.com/go-openapi/validate require ( - github.com/go-openapi/analysis v0.25.5 + github.com/go-openapi/analysis v0.26.0 github.com/go-openapi/errors v0.22.8 github.com/go-openapi/jsonpointer v1.0.0 github.com/go-openapi/loads v0.25.0 @@ -18,7 +18,7 @@ require ( require ( github.com/go-openapi/jsonreference v1.0.0 // indirect - github.com/go-openapi/swag/mangling v0.27.3 // indirect + github.com/go-openapi/swag/mangling v0.28.0 // indirect github.com/go-openapi/swag/pools v0.28.0 // indirect github.com/go-openapi/swag/typeutils v0.28.0 // indirect github.com/go-openapi/swag/yamlutils v0.28.0 // indirect diff --git a/go.sum b/go.sum index 1991b83..36049fc 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/go-openapi/analysis v0.25.5 h1:xPYEvTb90o1y0epuiOPAoG4QqahjP3cdp5xNlHeKJRI= -github.com/go-openapi/analysis v0.25.5/go.mod h1:d3UGtQC5uq5Kqqqis2VH09Km/v3vwsWrYkbp4gdm+Rc= +github.com/go-openapi/analysis v0.26.0 h1:1xECln1iMMmQnTjgcknC1vi1hA4KISt6IHpSwnqcuwI= +github.com/go-openapi/analysis v0.26.0/go.mod h1:40gERFi/2dyXA1FaqRRLxkv1IlC6X+GPDNd1xrYAjZE= github.com/go-openapi/errors v0.22.8 h1:oP7sW7TWc3wFFjrzzj0nI83H2qMBkNjNfSd+XRejk/I= github.com/go-openapi/errors v0.22.8/go.mod h1:BuUoHcYrU6E7V9gfj1I5wLQqgtIHnup/alXZ8KdgQ0w= github.com/go-openapi/jsonpointer v1.0.0 h1:kR9tHqY0CtZaOPVFm622dPVNhrvYpwr4uCxgL3h1H8s= @@ -22,8 +22,8 @@ github.com/go-openapi/swag/jsonutils/fixtures_test v0.28.0 h1:qV+VVUAx5Oro8WjVWp github.com/go-openapi/swag/jsonutils/fixtures_test v0.28.0/go.mod h1:mofwUWx70wvskwESqRJ//k/9kURmCgyJl5m5Ppoh5kY= github.com/go-openapi/swag/loading v0.28.0 h1:td8QZdZC9MIYGGSnSPKShKiK22I2tU5UQvuUhIBPRLU= github.com/go-openapi/swag/loading v0.28.0/go.mod h1:rXB0QiQX5mMveXEA7ouM4KiiM9jVJe4K6BVbwhD1M4k= -github.com/go-openapi/swag/mangling v0.27.3 h1:gRzzD1PAUoLTtGMgI3KpBmCSOlTuLTFWnviLxLcTnyg= -github.com/go-openapi/swag/mangling v0.27.3/go.mod h1:jtBE2+V+3pILxOR7Vgce+Cwp6A2PgZbvVqfNntbVs0w= +github.com/go-openapi/swag/mangling v0.28.0 h1:pH8eyeNO9SLYsTMWJrurnNfKmDa28XrlA+HePVD53VM= +github.com/go-openapi/swag/mangling v0.28.0/go.mod h1:jtBE2+V+3pILxOR7Vgce+Cwp6A2PgZbvVqfNntbVs0w= github.com/go-openapi/swag/pools v0.28.0 h1:HPMZWSAfce3rdVTFcjFiCIBtDg9h4x2QlRrHipwhxeU= github.com/go-openapi/swag/pools v0.28.0/go.mod h1:kVQefhSK5RWuRe7BXsL8htgBPAMpN7HDGpGEknqugeE= github.com/go-openapi/swag/stringutils v0.28.0 h1:ixsc9iYgDPubHL/8nSkbnryEHpD2VRlBMLKpQyPXcDU= diff --git a/ref_locations.go b/ref_locations.go index f1c64a1..1853340 100644 --- a/ref_locations.go +++ b/ref_locations.go @@ -3,84 +3,47 @@ package validate -// refLocations indexes the $ref values declared by a raw document, telling -// where each one sits. -// -// The analyzer keeps the same index — its reference map is keyed by document -// location — but hands over only the values, so the locations are recovered -// here. Finding them does not require interpreting a reference: it is enough -// to spot the "$ref" members while walking the document. +import ( + "github.com/go-openapi/analysis" +) + +// refLocations tells where a $ref value is declared in a document. // -// The result is best effort. A reference may be declared in several places, -// and only one location is kept; example values are skipped, but any other -// data that happens to hold a "$ref" string could still be mistaken for a -// declaration. A wrong answer costs a misleading pointer, never a wrong -// verdict, since nothing else is decided from it. +// The analyzer indexes references the other way around, by the location each +// was found at, so the index is inverted here. Only declarations are indexed: +// a "$ref" member sitting in an example or in a default value is data, and the +// analyzer never walks into it. type refLocations map[string]pathSegments -// newRefLocations indexes every reference declared by a raw document. -func newRefLocations(document any) refLocations { - locations := make(refLocations) - locations.collect(document, rootPath()) - - return locations -} - -// at returns where a reference is declared, or the document root when the -// reference was not found. -func (l refLocations) at(ref string) pathSegments { - return l[ref] -} - -func (l refLocations) collect(node any, at pathSegments) { - switch typed := node.(type) { - case map[string]any: - for key, value := range typed { - if key == jsonRef { - if ref, isString := value.(string); isString && ref != "" { - l.keep(ref, at) - } - - continue - } - - if isValueMember(key, at) { - // plain data: a "$ref" down there declares nothing - continue - } - - l.collect(value, at.child(key)) +// newRefLocations inverts the analyzer's reference index. +// +// A reference declared in several places keeps the smallest declaration, so +// that the answer does not depend on map iteration order. +func newRefLocations(analyzer *analysis.Spec) refLocations { + declarations := make(map[string]string) + for location, ref := range analyzer.AllRefsByLocation() { + value := ref.String() + if value == "" { + continue } - case []any: - for i, value := range typed { - l.collect(value, at.item(i)) + + if known, isKnown := declarations[value]; isKnown && known <= location { + continue } - } -} -// isValueMember tells if a member holds plain data rather than schemas. -// -// Examples and defaults are values, so a "$ref" member inside them declares -// nothing. The exception is "default" under "responses", which names a -// response rather than holding a value, and may legitimately be a $ref. -func isValueMember(key string, at pathSegments) bool { - switch key { - case swaggerExample, swaggerExamples: - return true - case jsonDefault: - return at.last() != swaggerResponses - default: - return false + declarations[value] = location } -} -// keep records a location for a reference, settling ties by the smallest -// pointer so that the answer does not depend on map iteration order. -func (l refLocations) keep(ref string, at pathSegments) { - known, isKnown := l[ref] - if isKnown && known.pointer() <= at.pointer() { - return + locations := make(refLocations, len(declarations)) + for value, location := range declarations { + locations[value] = localRefPath(location) } - l[ref] = at + return locations +} + +// at returns where a reference is declared, or the document root when the +// reference is not one the analyzer indexed. +func (l refLocations) at(ref string) pathSegments { + return l[ref] } diff --git a/ref_locations_test.go b/ref_locations_test.go index 410d910..a8d6d5c 100644 --- a/ref_locations_test.go +++ b/ref_locations_test.go @@ -7,25 +7,28 @@ import ( "encoding/json" "testing" + "github.com/go-openapi/analysis" "github.com/go-openapi/loads" "github.com/go-openapi/strfmt" "github.com/go-openapi/testify/v2/assert" "github.com/go-openapi/testify/v2/require" ) -func indexRefs(t *testing.T, doc string) refLocations { +// indexRefs analyzes a document body, wrapped into the smallest valid spec. +func indexRefs(t *testing.T, body string) refLocations { t.Helper() - var raw any - require.NoError(t, json.Unmarshal([]byte(doc), &raw)) + doc := `{"swagger":"2.0","info":{"title":"t","version":"1"},` + body + `}` + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) - return newRefLocations(raw) + return newRefLocations(analysis.New(d.Spec())) } func TestRefLocations_FindsDeclarations(t *testing.T) { t.Parallel() - locations := indexRefs(t, `{ + locations := indexRefs(t, ` "paths": { "/pets": { "get": { @@ -40,8 +43,7 @@ func TestRefLocations_FindsDeclarations(t *testing.T) { "definitions": { "Pet": {"properties": {"owner": {"$ref": "#/definitions/Owner"}}}, "Remote": {"$ref": "https://elsewhere.example/schema.json"} - } - }`) + }`) assert.EqualT(t, "/paths/~1pets/get/parameters/1", locations.at("#/parameters/tagsParam").pointer()) assert.EqualT(t, "/paths/~1pets/get/responses/200/schema", locations.at("#/definitions/Pet").pointer()) @@ -52,7 +54,7 @@ func TestRefLocations_FindsDeclarations(t *testing.T) { func TestRefLocations_UnknownReferenceIsTheRoot(t *testing.T) { t.Parallel() - locations := indexRefs(t, `{"definitions": {"Pet": {"type": "object"}}}`) + locations := indexRefs(t, `"definitions": {"Pet": {"type": "object"}}`) assert.True(t, locations.at("#/definitions/Nope").isEmpty()) assert.EqualT(t, "", locations.at("#/definitions/Nope").pointer()) @@ -62,26 +64,22 @@ func TestRefLocations_SkipsExampleData(t *testing.T) { t.Parallel() // an example may legitimately hold a "$ref" member: it declares nothing - locations := indexRefs(t, `{ + locations := indexRefs(t, ` "definitions": { "Pet": { "example": {"$ref": "#/definitions/NotAReference"}, "examples": {"application/json": {"$ref": "#/definitions/NotAReferenceEither"}} } - } - }`) + }`) assert.True(t, locations.at("#/definitions/NotAReference").isEmpty()) assert.True(t, locations.at("#/definitions/NotAReferenceEither").isEmpty()) } -func TestRefLocations_SkipsDefaultValuesButNotDefaultResponses(t *testing.T) { +func TestRefLocations_DefaultResponseIsADeclarationNotAValue(t *testing.T) { t.Parallel() - locations := indexRefs(t, `{ - "responses": { - "default": {"$ref": "#/responses/sharedError"} - }, + locations := indexRefs(t, ` "paths": { "/pets": { "get": { @@ -95,18 +93,12 @@ func TestRefLocations_SkipsDefaultValuesButNotDefaultResponses(t *testing.T) { "Pet": { "default": {"$ref": "#/definitions/NotAReference"} } - } - }`) - - t.Run("a default response is a declaration", func(t *testing.T) { - assert.EqualT(t, "/responses/default", locations.at("#/responses/sharedError").pointer()) - assert.EqualT(t, "/paths/~1pets/get/responses/default", - locations.at("#/responses/operationError").pointer()) - }) + }`) - t.Run("a default value is not", func(t *testing.T) { - assert.True(t, locations.at("#/definitions/NotAReference").isEmpty()) - }) + assert.EqualT(t, "/paths/~1pets/get/responses/default", + locations.at("#/responses/operationError").pointer()) + assert.True(t, locations.at("#/definitions/NotAReference").isEmpty(), + "a default value holds data: a $ref member there declares nothing") } func TestRefLocations_TieBreakIsStable(t *testing.T) { @@ -114,12 +106,11 @@ func TestRefLocations_TieBreakIsStable(t *testing.T) { // the same reference declared twice: whichever is kept, it must be the // same one on every run, since maps are walked in random order - const doc = `{ + const doc = ` "definitions": { "Zebra": {"$ref": "#/definitions/Pet"}, "Ant": {"$ref": "#/definitions/Pet"} - } - }` + }` first := indexRefs(t, doc).at("#/definitions/Pet").pointer() for range 20 { @@ -128,18 +119,44 @@ func TestRefLocations_TieBreakIsStable(t *testing.T) { assert.EqualT(t, "/definitions/Ant", first) } -func TestRefLocations_IgnoresNonStringAndEmptyRefs(t *testing.T) { +func TestRefLocations_CoverEveryAnalyzedReference(t *testing.T) { t.Parallel() - locations := indexRefs(t, `{ + // the index and the two diagnostics read the same analyzer, so every + // reference they can report is one the index knows where to find + d, err := loads.Analyzed(json.RawMessage(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "parameters": {"tagsParam": {"name": "tags", "in": "query", "type": "string"}}, + "paths": { + "/pets": { + "get": { + "parameters": [{"$ref": "#/parameters/tagsParam"}], + "responses": { + "200": {"description": "ok", "schema": {"$ref": "#/definitions/Pet"}}, + "default": {"$ref": "#/responses/oops"} + } + } + } + }, + "responses": {"oops": {"description": "oops"}}, "definitions": { - "A": {"$ref": ""}, - "B": {"$ref": {"not": "a reference"}}, - "C": {"properties": {"$ref": {"type": "string"}}} + "Pet": {"type": "object", "properties": {"owner": {"$ref": "#/definitions/Owner"}}}, + "Owner": {"type": "object"} } - }`) + }`), "") + require.NoError(t, err) - assert.Empty(t, locations) + analyzer := analysis.New(d.Spec()) + locations := newRefLocations(analyzer) + + refs := analyzer.AllRefs() + require.NotEmpty(t, refs) + for _, found := range refs { + ref := found + assert.False(t, locations.at(ref.String()).isEmpty(), + "expected a location for %q", ref.String()) + } } func TestValidateDubiousRefs_LocatesTheReference(t *testing.T) { diff --git a/spec.go b/spec.go index ebc48e3..e9f51f9 100644 --- a/spec.go +++ b/spec.go @@ -100,6 +100,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { } s.spec = sd s.analyzer = analysis.New(sd.Spec()) + // where each $ref sits, as authored: refs are reported against the + // unexpanded document, before expansion flattens them away + s.refLocations = newRefLocations(s.analyzer) // Raw spec unmarshalling errors var obj any @@ -108,9 +111,6 @@ 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) } - // where each $ref sits, as authored: refs are reported against the - // unexpanded document, before expansion flattens them away - s.refLocations = newRefLocations(obj) defer func() { // errs holds all errors and warnings, diff --git a/spec_ref_warnings_test.go b/spec_ref_warnings_test.go index db91c19..3700430 100644 --- a/spec_ref_warnings_test.go +++ b/spec_ref_warnings_test.go @@ -150,9 +150,7 @@ func dubiousValidatorFromJSON(t *testing.T, doc string) *SpecValidator { s.analyzer = analysis.New(d.Spec()) // as Validate does, so that warnings can say where a $ref sits - var raw any - require.NoError(t, json.Unmarshal(d.Raw(), &raw)) - s.refLocations = newRefLocations(raw) + s.refLocations = newRefLocations(s.analyzer) return s } From 9b2c7461cb94da6f1bda614f62b9f13fa7a5f77c Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 00:07:41 +0200 Subject: [PATCH 6/8] feat: point required-property findings at the offending entry A required property that is never defined, and one marked both required and readOnly, now report the entry of the required array they come from rather than the definition holding it: /definitions/Pet/required/1: "notDeclared" is present in required but not defined as property in definition "Pet" That is the text a reader has to go and amend, so it is what a consumer tracking positions in the document wants to anchor on. The search for the property descends into additionalProperties schemas, so the two locations are carried apart: the schema being searched moves down, while the required entry that started the search stays put. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- helpers.go | 1 + result_location_test.go | 62 +++++++++++++++++++++++++++++++++++++++++ spec.go | 25 ++++++++++++----- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/helpers.go b/helpers.go index b815443..07c19e8 100644 --- a/helpers.go +++ b/helpers.go @@ -38,6 +38,7 @@ const ( jsonItems = "items" jsonType = "type" jsonSchema = "schema" + jsonRequired = "required" jsonRef = "$ref" jsonDefault = "default" diff --git a/result_location_test.go b/result_location_test.go index 0c4e50a..22d258e 100644 --- a/result_location_test.go +++ b/result_location_test.go @@ -5,6 +5,7 @@ package validate import ( "encoding/json" + "strings" "testing" "github.com/go-openapi/loads" @@ -226,6 +227,67 @@ func TestResultLocations_SpecValidation(t *testing.T) { }) } +func TestResultLocations_RequiredEntryIsLocated(t *testing.T) { + t.Parallel() + + // the TUI anchors on where the offending text sits, so a required entry + // that names no property points at the entry, not at the definition + const raw = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {"/pets": {"get": {"operationId": "g", "responses": {"200": {"description": "ok"}}}}}, + "definitions": { + "Pet": { + "type": "object", + "required": ["name", "notDeclared"], + "properties": { + "name": {"type": "string"}, + "readOnlyOne": {"type": "string", "readOnly": true} + } + }, + "Owner": { + "type": "object", + "required": ["readOnlyOne"], + "properties": {"readOnlyOne": {"type": "string", "readOnly": true}} + } + } + }` + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.SetContinueOnErrors(true) + res, warns := validator.Validate(doc) + + t.Run("an undefined required property points at its entry", func(t *testing.T) { + var found bool + for _, located := range res.LocatedErrors() { + if !strings.Contains(located.Err.Error(), "notDeclared") { + continue + } + + found = true + // index 1 of Pet's required array, not "/definitions/Pet" + assert.EqualT(t, "/definitions/Pet/required/1", located.Pointer) + } + require.True(t, found, "expected the undefined required property to be reported") + }) + + t.Run("a required and readOnly property points at its entry", func(t *testing.T) { + var found bool + for _, located := range warns.LocatedErrors() { + if !strings.Contains(located.Err.Error(), "readOnly") { + continue + } + + found = true + assert.EqualT(t, "/definitions/Owner/required/0", located.Pointer) + } + require.True(t, found, "expected the readOnly-and-required warning to be reported") + }) +} + func pointersOf(located []Located) []string { pointers := make([]string, 0, len(located)) for _, l := range located { diff --git a/spec.go b/spec.go index e9f51f9..d2d6ac7 100644 --- a/spec.go +++ b/spec.go @@ -574,8 +574,12 @@ func (s *SpecValidator) validateRequiredDefinitions() *Result { DEFINITIONS: for d, schema := range s.spec.Spec().Definitions { if schema.Required != nil { // Safeguard - for _, pn := range schema.Required { - red := s.validateRequiredProperties(pn, d, newPathSegments(swaggerDefinitions, d), &schema) //#nosec + 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. @@ -590,7 +594,14 @@ DEFINITIONS: return res } -func (s *SpecValidator) validateRequiredProperties(path, in string, at pathSegments, v *spec.Schema) *Result { +// validateRequiredProperties checks one entry of a definition's 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, +) *Result { // Takes care of recursive property definitions, which may be nested in additionalProperties schemas res := pools.poolOfResults.BorrowResult() propertyMatch := false @@ -609,7 +620,7 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, at pathSegme for pp, pv := range v.PatternProperties { re, err := compileRegexp(pp) if err != nil { - res.addErrorsAt(at, invalidPatternMsg(pp, in)) + res.addErrorsAt(schemaAt, invalidPatternMsg(pp, in)) } else if re.MatchString(path) { patternMatch = true if !propertyMatch { @@ -626,7 +637,7 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, at pathSegme // 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, at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema) + red := s.validateRequiredProperties(path, in, schemaAt.child(jsonAdditionalProperties), requiredAt, v.AdditionalProperties.Schema) if red.IsValid() { additionalPropertiesMatch = true if !propertyMatch && !patternMatch { @@ -639,11 +650,11 @@ func (s *SpecValidator) validateRequiredProperties(path, in string, at pathSegme } if !propertyMatch && !patternMatch && !additionalPropertiesMatch { - res.addErrorsAt(at, requiredButNotDefinedMsg(path, in)) + res.addErrorsAt(requiredAt, requiredButNotDefinedMsg(path, in)) } if isReadOnly { - res.addWarningsAt(at, readOnlyAndRequiredMsg(in, path)) + res.addWarningsAt(requiredAt, readOnlyAndRequiredMsg(in, path)) } return res } From 8512f1a7b9caa472272e8f8e822e9fa15d3b3621 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 01:40:27 +0200 Subject: [PATCH 7/8] fix: locate parameters by the index the document addresses them at A parameter was reported at its name, which no document can address: they are held in an array. Over the fixture specs, that left most pointers unresolvable and made a consumer walk up to the array before finding anything. Parameters are now indexed from the unexpanded document, which is the only place the index survives: expansion merges the parameters an operation declares with those of its path item, and resolves the ones written as a $ref. The lookup falls back to the path item, so a parameter declared once for every operation under a path is found from any of them. Messages keep naming the parameter. A path token now carries a readable form beside the addressed one, so the pointer indexes while the message reads as it did: /paths/~1pets/get/parameters/1 paths./pets.get.parameters.tags A parameter too broken to be identified, one with no name, has no index to point at; the name stands in, as before. One message moves, and it is a correction: a parameter declared on a path item is no longer reported under an operation that did not declare it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 12 +-- example_validator.go | 10 +-- helpers.go | 14 ++-- param_locations.go | 122 ++++++++++++++++++++++++++++ param_locations_test.go | 175 ++++++++++++++++++++++++++++++++++++++++ path.go | 60 +++++++++++--- path_test.go | 28 +++++++ spec.go | 42 +++++----- spec_test.go | 3 +- 9 files changed, 417 insertions(+), 49 deletions(-) create mode 100644 param_locations.go create mode 100644 param_locations_test.go diff --git a/default_validator.go b/default_validator.go index a570d5b..6db0b89 100644 --- a/default_validator.go +++ b/default_validator.go @@ -81,7 +81,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.Default != nil && param.Required { - res.addWarningsAt(parameterPath(path, method, param.Name), requiredHasDefaultMsg(param.Name, param.In)) + res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), requiredHasDefaultMsg(param.Name, param.In)) } // reset explored schemas to get depth-first recursive-proof exploration @@ -93,7 +93,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, d.schemaOptions).Validate(param.Default) //#nosec if red.HasErrorsOrWarnings() { - res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -102,9 +102,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := d.validateDefaultValueItemsAgainstSchema(parameterPath(path, method, param.Name), param.In, ¶m, param.Items) //#nosec + red := d.validateDefaultValueItemsAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { - res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueItemsDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -113,9 +113,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate default value against schema - red := d.validateDefaultValueSchemaAgainstSchema(parameterPath(path, method, param.Name), param.In, param.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { - res.addErrorsAt(parameterPath(path, method, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) + res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) diff --git a/example_validator.go b/example_validator.go index f6a1d5d..a0670ca 100644 --- a/example_validator.go +++ b/example_validator.go @@ -83,7 +83,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, ex.schemaOptions).Validate(param.Example) //#nosec if red.HasErrorsOrWarnings() { - res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.MergeAsWarnings(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -92,9 +92,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { // Recursively follows Items and Schemas if param.Items != nil { - red := ex.validateExampleValueItemsAgainstSchema(parameterPath(path, method, param.Name), param.In, ¶m, param.Items) //#nosec + red := ex.validateExampleValueItemsAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, ¶m, param.Items) //#nosec if red.HasErrorsOrWarnings() { - res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueItemsDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueItemsDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) @@ -103,9 +103,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate example value against schema - red := ex.validateExampleValueSchemaAgainstSchema(parameterPath(path, method, param.Name), param.In, param.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) if red.HasErrorsOrWarnings() { - res.addWarningsAt(parameterPath(path, method, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) + res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) } else if red.wantsRedeemOnMerge { pools.poolOfResults.RedeemResult(red) diff --git a/helpers.go b/helpers.go index 07c19e8..1e8b7b3 100644 --- a/helpers.go +++ b/helpers.go @@ -58,13 +58,9 @@ func operationPath(path, method string) pathSegments { return newPathSegments(swaggerPaths, path, methodToken(method)) } -// parameterPath locates a parameter of an operation. -// -// Parameters are held in an array, so a name is not how the document addresses -// them. It is used all the same: it is what a reader recognizes, and resolving -// the index would mean carrying it down every recursion. -func parameterPath(path, method, name string) pathSegments { - return operationPath(path, method).children(swaggerParameters, name) +// parameterPath locates a parameter of an operation in the spec document. +func (s *SpecValidator) parameterPath(path, method, in, name string) pathSegments { + return s.paramLocations.at(path, method, in, name) } // responsePath locates a response of an operation in the spec document. @@ -321,10 +317,10 @@ func (h *paramHelper) resolveParam(path, method, operationID string, param *spec if err != nil { // Safeguard // NOTE: we may enter here when the whole parameter is an unresolved $ref refPath := strings.Join([]string{"\"" + path + "\"", method}, ".") - errorHelp.addPointerErrorAt(res, parameterPath(path, method, param.Name), err, param.Ref.String(), refPath) + errorHelp.addPointerErrorAt(res, s.parameterPath(path, method, param.In, param.Name), err, param.Ref.String(), refPath) return nil, res } - res.Merge(h.checkExpandedParam(param, param.Name, param.In, operationID, parameterPath(path, method, param.Name), isRef)) + res.Merge(h.checkExpandedParam(param, param.Name, param.In, operationID, s.parameterPath(path, method, param.In, param.Name), isRef)) return param, res } diff --git a/param_locations.go b/param_locations.go new file mode 100644 index 0000000..e8b4b22 --- /dev/null +++ b/param_locations.go @@ -0,0 +1,122 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strconv" + + "github.com/go-openapi/spec" +) + +// paramLocations tells where an operation declares a parameter. +// +// Parameters are held in an array, so a name is not how a document addresses +// one: only its index is. The index is not something a validator can work out +// from an expanded parameter either, because expansion merges the parameters +// an operation declares with those its path item declares, and resolves the +// ones written as a $ref. So the unexpanded document is indexed once, and +// looked up by what a validator does know: the operation, and the name and +// location of the parameter it is reporting on. +type paramLocations map[paramKey]pathSegments + +// paramKey identifies a parameter the way the swagger specification does: +// a name is unique only within an "in". +type paramKey struct { + path string + method string + in string + name string +} + +// newParamLocations indexes the parameters declared by an unexpanded document. +func newParamLocations(sp *spec.Swagger) paramLocations { + locations := make(paramLocations) + if sp == nil || sp.Paths == nil { + return locations + } + + for path, pathItem := range sp.Paths.Paths { + at := newPathSegments(swaggerPaths, path) + + // parameters declared by the path item are shared by all its + // operations: recorded once, without a method + locations.collect(sp, paramKey{path: path}, at, pathItem.Parameters) + + for method, op := range operationsOf(&pathItem) { //#nosec + if op == nil { + continue + } + + locations.collect(sp, paramKey{path: path, method: method}, at.child(method), op.Parameters) + } + } + + return locations +} + +// at returns where an operation declares a parameter. +// +// 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. +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 + } + + if found, isDeclared := l[paramKey{path: path, in: in, name: name}]; isDeclared { + return found + } + + return operationPath(path, method).children(swaggerParameters, name) +} + +func (l paramLocations) collect(sp *spec.Swagger, key paramKey, at pathSegments, params []spec.Parameter) { + for i := range params { + name, in, ok := parameterIdentity(sp, ¶ms[i]) + if !ok { + continue + } + + key.name = name + key.in = in + l[key] = at.child(swaggerParameters).childAs(strconv.Itoa(i), name) + } +} + +// parameterIdentity names a parameter as declared, resolving the one indirection +// a document may put in the way: an entry written as a local $ref. +func parameterIdentity(sp *spec.Swagger, param *spec.Parameter) (name, in string, ok bool) { + if param.Ref.String() == "" { + return param.Name, param.In, param.Name != "" + } + + shared := localRefPath(param.Ref.String()) + const sharedParameterDepth = 2 + if len(shared) != sharedParameterDepth || shared.beforeLast() != swaggerParameters { + return "", "", false + } + + declared, isDeclared := sp.Parameters[shared.last()] + if !isDeclared { + return "", "", false + } + + return declared.Name, declared.In, declared.Name != "" +} + +// operationsOf yields the operations of a path item, keyed as the document +// spells them. +func operationsOf(pathItem *spec.PathItem) map[string]*spec.Operation { + return map[string]*spec.Operation{ + "get": pathItem.Get, + "put": pathItem.Put, + "post": pathItem.Post, + "delete": pathItem.Delete, + "options": pathItem.Options, + "head": pathItem.Head, + "patch": pathItem.Patch, + } +} diff --git a/param_locations_test.go b/param_locations_test.go new file mode 100644 index 0000000..2f7971a --- /dev/null +++ b/param_locations_test.go @@ -0,0 +1,175 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "strings" + "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" +) + +const paramLocationsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "parameters": { + "sharedBody": {"name": "user", "in": "body", "schema": {"type": "object"}} + }, + "paths": { + "/pets": { + "parameters": [ + {"name": "onPathItem", "in": "query", "type": "string"} + ], + "get": { + "operationId": "list", + "parameters": [ + {"name": "first", "in": "query", "type": "string"}, + {"name": "second", "in": "header", "type": "string"}, + {"$ref": "#/parameters/sharedBody"} + ], + "responses": {"200": {"description": "ok"}} + }, + "post": { + "operationId": "create", + "parameters": [{"name": "only", "in": "query", "type": "string"}], + "responses": {"200": {"description": "ok"}} + } + } + } +}` + +const ( + methodGet = "GET" + methodPost = "POST" + inQuery = "query" + inHeader = "header" +) + +func paramLocationsOf(t *testing.T) paramLocations { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(paramLocationsFixture), "") + require.NoError(t, err) + + return newParamLocations(d.Spec()) +} + +func TestParamLocations_AddressedByIndex(t *testing.T) { + t.Parallel() + + locations := paramLocationsOf(t) + + for _, tt := range []struct { + name string + in string + method string + pointer string + }{ + {name: "first", in: inQuery, method: methodGet, pointer: "/paths/~1pets/get/parameters/0"}, + {name: "second", in: inHeader, method: methodGet, pointer: "/paths/~1pets/get/parameters/1"}, + {name: "only", in: inQuery, method: methodPost, pointer: "/paths/~1pets/post/parameters/0"}, + } { + assert.EqualT(t, tt.pointer, locations.at("/pets", tt.method, tt.in, tt.name).pointer(), + "unexpected location for %q", tt.name) + } +} + +func TestParamLocations_FallsBackToThePathItem(t *testing.T) { + t.Parallel() + + locations := paramLocationsOf(t) + + // declared once by the path item, shared by every operation under it + for _, method := range []string{methodGet, methodPost} { + assert.EqualT(t, "/paths/~1pets/parameters/0", + locations.at("/pets", method, inQuery, "onPathItem").pointer(), + "unexpected location under %s", method) + } +} + +func TestParamLocations_ResolvesASharedParameter(t *testing.T) { + t.Parallel() + + locations := paramLocationsOf(t) + + // the entry is written as a $ref, so its name comes from the definition, + // while the location stays the site the operation declares it at + assert.EqualT(t, "/paths/~1pets/get/parameters/2", + locations.at("/pets", methodGet, "body", "user").pointer()) +} + +func TestParamLocations_UnknownParameterKeepsItsName(t *testing.T) { + t.Parallel() + + 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 + unknown := locations.at("/pets", methodGet, inQuery, "neverDeclared") + assert.EqualT(t, "/paths/~1pets/get/parameters/neverDeclared", unknown.pointer()) + assert.EqualT(t, "paths./pets.get.parameters.neverDeclared", unknown.dotted()) +} + +func TestParamLocations_PointerIndexesButMessageNames(t *testing.T) { + t.Parallel() + + locations := paramLocationsOf(t) + located := locations.at("/pets", methodGet, inHeader, "second") + + assert.EqualT(t, "/paths/~1pets/get/parameters/1", located.pointer(), + "the document addresses a parameter by index") + assert.EqualT(t, "paths./pets.get.parameters.second", located.dotted(), + "a message is more useful naming it") +} + +func TestParamLocations_ReportedThroughSpecValidation(t *testing.T) { + t.Parallel() + + // a query parameter typed as an array without items: reported against the + // entry the operation declares, which is what a document can address + const raw = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/pets": { + "get": { + "operationId": "list", + "parameters": [ + {"name": "ok", "in": "query", "type": "string"}, + {"name": "tags", "in": "query", "type": "array"} + ], + "responses": {"200": {"description": "ok"}} + } + } + } + }` + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.SetContinueOnErrors(true) + res, _ := validator.Validate(doc) + + const declaredAt = "/paths/~1pets/get/parameters/1" + + var found bool + for _, located := range res.LocatedErrors() { + if !strings.Contains(located.Err.Error(), "tags") && !strings.HasPrefix(located.Pointer, declaredAt) { + continue + } + + found = true + // the check may report the parameter itself or something inside it, + // but never the operation or the array holding it + assert.StringContainsT(t, located.Pointer, declaredAt, + "expected %q to be located in the offending parameter", located.Err) + } + require.True(t, found, "expected the array parameter without items to be reported") +} diff --git a/path.go b/path.go index f37c846..1aee44f 100644 --- a/path.go +++ b/path.go @@ -22,7 +22,27 @@ import ( // The zero value is the location of the document root. // // [RFC 6901]: https://datatracker.ietf.org/doc/html/rfc6901 -type pathSegments []string +type pathSegments []pathToken + +// pathToken is one step of a location. +// +// A token addresses a member the way the document does, which is not always +// the way a reader recognizes it: an operation addresses its parameters by +// index, while a message is far more useful naming them. When the two differ, +// display carries the readable form and only the message uses it. +type pathToken struct { + token string + display string +} + +// readable renders a token the way a message should spell it. +func (t pathToken) readable() string { + if t.display != "" { + return t.display + } + + return t.token +} // newPathSegments builds a location from a list of unescaped tokens. func newPathSegments(tokens ...string) pathSegments { @@ -30,7 +50,12 @@ func newPathSegments(tokens ...string) pathSegments { return nil } - return pathSegments(tokens) + segments := make(pathSegments, len(tokens)) + for i, token := range tokens { + segments[i] = pathToken{token: token} + } + + return segments } // rootPath is the location of the document root. @@ -45,6 +70,16 @@ func (p pathSegments) String() string { return p.dotted() } // The receiver is never modified: sibling children may be derived from the // same parent without aliasing one another. func (p pathSegments) child(token string) pathSegments { + return p.appendToken(pathToken{token: token}) +} + +// childAs returns the location of a member the document addresses as token, +// which messages should spell as display instead. +func (p pathSegments) childAs(token, display string) pathSegments { + return p.appendToken(pathToken{token: token, display: display}) +} + +func (p pathSegments) appendToken(token pathToken) pathSegments { child := make(pathSegments, len(p)+1) copy(child, p) child[len(p)] = token @@ -56,7 +91,9 @@ func (p pathSegments) child(token string) pathSegments { func (p pathSegments) children(tokens ...string) pathSegments { child := make(pathSegments, len(p)+len(tokens)) copy(child, p) - copy(child[len(p):], tokens) + for i, token := range tokens { + child[len(p)+i] = pathToken{token: token} + } return child } @@ -75,7 +112,7 @@ func (p pathSegments) last() string { return "" } - return p[len(p)-1] + return p[len(p)-1].token } // beforeLast returns the token before the trailing one, or an empty string @@ -86,7 +123,7 @@ func (p pathSegments) beforeLast() string { return "" } - return p[len(p)-beforeLast] + return p[len(p)-beforeLast].token } // trimIndexes returns p without its trailing array index tokens. @@ -95,7 +132,7 @@ func (p pathSegments) beforeLast() string { // array it sits: the items of an example are still an example. func (p pathSegments) trimIndexes() pathSegments { end := len(p) - for end > 0 && isIndexToken(p[end-1]) { + for end > 0 && isIndexToken(p[end-1].token) { end-- } @@ -125,7 +162,7 @@ func (p pathSegments) hasSuffix(suffix pathSegments) bool { offset := len(p) - len(suffix) for i, token := range suffix { - if p[offset+i] != token { + if p[offset+i].token != token.token { return false } } @@ -141,7 +178,12 @@ func (p pathSegments) hasSuffix(suffix pathSegments) bool { // name of a validation error, and API consumers of go-swagger servers see it. // Use [pathSegments.pointer] whenever the location needs to be unambiguous. func (p pathSegments) dotted() string { - return strings.Join(p, ".") + readable := make([]string, len(p)) + for i, token := range p { + readable[i] = token.readable() + } + + return strings.Join(readable, ".") } // pointer renders the location as an RFC 6901 JSON pointer, e.g. @@ -154,7 +196,7 @@ func (p pathSegments) pointer() string { var w strings.Builder for _, token := range p { w.WriteByte('/') - w.WriteString(jsonpointer.Escape(token)) + w.WriteString(jsonpointer.Escape(token.token)) } return w.String() diff --git a/path_test.go b/path_test.go index 6c2bd81..9217b1c 100644 --- a/path_test.go +++ b/path_test.go @@ -80,6 +80,34 @@ func TestPathSegmentsRendering(t *testing.T) { } } +func TestPathSegmentsDisplayDiffersFromPointer(t *testing.T) { + t.Parallel() + + // a document addresses an array member by index, while a message reads + // far better naming it + path := newPathSegments("paths", "/pets", "get", "parameters").childAs("1", "tags") + + assert.EqualT(t, "/paths/~1pets/get/parameters/1", path.pointer()) + assert.EqualT(t, "paths./pets.get.parameters.tags", path.dotted()) + + t.Run("structure reads the addressed token, not the readable one", func(t *testing.T) { + t.Parallel() + + assert.EqualT(t, "1", path.last(), "expected the token the document uses") + assert.True(t, path.hasSuffix(newPathSegments("parameters", "1"))) + assert.False(t, path.hasSuffix(newPathSegments("parameters", "tags"))) + assert.EqualT(t, "paths./pets.get.parameters", path.trimIndexes().dotted()) + }) + + t.Run("children of a renamed token keep both forms", func(t *testing.T) { + t.Parallel() + + child := path.child("items") + assert.EqualT(t, "/paths/~1pets/get/parameters/1/items", child.pointer()) + assert.EqualT(t, "paths./pets.get.parameters.tags.items", child.dotted()) + }) +} + func TestPathSegmentsAppendIsCopyOnWrite(t *testing.T) { t.Parallel() diff --git a/spec.go b/spec.go index d2d6ac7..5568d38 100644 --- a/spec.go +++ b/spec.go @@ -49,14 +49,15 @@ func Spec(doc *loads.Document, formats strfmt.Registry, options ...Option) error // SpecValidator validates a swagger 2.0 spec. type SpecValidator struct { - schema *spec.Schema // swagger 2.0 schema - spec *loads.Document - analyzer *analysis.Spec - expanded *loads.Document - refLocations refLocations - KnownFormats strfmt.Registry - Options Opts // validation options - schemaOptions *SchemaValidatorOptions + schema *spec.Schema // swagger 2.0 schema + spec *loads.Document + analyzer *analysis.Spec + expanded *loads.Document + refLocations refLocations + paramLocations paramLocations + KnownFormats strfmt.Registry + Options Opts // validation options + schemaOptions *SchemaValidatorOptions } // NewSpecValidator creates a new swagger spec validator instance. @@ -103,6 +104,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // where each $ref sits, as authored: refs are reported against the // unexpanded document, before expansion flattens them away s.refLocations = newRefLocations(s.analyzer) + // where each operation declares its parameters: the document addresses + // them by index, and expansion loses that + s.paramLocations = newParamLocations(sd.Spec()) // Raw spec unmarshalling errors var obj any @@ -374,7 +378,7 @@ func (s *SpecValidator) validateItems() *Result { for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.TypeName() == arrayType && param.ItemsTypeName() == "" { - res.addErrorsAt(parameterPath(path, method, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) + res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) continue } if param.In != swaggerBody { @@ -382,7 +386,7 @@ func (s *SpecValidator) validateItems() *Result { items := param.Items for items.TypeName() == arrayType { if items.ItemsTypeName() == "" { - res.addErrorsAt(parameterPath(path, method, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) + res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), arrayInParamRequiresItemsMsg(param.Name, op.ID)) break } items = items.Items @@ -391,7 +395,7 @@ func (s *SpecValidator) validateItems() *Result { } else { // In: body if param.Schema != nil { - res.Merge(s.validateSchemaItems(*param.Schema, parameterPath(path, method, param.Name).child(jsonSchema), + res.Merge(s.validateSchemaItems(*param.Schema, s.parameterPath(path, method, param.In, param.Name).child(jsonSchema), fmt.Sprintf("body param %q", param.Name), op.ID)) } } @@ -721,10 +725,10 @@ func (s *SpecValidator) validateParameters() *Result { for _, pr := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { // An expanded parameter must validate the Parameter schema (an unexpanded $ref always passes high-level schema validation) - schv := newSchemaValidator(¶mSchema, s.schema, newPathSegments(swaggerPaths, path, methodToken(method), swaggerParameters, pr.Name), s.KnownFormats, s.schemaOptions) + schv := newSchemaValidator(¶mSchema, s.schema, s.parameterPath(path, method, pr.In, pr.Name), s.KnownFormats, s.schemaOptions) var obj any if err := jsonutils.FromDynamicJSON(pr, &obj); err != nil { - res.addErrorsAt(parameterPath(path, method, pr.Name), err) + res.addErrorsAt(s.parameterPath(path, method, pr.In, pr.Name), err) return res } @@ -733,7 +737,7 @@ func (s *SpecValidator) validateParameters() *Result { // Validate pattern regexp for parameters with a Pattern property if _, err := compileRegexp(pr.Pattern); err != nil { - res.addErrorsAt(parameterPath(path, method, pr.Name), invalidPatternInParamMsg(op.ID, pr.Name, pr.Pattern)) + res.addErrorsAt(s.parameterPath(path, method, pr.In, pr.Name), invalidPatternInParamMsg(op.ID, pr.Name, pr.Pattern)) } // There must be at most one parameter in body: list them all @@ -746,7 +750,7 @@ func (s *SpecValidator) validateParameters() *Result { paramNames = append(paramNames, pr.Name) // Path declared in path must have the required: true property if !pr.Required { - res.addErrorsAt(parameterPath(path, method, pr.Name), pathParamRequiredMsg(op.ID, pr.Name)) + res.addErrorsAt(s.parameterPath(path, method, pr.In, pr.Name), pathParamRequiredMsg(op.ID, pr.Name)) } } @@ -757,19 +761,19 @@ func (s *SpecValidator) validateParameters() *Result { if pr.Type != numberType && pr.Type != integerType && (pr.Maximum != nil || pr.Minimum != nil || pr.MultipleOf != nil) { // A non-numeric parameter has validation keywords for numeric instances (number and integer) - res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(s.parameterPath(path, method, pr.In, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } if pr.Type != stringType && // A non-string parameter has validation keywords for strings (pr.MaxLength != nil || pr.MinLength != nil || pr.Pattern != "") { - res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(s.parameterPath(path, method, pr.In, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } if pr.Type != arrayType && // A non-array parameter has validation keywords for arrays (pr.MaxItems != nil || pr.MinItems != nil || pr.UniqueItems) { - res.addWarningsAt(parameterPath(path, method, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) + res.addWarningsAt(s.parameterPath(path, method, pr.In, pr.Name), parameterValidationTypeMismatchMsg(pr.Name, path, pr.Type)) } } @@ -854,7 +858,7 @@ func (s *SpecValidator) checkUniqueParams(path, method string, op *spec.Operatio key := fmt.Sprintf("%s#%s", pr.In, pr.Name) if _, ok = pnames[key]; ok { - res.addErrorsAt(parameterPath(path, method, pr.Name), duplicateParamNameMsg(pr.In, pr.Name, op.ID)) + res.addErrorsAt(s.parameterPath(path, method, pr.In, pr.Name), duplicateParamNameMsg(pr.In, pr.Name, op.ID)) } pnames[key] = struct{}{} } diff --git a/spec_test.go b/spec_test.go index 4f382f9..0b8f6eb 100644 --- a/spec_test.go +++ b/spec_test.go @@ -229,7 +229,7 @@ func TestSpec_Issue18(t *testing.T) { assert.SliceContainsT(t, verifiedErrors, "body param \"user\" for \"\" has invalid items pattern: \")<-- bad pattern\"") assert.SliceContainsT(t, verifiedErrors, "default value for user in body does not validate its schema") assert.SliceContainsT(t, verifiedErrors, - "paths./foo.get.parameters.user.items in body has invalid pattern: \")<-- bad pattern\"") + "paths./foo.parameters.user.items in body has invalid pattern: \")<-- bad pattern\"") case strings.Contains(path, "parameters.json"): assert.SliceContainsT(t, verifiedErrors, "operation \"\" has invalid pattern in param \"userId\": \")<-- bad pattern\"") case strings.Contains(path, "schema.json"): @@ -405,6 +405,7 @@ func TestSpec_ValidateParameters(t *testing.T) { validator := NewSpecValidator(spec.MustLoadSwagger20Schema(), strfmt.Default) validator.spec = doc validator.analyzer = analysis.New(doc.Spec()) + validator.paramLocations = newParamLocations(doc.Spec()) return validator } From db684105efecf5aca7b7daac5e4935217cb531c1 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 9 Aug 2026 02:07:25 +0200 Subject: [PATCH 8/8] fix: point at nodes a document actually contains Two classes of pointer addressed nothing, so a consumer had to walk up before finding anything to show. A finding about something absent is now reported on the value that should hold it: a missing required property points at the object, not at the property that is not there. At the root that is the empty pointer, which addresses the whole document, and Located.Pointer says so rather than calling it unknown. Tokens a document needs to address a value, but that a message has never spelled, are marked structural: part of the pointer, absent from the dotted form. That puts "properties" between a schema and its members, "schema" under a response, and the media type under its examples: /definitions/Pet/properties/name/default definitions.Pet.name.default The predicates telling a schema apart from plain data skip structural tokens. They ask what a value is, and plumbing must not answer: an example addressed through its media type is still an example. Not covered, and left as it stands: a parameter or response written as a $ref is located where it is declared, so anything below that site resolves in the shared definition rather than there. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- default_validator.go | 7 +++-- example_validator.go | 15 ++++++---- helpers.go | 17 ++++++----- object_validator.go | 8 +++-- path.go | 65 ++++++++++++++++++++++++++++++++--------- path_test.go | 31 ++++++++++++++++++++ result.go | 12 ++++++-- result_location_test.go | 39 +++++++++++++++++++++++-- 8 files changed, 158 insertions(+), 36 deletions(-) diff --git a/default_validator.go b/default_validator.go index 6db0b89..d6da321 100644 --- a/default_validator.go +++ b/default_validator.go @@ -199,7 +199,8 @@ func (d *defaultValidator) validateDefaultInResponse( // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() - red := d.validateDefaultValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) + red := d.validateDefaultValueSchemaAgainstSchema( + responsePath(path, method, responseCodeAsStr).structuralChild(jsonSchema), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost res.addErrorsAt(responsePath(path, method, responseCodeAsStr), defaultValueInDoesNotValidateMsg(operationID, responseName)) @@ -244,10 +245,10 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegm res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } for propName, prop := range schema.Properties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec } for propName, prop := range schema.PatternProperties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) diff --git a/example_validator.go b/example_validator.go index a0670ca..07272a7 100644 --- a/example_validator.go +++ b/example_validator.go @@ -189,7 +189,8 @@ func (ex *exampleValidator) validateExampleInResponse( // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() - red := ex.validateExampleValueSchemaAgainstSchema(responsePath(path, method, responseCodeAsStr), "response", response.Schema) + red := ex.validateExampleValueSchemaAgainstSchema( + responsePath(path, method, responseCodeAsStr).structuralChild(jsonSchema), "response", response.Schema) if red.HasErrorsOrWarnings() { // Additional message to make sure the context of the error is not lost res.addWarningsAt(responsePath(path, method, responseCodeAsStr), exampleValueInDoesNotValidateMsg(operationID, responseName)) @@ -201,9 +202,13 @@ func (ex *exampleValidator) validateExampleInResponse( if response.Examples != nil { if response.Schema != nil { - if example, ok := response.Examples["application/json"]; ok { + if example, ok := response.Examples[jsonMimeApplicationJSON]; ok { + exampleAt := responsePath(path, method, responseCodeAsStr). + child(swaggerExamples). + structuralChild(jsonMimeApplicationJSON) res.MergeAsWarnings( - newSchemaValidator(response.Schema, s.spec.Spec(), responsePath(path, method, responseCodeAsStr).child(swaggerExamples), s.KnownFormats, s.schemaOptions).Validate(example), + newSchemaValidator(response.Schema, s.spec.Spec(), + exampleAt, s.KnownFormats, s.schemaOptions).Validate(example), ) } else { // Proposal for enhancement: validate other media types too @@ -249,10 +254,10 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSeg res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } for propName, prop := range schema.Properties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec } for propName, prop := range schema.PatternProperties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(propName), in, &prop)) //#nosec + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec } 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 1e8b7b3..027f134 100644 --- a/helpers.go +++ b/helpers.go @@ -34,13 +34,14 @@ const ( ) const ( - jsonProperties = "properties" - jsonItems = "items" - jsonType = "type" - jsonSchema = "schema" - jsonRequired = "required" - jsonRef = "$ref" - jsonDefault = "default" + jsonProperties = "properties" + jsonPatternProperties = "patternProperties" + jsonItems = "items" + jsonType = "type" + jsonSchema = "schema" + jsonRequired = "required" + jsonRef = "$ref" + jsonDefault = "default" jsonAllOf = "allOf" jsonAdditionalItems = "additionalItems" @@ -51,6 +52,8 @@ const ( swaggerResponses = "responses" swaggerParameters = "parameters" swaggerHeaders = "headers" + + jsonMimeApplicationJSON = "application/json" ) // operationPath locates an operation in the spec document. diff --git a/object_validator.go b/object_validator.go index d67e44b..2cc8dce 100644 --- a/object_validator.go +++ b/object_validator.go @@ -176,7 +176,7 @@ func (o *objectValidator) checkArrayMustHaveItems(res *Result, val map[string]an return } - res.addErrorsAt(o.Path.child(jsonItems), errors.Required(jsonItems, o.Path.dotted(), item)) + res.addErrorsAt(o.Path, errors.Required(jsonItems, o.Path.dotted(), item)) } func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string]any) { @@ -196,7 +196,7 @@ func (o *objectValidator) checkItemsMustBeTypeArray(res *Result, val map[string] t, typeFound := val[jsonType] if !typeFound { // there is no type - res.addErrorsAt(o.Path.child(jsonType), errors.Required(jsonType, o.Path.dotted(), t)) + res.addErrorsAt(o.Path, errors.Required(jsonType, o.Path.dotted(), t)) } if tpe, isString := t.(string); !isString || tpe != arrayType { @@ -371,7 +371,9 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu continue } - res.addErrorsAt(o.Path.child(k), errors.Required(o.Path.child(k).dotted(), o.In, v)) + // located on the object that lacks the property: the property itself + // has no node to point at, and the object is what has to be amended + res.addErrorsAt(o.Path, errors.Required(o.Path.child(k).dotted(), o.In, v)) } } diff --git a/path.go b/path.go index 1aee44f..7bbe2ec 100644 --- a/path.go +++ b/path.go @@ -4,6 +4,7 @@ package validate import ( + "slices" "strconv" "strings" @@ -33,6 +34,12 @@ type pathSegments []pathToken type pathToken struct { token string display string + + // structural marks a token a document needs to address the value, but + // that messages have never shown: a "properties" between a schema and one + // of its members, say. It is part of the pointer and absent from the + // dotted form. + structural bool } // readable renders a token the way a message should spell it. @@ -79,6 +86,12 @@ func (p pathSegments) childAs(token, display string) pathSegments { return p.appendToken(pathToken{token: token, display: display}) } +// structuralChild returns the location of a member a document addresses but +// messages do not name. +func (p pathSegments) structuralChild(token string) pathSegments { + return p.appendToken(pathToken{token: token, structural: true}) +} + func (p pathSegments) appendToken(token pathToken) pathSegments { child := make(pathSegments, len(p)+1) copy(child, p) @@ -106,24 +119,46 @@ func (p pathSegments) item(index int) pathSegments { // isEmpty tells if p locates the document root. func (p pathSegments) isEmpty() bool { return len(p) == 0 } -// last returns the trailing token, or an empty string at the document root. +// last returns the trailing meaningful token, or an empty string at the +// document root. +// +// Structural tokens are skipped: they say how a document addresses the value, +// not what the value is, and the callers here are asking the latter. func (p pathSegments) last() string { - if len(p) == 0 { - return "" + if token, ok := p.meaningfulAt(0); ok { + return token } - return p[len(p)-1].token + return "" } -// beforeLast returns the token before the trailing one, or an empty string -// when p holds fewer than two tokens. +// beforeLast returns the meaningful token before the trailing one, or an empty +// string when p holds fewer than two of them. func (p pathSegments) beforeLast() string { - const beforeLast = 2 - if len(p) < beforeLast { - return "" + if token, ok := p.meaningfulAt(1); ok { + return token + } + + return "" +} + +// meaningfulAt returns the nth token from the end, counting only the tokens a +// message would show. +func (p pathSegments) meaningfulAt(n int) (string, bool) { + seen := 0 + for _, token := range slices.Backward(p) { + if token.structural { + continue + } + + if seen == n { + return token.token, true + } + + seen++ } - return p[len(p)-beforeLast].token + return "", false } // trimIndexes returns p without its trailing array index tokens. @@ -178,9 +213,13 @@ func (p pathSegments) hasSuffix(suffix pathSegments) bool { // name of a validation error, and API consumers of go-swagger servers see it. // Use [pathSegments.pointer] whenever the location needs to be unambiguous. func (p pathSegments) dotted() string { - readable := make([]string, len(p)) - for i, token := range p { - readable[i] = token.readable() + readable := make([]string, 0, len(p)) + for _, token := range p { + if token.structural { + continue + } + + readable = append(readable, token.readable()) } return strings.Join(readable, ".") diff --git a/path_test.go b/path_test.go index 9217b1c..292be1b 100644 --- a/path_test.go +++ b/path_test.go @@ -108,6 +108,37 @@ func TestPathSegmentsDisplayDiffersFromPointer(t *testing.T) { }) } +func TestPathSegmentsStructuralTokensAreAddressedNotShown(t *testing.T) { + t.Parallel() + + // a document needs "properties" to address a member of a schema, but no + // message has ever spelled it + path := newPathSegments("definitions", "Pet"). + structuralChild(jsonProperties). + child("name"). + child(jsonDefault) + + assert.EqualT(t, "/definitions/Pet/properties/name/default", path.pointer()) + assert.EqualT(t, "definitions.Pet.name.default", path.dotted()) + + t.Run("a structural token is not what the value is", func(t *testing.T) { + t.Parallel() + + // the predicates telling schema from data ask what the value is, and + // plumbing must not answer + inside := newPathSegments("responses", "200", "examples").structuralChild("application/json") + assert.EqualT(t, swaggerExamples, inside.last()) + assert.EqualT(t, "200", inside.beforeLast()) + }) + + t.Run("structural tokens still address", func(t *testing.T) { + t.Parallel() + + assert.True(t, path.hasSuffix(newPathSegments("properties", "name", "default")), + "expected structure to see the token a document uses") + }) +} + func TestPathSegmentsAppendIsCopyOnWrite(t *testing.T) { t.Parallel() diff --git a/result.go b/result.go index 5219b31..fc6f8e7 100644 --- a/result.go +++ b/result.go @@ -20,8 +20,16 @@ type Located struct { Err error // Pointer locates the offending value as an RFC 6901 JSON pointer, - // relative to the validated document. It is empty when the producer of - // the error did not know where it happened. + // relative to the validated document. + // + // It is empty when the document as a whole is the answer: either because + // the finding is about the document rather than a value in it, such as a + // duplicate operation id, or because the value in question is the root. + // An empty pointer is a valid one, addressing the whole document. + // + // A finding about something a document does not contain, a missing + // required property say, is located on the value that should contain it: + // what is absent has no node to point at. Pointer string } diff --git a/result_location_test.go b/result_location_test.go index 22d258e..11d209b 100644 --- a/result_location_test.go +++ b/result_location_test.go @@ -159,7 +159,9 @@ func TestResultLocations_SchemaValidation(t *testing.T) { } assert.SliceContainsT(t, pointers, "/friends/0/name") - assert.SliceContainsT(t, pointers, "/friends/0/age") + // a missing property has no node of its own: the object lacking it is + // what has to be amended, and what a document can address + assert.SliceContainsT(t, pointers, "/friends/0") assert.SliceContainsT(t, pointers, "/n~0x~1y", "expected the token to be escaped") } @@ -203,12 +205,12 @@ func TestResultLocations_SpecValidation(t *testing.T) { require.False(t, res.IsValid()) t.Run("a default is located in the definition that declares it", func(t *testing.T) { - assert.SliceContainsT(t, pointersOf(res.LocatedErrors()), "/definitions/Pet/name/default") + assert.SliceContainsT(t, pointersOf(res.LocatedErrors()), "/definitions/Pet/properties/name/default") }) t.Run("an example is located under its response", func(t *testing.T) { assert.SliceContainsT(t, pointersOf(warns.LocatedErrors()), - "/paths/~1pets~1{id}/get/responses/200/examples/friends/0/name") + "/paths/~1pets~1{id}/get/responses/200/examples/application~1json/friends/0/name") }) t.Run("an unused definition is located", func(t *testing.T) { @@ -227,6 +229,37 @@ func TestResultLocations_SpecValidation(t *testing.T) { }) } +func TestResultLocations_MissingValueIsLocatedOnItsHolder(t *testing.T) { + t.Parallel() + + // what is absent has no node to point at, so the value that should hold + // it is reported: that is what a reader has to open, and the only thing a + // document can address + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{ + "type": "object", + "required": ["atRoot"], + "properties": { + "nested": {"type": "object", "required": ["deep"]} + } + }`), schema)) + + res := NewSchemaValidator(schema, nil, "", strfmt.Default). + Validate(map[string]any{"nested": map[string]any{}}) + require.False(t, res.IsValid()) + + located := res.LocatedErrors() + byMessage := make(map[string]string, len(located)) + for _, l := range located { + byMessage[l.Err.Error()] = l.Pointer + } + + assert.EqualT(t, "/nested", byMessage["nested.deep in body is required"], + "expected the object lacking the property") + assert.EqualT(t, "", byMessage["atRoot in body is required"], + "expected the document root, which an empty pointer addresses") +} + func TestResultLocations_RequiredEntryIsLocated(t *testing.T) { t.Parallel()