openapi3: type the remaining bare-error validation sites#1187
Merged
fenollp merged 25 commits intoMay 21, 2026
Conversation
fenollp
requested changes
May 18, 2026
reuvenharrison
added a commit
to oasdiff/kin-openapi
that referenced
this pull request
May 18, 2026
The wrapper became a one-line passthrough after newUnresolvedRef was added. Drop it and update all 40+ call sites across schema.go, refs.go, and the refs.tmpl template to call the typed constructor directly. Per @fenollp review on getkin#1187. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five fmt.Errorf call sites in the openapi3 validators returned bare errors that consumers could only inspect by substring matching. Wrap each in a typed cluster so errors.As / errors.Is consumers can dispatch on the error kind and pull structured fields out: - PathParameterRequiredError (parameter.go) Param + Origin - DuplicateOperationIDError (paths.go) OperationID + Endpoint1/Endpoint2 - ExtraSiblingFieldsError (extension.go, refs) Fields []string - SchemaTypeError (schema.go) Type + Origin Error() strings are unchanged byte-for-byte (existing string-comparison consumers, apis_guru golden fixtures, validation_error_test assertions all pass without edits). The benefit is purely additive: callers gain the typed-cluster dispatch they already use for the RequiredFieldError / FieldVersionMismatchError / etc. clusters added in getkin#1183. Promised in getkin#1185's heads-up comment after running oasdiff validate across ~500 fixtures and finding these as the remaining 22% of findings that fell through to a downstream catchall.
…iblingFieldsError
Downstream consumers (oasdiff validate, IDE plugins, CI annotations)
need file:line:column to render these findings inline. Both error
types previously had no Origin field, so callers could only pin them
to a file. Adding Origin makes them consistent with the other typed
clusters (PathParameterRequiredError, SchemaTypeError) introduced
in this PR.
Changes:
* DuplicateOperationIDError gains Origin *Origin. Populated in
paths.go from operation.Origin of the second (offending) operation
that repeats an existing operationId.
* ExtraSiblingFieldsError gains Origin *Origin. Populated from the
parent object's Origin in two places:
- refs.tmpl (regenerated into refs.go): passes x.Origin from the
*Ref receiver. Covers 9 ref-side call sites (CallbackRef,
ExampleRef, HeaderRef, LinkRef, ParameterRef, RequestBodyRef,
ResponseRef, SchemaRef, SecuritySchemeRef).
- validateExtensions signature gains origin *Origin parameter,
threaded through to the constructor. The 28 callers across
openapi3 all pass their receiver's Origin (no new threading
needed since every type carrying Extensions already carries
Origin). The existing FIXME on validateExtensions notes the
function needs cleanup; this is a minimal step in that
direction without a broader refactor.
Backward-compatibility: both Origin fields are nil-safe (loaders
that don't set IncludeOrigin still emit valid errors). Error()
strings unchanged.
… ExtraSiblingFieldsError Mirrors the existing TestValidationError_OriginPopulatedOnLoaderTracking / TestValidationError_OriginNilWithoutLoaderTracking pair: for each new Origin-carrying cluster, one test asserts Origin is populated with a positive Line when the document was loaded with IncludeOrigin=true, and one asserts Origin is nil when the loader did not track origins. Pins the contract so future kin refactors can't silently drop the field.
Two more bare-error validation sites surfaced during corpus testing
across testdata/APIs-guru-openapi-directory (~4,000 specs); both
fall through to oasdiff's catchall today and are natural follow-ons
to the four typed clusters earlier in this PR.
* InvalidParameterInError clusters "parameter can't have 'in' value
X" — the spec accepts only {path, query, header, cookie}; this
fires when a parameter declares anything else (commonly `body`, a
Swagger 2.0 leftover that auto-generated specs leak).
* SchemaPatternRegexError clusters "schema pattern failed to
compile" — kin uses Go's RE2 which rejects Perl features like
`(?!...)` lookahead; specs that leak Perl patterns into JSON
Schema `pattern:` fields trip this. The cluster carries the
offending pattern and chains through to the original *SchemaError
via Unwrap so callers walking with errors.As(*SchemaError) still
match.
Both carry Origin when the loader tracks it, mirroring the pattern
established by the other typed clusters in this PR. Error() strings
unchanged for InvalidParameterIn; slightly enriched for
SchemaPatternRegex ("schema pattern X failed to compile: Y" wrapping
the legacy SchemaError message).
Tests in validation_error_test.go: typed dispatch + Origin
populated when IncludeOrigin=true + backward-compat unwrap to
*SchemaError for the SchemaPatternRegex case.
.github/docs/openapi3.txt regenerated.
…emaError
The previous commit changed the message format ('schema pattern X failed to compile: ...' prefix), which forced ~134 golden fixtures in testdata/apis_guru_openapi_directory/ to update.
The Pattern is already exposed as a structured field on the typed cluster; consumers who need it can pull it via errors.As without changing the rendered string. Reverting the message change keeps the PR purely additive on the rendering side and drops the golden churn.
Error() now delegates to Cause.Error(); Unwrap still returns Cause so errors.As(*SchemaError) chains correctly.
…UnresolvedRef sites Three more bare-error validation sites surfaced during corpus testing across testdata/APIs-guru-openapi-directory: the remaining 10 catchall hits after the earlier 6-cluster work cluster into just these three untyped sites. Including them here finishes the sweep intended by this PR. * InvalidSecuritySchemeTypeError clusters 'security scheme type can't be X' — the spec accepts only apiKey, http, oauth2, openIdConnect, mutualTLS; this fires when a scheme declares anything else (commonly 'cookie' from Swagger 2.0 leftovers). * InvalidHTTPSchemeError clusters 'security scheme of type http has invalid scheme value X' — the OpenAPI/HTTP-auth registry accepts only bearer, basic, negotiate, digest; this fires for custom values like 'mutual' or 'OAuth'. * UnresolvedRefError clusters 'found unresolved ref: X' fired by the validators when a Ref string is set but Value is nil (most commonly an external ref that didn't get fetched). To plumb Origin through, foundUnresolvedRef gained a third parameter (origin *Origin) and all 25 schema.go call sites plus the 9 refs.tmpl-generated sites pass their receiver's Origin. All three carry Origin when the loader tracks it. Error() strings unchanged byte-for-byte. Tests in validation_error_test.go: typed dispatch + Origin populated when IncludeOrigin=true for both security-scheme types. UnresolvedRef is constructed programmatically since the loader is strict about ref resolution at load time — the case being tested is the runtime-unresolved shape encountered in real-world specs with external refs that weren't fetched. .github/docs/openapi3.txt regenerated.
The CI fence 'git grep -E (fmt|errors)[^(]+\(.[A-Z]' searches the
whole tree for capitalized error strings, but the regex is naive
and trips on comment text that contains the literal pattern
'errors.As(*SchemaError)'. Reworded both comments to use prose
('callers using errors.As against the legacy *SchemaError') so the
fence no longer matches.
No functional change.
…idate
Completes the bare-error sweep on security_scheme.go after the three
sites added earlier in this PR (InvalidSecuritySchemeTypeError,
InvalidHTTPSchemeError, and the no-OIDC-URL case below). None of
these surfaced in the corpus top-10 catchall list (so they're rare
in the wild) but typing them lets oasdiff and other downstream
consumers dispatch every code path in this file under the same
errors.As shape.
Mapping:
* line 172 'no OIDC URL found for openIdConnect security scheme X'
-> RequiredFieldError wrapping OpenIDConnectURLRequired leaf,
Field 'openIdConnectUrl'.
* line 187 'apiKey should have in. It can be query, header or
cookie, not X' -> new typed cluster APIKeyInInvalidError mirroring
InvalidParameterInError's shape (single-value rejection, preserves
the multi-clause original message).
* lines 193, 195, 201, 214 (four 'type X can't have Y' cases) ->
ForbiddenFieldError wrapping SecuritySchemeInForbidden,
SecuritySchemeNameForbidden, SecuritySchemeBearerFormatForbidden,
SecuritySchemeFlowsForbidden leaves respectively.
* line 208 'type X should have flows' -> RequiredFieldError wrapping
SecuritySchemeFlowsRequired leaf, Field 'flows'.
The single remaining wrap at line 211 ('security scheme flow is
invalid: %w') is deliberately left as-is: the inner flow.Validate()
returns typed errors that the %w preserves via the chain, so
errors.As against any inner cluster still works. Replacing the
prefix with a typed context wrapper would mirror SectionValidationError
from getkin#1183 but is a structurally different change and is out of
scope here.
All seven sites carry Origin via the existing newRequiredField /
newForbiddenField constructors that thread ss.Origin through, or
directly for APIKeyInInvalidError. Error() strings byte-identical
to the original fmt.Errorf calls.
Tests cover all seven dispatches: the cluster + the leaf are both
reachable via errors.As, and the field name on RequiredFieldError /
ForbiddenFieldError matches the spec field name.
…ions
Sweep result: every fmt.Errorf without %w inside any Validate
function across the openapi3 package is now wrapped in a typed
cluster. The remaining fmt.Errorf calls inside Validate functions
are context wrappers using %w (e.g. 'parameter %q schema is
invalid: %w'); their inner errors are already typed so errors.As
walks the chain correctly. Wrapper-style sites are a separate
SectionValidationError-style refactor and out of scope here.
New typed clusters:
* PathMustStartWithSlashError (paths.go) — path keys must begin
with '/'.
* ConflictingPathsError (paths.go) — two paths normalize to the
same template.
* DuplicateParameterError (parameter.go) — two parameters share
(In, Name) on a single operation.
* InvalidSerializationMethodError (encoding.go, parameter.go,
header.go) — one cluster covers the three sites; the Subject
field discriminates ('media type' / 'path' / 'query' / 'header'
/ 'cookie').
Reused existing clusters with new leaves:
* parameter example/examples → MutuallyExclusiveFieldsError
wrapping ParameterExampleAndExamplesExclusive.
* server-variable default missing → RequiredFieldError wrapping
ServerVariableDefaultRequired.
All carry Origin when the loader tracks it. Error() strings
byte-identical to the originals.
Pre-existing bug fix (paths.go): the conflicting-paths check was
unreachable because the dedupe map was being keyed by the literal
path while the lookup used the normalized form. Changed the set
to use the normalized key so the typed cluster actually fires.
Verified the full corpus still passes after the fix.
Tests in validation_error_test.go cover all six dispatches and
the bug-fix path. fmt imports dropped via goimports from
encoding.go, paths.go, and server.go (no longer needed).
Completes the typed-error sweep started earlier in this PR by replacing the remaining bare fmt.Errorf-with-%w wrap sites with typed context wrappers. Mirrors the SectionValidationError / PathValidationError / OperationValidationError pattern from getkin#1183. After this commit, no fmt.Errorf calls remain inside any Validate function in the openapi3 package - every error path is either a typed leaf (cluster + leaf, per the earlier commits in this PR) or a typed context wrapper. New typed wrappers: * ComponentValidationError (Section, Name, Cause) - components.go * ExternalDocsURLValidationError (Cause) - external_docs.go * HeaderFieldValidationError (Field, Cause) - header.go (5 sites) * MediaTypeExampleValidationError (ExampleName, Cause) - media_type.go * WebhookValidationError (Name, Cause) - openapi3.go * ParameterFieldValidationError (ParameterName, Field, Cause) - parameter.go (5 sites) * ParameterExampleValidationError (ExampleName, Cause) - parameter.go (2 sites) * SecuritySchemeFlowValidationError (Cause) - security_scheme.go * OAuthFlowValidationError (FlowKind, Cause) - security_scheme.go (4 sites) * OAuthFlowFieldValidationError (Field, Cause) - security_scheme.go Each wrapper implements Error() preserving the original rendered message format byte-for-byte and Unwrap() returning Cause so errors.As chains reach inner typed leaves transparently. fmt imports removed from external_docs.go, header.go, media_type.go via goimports (no longer needed).
The 3-layer model (Base / Cluster / Leaf) at the top of validation_error.go did not mention context wrappers, even though getkin#1183 introduced them and this PR adds 10 more. A future contributor reading either file in isolation would miss how the families relate. * Extends the validation_error.go header to name the 4 categories, shows the canonical 3-link chain (context wrapper -> cluster -> leaf), and points at validation_error_context.go for the wrapper inventory. * Adds a file-level header to validation_error_context.go that defines what a context wrapper is, contrasts it with cluster and leaf (carries WHERE, not WHAT), shows the convention all wrappers share, and groups the inventory into document-wide vs narrow-scope wrappers. No code change; pure documentation.
Now redundant with the file-level header added in the previous commit (which groups the wrappers into document-wide vs narrow-scope and explains the convention). Replace the divider with a short pointer back to the file-level header so readers scrolling past the first three wrappers know where to look for the full picture.
The previous header claimed Error() formats as '<context>: <cause>' but no wrapper actually does that — each preserves the original fmt.Errorf-with-%w message format byte-for-byte (e.g. SectionValidationError renders 'invalid <section>: <cause>', HeaderFieldValidationError renders 'header <field> is invalid: <cause>'). Rewrite the convention section to honestly describe what is shared (discriminator fields + Cause + Unwrap) and what is not (Error() format).
'dispatch on' was vague Go-dev jargon; 'detect via errors.As' says the same thing concretely. Also fixes the broken grammar in the second clause.
The comment duplicated content from the SchemaPatternRegexError type docstring in validation_error.go. Removed; the type's own docstring remains the canonical explanation of why the wrap exists and how backward compat is preserved.
…or() Prior regen ran against a transiently-corrupted working tree where the receiver typo (*ExtraSiblingFieldsError -> *x) made go doc skip the method. Source was always correct in HEAD; only the generated snapshot was wrong. This regen runs against the correct source.
Adds *_CarriesOrigin and *_OriginNilWithoutLoaderTracking tests for the 11 clusters carrying an Origin field that were only covered by a baseline test. Mirrors the existing pattern (DuplicateOperationID, ExtraSiblingFields) so every Origin-carrying cluster now has all three checks: baseline dispatch, loader-tracked Origin populated, default loader's Origin is nil. 20 new tests added; all green. A loadDocFromYAMLWithOrigin helper is added next to loadDocFromYAML so the pattern stays a single line at each call site.
Previous version loaded a YAML doc with a bad encoding style and called doc.Validate(), expecting the encoding validation to fire. But MediaType.Validate does not call Encoding.Validate, so the encoding path is never reached from the document-level Validate. Switched to direct enc.Validate() with a populated Origin, matching the pattern already used for the OriginNilWithoutLoaderTracking sibling test.
The wrapper became a one-line passthrough after newUnresolvedRef was added. Drop it and update all 40+ call sites across schema.go, refs.go, and the refs.tmpl template to call the typed constructor directly. Per @fenollp review on getkin#1187. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
reuvenharrison
force-pushed
the
feat/type-remaining-bare-error-sites
branch
from
4148d86 to
465bf0f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1185, as promised in this heads-up comment. Testing across the ~4,000 real-world specs in
testdata/after #1185 merged showed a substantial portion of findings falling through to a catchall and traced back to barefmt.Errorfcall sites in the validators that returned errors consumers could only inspect by substring matching.Scope: every
fmt.Errorfcall inside everyValidatefunction in the openapi3 package, both bare leaves and context wrappers, is now wrapped in a typed cluster, typed leaf, or typed context wrapper. Consumers can dispatch on every error path viaerrors.Asand extract the relevant context (which field, which section, which parameter, which OAuth flow, etc.) without parsing rendered messages.New typed clusters
PathParameterRequiredErrorparameter.goParam string,Origin *OriginDuplicateOperationIDErrorpaths.goOperationID string,Endpoint1 string,Endpoint2 string,Origin *OriginExtraSiblingFieldsErrorextension.go+ every*Ref.validateExtras(viarefs.tmpl)Fields []string,Origin *OriginSchemaTypeErrorschema.goType string,Origin *OriginInvalidParameterInErrorparameter.goValue string,Origin *OriginSchemaPatternRegexErrorschema_pattern.go(RE2 rejecting Perl features)Pattern string,Cause error,Origin *OriginInvalidSecuritySchemeTypeErrorsecurity_scheme.goType string,Origin *OriginInvalidHTTPSchemeErrorsecurity_scheme.goScheme string,Origin *OriginUnresolvedRefErrorrefs.tmpl-generatedrefs.go(9 sites) +schema.go(25 sites)Ref string,Origin *OriginAPIKeyInInvalidErrorsecurity_scheme.goValue string,Origin *OriginPathMustStartWithSlashErrorpaths.goPath string,Origin *OriginConflictingPathsErrorpaths.goPath1 string,Path2 string,Origin *OriginDuplicateParameterErrorparameter.goIn string,Name string,Origin *OriginInvalidSerializationMethodErrorencoding.go+parameter.go+header.go(one cluster, three sites)Subject string,Style string,Explode bool,Origin *OriginLeaf reuses of existing typed clusters
security_scheme.go:RequiredFieldErrorforOpenIDConnectURLRequiredandSecuritySchemeFlowsRequired;ForbiddenFieldErrorforSecuritySchemeInForbidden,SecuritySchemeNameForbidden,SecuritySchemeBearerFormatForbidden,SecuritySchemeFlowsForbidden.parameter.go:MutuallyExclusiveFieldsErrorforParameterExampleAndExamplesExclusive.server.go:RequiredFieldErrorforServerVariableDefaultRequired.New typed context wrappers
Mirrors the SectionValidationError / PathValidationError / OperationValidationError pattern from #1183. Each wraps a previously-bare
fmt.Errorfcontext wrap so the surrounding context is dispatchable viaerrors.As, and Unwrap chains to the inner typed leaf.ComponentValidationErrorcomponents.goSection string,Name string,Cause errorExternalDocsURLValidationErrorexternal_docs.goCause errorHeaderFieldValidationErrorheader.go(5 sites)Field string,Cause errorMediaTypeExampleValidationErrormedia_type.goExampleName string,Cause errorWebhookValidationErroropenapi3.goName string,Cause errorParameterFieldValidationErrorparameter.go(5 sites)ParameterName string,Field string,Cause errorParameterExampleValidationErrorparameter.go(2 sites)ExampleName string,Cause errorSecuritySchemeFlowValidationErrorsecurity_scheme.goCause errorOAuthFlowValidationErrorsecurity_scheme.go(4 sites)FlowKind string,Cause errorOAuthFlowFieldValidationErrorsecurity_scheme.goField string,Cause errorOrigin tracking
Every leaf cluster carries an
Origin *Originfield populated when the document was loaded withLoader.IncludeOrigin = true. To make this possible:validateExtensionsgained anorigin *Originparameter; all 28 callers pass their receiver'sOrigin.newUnresolvedRefis the typed constructor forUnresolvedRefError; all 34 call sites (9 inrefs.tmpl-generatedrefs.go+ 25 inschema.go) call it directly with their ref-receiver'sOrigin.refs.tmplupdated;refs.goregenerated.SchemaPatternRegexErrorchains through to the original*SchemaErrorviaUnwrapso callers usingerrors.Asagainst the legacy*SchemaErrorstill match.Compatibility
Error()strings are byte-identical for every cluster and every context wrapper. Existing golden fixtures pass unchanged.Tests
validation_error_test.gocovers, for each cluster and wrapper: typed-dispatch viaerrors.As, populated identifying fields, Origin populated when the loader tracks it (and nil otherwise), and for context wrappers, that the chain reaches the inner typed leaf viaUnwrap.