libopenapi-validator icon indicating copy to clipboard operation
libopenapi-validator copied to clipboard

Improved Error Reporting Consistency - SchemaValidationFailures are consistently returned, and error mapping information is consistently mapped

Open mjbonifacio opened this issue 3 months ago • 1 comments

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:

  1. Pre-validation Error Cleanup
  2. JSON Pointer Helpers
  3. Parameter Schema Context
  4. Unify Context and Centralize

Overview

Continues on the work in https://github.com/pb33f/libopenapi-validator/pull/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:

  • Where did validation fail?
  • What failed (i.e. field/param)?
  • What is the path to where validation failed (if applicable)?
  • a human readable error message (already exists)

All locations are noted from a keyword or constraint location in an OpenAPI schema.

Major Changes

  1. Inconsistent SchemaValidationFailure usage: Pre-validation errors (JSON decode failures, schema compilation errors) included SchemaValidationFailure objects, while actual schema violations (on specific parameters etc.) sometimes lacked them.

  2. Incomplete location information: KeywordLocation was 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.

  3. Inconsistent field population: Critical fields like ReferenceSchema, Context, and KeywordLocation were missing or inconsistent across different validation types.

  4. Redundant/unused fields: AbsoluteKeywordLocation was always empty due to schema inlining, and the Location field 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

  • Pre-validation errors (schema compilation, JSON decode, missing schemas) → NO SchemaValidationFailure
  • Schema constraint violations (type mismatches, enum violations, range errors) → ALWAYS include SchemaValidationFailure with complete context

2. Complete OpenAPI-Aware Location Information

  • KeywordLocation contains full JSON Pointer path from OpenAPI document root to the exact schema keyword
  • Format: /paths/{escaped-path}/{operation}/parameters/{name}/schema/{keyword}
  • Example: /paths/~1users~1{id}/get/parameters/id/schema/minimum
  • RFC 6901 compliant escaping (~~0, /~1)

3. Consistent Field Population

  • ReferenceSchema: Rendered schema as JSON string (for human consumption)
  • Context: Raw *base.Schema object (for programmatic access)
  • KeywordLocation: Full OpenAPI path to failed schema keyword
  • FieldName, FieldPath, InstancePath: Consistently populated across all validation types

4. Cleanup

  • Removed Location field: Ambiguous and superseded by KeywordLocation + FieldPath
  • Removed AbsoluteKeywordLocation: Never populated due to schema inlining

Benefits

API consumers can now:

  1. ✅ Distinguish between pre-validation failures and actual schema violations
  2. ✅ Programmatically locate the exact schema keyword violated in their OpenAPI spec
  3. ✅ Access both human-readable (ReferenceSchema) and programmatic (Context) schema representations
  4. ✅ Build better error messages and debugging tools
  5. ✅ Implement automated spec correction or validation guidance

Breaking Changes

⚠️ Removed Field: Location

The deprecated Location field has been completely removed from SchemaValidationFailure.

Migration Guide:

  • If you were using Location for the schema location → use KeywordLocation
  • If you were using Location for the instance location → use FieldPath

Example:

// Before
if err.SchemaValidationErrors[0].Location == "/properties/email/format" {
    // schema location usage
}

// After  
if err.SchemaValidationErrors[0].KeywordLocation == "/properties/email/format" {
    // schema location usage - more explicit
}

// OR
if err.SchemaValidationErrors[0].FieldPath == "$.email" {
    // instance location usage - use FieldPath instead
}

⚠️ Removed Field: AbsoluteKeywordLocation

This field was always empty because RenderInline() resolves all $ref references before validation.

Validation Types Covered

  • ✅ Request body validation
  • ✅ Response body validation
  • ✅ Response header validation
  • ✅ Path parameter validation (8 error functions)
  • ✅ Query parameter validation (14 error functions)
  • ✅ Header parameter validation (7 error functions)
  • ✅ Cookie parameter validation (6 error functions)
  • ✅ Schema/document validation

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:

  1. PR 1: Pre-validation Error Cleanup

    • Distinguishes pre-validation errors from schema constraint violations
    • Removes AbsoluteKeywordLocation field
    • 6 commits covering schema, document, parameter, request, and response validation
  2. PR 2: JSON Pointer Helpers

    • Introduces centralized RFC 6901 JSON Pointer construction helpers
    • Demonstrates pattern with response header validation
    • 2 commits
  3. PR 3: Parameter Schema Context

    • Adds full OpenAPI context to all parameter errors (path, query, header, cookie)
    • 4 commits (one per parameter type)
  4. PR 4: Unify Context and Centralize

    • Removes deprecated Location field
    • Consolidates all JSON Pointer construction to use helpers (72+ locations)
    • Unifies Context field to use *base.Schema consistently
    • 2 commits

