From 8a13cf3da06ed8c21bef28651eb31b203d021592 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 16:22:18 +0200 Subject: [PATCH 1/5] chore(lint): disable exhaustruct_v5 alongside exhaustruct golangci-lint renamed exhaustruct to exhaustruct_v5, and the disable list carried only the old name, so the linter fired 120 times across the repo. wsl/wsl_v5 and gomodguard/gomodguard_v2 already list both names. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- .golangci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.golangci.yml b/.golangci.yml index b8875d7..c05f38e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -10,6 +10,7 @@ linters: - gomodguard - gomodguard_v2 - exhaustruct + - exhaustruct_v5 - ireturn - nlreturn - nestif From 42e88efaeebb14645ec28e0ec663875a7319ca24 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 16:22:39 +0200 Subject: [PATCH 2/5] feat(spec): check security requirements against securityDefinitions A security requirement names a scheme and lists the scopes an operation needs from it. The JSON meta-schema types it as an object of string arrays and never reads securityDefinitions, so it cannot check either against the other. validateSecurityRequirements walks the document's own security array and each operation's, and reports: - a name securityDefinitions does not declare (error); - scopes on a scheme that is not oauth2, which must list none (error); - an oauth2 scope its scheme does not declare (warning). The last one warns rather than errors because Swagger 2.0 never says a requirement and its scheme must agree on scope names. testdata/bugs/2649 (the GoToSocial spec) requires read:bookmarks, read:reports and write:reports without declaring any of them, and stays valid. Findings carry a location, /paths/~1pets/get/security/0/basic_auth. An empty requirement object and an empty security array both pass: they mean optional security and no security. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- doc.go | 3 + helpers.go | 4 + security_requirements.go | 99 +++++++++++++++++ security_requirements_test.go | 204 ++++++++++++++++++++++++++++++++++ spec.go | 1 + spec_messages.go | 26 +++++ 6 files changed, 337 insertions(+) create mode 100644 security_requirements.go create mode 100644 security_requirements_test.go diff --git a/doc.go b/doc.go index 5218ec8..5f0a22f 100644 --- a/doc.go +++ b/doc.go @@ -25,6 +25,8 @@ // [x] path uniqueness: each api path should be non-verbatim (account for path param names) unique per method. Validation can be laxed by disabling StrictPathParamUniqueness. // [x] each security reference should contain only unique scopes // [x] each security scope in a security definition should be unique +// [x] each security requirement must name a scheme declared in securityDefinitions +// [x] only an oauth2 security requirement may list scopes: every other scheme type must list none // [x] parameters in path must be unique // [x] each path parameter must correspond to a parameter placeholder and vice versa // [x] each referenceable definition must have references @@ -47,6 +49,7 @@ // [x] unsupported validation of examples on non-JSON media types // [x] examples in response without schema // [x] readOnly properties should not be required +// [x] an oauth2 security requirement names a scope its security scheme does not declare // // # Validating a schema // diff --git a/helpers.go b/helpers.go index 62deb97..567e5f8 100644 --- a/helpers.go +++ b/helpers.go @@ -56,6 +56,10 @@ const ( swaggerParameters = "parameters" swaggerHeaders = "headers" swaggerOperationID = "operationId" + swaggerSecurity = "security" + + // securitySchemeOAuth2 is the only security scheme type whose requirements carry scopes. + securitySchemeOAuth2 = "oauth2" jsonMimeApplicationJSON = "application/json" ) diff --git a/security_requirements.go b/security_requirements.go new file mode 100644 index 0000000..b6c10f3 --- /dev/null +++ b/security_requirements.go @@ -0,0 +1,99 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/spec" +) + +// validateSecurityRequirements checks the security requirements declared by the document and by +// each of its operations against the security definitions. +// +// A requirement names a security scheme and lists the scopes an operation needs from it: +// +// security: +// - petstore_auth: [ "write:pets" ] +// - api_key: [] +// +// Three rules apply: +// +// - the name must be declared in securityDefinitions (error) +// - only an oauth2 requirement carries scopes; every other scheme type must list none (error) +// - an oauth2 requirement should only name scopes its scheme declares (warning) +// +// The JSON meta-schema types a requirement as an object of string arrays and never reads +// securityDefinitions, so it can express none of the three. +// +// An empty requirement object ({}) names no scheme and passes. So does an empty security array, +// which an operation uses to drop the requirements the document sets for every operation. +func (s *SpecValidator) validateSecurityRequirements() *Result { + res := validatorPools.results.Borrow() + definitions := s.spec.Spec().SecurityDefinitions + + res.Merge(checkSecurityRequirements( + newPathSegments(swaggerSecurity), + s.spec.Spec().Security, + definitions, + )) + + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + if op == nil { + continue + } + + res.Merge(checkSecurityRequirements( + operationPath(path, method).child(swaggerSecurity), + op.Security, + definitions, + )) + } + } + + return res +} + +// checkSecurityRequirements checks the list of security requirements held at the given location. +// +// Requirements are checked in the order the document lists them, and the schemes one requirement +// names in sorted order: a requirement is a map, so the document's own order is lost (see +// [sortedKeys]). +func checkSecurityRequirements(at pathSegments, requirements []map[string][]string, definitions spec.SecurityDefinitions) *Result { + res := validatorPools.results.Borrow() + + for i, requirement := range requirements { + for _, name := range sortedKeys(requirement) { + scopes := requirement[name] + schemeAt := at.item(i).child(name) + + scheme, isDeclared := definitions[name] + if !isDeclared || scheme == nil { + res.addErrorsAt(schemeAt, securitySchemeNotDeclaredMsg(name)) + + continue + } + + if scheme.Type != securitySchemeOAuth2 { + if len(scopes) > 0 { + res.addErrorsAt(schemeAt, securityScopesNotEmptyMsg(name, strings.Join(scopes, ", "), scheme.Type)) + } + + continue + } + + for _, scope := range scopes { + if _, isKnown := scheme.Scopes[scope]; !isKnown { + res.addWarningsAt(schemeAt, securityScopeNotDeclaredMsg(name, scope)) + } + } + } + } + + return res +} diff --git a/security_requirements_test.go b/security_requirements_test.go new file mode 100644 index 0000000..9944c7d --- /dev/null +++ b/security_requirements_test.go @@ -0,0 +1,204 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "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" +) + +// securitySpec builds a specification whose security definitions cover the three scheme shapes a +// requirement can name: an apiKey, a basic auth, and an oauth2 scheme declaring one scope. +// +// rootSecurity and opSecurity are spliced in as authored JSON so that a test can write an empty +// array, an empty requirement object, or no member at all. +func securitySpec(rootSecurity, opSecurity string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "securityDefinitions": { + "api_key": {"type": "apiKey", "name": "api_key", "in": "header"}, + "basic_auth": {"type": "basic"}, + "petstore_auth": { + "type": "oauth2", + "flow": "implicit", + "authorizationUrl": "https://example.com/auth", + "scopes": {"read:pets": "read your pets"} + } + }, + %s + "paths": { + "/pets": { + "get": { + "responses": {"200": {"description": "ok"}}%s + } + } + } + }`, rootSecurity, opSecurity) +} + +func rootSecurity(requirements string) string { + return `"security": ` + requirements + `,` +} + +func opSecurity(requirements string) string { + return `, "security": ` + requirements +} + +// securityValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the point +// validateSecurityRequirements needs: the document and an analyzer over it. +func securityValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + + return s +} + +func TestValidateSecurityRequirements(t *testing.T) { + t.Parallel() + + const undeclaredScheme = `security requirement "unknown_scheme" is not declared in securityDefinitions` + + tests := []struct { + name string + doc string + wantErrors []string + wantWarnings []string + }{ + { + name: "scheme declared in securityDefinitions", + doc: securitySpec(rootSecurity(`[{"api_key": []}]`), opSecurity(`[{"petstore_auth": ["read:pets"]}]`)), + }, + { + name: "undeclared scheme at the document level", + doc: securitySpec(rootSecurity(`[{"unknown_scheme": []}]`), ""), + wantErrors: []string{undeclaredScheme}, + }, + { + name: "undeclared scheme on an operation", + doc: securitySpec("", opSecurity(`[{"unknown_scheme": []}]`)), + wantErrors: []string{undeclaredScheme}, + }, + { + name: "scopes on an apiKey scheme", + doc: securitySpec("", opSecurity(`[{"api_key": ["read:pets"]}]`)), + wantErrors: []string{ + `security requirement "api_key" lists scopes (read:pets), but the security scheme it names is of type "apiKey": only oauth2 requirements carry scopes`, + }, + }, + { + name: "scopes on a basic scheme, at the document level", + doc: securitySpec(rootSecurity(`[{"basic_auth": ["a", "b"]}]`), ""), + wantErrors: []string{ + `security requirement "basic_auth" lists scopes (a, b), but the security scheme it names is of type "basic": only oauth2 requirements carry scopes`, + }, + }, + { + name: "oauth2 scope the scheme does not declare", + doc: securitySpec("", opSecurity(`[{"petstore_auth": ["read:pets", "write:pets"]}]`)), + wantWarnings: []string{ + `security requirement "petstore_auth" requires scope "write:pets", which the security scheme does not declare`, + }, + }, + { + name: "empty requirement object makes security optional", + doc: securitySpec(rootSecurity(`[{"api_key": []}, {}]`), ""), + }, + { + name: "empty security array drops the document requirements", + doc: securitySpec(rootSecurity(`[{"api_key": []}]`), opSecurity(`[]`)), + }, + { + name: "no security member at all", + doc: securitySpec("", ""), + }, + { + name: "several requirements are all checked", + doc: securitySpec(rootSecurity(`[{"first_unknown": []}, {"second_unknown": []}]`), ""), + wantErrors: []string{ + `security requirement "first_unknown" is not declared in securityDefinitions`, + `security requirement "second_unknown" is not declared in securityDefinitions`, + }, + }, + { + name: "several schemes in one requirement are all checked", + doc: securitySpec("", opSecurity(`[{"api_key": ["nope"], "unknown_scheme": []}]`)), + wantErrors: []string{ + `security requirement "api_key" lists scopes (nope), but the security scheme it names is of type "apiKey": only oauth2 requirements carry scopes`, + undeclaredScheme, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + res := securityValidatorFromJSON(t, tc.doc).validateSecurityRequirements() + assert.Equal(t, tc.wantErrors, nonEmpty(errorMessages(res))) + assert.Equal(t, tc.wantWarnings, nonEmpty(warningMessages(res))) + }) + } +} + +// nonEmpty reports an empty list of messages as nil, so that a test case may leave its +// expectation out altogether. +func nonEmpty(messages []string) []string { + if len(messages) == 0 { + return nil + } + + return messages +} + +// TestSecurityRequirementLocations pins down where a security finding says it happened. The +// pointers are produced by a full Validate, which is what trims a location down to a node the +// document holds. +func TestSecurityRequirementLocations(t *testing.T) { + t.Parallel() + + doc := securitySpec( + rootSecurity(`[{"api_key": []}, {"unknown_scheme": []}]`), + opSecurity(`[{"basic_auth": ["nope"]}, {"petstore_auth": ["write:pets"]}]`), + ) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + errs, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + + assert.SliceContainsT(t, pointersOf(errs.LocatedErrors()), "/security/1/unknown_scheme") + assert.SliceContainsT(t, pointersOf(errs.LocatedErrors()), "/paths/~1pets/get/security/0/basic_auth") + assert.SliceContainsT(t, pointersOf(warns.LocatedErrors()), "/paths/~1pets/get/security/1/petstore_auth") +} + +// TestSecurityScopeWarningKeepsSpecValid guards the choice made for the third rule: an oauth2 +// requirement naming an undeclared scope warns, and the specification stays valid. +func TestSecurityScopeWarningKeepsSpecValid(t *testing.T) { + t.Parallel() + + doc := securitySpec("", opSecurity(`[{"petstore_auth": ["write:pets"]}]`)) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + require.NoError(t, Spec(d, strfmt.Default)) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + assert.SliceContainsT(t, errorMessages(warns), + `security requirement "petstore_auth" requires scope "write:pets", which the security scheme does not declare`) +} diff --git a/spec.go b/spec.go index 0dc0dd3..f8f8b32 100644 --- a/spec.go +++ b/spec.go @@ -153,6 +153,7 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { errs.Merge(s.validateDuplicatePropertyNames()) // error - errs.Merge(s.validateParameters()) // error - errs.Merge(s.validateItems()) // error - + errs.Merge(s.validateSecurityRequirements()) // error and warning // Properties in required definition MUST validate their schema // Properties SHOULD NOT be declared as both required and readOnly (warning) diff --git a/spec_messages.go b/spec_messages.go index 0a0739a..2f9697d 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -136,6 +136,14 @@ const ( // in the definition itself. RequiredButNotDefinedInSchemaError = "%q is present in required but not defined as property in schema %q" + // SecuritySchemeNotDeclaredError indicates a security requirement naming a scheme that + // securityDefinitions does not declare. + SecuritySchemeNotDeclaredError = "security requirement %q is not declared in securityDefinitions" + + // SecurityScopesNotEmptyError indicates a security requirement listing scopes on a scheme that is + // not oauth2. Only an oauth2 requirement carries scopes; every other type must list none. + SecurityScopesNotEmptyError = "security requirement %q lists scopes (%s), but the security scheme it names is of type %q: only oauth2 requirements carry scopes" + // SomeParametersBrokenError indicates that some parameters could not be resolved, which might result in partial checks to be carried on. SomeParametersBrokenError = "some parameters definitions are broken in %q.%s. Cannot carry on full checks on parameters for operation %s" @@ -171,6 +179,11 @@ const ( // RequiredHasDefaultWarning indicates that a required parameter property should not have a default. RequiredHasDefaultWarning = "%s in %s has a default value and is required as parameter" + // SecurityScopeNotDeclaredWarning flags an oauth2 security requirement asking for a scope that the + // scheme does not list in its scopes. Swagger 2.0 does not spell out that the two must agree, so + // this is reported as a warning: a specification that names an undeclared scope stays valid. + SecurityScopeNotDeclaredWarning = "security requirement %q requires scope %q, which the security scheme does not declare" + // UnusedDefinitionWarning ... UnusedDefinitionWarning = "definition %q is not used anywhere" @@ -416,6 +429,19 @@ func invalidObjectMsg(path, in string) errors.Error { // func invalidResponseDefinitionAsSchemaMsg(path, method string) errors.Error { // return errors.New(errors.CompositeErrorCode, InvalidResponseDefinitionAsSchemaError, path, method) // } + +func securitySchemeNotDeclaredMsg(name string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecuritySchemeNotDeclaredError, name) +} + +func securityScopesNotEmptyMsg(name, scopes, schemeType string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecurityScopesNotEmptyError, name, scopes, schemeType) +} + +func securityScopeNotDeclaredMsg(name, scope string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecurityScopeNotDeclaredWarning, name, scope) +} + func someParametersBrokenMsg(path, method, operationID string) errors.Error { return errors.New(errors.CompositeErrorCode, SomeParametersBrokenError, path, method, operationID) } From fc4386855ea287c79e5b31d2f2f9b02bd2b8873f Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 17:19:22 +0200 Subject: [PATCH 3/5] feat(spec): check a discriminator is defined and required A discriminator names the property that tells subtypes apart, and Swagger 2.0 asks two things of it: the schema must define that property, and must list it as required. An instance carries its subtype in that property, so a subtype cannot be resolved from an instance with nowhere to put the value, or free to leave it out. The JSON meta-schema types discriminator as a plain string and never compares it against properties or required, so it can express neither check. validateDiscriminators walks every definition and the schemas it holds inline, and reports both faults where they apply. A property an allOf member contributes counts as defined, and one that member requires counts as required: declaresProperty already reads a composed definition that way for the required rule, and requiresProperty is its required-list counterpart. A schema written as a $ref is left alone, so a fault is reported where the definition is written rather than once per pointer at it, and a recursive definition terminates. The third clause, that the value must name this schema or one that inherits it, constrains the data rather than the document. It stays unimplemented and moves to the known limitations in doc.go. Fixes #54 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- discriminator.go | 156 ++++++++++++++++++++++++ discriminator_test.go | 275 ++++++++++++++++++++++++++++++++++++++++++ doc.go | 3 +- helpers.go | 1 + spec.go | 2 +- spec_messages.go | 17 +++ 6 files changed, 452 insertions(+), 2 deletions(-) create mode 100644 discriminator.go create mode 100644 discriminator_test.go diff --git a/discriminator.go b/discriminator.go new file mode 100644 index 0000000..7a4ab41 --- /dev/null +++ b/discriminator.go @@ -0,0 +1,156 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "slices" + + "github.com/go-openapi/spec" +) + +// validateDiscriminators checks the discriminator of every definition, and of every schema a +// definition holds inline. +// +// A discriminator names the property that tells subtypes apart: +// +// Pet: +// discriminator: petType +// required: [ petType ] +// properties: +// petType: { type: string } +// +// Swagger 2.0 asks two things of that property: the schema must define it, and must list it as +// required. Both matter for the same reason — an instance carries its subtype in that property, +// so a subtype cannot be resolved from an instance that has nowhere to put the value, or that is +// free to leave it out. +// +// The JSON meta-schema types discriminator as a plain string and never compares it against +// properties or required, so it can express neither check. +// +// A property contributed by an allOf member counts as defined, and one that member requires counts +// as required: [SpecValidator.declaresProperty] already reads a composed definition that way for +// the required rule, and a discriminator resolves against the instance the whole composition +// describes. +// +// The third clause of the rule — the value must name this schema or one that inherits it — +// constrains the data, not the document, so it belongs to schema validation rather than here. +func (s *SpecValidator) validateDiscriminators() *Result { + res := validatorPools.results.Borrow() + definitions := s.spec.Spec().Definitions + + for _, name := range sortedKeys(definitions) { + schema := definitions[name] + s.walkDiscriminators(newPathSegments(swaggerDefinitions, name), &schema, res) + } + + return res +} + +// walkDiscriminators checks the discriminator of a schema, then of every schema it holds inline. +// +// A schema written as a $ref is left alone: it is checked where it is defined, and following it +// here would report the same fault twice and, for a recursive definition, would not terminate. +// This mirrors [SpecValidator.walkRequired]. +func (s *SpecValidator) walkDiscriminators(at pathSegments, v *spec.Schema, res *Result) { + if v == nil || v.Ref.String() != "" { + return + } + + s.checkDiscriminator(at, v, res) + + for _, name := range sortedKeys(v.Properties) { + held := v.Properties[name] + s.walkDiscriminators(at.structuralChild(jsonProperties).child(name), &held, res) + } + + for _, pattern := range sortedKeys(v.PatternProperties) { + held := v.PatternProperties[pattern] + s.walkDiscriminators(at.structuralChild(jsonPatternProperties).child(pattern), &held, res) + } + + if v.Items != nil { + if v.Items.Schema != nil { + s.walkDiscriminators(at.child(jsonItems), v.Items.Schema, res) + } + for i := range v.Items.Schemas { + s.walkDiscriminators(at.child(jsonItems).item(i), &v.Items.Schemas[i], res) + } + } + + if v.AdditionalProperties != nil && v.AdditionalProperties.Schema != nil { + s.walkDiscriminators(at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema, res) + } + + for _, composition := range []struct { + keyword string + members []spec.Schema + }{ + {jsonAllOf, v.AllOf}, + {jsonAnyOf, v.AnyOf}, + {jsonOneOf, v.OneOf}, + } { + for i := range composition.members { + s.walkDiscriminators(at.child(composition.keyword).item(i), &composition.members[i], res) + } + } + + if v.Not != nil { + s.walkDiscriminators(at.child(jsonNot), v.Not, res) + } +} + +// checkDiscriminator checks the discriminator a single schema declares. A schema without one has +// nothing to answer for. +// +// Both findings are reported against the discriminator itself, which is the entry a reader has to +// go and amend, and both are reported when they apply: a discriminator naming a property that is +// neither defined nor required is two separate slips to fix. +func (s *SpecValidator) checkDiscriminator(at pathSegments, v *spec.Schema, res *Result) { + if v.Discriminator == "" { + return + } + + of := identify(at).name + discriminatorAt := at.child(jsonDiscriminator) + + if _, declared := s.declaresProperty(v, v.Discriminator, maxCompositionHops); !declared { + res.addErrorsAt(discriminatorAt, discriminatorNotDefinedMsg(v.Discriminator, of)) + } + + if !s.requiresProperty(v, v.Discriminator, maxCompositionHops) { + res.addErrorsAt(discriminatorAt, discriminatorNotRequiredMsg(v.Discriminator, of)) + } +} + +// requiresProperty reports whether a schema, or any schema composed into it by allOf, lists the +// named property as required. +// +// It is the required-list counterpart of [SpecValidator.declaresProperty], and follows allOf the +// same way, including the local $ref an allOf member may be written as. +func (s *SpecValidator) requiresProperty(v *spec.Schema, name string, hops int) bool { + if v == nil || hops <= 0 { + return false + } + + if slices.Contains(v.Required, name) { + return true + } + + for i := range v.AllOf { + member := &v.AllOf[i] + if member.Ref.String() != "" { + resolved, err := s.resolveRef(&member.Ref) + if err != nil { + continue + } + member = resolved + } + + if s.requiresProperty(member, name, hops-1) { + return true + } + } + + return false +} diff --git a/discriminator_test.go b/discriminator_test.go new file mode 100644 index 0000000..ef2b420 --- /dev/null +++ b/discriminator_test.go @@ -0,0 +1,275 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "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" +) + +// discriminatorSpec builds a specification holding the given definitions and nothing else of note. +func discriminatorSpec(definitions string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {}, + "definitions": %s + }`, definitions) +} + +// discriminatorValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the +// point validateDiscriminators needs. +func discriminatorValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + + return s +} + +func TestValidateDiscriminators(t *testing.T) { + t.Parallel() + + const ( + petTypeNotDefined = `discriminator "petType" of "Pet" is not defined as a property of that schema` + petTypeNotRequired = `discriminator "petType" of "Pet" is not in the required property list` + ) + + tests := []struct { + name string + defs string + wantErrors []string + }{ + { + name: "discriminator defined and required", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "required": ["petType"], + "properties": {"petType": {"type": "string"}} + } + }`, + }, + { + name: "no discriminator at all", + defs: `{"Pet": {"type": "object", "properties": {"name": {"type": "string"}}}}`, + }, + { + name: "discriminator names an undeclared property", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "required": ["petType"], + "properties": {"name": {"type": "string"}} + } + }`, + wantErrors: []string{petTypeNotDefined}, + }, + { + name: "discriminator property is optional", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "properties": {"petType": {"type": "string"}} + } + }`, + wantErrors: []string{petTypeNotRequired}, + }, + { + name: "discriminator neither defined nor required reports both", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "properties": {"name": {"type": "string"}} + } + }`, + wantErrors: []string{ + petTypeNotDefined, + petTypeNotRequired, + }, + }, + { + name: "property and required contributed by an allOf $ref", + defs: `{ + "Base": { + "type": "object", + "required": ["petType"], + "properties": {"petType": {"type": "string"}} + }, + "Pet": { + "discriminator": "petType", + "allOf": [ + {"$ref": "#/definitions/Base"}, + {"type": "object", "properties": {"name": {"type": "string"}}} + ] + } + }`, + }, + { + name: "property contributed by an inline allOf member", + defs: `{ + "Pet": { + "discriminator": "petType", + "allOf": [ + {"type": "object", "required": ["petType"], "properties": {"petType": {"type": "string"}}} + ] + } + }`, + }, + { + name: "allOf contributes the property but nothing requires it", + defs: `{ + "Base": {"type": "object", "properties": {"petType": {"type": "string"}}}, + "Pet": { + "discriminator": "petType", + "allOf": [{"$ref": "#/definitions/Base"}] + } + }`, + wantErrors: []string{petTypeNotRequired}, + }, + { + name: "several definitions are all checked, in name order", + defs: `{ + "Alpha": {"type": "object", "discriminator": "kind", "required": ["kind"]}, + "Beta": {"type": "object", "discriminator": "sort", "required": ["sort"]} + }`, + wantErrors: []string{ + `discriminator "kind" of "Alpha" is not defined as a property of that schema`, + `discriminator "sort" of "Beta" is not defined as a property of that schema`, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + res := discriminatorValidatorFromJSON(t, discriminatorSpec(tc.defs)).validateDiscriminators() + assert.Equal(t, tc.wantErrors, nonEmpty(errorMessages(res))) + assert.Empty(t, warningMessages(res), "the rule reports errors only") + }) + } +} + +// TestDiscriminatorInNestedSchema covers a discriminator that sits below a definition rather than +// on it: the same slip, wherever it is written. +func TestDiscriminatorInNestedSchema(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Zoo": { + "type": "object", + "properties": { + "resident": { + "type": "object", + "discriminator": "petType", + "properties": {"name": {"type": "string"}} + } + } + } + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Equal(t, []string{ + `discriminator "petType" of "Zoo.resident" is not defined as a property of that schema`, + `discriminator "petType" of "Zoo.resident" is not in the required property list`, + }, errorMessages(res)) +} + +// TestDiscriminatorBehindRefCheckedOnce guards the choice not to follow a $ref: the fault is +// reported where the definition is written, not again at every schema pointing at it. +func TestDiscriminatorBehindRefCheckedOnce(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Pet": {"type": "object", "discriminator": "petType"}, + "First": {"$ref": "#/definitions/Pet"}, + "Second": {"type": "object", "properties": {"pet": {"$ref": "#/definitions/Pet"}}} + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Equal(t, []string{ + `discriminator "petType" of "Pet" is not defined as a property of that schema`, + `discriminator "petType" of "Pet" is not in the required property list`, + }, errorMessages(res)) +} + +// TestDiscriminatorRecursiveDefinitionTerminates guards the same choice against the reason it was +// made: a definition holding itself would not terminate if the walk followed $ref. +func TestDiscriminatorRecursiveDefinitionTerminates(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Node": { + "type": "object", + "discriminator": "kind", + "required": ["kind"], + "properties": { + "kind": {"type": "string"}, + "child": {"$ref": "#/definitions/Node"} + } + } + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Empty(t, errorMessages(res)) +} + +// TestDiscriminatorLocations pins down where a discriminator finding says it happened. The +// pointers come from a full Validate, which trims a location to a node the document holds. +func TestDiscriminatorLocations(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Pet": {"type": "object", "discriminator": "petType", "required": ["petType"]}, + "Zoo": { + "type": "object", + "properties": { + "resident": {"type": "object", "discriminator": "kind", "required": ["kind"]} + } + } + }`) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + validator := NewSpecValidator(d.Schema(), strfmt.Default) + validator.SetContinueOnErrors(true) + errs, _ := validator.Validate(d) + + pointers := pointersOf(errs.LocatedErrors()) + assert.SliceContainsT(t, pointers, "/definitions/Pet/discriminator") + assert.SliceContainsT(t, pointers, "/definitions/Zoo/properties/resident/discriminator") +} + +// TestDiscriminatorMakesSpecInvalid checks that the rule reaches Spec, the package's front door, +// rather than only the validator method the other tests call. +func TestDiscriminatorMakesSpecInvalid(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{"Pet": {"type": "object", "discriminator": "petType"}}`) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + err = Spec(d, strfmt.Default) + require.Error(t, err) + assert.StringContainsT(t, err.Error(), `discriminator "petType" of "Pet"`) +} diff --git a/doc.go b/doc.go index 5f0a22f..7937976 100644 --- a/doc.go +++ b/doc.go @@ -25,6 +25,7 @@ // [x] path uniqueness: each api path should be non-verbatim (account for path param names) unique per method. Validation can be laxed by disabling StrictPathParamUniqueness. // [x] each security reference should contain only unique scopes // [x] each security scope in a security definition should be unique +// [x] a discriminator must name a property the schema defines and lists as required // [x] each security requirement must name a scheme declared in securityDefinitions // [x] only an oauth2 security requirement may list scopes: every other scheme type must list none // [x] parameters in path must be unique @@ -73,7 +74,7 @@ // [ ] default values and examples on responses only support application/json producer type // [ ] invalid numeric constraints (such as Minimum, etc..) are not checked except for default and example values // [ ] rules for collectionFormat are not implemented -// [ ] no validation rule for polymorphism support (discriminator) [not done here] +// [ ] a discriminator value is not checked against the schema names it may take [data, not document] // [ ] valid js ECMA regexp not supported by Go regexp engine are considered invalid // [ ] arbitrary large numbers are not supported: max is math.MaxFloat64 package validate diff --git a/helpers.go b/helpers.go index 567e5f8..b623be1 100644 --- a/helpers.go +++ b/helpers.go @@ -42,6 +42,7 @@ const ( jsonRequired = "required" jsonRef = "$ref" jsonDefault = "default" + jsonDiscriminator = "discriminator" jsonAllOf = "allOf" jsonAnyOf = "anyOf" diff --git a/spec.go b/spec.go index f8f8b32..ad9dc90 100644 --- a/spec.go +++ b/spec.go @@ -31,7 +31,6 @@ import ( // // - Proposal for enhancement: $ref should not have siblings // - Proposal for enhancement: make sure documentation reflects all checks and warnings -// - Proposal for enhancement: check on discriminators // - Proposal for enhancement: explicit message on unsupported keywords (better than "forbidden property"...) // - Proposal for enhancement: full list of unresolved refs // - Proposal for enhancement: validate numeric constraints (issue#581): this should be handled like defaults and examples @@ -154,6 +153,7 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { errs.Merge(s.validateParameters()) // error - errs.Merge(s.validateItems()) // error - errs.Merge(s.validateSecurityRequirements()) // error and warning + errs.Merge(s.validateDiscriminators()) // error - // Properties in required definition MUST validate their schema // Properties SHOULD NOT be declared as both required and readOnly (warning) diff --git a/spec_messages.go b/spec_messages.go index 2f9697d..7dd58aa 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -44,6 +44,15 @@ const ( // DefaultValueInDoesNotValidateError ... DefaultValueInDoesNotValidateError = "in operation %q, default value in %s does not validate its schema" + // DiscriminatorNotDefinedError indicates a schema whose discriminator names a property the + // schema does not declare. A discriminator tells subtypes apart by the value of that property, + // so an instance has nowhere to carry the value when the property is not declared. + DiscriminatorNotDefinedError = "discriminator %q of %q is not defined as a property of that schema" + + // DiscriminatorNotRequiredError indicates a schema whose discriminator property is declared but + // left optional. An instance that omits it cannot be resolved to a subtype. + DiscriminatorNotRequiredError = "discriminator %q of %q is not in the required property list" + // DuplicateParamNameError ... DuplicateParamNameError = "duplicate parameter name %q for %q in operation %q" @@ -430,6 +439,14 @@ func invalidObjectMsg(path, in string) errors.Error { // return errors.New(errors.CompositeErrorCode, InvalidResponseDefinitionAsSchemaError, path, method) // } +func discriminatorNotDefinedMsg(discriminator, in string) errors.Error { + return errors.New(errors.CompositeErrorCode, DiscriminatorNotDefinedError, discriminator, in) +} + +func discriminatorNotRequiredMsg(discriminator, in string) errors.Error { + return errors.New(errors.CompositeErrorCode, DiscriminatorNotRequiredError, discriminator, in) +} + func securitySchemeNotDeclaredMsg(name string) errors.Error { return errors.New(errors.CompositeErrorCode, SecuritySchemeNotDeclaredError, name) } From 2754a515be4ac3adc0f5364d9b770843e1b83836 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 18:03:14 +0200 Subject: [PATCH 4/5] feat(spec): warn when collectionFormat cannot apply collectionFormat says how to join the members of an array into one value on the wire. There is nothing to join when the type is not array, so a collectionFormat written on a string or an integer does nothing. validateCollectionFormats warns about one, in a parameter, a response header, or the items of either, at any nesting depth. Swagger 2.0 defines the member as applying when the type is array without forbidding it elsewhere, so this is a warning and the document stays valid. Only spec.SimpleSchema carries a collectionFormat, through spec.Items, spec.Header and spec.Parameter; spec.Schema has none. The rule therefore walks what the operations declare and never reads a schema, so validating an ordinary JSON schema cannot reach it. A test pins that down and breaks if the member ever moves onto Schema. The rest of the collectionFormat rules need no code: the meta-schema already caps the value to csv, ssv, tsv or pipes, widens it with multi for a query or formData parameter, and forbids the member outright on a body parameter, at every location each applies to. doc.go claimed none of the rules were implemented; it now records what is checked and where. validateItems built its list of responses inline. That moves to responsesOf, which both rules call. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- collection_format.go | 131 +++++++++++++++++++++ collection_format_test.go | 242 ++++++++++++++++++++++++++++++++++++++ doc.go | 7 +- helpers.go | 15 +-- spec.go | 23 +--- spec_messages.go | 10 ++ 6 files changed, 399 insertions(+), 29 deletions(-) create mode 100644 collection_format.go create mode 100644 collection_format_test.go diff --git a/collection_format.go b/collection_format.go new file mode 100644 index 0000000..a77ba56 --- /dev/null +++ b/collection_format.go @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "fmt" + "strconv" + + "github.com/go-openapi/spec" +) + +// validateCollectionFormats warns about a collectionFormat that has no effect. +// +// collectionFormat says how to join the members of an array into one value on the wire — csv, ssv, +// tsv, pipes, or multi for a parameter repeated once per member: +// +// parameters: +// - name: tags +// in: query +// type: array +// items: { type: string } +// collectionFormat: pipes +// +// There is nothing to join when the type is not array, so a collectionFormat written on a string or +// an integer does nothing. Swagger 2.0 says the member "determines the format of the array if type +// array is used" and stops there — it never forbids writing it elsewhere, so this is a warning and +// the specification stays valid. +// +// The meta-schema already covers the rest of the collectionFormat rules, and covers them at every +// location: the value must be one of csv, ssv, tsv or pipes, widened with multi for a query or +// formData parameter, where repeating the parameter is possible. A body parameter cannot carry the +// member at all. None of that needs a rule here. +// +// Only a parameter, a header and an items carry a collectionFormat — [spec.SimpleSchema] holds it, +// and [spec.Schema] has no such member. So this walks what the operations of the document declare, +// the way [SpecValidator.validateItems] does, and never looks at a schema. A JSON schema validated +// on its own is untouched by this rule. +func (s *SpecValidator) validateCollectionFormats() *Result { + res := validatorPools.results.Borrow() + + operations := s.analyzer.Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + + for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { + if param.In == swaggerBody { + // a body parameter describes itself with a schema, and the meta-schema + // forbids it a collectionFormat outright + continue + } + + at := s.parameterPath(path, method, param.In, param.Name) + in := fmt.Sprintf("parameter %q", param.Name) + checkCollectionFormat(at, param.CollectionFormat, param.Type, in, res) + checkItemsCollectionFormats(at, param.Items, in, res) + } + + for _, response := range responsesOf(op) { + at := responsePath(path, method, response.code) + for _, name := range sortedKeys(response.resp.Headers) { + header := response.resp.Headers[name] + headerAt := at.children(swaggerHeaders, name) + in := fmt.Sprintf("header %q", name) + checkCollectionFormat(headerAt, header.CollectionFormat, header.Type, in, res) + checkItemsCollectionFormats(headerAt, header.Items, in, res) + } + } + } + } + + return res +} + +// codedResponse is a response together with the code the operation files it under, "default" +// included. +type codedResponse struct { + code string + resp spec.Response +} + +// responsesOf lists the responses an operation declares, in a settled order: the default response +// first, then the status codes in ascending order. +func responsesOf(op *spec.Operation) []codedResponse { + if op == nil || op.Responses == nil { + return nil + } + + var responses []codedResponse + if op.Responses.Default != nil { + responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) + } + + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + responses = append(responses, codedResponse{ + code: strconv.Itoa(code), + resp: op.Responses.StatusCodeResponses[code], + }) + } + + return responses +} + +// checkItemsCollectionFormats checks the items of a parameter or header, then the items of those +// items, as deep as the document nests them. +// +// Every level is named the same way in a message: an array of arrays that writes a pointless +// collectionFormat twice is one thing to fix, and the deeper location is reported only when the +// shallower one is sound. +func checkItemsCollectionFormats(at pathSegments, items *spec.Items, in string, res *Result) { + for items != nil { + at = at.child(jsonItems) + checkCollectionFormat(at, items.CollectionFormat, items.Type, "items of "+in, res) + items = items.Items + } +} + +// checkCollectionFormat warns when a collectionFormat is written on something that is not an array. +// +// A missing collectionFormat has nothing to answer for, and neither has a missing type: a document +// that leaves the type out is already reported by the meta-schema, and guessing what it meant here +// would only add noise. +func checkCollectionFormat(at pathSegments, collectionFormat, typ, in string, res *Result) { + if collectionFormat == "" || typ == "" || typ == arrayType { + return + } + + res.addWarningsAt(at.child(swaggerCollectionFormat), collectionFormatIgnoredMsg(collectionFormat, in, typ)) +} diff --git a/collection_format_test.go b/collection_format_test.go new file mode 100644 index 0000000..8e1d1e9 --- /dev/null +++ b/collection_format_test.go @@ -0,0 +1,242 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/go-openapi/analysis" + "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" +) + +// collectionFormatSpec builds a specification whose only operation declares the given parameters +// and the given headers on its 200 response. +func collectionFormatSpec(parameters, headers string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/x": { + "get": { + "parameters": [%s], + "responses": {"200": {"description": "ok", "headers": {%s}}} + } + } + } + }`, parameters, headers) +} + +// collectionFormatValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the +// point validateCollectionFormats needs. +func collectionFormatValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + s.paramLocations = newParamLocations(d.Spec()) + + return s +} + +func TestValidateCollectionFormats(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parameters string + headers string + wantWarnings []string + }{ + { + name: "collectionFormat on an array", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string"},"collectionFormat":"pipes"}`, + }, + { + name: "array without a collectionFormat", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string"}}`, + }, + { + name: "no collectionFormat anywhere", + parameters: `{"name":"q","in":"query","type":"string"}`, + }, + { + name: "collectionFormat on a string parameter", + parameters: `{"name":"q","in":"query","type":"string","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on an integer parameter", + parameters: `{"name":"q","in":"query","type":"integer","collectionFormat":"pipes"}`, + wantWarnings: []string{ + `collectionFormat "pipes" is ignored in parameter "q": it joins the members of an array, and the type is "integer"`, + }, + }, + { + name: "collectionFormat on a file parameter", + parameters: `{"name":"f","in":"formData","type":"file","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in parameter "f": it joins the members of an array, and the type is "file"`, + }, + }, + { + name: "collectionFormat on the items of an array", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string","collectionFormat":"ssv"}}`, + wantWarnings: []string{ + `collectionFormat "ssv" is ignored in items of parameter "q": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on the items of an array of arrays", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"array","items":{"type":"string"},"collectionFormat":"tsv"}}`, + }, + { + name: "collectionFormat on a string response header", + headers: `"X":{"type":"string","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in header "X": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on an array response header", + headers: `"X":{"type":"array","items":{"type":"string"},"collectionFormat":"csv"}`, + }, + { + name: "collectionFormat on the items of a response header", + headers: `"X":{"type":"array","items":{"type":"integer","collectionFormat":"pipes"}}`, + wantWarnings: []string{ + `collectionFormat "pipes" is ignored in items of header "X": it joins the members of an array, and the type is "integer"`, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(tc.parameters, tc.headers) + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + + assert.Equal(t, tc.wantWarnings, nonEmpty(warningMessages(res))) + assert.Empty(t, errorMessages(res), "the rule reports warnings only") + }) + } +} + +// TestCollectionFormatKeepsSpecValid holds the rule to a warning: Swagger 2.0 says where +// collectionFormat applies without forbidding it elsewhere, so a specification that writes one on a +// string stays valid. +func TestCollectionFormatKeepsSpecValid(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(`{"name":"q","in":"query","type":"string","collectionFormat":"csv"}`, "") + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + require.NoError(t, Spec(d, strfmt.Default)) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + assert.SliceContainsT(t, errorMessages(warns), + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`) +} + +// TestCollectionFormatLocations pins down where the warning says it happened. +func TestCollectionFormatLocations(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec( + `{"name":"q","in":"query","type":"array","items":{"type":"string","collectionFormat":"ssv"}}`, + `"X":{"type":"string","collectionFormat":"csv"}`, + ) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + + pointers := pointersOf(warns.LocatedErrors()) + assert.SliceContainsT(t, pointers, "/paths/~1x/get/parameters/0/items/collectionFormat") + assert.SliceContainsT(t, pointers, "/paths/~1x/get/responses/200/headers/X/collectionFormat") +} + +// TestCollectionFormatThroughRef covers a parameter reached through a $ref: the walk sees it +// expanded, so the warning is reported just as it is for one written in place. +func TestCollectionFormatThroughRef(t *testing.T) { + t.Parallel() + + doc := `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "parameters": { + "Q": {"name": "q", "in": "query", "type": "string", "collectionFormat": "csv"} + }, + "paths": { + "/x": { + "get": { + "parameters": [{"$ref": "#/parameters/Q"}], + "responses": {"200": {"description": "ok"}} + } + } + } + }` + + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + assert.Equal(t, []string{ + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`, + }, warningMessages(res)) +} + +// TestCollectionFormatSkipsBodyParameters guards the one location the meta-schema handles on its +// own: a body parameter may not carry a collectionFormat at all, and that is an error rather than +// this warning. +func TestCollectionFormatSkipsBodyParameters(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(`{"name":"b","in":"body","schema":{"type":"string"}}`, "") + + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + assert.Empty(t, warningMessages(res)) +} + +// TestCollectionFormatIsNotASchemaKeyword holds the rule to what it walks. collectionFormat is a +// member of [spec.SimpleSchema], which a parameter, a header and an items carry; [spec.Schema] has +// none, so validating an ordinary JSON schema can never reach this rule. +// +// The guard is on the type, not on the walk: a compile-time field access is what would break if a +// later version of go-openapi/spec moved the member onto Schema. +func TestCollectionFormatIsNotASchemaKeyword(t *testing.T) { + t.Parallel() + + var ( + _ = spec.Items{}.CollectionFormat + _ = spec.Header{}.CollectionFormat + _ = spec.Parameter{}.CollectionFormat + ) + + raw, err := json.Marshal(spec.Schema{ + SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{stringType}}, + }) + require.NoError(t, err) + + var members map[string]any + require.NoError(t, json.Unmarshal(raw, &members)) + assert.MapNotContainsT(t, members, swaggerCollectionFormat) + + // and validating data against a schema reports nothing of the sort + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{"type": "string"}`), schema)) + require.NoError(t, AgainstSchema(schema, "a,b,c", strfmt.Default)) +} diff --git a/doc.go b/doc.go index 7937976..ae2a389 100644 --- a/doc.go +++ b/doc.go @@ -12,6 +12,11 @@ // Validates a spec document (from JSON or YAML) against the JSON schema for swagger, // then checks a number of extra rules that can't be expressed in JSON schema. // +// The lists below hold the extra rules only. The meta-schema already settles a great deal on its +// own, and where it does, no rule is repeated here: collectionFormat, say, must be one of csv, ssv, +// tsv or pipes, widened with multi for a query or formData parameter, and a body parameter may not +// carry one at all — all of that comes out of the meta-schema, at every location it applies to. +// // Entry points: // // - Spec() @@ -51,6 +56,7 @@ // [x] examples in response without schema // [x] readOnly properties should not be required // [x] an oauth2 security requirement names a scope its security scheme does not declare +// [x] collectionFormat is written on a parameter, header or items whose type is not array // // # Validating a schema // @@ -73,7 +79,6 @@ // [ ] errors and warnings are not reported with key/line number in spec // [ ] default values and examples on responses only support application/json producer type // [ ] invalid numeric constraints (such as Minimum, etc..) are not checked except for default and example values -// [ ] rules for collectionFormat are not implemented // [ ] a discriminator value is not checked against the schema names it may take [data, not document] // [ ] valid js ECMA regexp not supported by Go regexp engine are considered invalid // [ ] arbitrary large numbers are not supported: max is math.MaxFloat64 diff --git a/helpers.go b/helpers.go index b623be1..ea71436 100644 --- a/helpers.go +++ b/helpers.go @@ -51,13 +51,14 @@ const ( jsonAdditionalItems = "additionalItems" jsonAdditionalProperties = "additionalProperties" - swaggerPaths = "paths" - swaggerDefinitions = "definitions" - swaggerResponses = "responses" - swaggerParameters = "parameters" - swaggerHeaders = "headers" - swaggerOperationID = "operationId" - swaggerSecurity = "security" + swaggerPaths = "paths" + swaggerDefinitions = "definitions" + swaggerResponses = "responses" + swaggerParameters = "parameters" + swaggerHeaders = "headers" + swaggerOperationID = "operationId" + swaggerSecurity = "security" + swaggerCollectionFormat = "collectionFormat" // securitySchemeOAuth2 is the only security scheme type whose requirements carry scopes. securitySchemeOAuth2 = "oauth2" diff --git a/spec.go b/spec.go index ad9dc90..fea2bc0 100644 --- a/spec.go +++ b/spec.go @@ -10,7 +10,6 @@ import ( "fmt" "slices" "sort" - "strconv" "strings" "github.com/go-openapi/analysis" @@ -154,6 +153,7 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { errs.Merge(s.validateItems()) // error - errs.Merge(s.validateSecurityRequirements()) // error and warning errs.Merge(s.validateDiscriminators()) // error - + errs.Merge(s.validateCollectionFormats()) // warning only // Properties in required definition MUST validate their schema // Properties SHOULD NOT be declared as both required and readOnly (warning) @@ -459,26 +459,7 @@ func (s *SpecValidator) validateItems() *Result { } } - type codedResponse struct { - code string - resp spec.Response - } - var responses []codedResponse - if op.Responses != nil { - if op.Responses.Default != nil { - responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) - } - if op.Responses.StatusCodeResponses != nil { - for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { - responses = append(responses, codedResponse{ - code: strconv.Itoa(code), - resp: op.Responses.StatusCodeResponses[code], - }) - } - } - } - - for _, resp := range responses { + for _, resp := range responsesOf(op) { at := responsePath(path, method, resp.code) // Response headers with array for _, hn := range sortedKeys(resp.resp.Headers) { diff --git a/spec_messages.go b/spec_messages.go index 7dd58aa..541af4d 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -202,6 +202,12 @@ const ( // UnusedResponseWarning ... UnusedResponseWarning = "response %q is not used anywhere" + // CollectionFormatIgnoredWarning flags a collectionFormat on a parameter, header or items whose + // type is not array. collectionFormat says how to join the members of an array into one value, so + // it does nothing anywhere else. It is a warning, not an error: Swagger 2.0 defines the member as + // applying when the type is array, and does not forbid writing it elsewhere. + CollectionFormatIgnoredWarning = "collectionFormat %q is ignored in %s: it joins the members of an array, and the type is %q" + // DubiousAbsoluteRefWarning flags a $ref pointing to an absolute local file location that escapes the // spec's base path. Absolute local references are legitimate when they stay beneath the base path // (flattening/expansion introduces such anchors for cyclical $refs), but an absolute reference that @@ -439,6 +445,10 @@ func invalidObjectMsg(path, in string) errors.Error { // return errors.New(errors.CompositeErrorCode, InvalidResponseDefinitionAsSchemaError, path, method) // } +func collectionFormatIgnoredMsg(collectionFormat, in, typ string) errors.Error { + return errors.New(errors.CompositeErrorCode, CollectionFormatIgnoredWarning, collectionFormat, in, typ) +} + func discriminatorNotDefinedMsg(discriminator, in string) errors.Error { return errors.New(errors.CompositeErrorCode, DiscriminatorNotDefinedError, discriminator, in) } From b677c6f33f26e4478a44ac3f929be58d1b316734 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Mon, 24 Aug 2026 19:06:36 +0200 Subject: [PATCH 5/5] doc: mark the discriminator value check as out of scope The check sat in the "not yet supported" list, which reads as a backlog item. It is a decision: the clause constrains an instance rather than the document, so it moves out of the list into a sentence saying why. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- doc.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc.go b/doc.go index ae2a389..f2d02d1 100644 --- a/doc.go +++ b/doc.go @@ -79,7 +79,11 @@ // [ ] errors and warnings are not reported with key/line number in spec // [ ] default values and examples on responses only support application/json producer type // [ ] invalid numeric constraints (such as Minimum, etc..) are not checked except for default and example values -// [ ] a discriminator value is not checked against the schema names it may take [data, not document] // [ ] valid js ECMA regexp not supported by Go regexp engine are considered invalid // [ ] arbitrary large numbers are not supported: max is math.MaxFloat64 +// +// Left out by design, rather than pending: a discriminator value is not checked against the schema +// names it may take. That clause of the swagger rule constrains an instance rather than the +// document, so there is nothing in a specification for [SpecValidator] to read, and checking it +// would mean resolving polymorphic payloads, which this package does not do. package validate