-
Notifications
You must be signed in to change notification settings - Fork 38
Improved Error Reporting Consistency - SchemaValidationFailures are consistently returned, and error mapping information is consistently mapped #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mjbonifacio
wants to merge
27
commits into
pb33f:main
Choose a base branch
from
mjbonifacio:unify-context-and-centralize
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
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
…emove SchemaValidationFailure from when json schema compilation fails
…inlining AbsoluteKeywordLocation was never populated because libopenapi's RenderInline() method resolves and inlines all $ref references before schemas reach the JSON Schema validator. Since the validator never encounters $ref pointers, this field remained empty in all cases and served no purpose. Removed from: - SchemaValidationFailure struct definition - All instantiation sites (schema_validation, parameters, requests) - Improved KeywordLocation documentation with JSON Pointer reference
…dation errors, add KeywordLocation to schema violations Pre-validation errors (compilation, JSON decode, empty body) now correctly omit SchemaValidationFailure objects, as they don't represent actual schema constraint violations. Actual schema violations now include KeywordLocation (JSON Pointer path to the failing keyword) for better error context. Also fixed Location field to use er.InstanceLocation for consistency with schema_validation/validate_schema.go.
…idation errors, add KeywordLocation to schema violations Pre-validation errors (compilation, missing response, IO read, JSON decode) now correctly omit SchemaValidationFailure objects, as they don't represent actual schema constraint violations. Actual schema violations now include KeywordLocation (JSON Pointer path to the failing keyword) for better error context. Also fixed Location field to use er.InstanceLocation for consistency with request validation and schema validation.
Creates helper functions in helpers package for constructing RFC 6901-compliant JSON Pointer paths to OpenAPI specification locations. New functions: - EscapeJSONPointerSegment: Escapes ~ and / characters per RFC 6901 - ConstructParameterJSONPointer: Builds paths for parameter schemas - ConstructResponseHeaderJSONPointer: Builds paths for response headers This eliminates duplication of the escaping logic across 72+ locations in the codebase and provides a single source of truth for JSON Pointer construction. Pattern: Before: Manual escaping in each error function (3 lines of code each) After: Single function call with semantic naming Next step: Refactor all existing inline JSON Pointer construction to use these helpers.
When a response header is marked as required in the OpenAPI schema and is missing from the response, this is a schema constraint violation. Added SchemaValidationFailure with full OpenAPI path context for KeywordLocation. Updated ValidateResponseHeaders signature to accept pathTemplate and statusCode to construct full JSON Pointer paths like: /paths/~1health/get/responses/200/headers/chicken-nuggets/required This makes header validation consistent with request/response body validation, which also uses full OpenAPI document paths for KeywordLocation. Note: Considered using relative paths (/header-name/required) but chose full paths for consistency with body validation patterns. Both approaches have tradeoffs documented in PR description.
…r ReferenceSchema
- Render parameter schema once in path_parameters.go instead of in each error function
- Pass renderedSchema to all 8 path parameter error functions (bool, enum, integer, number, array variants)
- Update Context field to use raw base.Schema (programmatic access)
- Update ReferenceSchema field to use rendered JSON string (API consumers)
- Use full OpenAPI JSON Pointer paths for KeywordLocation (e.g., /paths/~1users~1{id}/parameters/id/schema/type)
- Serialize full schema objects for ReferenceSchema instead of just type strings
- Update resolveNumber and resolveInteger helpers to accept and pass renderedSchema
Note: This approach (Context=raw schema, ReferenceSchema=rendered string) will be reviewed later for consistency across the codebase
Changes: - Remove 'Build full OpenAPI path for KeywordLocation' comments - Remove inline comments from previous commits - Add renderedSchema parameter to QueryParameterMissing - Set ReferenceSchema field in QueryParameterMissing SchemaValidationFailure - Render schema for missing required parameters before creating error - Update tests to pass renderedSchema parameter
Adds full OpenAPI context to all 7 header parameter error functions:
- HeaderParameterMissing
- HeaderParameterCannotBeDecoded (now includes SchemaValidationFailure)
- IncorrectHeaderParamEnum
- InvalidHeaderParamInteger
- InvalidHeaderParamNumber
- IncorrectHeaderParamBool
- IncorrectHeaderParamArrayBoolean
- IncorrectHeaderParamArrayNumber
All errors now include:
- KeywordLocation: Full JSON Pointer from OpenAPI root (e.g., /paths/{path}/{method}/parameters/{name}/schema/type)
- ReferenceSchema: Rendered schema as JSON string
- Context: Raw base.Schema object
- Proper FieldName and InstancePath
Updated ValidateHeaderArray to accept and pass path/operation/schema context.
Updated all test cases to pass new required parameters.
Adds full OpenAPI context to all 6 cookie parameter error functions:
- InvalidCookieParamInteger
- InvalidCookieParamNumber
- IncorrectCookieParamBool
- IncorrectCookieParamEnum
- IncorrectCookieParamArrayBoolean
- IncorrectCookieParamArrayNumber
All errors now include:
- KeywordLocation: Full JSON Pointer from OpenAPI root (e.g., /paths/{path}/{method}/parameters/{name}/schema/type)
- ReferenceSchema: Rendered schema as JSON string
- Context: Raw base.Schema object
- Proper FieldName and InstancePath
Updated ValidateCookieArray to accept and pass path/operation/schema context.
Updated all test cases to pass new required parameters.
This completes consistent SchemaValidationFailure population across all parameter types (path, query, header, cookie).
Removed the deprecated Location field entirely from SchemaValidationFailure struct and updated all code and tests to use KeywordLocation instead. Changes: - Removed Location field from SchemaValidationFailure struct - Updated Error() method to use FieldPath instead of Location - Removed .Location assignment in schema_validation/validate_document.go - Updated all test assertions to use KeywordLocation instead of Location - Updated tests to reflect that schema compilation errors do not have SchemaValidationFailure objects (they were removed in earlier commits) This completes the transition to the new error reporting model where: - KeywordLocation: Full JSON Pointer to schema keyword (for all schema violations) - FieldPath: JSONPath to the failing instance (for body validation) - InstancePath: Structured path segments to failing instance - Location field: Removed entirely
Replaces all manual JSON Pointer construction with calls to the new helper
functions, eliminating 72+ instances of duplicated escaping logic.
Changes:
- errors/parameter_errors.go: Replaced all manual escaping with
helpers.ConstructParameterJSONPointer() calls
- All 35 parameter error functions now use helper
- Handles type, enum, items/type, items/enum, maxItems, minItems, uniqueItems
- responses/validate_headers.go: Replaced manual escaping with
helpers.ConstructResponseHeaderJSONPointer()
- errors/validation_error_test.go: Updated tests to use FieldPath instead of
deprecated Location field
Benefits:
- Single source of truth for JSON Pointer construction
- Reduced code duplication (3 lines → 1 line per usage)
- More maintainable and less error-prone
- Semantic function names make intent clearer
Each function call reduced from:
escapedPath := strings.ReplaceAll(pathTemplate, "~", "~0")
escapedPath = strings.ReplaceAll(escapedPath, "/", "~1")
escapedPath = strings.TrimPrefix(escapedPath, "~1")
keywordLocation := fmt.Sprintf("/paths/%s/%s/...", escapedPath, ...)
To:
keywordLocation := helpers.ConstructParameterJSONPointer(path, method, param, keyword)
- Added missing files from upstream: property_locator.go, xml_validator.go, etc. - Fixed Location field references (removed in our changes) - Fixed enrichSchemaValidationFailure signature to remove location parameter - Added OriginalError field back for backwards compatibility - Fixed XML validation API compatibility with goxml2json - Added missing dependency: github.com/basgys/goxml2json This commit prepares for merging origin/main into our branch.
Resolved conflicts by keeping our versions with Location field removed and XML API fixed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
==========================================
+ Coverage 97.41% 97.50% +0.09%
==========================================
Files 45 46 +1
Lines 3987 4332 +345
==========================================
+ Hits 3884 4224 +340
- Misses 103 107 +4
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Update goxml2json to v1.1.1-0.20231018121955-e66ee54ceaad (matches upstream) - Restore WithTypeConverter call in validate_xml.go for numeric type conversion - Update property_locator_test.go to use OriginalJsonSchemaError (our convention)
7a314ad to
fabe24d
Compare
- Remove extra blank lines (gofumpt) - Remove redundant nil checks before len() (staticcheck)
- Keep only OriginalJsonSchemaError (our convention) - OriginalError was mistakenly re-added during upstream merge
- Updated path parameters to use ValidateSingleParameterSchemaWithPath - Updated header parameters to use ValidateSingleParameterSchemaWithPath - Now all parameter types (query, path, header, cookie) get full OpenAPI context - Ensures KeywordLocation is consistent across all parameter validation
831984f to
6013125
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
TL;DR
Review these 4 PRs in order in my fork for smaller chunks, but add discussion here so that they can live throughout the project's history:
Overview
Continues on the work in #188 to fully improve on validation error reporting.
This PR aims to make validation error reporting fully consistent so that library users can fully answer the following questions in 4xx API responses to client requests:
All locations are noted from a keyword or constraint location in an OpenAPI schema.
Major Changes
Inconsistent
SchemaValidationFailureusage: Pre-validation errors (JSON decode failures, schema compilation errors) includedSchemaValidationFailureobjects, while actual schema violations (on specific parameters etc.) sometimes lacked them.Incomplete location information:
KeywordLocationwas often empty or contained relative paths instead of absolute RFC 6901 JSON Pointer paths from the OpenAPI document root, making it hard to identify which part of the OpenAPI spec was violated.Inconsistent field population: Critical fields like
ReferenceSchema,Context, andKeywordLocationwere missing or inconsistent across different validation types.Redundant/unused fields:
AbsoluteKeywordLocationwas always empty due to schema inlining, and theLocationfield grew to be used in different ways over the life of the repo.Solution
This PR establishes clear patterns for error reporting:
1. Clear Error Type Separation
SchemaValidationFailureSchemaValidationFailurewith complete context2. Complete OpenAPI-Aware Location Information
KeywordLocationcontains full JSON Pointer path from OpenAPI document root to the exact schema keyword/paths/{escaped-path}/{operation}/parameters/{name}/schema/{keyword}/paths/~1users~1{id}/get/parameters/id/schema/minimum~→~0,/→~1)3. Consistent Field Population
ReferenceSchema: Rendered schema as JSON string (for human consumption)Context: Raw*base.Schemaobject (for programmatic access)KeywordLocation: Full OpenAPI path to failed schema keywordFieldName,FieldPath,InstancePath: Consistently populated across all validation types4. Cleanup
Locationfield: Ambiguous and superseded byKeywordLocation+FieldPathAbsoluteKeywordLocation: Never populated due to schema inliningBenefits
API consumers can now:
ReferenceSchema) and programmatic (Context) schema representationsBreaking Changes
LocationThe deprecated
Locationfield has been completely removed fromSchemaValidationFailure.Migration Guide:
Locationfor the schema location → useKeywordLocationLocationfor the instance location → useFieldPathExample:
AbsoluteKeywordLocationThis field was always empty because
RenderInline()resolves all$refreferences before validation.Validation Types Covered
Detailed Breakdown
For easier review, this changeset has been broken down into 4 stacked PRs in my fork. Each PR focuses on a specific aspect:
PR 1: Pre-validation Error Cleanup
AbsoluteKeywordLocationfieldPR 2: JSON Pointer Helpers
PR 3: Parameter Schema Context
PR 4: Unify Context and Centralize
LocationfieldContextfield to use*base.SchemaconsistentlyEach PR includes:
Testing
SchemaValidationFailureSchemaValidationFailureKeywordLocationassertions updated to expect full OpenAPI pathsScope of Changes (14 commits total)
The changeset touches error reporting across the entire library:
errors/validation_error.go- Struct definitionserrors/parameter_errors.go- 35 parameter error functions updatedhelpers/json_pointer.go- New centralized JSON Pointer helpersparameters/*.go- All parameter validation (path, query, header, cookie)requests/validate_request.go- Request body validationresponses/*.go- Response body and header validationschema_validation/*.go- Schema and document validationCommit Phases
Phase 1: Pre-validation Error Cleanup (6 commits)
Phase 2: Centralized JSON Pointer Helpers (2 commits)
7. Add centralized JSON Pointer construction helpers
8. Response headers: add SchemaValidationFailure with full OpenAPI path (using helpers)
Phase 3: Parameter Schema Context (4 commits)
9. Path parameters: render schema once, pass to error functions
10. Query parameters: add full OpenAPI context + missing required param fix
11. Header parameters: add full OpenAPI context
12. Cookie parameters: add full OpenAPI context
Phase 4: Unify and Centralize (2 commits)
13. Remove deprecated Location field from SchemaValidationFailure (+ Context field unification)
14. Refactor: use centralized JSON Pointer helpers across codebase (72+ locations)
Review Notes
📋 For easier review, I recommend reviewing the stacked PRs in my fork sequentially:
Each PR builds on the previous one and can be reviewed independently for logic and correctness.
Requested blessing
AbsoluteKeywordLocationremoval: This field was always empty due toRenderInline()resolving refs. Is this observation correct, or are there use cases where this field should be populated?