Each PR includes:

  • Detailed rationale for changes
  • Tables showing before/after for each validation type
  • Examples of the improvements

Testing

  • ✅ All existing tests updated to reflect new error structure
  • ✅ Schema compilation error tests correctly expect NO SchemaValidationFailure
  • ✅ Schema constraint violation tests correctly expect complete SchemaValidationFailure
  • KeywordLocation assertions updated to expect full OpenAPI paths
  • ✅ No linter errors

Scope of Changes (14 commits total)

The changeset touches error reporting across the entire library:

  • errors/validation_error.go - Struct definitions
  • errors/parameter_errors.go - 35 parameter error functions updated
  • helpers/json_pointer.go - New centralized JSON Pointer helpers
  • parameters/*.go - All parameter validation (path, query, header, cookie)
  • requests/validate_request.go - Request body validation
  • responses/*.go - Response body and header validation
  • schema_validation/*.go - Schema and document validation
  • All corresponding test files

Commit Phases

Phase 1: Pre-validation Error Cleanup (6 commits)

  1. Remove SchemaValidationFailure from schema pre-validation errors
  2. Remove SchemaValidationFailure from document compilation errors
  3. Parameters: add KeywordLocation when formatting JSON schema errors, remove SchemaValidationFailure when compilation fails
  4. Remove AbsoluteKeywordLocation field - never populated due to schema inlining
  5. Request body validation: remove SchemaValidationFailure from pre-validation errors
  6. Response body validation: remove SchemaValidationFailure from pre-validation errors

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:

  1. Start with PR #4 to understand the conceptual changes
  2. Review PR #5 to see the helper function approach
  3. Review PR #6 to see the pattern applied to parameters
  4. Review PR #7 to see the final cleanup and consolidation

Each PR builds on the previous one and can be reviewed independently for logic and correctness.

Requested blessing

  1. AbsoluteKeywordLocation removal: This field was always empty due to RenderInline() resolving refs. Is this observation correct, or are there use cases where this field should be populated?

mjbonifacio avatar Nov 15 '25 21:11 mjbonifacio

Codecov Report

:x: Patch coverage is 99.05363% with 6 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 97.50%. Comparing base (bde0446) to head (d750797).

Files with missing lines Patch % Lines
parameters/query_parameters.go 87.50% 3 Missing and 1 partial :warning:
errors/validation_error.go 66.66% 1 Missing :warning:
parameters/path_parameters.go 97.22% 1 Missing :warning:
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     
Flag Coverage Δ
unittests 97.50% <99.05%> (+0.09%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Nov 15 '25 21:11 codecov[bot]

I am going to have to spend some time on this, it's a lot to read and grok.

daveshanley avatar Dec 01 '25 14:12 daveshanley

I am going to have to spend some time on this, it's a lot to read and grok.

For some context, we're just trying to make it so the validation errors raised by the library follow a pattern that we can code against. At the moment, there's no ErrorCode returned that we can switch-case on so we're left with doing weird things like using a regex on the Reason field then branching logic based on that. The reason we have to do that is depending where the error came from, some fields may or may not be populated so it's a confusing experience.

What we're trying to do is catalog the errors into two groups then as long as it fits into one of those groups you'll always get the same fields hydrated.

  1. ValidationError (root) - Errors unrelated to the schema like missing request body, invalid response code, etc.
  2. SchemaValidationError - Errors related to JSONSchema (or OpenAPI schema, specifically for parameters).

its-hammer-time avatar Dec 01 '25 20:12 its-hammer-time

I am going to have to spend some time on this, it's a lot to read and grok.

For some context, we're just trying to make it so the validation errors raised by the library follow a pattern that we can code against. At the moment, there's no ErrorCode returned that we can switch-case on so we're left with doing weird things like using a regex on the Reason field then branching logic based on that. The reason we have to do that is depending where the error came from, some fields may or may not be populated so it's a confusing experience.

What we're trying to do is catalog the errors into two groups then as long as it fits into one of those groups you'll always get the same fields hydrated.

  1. ValidationError (root) - Errors unrelated to the schema like missing request body, invalid response code, etc.
  2. SchemaValidationError - Errors related to JSONSchema (or OpenAPI schema, specifically for parameters).

Yes, I agree with the notion and the intent. I just have to spend some time reading this slowly. Speed is not our friend with situations like this .

daveshanley avatar Dec 02 '25 11:12 daveshanley