From c3479d927f1c8a3efbb6e8cc4f9aa8e318f79a03 Mon Sep 17 00:00:00 2001 From: wadakatu Date: Mon, 18 May 2026 18:13:28 +0900 Subject: [PATCH 1/2] fix(validation): surface a skip for non-JSON content types that declare a schema (#254) When a non-JSON Content-Type matched a spec media-type key and that entry carried a `schema`, the body could not be validated by the JSON Schema engine, yet the run was still recorded as a clean success. Surface this as `Skipped` with a skip reason on both the request and response sides so the gap is visible. Non-JSON entries without a `schema` continue to succeed silently as before. --- docs/api-reference.md | 2 +- docs/setup.md | 2 +- docs/supported-features.md | 2 +- src/OpenApiRequestValidator.php | 66 +++++++- src/OpenApiResponseValidator.php | 16 ++ src/OpenApiValidationResult.php | 15 +- .../Request/RequestBodyValidationResult.php | 39 +++++ .../Request/RequestBodyValidator.php | 78 +++++---- .../Response/ResponseBodyValidationResult.php | 9 ++ .../Response/ResponseBodyValidator.php | 21 +++ tests/Integration/ResponseValidationTest.php | 30 +++- tests/Unit/OpenApiRequestValidatorTest.php | 57 ++++++- tests/Unit/OpenApiResponseValidatorTest.php | 56 ++++++- .../Request/RequestBodyValidatorTest.php | 153 ++++++++++++------ .../Response/ResponseBodyValidatorTest.php | 38 ++++- .../specs/non-json-content-schema.json | 69 ++++++++ 16 files changed, 563 insertions(+), 90 deletions(-) create mode 100644 src/Validation/Request/RequestBodyValidationResult.php create mode 100644 tests/fixtures/specs/non-json-content-schema.json diff --git a/docs/api-reference.md b/docs/api-reference.md index 5477801..1d7208c 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -10,7 +10,7 @@ Main validator class. Validates a response body against the spec. The constructor accepts a `maxErrors` parameter (default: `20`) that limits how many validation errors the underlying JSON Schema validator collects. Use `0` for unlimited, `1` to stop at the first error. -The optional `responseContentType` parameter enables content negotiation: when provided, non-JSON content types (e.g., `text/html`) are checked for spec presence only, while JSON-compatible types proceed to full schema validation. +The optional `responseContentType` parameter enables content negotiation: when provided, non-JSON content types (e.g., `text/html`) are checked for spec presence only, while JSON-compatible types proceed to full schema validation. When a non-JSON content type matches a spec media-type key that declares a `schema`, the body cannot be evaluated by the JSON Schema engine — the result is reported as `Skipped` (with a `skipReason`) rather than a clean success, so the unvalidated body is not miscounted. ```php $validator = new OpenApiResponseValidator(maxErrors: 20); diff --git a/docs/setup.md b/docs/setup.md index 2e858c9..495c1a6 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -338,7 +338,7 @@ Notes: - Patterns are regex strings **without** `/` delimiters or `^$` anchors; they are anchored automatically, so `5\d\d` matches exactly `500`–`599` (not `5000`). - The skip check sits **between** the "path / method not in spec" checks and the "status code not defined" / schema-validation checks. A skipped code therefore suppresses both status-code failure modes (undocumented code AND body mismatch for a documented code), but typos in the request path or method still fail loudly. -- Skipped endpoints count as covered — the endpoint was exercised, just not schema-validated. Coverage semantics here match how non-JSON content types and schema-less `204` responses are handled, but `OpenApiValidationResult::isSkipped()` returns `true` **only** for status-code skips; the other no-body-validation branches still return a plain `success()`. +- Skipped endpoints count as covered — the endpoint was exercised, just not schema-validated. Coverage semantics here match how non-JSON content types and schema-less `204` responses are handled. `OpenApiValidationResult::isSkipped()` returns `true` for status-code skips **and** for responses/requests whose body could not be schema-validated because the spec declared it under a non-JSON content type (with a `schema` the JSON Schema engine cannot evaluate). A schema-less `204` and a non-JSON content type with no `schema` have nothing to validate and still return a plain `success()`. - `OpenApiValidationResult::isSkipped()` is exposed for callers who want to distinguish a skip from a genuine success. `skipReason()` identifies the matched pattern. `outcome()` returns an `OpenApiValidationOutcome` enum (`Success` / `Failure` / `Skipped`) for callers who want exhaustive `match` handling instead of two bool predicates. - **Observability trade-off**: a real regression that causes an unrelated `500` will not fail this assertion. Keep your HTTP-level assertions (`$response->assertOk()`, status-code expectations in the test) alongside the contract check so a stray 5xx still surfaces — the contract assertion alone is not a substitute for status-code assertions on happy paths. - **Coverage signal**: skipped responses surface as their own row inside each endpoint's response table — `⚠` (`:warning:` in Markdown) on the per-`(status, content-type)` line, with the matched skip pattern shown inline. The endpoint marker becomes `◐` (partial) when other responses are still validated, or stays `✓` only when every declared response is covered. The response-level rate (`responseCovered / responseTotal`) excludes skipped definitions, so a happy-path regression that silently returns `500` in every test no longer hides behind a 100% endpoint count. `skipReason()` is available on each `OpenApiValidationResult` for callers who want to log the matched pattern from a custom renderer. diff --git a/docs/supported-features.md b/docs/supported-features.md index 6477f1e..9e62baf 100644 --- a/docs/supported-features.md +++ b/docs/supported-features.md @@ -37,7 +37,7 @@ Detection looks at each property schema's own top-level `readOnly` / `writeOnly` ## Body validation - **Validated**: `application/json` and any `+json` structured-syntax suffix (RFC 6838), and content keys using ranges (`application/*`, `*/*`) — the matcher tries exact match first, then `/*`, then `*/*`. - **Multi-JSON-per-status specs** (e.g. `application/json` + `application/problem+json` for the same status): when the actual response Content-Type is supplied, schema validation prefers the spec key that exactly matches the response Content-Type before falling back to the first JSON key. A problem-details body served as `application/problem+json` is judged against its own schema, not the success-shape `application/json` schema. Vendor `+json` suffixes the spec doesn't enumerate (e.g. `application/vnd.example.v1+json`) still fall through to the first JSON key, preserving the legacy interchangeable-JSON behaviour for that case. -- **Presence-only** (no schema validation): every other media type, including `application/xml`, `multipart/form-data`, `application/x-www-form-urlencoded`, `text/plain`, and `application/octet-stream`. The validator confirms the spec declares the content type but does not check the body. The orchestrator marks these responses as `Skipped` for coverage reporting. +- **Presence-only** (no schema validation): every other media type, including `application/xml`, `multipart/form-data`, `application/x-www-form-urlencoded`, `text/plain`, and `application/octet-stream`. The validator confirms the spec declares the content type but does not check the body. When the matched media-type entry declares a `schema` (OpenAPI permits a schema on any media type, but this JSON Schema engine cannot evaluate a non-JSON one), the orchestrator marks the response/request as `Skipped` with a `skipReason` so the unvalidated body is surfaced in coverage rather than counted as a clean pass. A non-JSON entry with no `schema` has nothing to validate and stays a plain success. - **Multipart `encoding` object**: per-part `contentType` / `headers` / `style` / `explode` are not consulted. - **Cascading `additionalProperties: false` errors** are stripped automatically. opis's `PropertiesKeyword` skips its `addCheckedProperties()` call whenever any sub-property fails its schema, leaving `$checked` empty in the validation context. The follow-on `additionalProperties: false` keyword then reports every property the data carries — including ones explicitly declared in the schema's `properties` — as "additional". The validator walks opis's `ValidationError` tree, reads the raw list of "additional" property names from `args()['properties']`, and filters out names that ARE declared in the schema's `properties` keyword at that path. A single failure shows as one error, not a paired pseudo-error naming declared properties as not-allowed. Genuine additional properties still surface; mixed cases keep only the real extras in the message. The property-name comparison is fully structural (raw arrays + raw path segments — no string parsing of the rendered message for the names), so property names containing commas, whitespace, empty strings, or JSON-Pointer-escape-worthy characters survive correctly. The walker also descends through `items` for array-element segments, so cascades through `{ data: [Item] }`-shaped envelopes (single-schema items and Draft 07 tuple-form items, including the shape `OpenApiSchemaConverter` lowers OAS 3.1 `prefixItems` to) collapse the same way. The walker recognises only `properties.` and `items` transitions and treats every other shape as unresolvable — composition keywords (`oneOf` / `allOf` / `anyOf`), `additionalProperties: `, `patternProperties`, `additionalItems`, and boolean schemas at item level all fall through to keeping the original message untouched, so a real additional-property violation is never silently swallowed. diff --git a/src/OpenApiRequestValidator.php b/src/OpenApiRequestValidator.php index 3841fc2..feb74cb 100644 --- a/src/OpenApiRequestValidator.php +++ b/src/OpenApiRequestValidator.php @@ -4,12 +4,14 @@ namespace Studio\OpenApiContractTesting; +use RuntimeException; use Studio\OpenApiContractTesting\Spec\OpenApiPathMatcher; use Studio\OpenApiContractTesting\Spec\OpenApiSpecLoader; use Studio\OpenApiContractTesting\Validation\Request\HeaderParameterValidator; use Studio\OpenApiContractTesting\Validation\Request\ParameterCollector; use Studio\OpenApiContractTesting\Validation\Request\PathParameterValidator; use Studio\OpenApiContractTesting\Validation\Request\QueryParameterValidator; +use Studio\OpenApiContractTesting\Validation\Request\RequestBodyValidationResult; use Studio\OpenApiContractTesting\Validation\Request\RequestBodyValidator; use Studio\OpenApiContractTesting\Validation\Request\SecurityValidator; use Studio\OpenApiContractTesting\Validation\Support\PathDiagnosticsFormatter; @@ -171,16 +173,33 @@ public function validate( // The boundary is per-sub-validator and permissive: a capture at one stage // does NOT short-circuit later stages — every sub-validator still runs so // a single test run surfaces as much contract drift as possible. + // The body validator returns a richer DTO (errors + an optional + // skipReason) rather than a bare string[], so it cannot flow through + // ValidatorErrorBoundary::safely() like the other sub-validators. + // validateBody() runs it behind the same narrow RuntimeException + // boundary inline — mirrors OpenApiResponseValidator::validateBody(). + $bodyResult = $this->validateBody($specName, $method, $matchedPath, $operation, $body, $contentType, $version); + $errors = [ ...$collected->specErrors, ...ValidatorErrorBoundary::safely('path', $specName, $method, $matchedPath, fn(): array => $this->pathValidator->validate($method, $matchedPath, $collected->parameters, $pathVariables, $version)), ...ValidatorErrorBoundary::safely('query', $specName, $method, $matchedPath, fn(): array => $this->queryValidator->validate($method, $matchedPath, $collected->parameters, $queryParams, $version)), ...ValidatorErrorBoundary::safely('header', $specName, $method, $matchedPath, fn(): array => $this->headerValidator->validate($method, $matchedPath, $collected->parameters, $headers, $version)), ...ValidatorErrorBoundary::safely('security', $specName, $method, $matchedPath, fn(): array => $this->securityValidator->validate($method, $matchedPath, $spec, $operation, $headers, $queryParams, $cookies)), - ...ValidatorErrorBoundary::safely('request-body', $specName, $method, $matchedPath, fn(): array => $this->bodyValidator->validate($specName, $method, $matchedPath, $operation, $body, $contentType, $version)), + ...$bodyResult->errors, ]; if ($errors === []) { + // Issue #254: a non-JSON request Content-Type matched a spec + // media-type key declaring a `schema` this JSON-Schema engine + // cannot evaluate. No sibling validator failed, so the request + // is non-failing — but the body went unchecked, so surface a + // Skipped result (rather than a clean Success) and forward the + // reason to coverage tracking. + if ($bodyResult->skipReason !== null) { + return OpenApiValidationResult::skipped($matchedPath, $bodyResult->skipReason); + } + return OpenApiValidationResult::success($matchedPath); } @@ -229,6 +248,51 @@ public function validate( return OpenApiValidationResult::failure($errors, $matchedPath); } + /** + * Run the request-body validator behind the same narrow + * `RuntimeException` boundary {@see ValidatorErrorBoundary::safely()} + * applies to the other sub-validators: a `RuntimeException` (typically + * an opis/json-schema `SchemaException` raised from schema conversion + * or validation) is converted to an error string instead of aborting + * the orchestrator. The body validator returns a + * {@see RequestBodyValidationResult} DTO carrying an optional + * `skipReason`, so it cannot reuse the string[]-returning helper as-is + * — same reasoning as {@see OpenApiResponseValidator::validateBody()}. + * `\LogicException` and `\Error` still bubble so programmer bugs are + * not silently downgraded to contract errors. + * + * @param array $operation + */ + private function validateBody( + string $specName, + string $method, + string $matchedPath, + array $operation, + DecodedBody $body, + ?string $contentType, + OpenApiVersion $version, + ): RequestBodyValidationResult { + try { + return $this->bodyValidator->validate($specName, $method, $matchedPath, $operation, $body, $contentType, $version); + } catch (RuntimeException $e) { + $previous = $e->getPrevious(); + $previousSuffix = $previous !== null + ? sprintf(' (caused by %s: %s)', $previous::class, $previous->getMessage()) + : ''; + + return new RequestBodyValidationResult([sprintf( + "[%s] %s %s in '%s' spec: %s threw: %s%s", + 'request-body', + $method, + $matchedPath, + $specName, + $e::class, + $e->getMessage(), + $previousSuffix, + )]); + } + } + /** * @param string[] $specPaths */ diff --git a/src/OpenApiResponseValidator.php b/src/OpenApiResponseValidator.php index b22c632..d83f5dd 100644 --- a/src/OpenApiResponseValidator.php +++ b/src/OpenApiResponseValidator.php @@ -209,6 +209,22 @@ public function validate( $version, ); + // The body validator matched a non-JSON media-type key that declares + // a `schema` this JSON-Schema engine cannot evaluate (issue #254). + // The body was not checked, so surface a Skipped result rather than + // a clean Success — but only when headers also passed; a real header + // failure must still fail loudly (it falls through to the error + // merge below). matchedContentType is forwarded so coverage records + // the skip against that exact media-type row. + if ($bodyResult->skipReason !== null && $headerErrors === []) { + return OpenApiValidationResult::skipped( + $matchedPath, + $bodyResult->skipReason, + $statusCodeStr, + $bodyResult->matchedContentType, + ); + } + // The body validator returns ([], null) for two distinct cases: // (a) 204-style — spec has no `content` block; nothing to validate, // legitimately Success. diff --git a/src/OpenApiValidationResult.php b/src/OpenApiValidationResult.php index 50c2ff8..3ce2c02 100644 --- a/src/OpenApiValidationResult.php +++ b/src/OpenApiValidationResult.php @@ -26,7 +26,9 @@ * `matchedContentType` is the spec media-type key (with the spec author's * original casing) the body was checked against, or null when no body * lookup occurred (204, non-JSON-only specs, content-type-not-in-spec - * failures, skipped responses). + * failures, and most skipped responses). A Skipped result carries it + * only for the issue #254 case — a non-JSON media type whose declared + * `schema` this JSON-Schema engine cannot evaluate. * * @param string[] $errors */ @@ -110,11 +112,20 @@ public static function failure( * the spec response map is consulted. Coverage tracking reconciles the * literal status against any spec range keys (`5XX`/`5xx`/`default`) at * compute time, marking the spec-declared response as `skipped`. + * + * `matchedContentType` is null for most skip cases (status-code skip, + * non-JSON-only specs with no Content-Type header — no spec media-type + * key was resolved). It carries the spec media-type key only when the + * skip happened *after* a content-type lookup matched a declared key — + * the "non-JSON media type with an unvalidatable `schema`" case (issue + * #254). Passing it through lets coverage record the skip against that + * exact media-type row instead of the wildcard bucket. */ public static function skipped( ?string $matchedPath = null, ?string $reason = null, ?string $matchedStatusCode = null, + ?string $matchedContentType = null, ): self { return new self( OpenApiValidationOutcome::Skipped, @@ -122,7 +133,7 @@ public static function skipped( $matchedPath, $reason, $matchedStatusCode, - null, + $matchedContentType, ); } diff --git a/src/Validation/Request/RequestBodyValidationResult.php b/src/Validation/Request/RequestBodyValidationResult.php new file mode 100644 index 0000000..4f4c1f8 --- /dev/null +++ b/src/Validation/Request/RequestBodyValidationResult.php @@ -0,0 +1,39 @@ + $operation - * - * @return string[] */ public function validate( string $specName, @@ -50,18 +52,18 @@ public function validate( DecodedBody $requestBody, ?string $contentType, OpenApiVersion $version, - ): array { + ): RequestBodyValidationResult { // OpenAPI: a missing requestBody means the operation accepts no body — treat as success. if (!isset($operation['requestBody'])) { - return []; + return new RequestBodyValidationResult([]); } // A present-but-non-array requestBody signals a malformed spec (stray scalar). // Contract-testing tools should surface this, not mask it as "no body". if (!is_array($operation['requestBody'])) { - return [ + return new RequestBodyValidationResult([ "Malformed 'requestBody' for {$method} {$matchedPath} in '{$specName}' spec: expected object, got scalar.", - ]; + ]); } /** @var array $requestBodySpec */ @@ -70,13 +72,13 @@ public function validate( $required = ($requestBodySpec['required'] ?? false) === true; if (!isset($requestBodySpec['content'])) { - return []; + return new RequestBodyValidationResult([]); } if (!is_array($requestBodySpec['content'])) { - return [ + return new RequestBodyValidationResult([ "Malformed 'requestBody.content' for {$method} {$matchedPath} in '{$specName}' spec: expected object, got scalar.", - ]; + ]); } /** @var array $content */ @@ -88,9 +90,9 @@ public function validate( // would TypeError on downstream array accesses. Surface it as a loud spec // error instead, matching the sibling guard on `requestBody.content` above. if (!is_array($mediaTypeSpec)) { - return [ + return new RequestBodyValidationResult([ "Malformed 'requestBody.content[\"{$mediaType}\"]' for {$method} {$matchedPath} in '{$specName}' spec: expected object, got scalar.", - ]; + ]); } // `schema: "oops"` (or any other non-array scalar) would slip past the @@ -99,9 +101,9 @@ public function validate( // TypeError instead of a spec-level error. array_key_exists rather than // isset so an explicit `schema: null` is also flagged. if (array_key_exists('schema', $mediaTypeSpec) && !is_array($mediaTypeSpec['schema'])) { - return [ + return new RequestBodyValidationResult([ "Malformed 'requestBody.content[\"{$mediaType}\"].schema' for {$method} {$matchedPath} in '{$specName}' spec: expected object, got scalar.", - ]; + ]); } } @@ -112,15 +114,35 @@ public function validate( $normalizedType = ContentTypeMatcher::normalizeMediaType($contentType); if (!ContentTypeMatcher::isJsonContentType($normalizedType)) { - if (ContentTypeMatcher::isContentTypeInSpec($normalizedType, $content)) { - return []; + $matchedKey = ContentTypeMatcher::findContentTypeKey($normalizedType, $content); + if ($matchedKey !== null) { + // A matched non-JSON media type that declares a `schema` + // is an unvalidatable contract: OpenAPI permits a schema + // on any media type, but this engine only evaluates JSON + // Schema. Surface a skip (issue #254) so the unchecked + // body is not recorded as a clean pass. A non-JSON entry + // with no `schema` has nothing to validate — stay + // silently successful, as before. + if (isset($content[$matchedKey]['schema'])) { + return new RequestBodyValidationResult( + [], + sprintf( + "request Content-Type '%s' matched non-JSON spec media type '%s', " + . 'which declares a schema this validator cannot evaluate (JSON Schema engine only)', + $normalizedType, + $matchedKey, + ), + ); + } + + return new RequestBodyValidationResult([]); } $defined = implode(', ', array_keys($content)); - return [ + return new RequestBodyValidationResult([ "Request Content-Type '{$normalizedType}' is not defined for {$method} {$matchedPath} in '{$specName}' spec. Defined content types: {$defined}", - ]; + ]); } // JSON-compatible request: fall through to existing JSON schema validation. @@ -135,11 +157,11 @@ public function validate( // This validator only handles JSON schemas; non-JSON types (e.g. application/xml, // application/octet-stream) are outside its scope. if ($jsonContentType === null) { - return []; + return new RequestBodyValidationResult([]); } if (!isset($content[$jsonContentType]['schema'])) { - return []; + return new RequestBodyValidationResult([]); } // An absent body is acceptable unless the spec marks the requestBody @@ -148,12 +170,12 @@ public function validate( // to schema type-checking below instead of taking this branch. if (!$requestBody->present) { if (!$required) { - return []; + return new RequestBodyValidationResult([]); } - return [ + return new RequestBodyValidationResult([ "Request body is empty but {$method} {$matchedPath} defines a required JSON request body schema in '{$specName}' spec.", - ]; + ]); } $bodyValue = $requestBody->value; @@ -187,7 +209,7 @@ public function validate( } } - return $errors; + return new RequestBodyValidationResult($errors); } /** diff --git a/src/Validation/Response/ResponseBodyValidationResult.php b/src/Validation/Response/ResponseBodyValidationResult.php index 3582591..5659994 100644 --- a/src/Validation/Response/ResponseBodyValidationResult.php +++ b/src/Validation/Response/ResponseBodyValidationResult.php @@ -13,6 +13,14 @@ * `null` when no content-type lookup was performed (204-style responses, * non-JSON specs we don't validate, content-type-not-in-spec failures). * + * `skipReason`, when non-null, marks that the validator deliberately did NOT + * check the body even though a media-type key matched and that key declared + * a `schema`: the response Content-Type is a non-JSON media type this + * JSON-Schema engine cannot evaluate (issue #254). `errors` stays empty in + * that case — it is a skip, not a failure — and the orchestrator turns it + * into an `OpenApiValidationResult::skipped()` so the unvalidated body is + * not miscounted as a clean pass. + * * Coverage tracking uses `matchedContentType` to record per-(status, media-type) * granularity instead of treating the whole endpoint as a single bucket. * @@ -26,5 +34,6 @@ public function __construct( public array $errors, public ?string $matchedContentType, + public ?string $skipReason = null, ) {} } diff --git a/src/Validation/Response/ResponseBodyValidator.php b/src/Validation/Response/ResponseBodyValidator.php index 4a2ab7e..80a7cfe 100644 --- a/src/Validation/Response/ResponseBodyValidator.php +++ b/src/Validation/Response/ResponseBodyValidator.php @@ -20,6 +20,7 @@ use function in_array; use function is_array; use function is_string; +use function sprintf; /** * @internal Not part of the package's public API. Do not use from user code. @@ -75,6 +76,26 @@ public function validate( // Non-JSON response: check if the content type is defined in the spec. $matchedKey = ContentTypeMatcher::findContentTypeKey($normalizedType, $content); if ($matchedKey !== null) { + // A matched non-JSON media type that declares a `schema` + // is an unvalidatable contract: OpenAPI permits a schema + // on any media type, but this engine only evaluates JSON + // Schema. Surface a skip (issue #254) so the unchecked + // body is not recorded as a clean pass. A non-JSON entry + // with no `schema` has nothing to validate — stay + // silently successful, as before. + if (isset($content[$matchedKey]['schema'])) { + return new ResponseBodyValidationResult( + [], + $matchedKey, + sprintf( + "response Content-Type '%s' matched non-JSON spec media type '%s', " + . 'which declares a schema this validator cannot evaluate (JSON Schema engine only)', + $normalizedType, + $matchedKey, + ), + ); + } + return new ResponseBodyValidationResult([], $matchedKey); } diff --git a/tests/Integration/ResponseValidationTest.php b/tests/Integration/ResponseValidationTest.php index 707887a..594f30d 100644 --- a/tests/Integration/ResponseValidationTest.php +++ b/tests/Integration/ResponseValidationTest.php @@ -118,11 +118,15 @@ public function non_json_endpoint_records_skipped_when_no_content_type_supplied( } #[Test] - public function non_json_endpoint_with_explicit_content_type_marks_validated(): void + public function non_json_endpoint_with_explicit_content_type_records_skipped(): void { // Same endpoint, but the caller supplies the actual response - // Content-Type. The body validator can confirm `text/html` is in - // the spec's content map and credits the coverage row as validated. + // Content-Type. The spec's `text/html` 200 entry declares a `schema`, + // and OpenAPI permits a schema on any media type — but this engine + // only evaluates JSON Schema, so the body cannot be checked. Issue + // #254: matching the spec key must NOT credit the row as validated; + // the result is Skipped and coverage records the skip against the + // exact `text/html` media-type row (not the wildcard bucket). $result = $this->validator->validate( 'petstore-3.0', 'GET', @@ -132,13 +136,29 @@ public function non_json_endpoint_with_explicit_content_type_marks_validated(): 'text/html', ); $this->assertTrue($result->isValid()); - $this->assertFalse($result->isSkipped()); + $this->assertTrue($result->isSkipped()); + $this->assertSame('text/html', $result->matchedContentType()); + $this->assertNotNull($result->skipReason()); $this->recordResult('petstore-3.0', 'GET', $result); $coverage = OpenApiCoverageTracker::computeCoverage('petstore-3.0'); $endpoints = $this->indexEndpoints($coverage['endpoints']); - $this->assertSame(EndpointCoverageState::AllCovered, $endpoints['GET /v1/logout']['state']); + $logout = $endpoints['GET /v1/logout']; + $this->assertSame(EndpointCoverageState::Partial, $logout['state']); + $this->assertSame(1, $logout['skippedResponseCount']); + + // The skip is recorded against the concrete `text/html` row, so the + // spec's declared media type is visibly accounted for as skipped. + $textHtmlRow = null; + foreach ($logout['responses'] as $row) { + if ($row['contentTypeKey'] === 'text/html') { + $textHtmlRow = $row; + } + } + $this->assertNotNull($textHtmlRow); + $this->assertSame(ResponseCoverageState::Skipped, $textHtmlRow['state']); + $this->assertNotNull($textHtmlRow['skipReason']); } #[Test] diff --git a/tests/Unit/OpenApiRequestValidatorTest.php b/tests/Unit/OpenApiRequestValidatorTest.php index 060311b..b429758 100644 --- a/tests/Unit/OpenApiRequestValidatorTest.php +++ b/tests/Unit/OpenApiRequestValidatorTest.php @@ -289,10 +289,14 @@ public function v30_invalid_body_when_not_required_fails(): void // ======================================== #[Test] - public function v30_non_json_content_type_with_spec_match_succeeds(): void + public function v30_non_json_content_type_with_spec_match_is_skipped(): void { - // Body intentionally violates the JSON pet schema (integer, not an object with "name"). - // If the non-JSON short-circuit ever regresses into schema validation this will fail. + // Body intentionally violates the JSON pet schema (integer, not an + // object with "name"). The `text/plain` requestBody entry declares a + // `schema`, but this JSON-Schema engine cannot evaluate a non-JSON + // schema — issue #254 surfaces this as Skipped (isValid() stays + // true) rather than a clean Success. If the non-JSON short-circuit + // ever regresses into schema validation, isValid() would turn false. $result = $this->validator->validate( 'petstore-3.0', 'POST', @@ -304,6 +308,8 @@ public function v30_non_json_content_type_with_spec_match_succeeds(): void ); $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); + $this->assertNotNull($result->skipReason()); } #[Test] @@ -320,6 +326,51 @@ public function v30_non_json_content_type_spec_match_is_case_insensitive(): void ); $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); + } + + #[Test] + public function non_json_request_content_type_with_a_schema_is_skipped(): void + { + // Issue #254: the `text/plain` requestBody entry declares a `schema`. + // The request Content-Type matches that key, but a non-JSON schema + // cannot be evaluated — the orchestrator surfaces a Skipped result so + // the unvalidated body is not miscounted as a clean validated request. + $result = $this->validator->validate( + 'non-json-content-schema', + 'POST', + '/text-with-schema', + [], + [], + 'plain body', + 'text/plain', + ); + + $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); + $this->assertSame('/text-with-schema', $result->matchedPath()); + $this->assertNotNull($result->skipReason()); + $this->assertStringContainsString('JSON Schema engine only', (string) $result->skipReason()); + } + + #[Test] + public function non_json_request_content_type_without_a_schema_succeeds(): void + { + // The `text/plain` requestBody entry declares NO `schema` — nothing to + // validate, so the request is a clean Success, not Skipped. + $result = $this->validator->validate( + 'non-json-content-schema', + 'POST', + '/text-without-schema', + [], + [], + 'plain body', + 'text/plain', + ); + + $this->assertTrue($result->isValid()); + $this->assertFalse($result->isSkipped()); + $this->assertSame('/text-without-schema', $result->matchedPath()); } #[Test] diff --git a/tests/Unit/OpenApiResponseValidatorTest.php b/tests/Unit/OpenApiResponseValidatorTest.php index 58f17f1..ebad2ff 100644 --- a/tests/Unit/OpenApiResponseValidatorTest.php +++ b/tests/Unit/OpenApiResponseValidatorTest.php @@ -835,8 +835,13 @@ public function v30_null_response_content_type_preserves_existing_behavior(): vo } #[Test] - public function v30_non_json_only_spec_with_matching_response_content_type_succeeds(): void + public function v30_non_json_only_spec_with_matching_response_content_type_is_skipped(): void { + // The spec's `text/html` 200 entry declares a `schema`. Matching the + // response Content-Type to that key does NOT make the body validated + // — this engine cannot evaluate a non-JSON schema (issue #254), so + // the result is Skipped (isValid() stays true) and carries the + // matched media-type key for coverage. $result = $this->validator->validate( 'petstore-3.0', 'GET', @@ -847,7 +852,56 @@ public function v30_non_json_only_spec_with_matching_response_content_type_succe ); $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); $this->assertSame('/v1/logout', $result->matchedPath()); + $this->assertSame('text/html', $result->matchedContentType()); + $this->assertNotNull($result->skipReason()); + } + + #[Test] + public function non_json_response_content_type_with_a_schema_is_skipped(): void + { + // Issue #254: the spec declares `text/plain` WITH a `schema` for the + // 200 response. The response Content-Type matches that key, but a + // non-JSON schema cannot be evaluated by this JSON-Schema engine — + // the orchestrator must surface a Skipped result, not a clean + // Success, so the unvalidated body is not miscounted. + $result = $this->validator->validate( + 'non-json-content-schema', + 'GET', + '/text-with-schema', + 200, + 'plain body', + 'text/plain', + ); + + $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); + $this->assertSame('/text-with-schema', $result->matchedPath()); + $this->assertSame('text/plain', $result->matchedContentType()); + $this->assertNotNull($result->skipReason()); + $this->assertStringContainsString('JSON Schema engine only', (string) $result->skipReason()); + } + + #[Test] + public function non_json_response_content_type_without_a_schema_succeeds(): void + { + // The `text/plain` 200 entry declares NO `schema` — there is nothing + // to validate, so the body is silently accepted (Success, not + // Skipped). This keeps coverage reports quiet for endpoints whose + // spec genuinely has no schema to check. + $result = $this->validator->validate( + 'non-json-content-schema', + 'GET', + '/text-without-schema', + 200, + 'plain body', + 'text/plain', + ); + + $this->assertTrue($result->isValid()); + $this->assertFalse($result->isSkipped()); + $this->assertSame('/text-without-schema', $result->matchedPath()); } // ======================================== diff --git a/tests/Unit/Validation/Request/RequestBodyValidatorTest.php b/tests/Unit/Validation/Request/RequestBodyValidatorTest.php index cdd7957..8dd6a63 100644 --- a/tests/Unit/Validation/Request/RequestBodyValidatorTest.php +++ b/tests/Unit/Validation/Request/RequestBodyValidatorTest.php @@ -24,7 +24,7 @@ protected function setUp(): void #[Test] public function validate_returns_empty_when_operation_defines_no_body(): void { - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'GET', '/pets', @@ -34,7 +34,7 @@ public function validate_returns_empty_when_operation_defines_no_body(): void OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -49,7 +49,7 @@ public function validate_flags_missing_required_body(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -59,8 +59,8 @@ public function validate_flags_missing_required_body(): void OpenApiVersion::V3_0, ); - $this->assertCount(1, $errors); - $this->assertStringContainsString('Request body is empty', $errors[0]); + $this->assertCount(1, $result->errors); + $this->assertStringContainsString('Request body is empty', $result->errors[0]); } #[Test] @@ -81,7 +81,7 @@ public function validate_flags_present_literal_null_body_against_object_schema_w ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -91,8 +91,8 @@ public function validate_flags_present_literal_null_body_against_object_schema_w OpenApiVersion::V3_0, ); - $this->assertNotEmpty($errors); - $this->assertStringContainsString('must match the type', $errors[0]); + $this->assertNotEmpty($result->errors); + $this->assertStringContainsString('must match the type', $result->errors[0]); } #[Test] @@ -110,7 +110,7 @@ public function validate_flags_present_literal_null_body_against_object_schema_w ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -120,9 +120,9 @@ public function validate_flags_present_literal_null_body_against_object_schema_w OpenApiVersion::V3_0, ); - $this->assertNotEmpty($errors); - $this->assertStringContainsString('must match the type', $errors[0]); - $this->assertStringNotContainsString('Request body is empty', $errors[0]); + $this->assertNotEmpty($result->errors); + $this->assertStringContainsString('must match the type', $result->errors[0]); + $this->assertStringNotContainsString('Request body is empty', $result->errors[0]); } #[Test] @@ -138,7 +138,7 @@ public function validate_accepts_present_literal_null_body_against_oas_31_nullab ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -148,7 +148,7 @@ public function validate_accepts_present_literal_null_body_against_oas_31_nullab OpenApiVersion::V3_1, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -168,7 +168,7 @@ public function validate_accepts_present_literal_null_body_against_oas_30_nullab ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -178,7 +178,7 @@ public function validate_accepts_present_literal_null_body_against_oas_30_nullab OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -197,7 +197,7 @@ public function validate_still_treats_absent_body_as_no_body(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -207,7 +207,7 @@ public function validate_still_treats_absent_body_as_no_body(): void OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -221,7 +221,7 @@ public function validate_flags_unknown_non_json_content_type(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -231,8 +231,8 @@ public function validate_flags_unknown_non_json_content_type(): void OpenApiVersion::V3_0, ); - $this->assertCount(1, $errors); - $this->assertStringContainsString("Content-Type 'application/xml' is not defined", $errors[0]); + $this->assertCount(1, $result->errors); + $this->assertStringContainsString("Content-Type 'application/xml' is not defined", $result->errors[0]); } #[Test] @@ -240,7 +240,7 @@ public function validate_flags_malformed_non_array_request_body(): void { $operation = ['requestBody' => 'oops']; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -250,8 +250,8 @@ public function validate_flags_malformed_non_array_request_body(): void OpenApiVersion::V3_0, ); - $this->assertCount(1, $errors); - $this->assertStringContainsString("Malformed 'requestBody'", $errors[0]); + $this->assertCount(1, $result->errors); + $this->assertStringContainsString("Malformed 'requestBody'", $result->errors[0]); } #[Test] @@ -265,7 +265,7 @@ public function validate_flags_malformed_media_type_schema(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -275,9 +275,9 @@ public function validate_flags_malformed_media_type_schema(): void OpenApiVersion::V3_0, ); - $this->assertCount(1, $errors); - $this->assertStringContainsString('.schema\'', $errors[0]); - $this->assertStringContainsString('expected object, got scalar', $errors[0]); + $this->assertCount(1, $result->errors); + $this->assertStringContainsString('.schema\'', $result->errors[0]); + $this->assertStringContainsString('expected object, got scalar', $result->errors[0]); } #[Test] @@ -297,7 +297,7 @@ public function validate_validates_json_body_against_schema(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/pets', @@ -307,7 +307,7 @@ public function validate_validates_json_body_against_schema(): void OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -327,7 +327,7 @@ public function validate_accepts_empty_object_body_against_type_object(): void ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -337,7 +337,7 @@ public function validate_accepts_empty_object_body_against_type_object(): void OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -353,7 +353,7 @@ public function validate_accepts_empty_object_body_against_oas_31_nullable_objec ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -363,7 +363,7 @@ public function validate_accepts_empty_object_body_against_oas_31_nullable_objec OpenApiVersion::V3_1, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -386,7 +386,7 @@ public function validate_does_not_coerce_empty_array_when_schema_has_no_explicit ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -398,7 +398,7 @@ public function validate_does_not_coerce_empty_array_when_schema_has_no_explicit // No error AND no coercion fired — the property bag is permissive // so empty input passes for either array or object interpretation. - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -424,7 +424,7 @@ public function validate_does_not_coerce_empty_array_for_oneof_with_object_branc ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -441,8 +441,8 @@ public function validate_does_not_coerce_empty_array_for_oneof_with_object_branc // a `required` (missing foo) error instead of a type-mismatch error. // The substring "must match the type" only appears in the pre-coercion // world; the post-coercion world would say "required properties". - $this->assertNotEmpty($errors); - $this->assertStringContainsString('must match the type', $errors[0]); + $this->assertNotEmpty($result->errors); + $this->assertStringContainsString('must match the type', $result->errors[0]); } #[Test] @@ -462,7 +462,7 @@ public function validate_does_not_coerce_empty_array_when_schema_is_array_type() ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -472,7 +472,7 @@ public function validate_does_not_coerce_empty_array_when_schema_is_array_type() OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); } #[Test] @@ -501,7 +501,7 @@ public function validate_still_flags_missing_required_property_after_empty_objec ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -511,8 +511,8 @@ public function validate_still_flags_missing_required_property_after_empty_objec OpenApiVersion::V3_0, ); - $this->assertNotEmpty($errors); - $this->assertStringContainsString('required properties (foo)', $errors[0]); + $this->assertNotEmpty($result->errors); + $this->assertStringContainsString('required properties (foo)', $result->errors[0]); } #[Test] @@ -533,7 +533,7 @@ public function validate_accepts_empty_object_body_when_request_body_is_optional ], ]; - $errors = $this->validator->validate( + $result = $this->validator->validate( 'spec', 'POST', '/p', @@ -543,6 +543,67 @@ public function validate_accepts_empty_object_body_when_request_body_is_optional OpenApiVersion::V3_0, ); - $this->assertSame([], $errors); + $this->assertSame([], $result->errors); + } + + #[Test] + public function validate_skips_non_json_content_type_when_spec_entry_has_a_schema(): void + { + // Issue #254: the request Content-Type is a non-JSON media type that + // matches a spec media-type key, and that key declares a `schema`. + // OpenAPI permits a schema on any media type, but this engine only + // evaluates JSON Schema — the body cannot be checked. The validator + // must surface a skip (empty errors + non-null skipReason) so the + // unvalidated body is not recorded as a clean pass. + $operation = [ + 'requestBody' => [ + 'content' => [ + 'text/plain' => ['schema' => ['type' => 'string']], + ], + ], + ]; + + $result = $this->validator->validate( + 'spec', + 'POST', + '/pets', + $operation, + DecodedBody::present('raw pet body'), + 'text/plain; charset=utf-8', + OpenApiVersion::V3_0, + ); + + $this->assertSame([], $result->errors); + $this->assertNotNull($result->skipReason); + $this->assertStringContainsString('text/plain', $result->skipReason); + $this->assertStringContainsString('JSON Schema engine only', $result->skipReason); + } + + #[Test] + public function validate_does_not_skip_non_json_content_type_without_a_schema(): void + { + // A non-JSON media type with NO `schema` has nothing to validate — + // it stays silently successful (no errors, no skipReason), so it is + // not noisily surfaced in coverage as an unvalidated endpoint. + $operation = [ + 'requestBody' => [ + 'content' => [ + 'text/plain' => [], + ], + ], + ]; + + $result = $this->validator->validate( + 'spec', + 'POST', + '/pets', + $operation, + DecodedBody::present('raw pet body'), + 'text/plain', + OpenApiVersion::V3_0, + ); + + $this->assertSame([], $result->errors); + $this->assertNull($result->skipReason); } } diff --git a/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php b/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php index b7e79ad..acde5ef 100644 --- a/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php +++ b/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php @@ -156,8 +156,15 @@ public function validate_accepts_present_literal_null_body_against_oas_30_nullab } #[Test] - public function validate_accepts_non_json_content_type_when_defined_in_spec(): void + public function validate_skips_non_json_content_type_when_spec_entry_has_a_schema(): void { + // Issue #254: the response Content-Type matched the spec's `text/plain` + // media-type key, and that key declares a `schema`. OpenAPI permits a + // schema on any media type, but this engine only evaluates JSON Schema + // — the body cannot be checked. The validator must surface a skip + // (empty errors + non-null skipReason) so the unvalidated body is not + // recorded as a clean pass. matchedContentType is still the matched + // key so coverage records the skip against that exact media-type row. $content = [ 'text/plain' => ['schema' => ['type' => 'string']], ]; @@ -175,6 +182,35 @@ public function validate_accepts_non_json_content_type_when_defined_in_spec(): v $this->assertSame([], $result->errors); $this->assertSame('text/plain', $result->matchedContentType); + $this->assertNotNull($result->skipReason); + $this->assertStringContainsString('text/plain', $result->skipReason); + $this->assertStringContainsString('JSON Schema engine only', $result->skipReason); + } + + #[Test] + public function validate_does_not_skip_non_json_content_type_without_a_schema(): void + { + // A non-JSON media type with NO `schema` has nothing to validate — it + // stays silently successful (no errors, no skipReason) so it is not + // noisily surfaced in coverage as an unvalidated endpoint. + $content = [ + 'text/plain' => [], + ]; + + $result = $this->validator->validate( + 'spec', + 'GET', + '/robots.txt', + 200, + $content, + DecodedBody::present('User-agent: *'), + 'text/plain; charset=utf-8', + OpenApiVersion::V3_0, + ); + + $this->assertSame([], $result->errors); + $this->assertSame('text/plain', $result->matchedContentType); + $this->assertNull($result->skipReason); } #[Test] diff --git a/tests/fixtures/specs/non-json-content-schema.json b/tests/fixtures/specs/non-json-content-schema.json new file mode 100644 index 0000000..a332c02 --- /dev/null +++ b/tests/fixtures/specs/non-json-content-schema.json @@ -0,0 +1,69 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Non-JSON content schema fixture", + "version": "1.0.0" + }, + "paths": { + "/text-with-schema": { + "get": { + "operationId": "getTextWithSchema", + "responses": { + "200": { + "description": "text/plain WITH a schema — unvalidatable, must surface a skip", + "content": { + "text/plain": { + "schema": { + "type": "string" + } + } + } + } + } + }, + "post": { + "operationId": "postTextWithSchema", + "requestBody": { + "content": { + "text/plain": { + "schema": { + "type": "string" + } + } + } + }, + "responses": { + "204": { + "description": "Accepted" + } + } + } + }, + "/text-without-schema": { + "get": { + "operationId": "getTextWithoutSchema", + "responses": { + "200": { + "description": "text/plain with NO schema — nothing to validate, silent success", + "content": { + "text/plain": {} + } + } + } + }, + "post": { + "operationId": "postTextWithoutSchema", + "requestBody": { + "content": { + "text/plain": {} + } + }, + "responses": { + "204": { + "description": "Accepted" + } + } + } + } + } +} From 87b9cb01905a2a7e533f4052d613ef1643d455d3 Mon Sep 17 00:00:00 2001 From: wadakatu Date: Mon, 18 May 2026 18:36:46 +0900 Subject: [PATCH 2/2] test(validation): close review gaps and guard body-result DTO invariants (#254) Add constructor guards to RequestBodyValidationResult and ResponseBodyValidationResult that reject contradictory states via InvalidArgumentException: a skipReason set together with errors, and on the response side a skipReason set together with a null matchedContentType. This follows the precedent of the OpenApiValidationResult::failure([]) guard. Close the test gaps raised in review: - a non-JSON body skip must not mask a missing required response header error - a request-side skip reason must be recorded in coverage - skip on a wildcard (application/*) range match - the DTO guards fire on contradictory input Strengthen two existing tests that weakly pinned the old buggy behavior with assertions for the new skip behavior. Add a comment on non-array schema handling and fix a stale comment in OpenApiResponseValidator after the DTO grew to three fields. --- src/OpenApiResponseValidator.php | 17 +++--- .../Request/RequestBodyValidationResult.php | 20 ++++++- .../Request/RequestBodyValidator.php | 6 +++ .../Response/ResponseBodyValidationResult.php | 28 +++++++++- .../Response/ResponseBodyValidator.php | 6 +++ tests/Integration/ResponseValidationTest.php | 18 ++++++- tests/Unit/OpenApiResponseValidatorTest.php | 33 +++++++++++- ...esOpenApiSchemaAutoValidateRequestTest.php | 36 +++++++++++++ .../Request/RequestBodyValidatorTest.php | 45 ++++++++++++++++ .../Response/ResponseBodyValidatorTest.php | 53 +++++++++++++++++++ .../specs/non-json-content-schema.json | 25 +++++++++ 11 files changed, 275 insertions(+), 12 deletions(-) diff --git a/src/OpenApiResponseValidator.php b/src/OpenApiResponseValidator.php index d83f5dd..e2a583a 100644 --- a/src/OpenApiResponseValidator.php +++ b/src/OpenApiResponseValidator.php @@ -225,15 +225,20 @@ public function validate( ); } - // The body validator returns ([], null) for two distinct cases: + // The body validator returns `errors: []` + `matchedContentType: null` + // (and `skipReason: null`, so the branch above did not fire) for two + // distinct cases: // (a) 204-style — spec has no `content` block; nothing to validate, // legitimately Success. // (b) Spec declares only non-JSON content types (e.g. `text/plain`) - // and we have no schema engine for them; the result is "we - // didn't actually check anything". Without this branch the - // orchestrator would mark the response as a clean Success and - // coverage would credit the spec's declared content-type as - // validated even though no validation occurred. + // with no `schema` and no actual response Content-Type was + // supplied to look one up; the result is "we didn't actually + // check anything". Without this branch the orchestrator would + // mark the response as a clean Success and coverage would credit + // the spec's declared content-type as validated even though no + // validation occurred. (A non-JSON type that DID match a key + // declaring a `schema` is handled by the skipReason branch above + // and never reaches here.) // Distinguishing them requires looking at the spec — `content` // present + non-empty + bodyResult.matchedContentType null + body // had no errors → case (b). diff --git a/src/Validation/Request/RequestBodyValidationResult.php b/src/Validation/Request/RequestBodyValidationResult.php index 4f4c1f8..5c97db1 100644 --- a/src/Validation/Request/RequestBodyValidationResult.php +++ b/src/Validation/Request/RequestBodyValidationResult.php @@ -4,7 +4,9 @@ namespace Studio\OpenApiContractTesting\Validation\Request; +use InvalidArgumentException; use Studio\OpenApiContractTesting\OpenApiRequestValidator; +use Studio\OpenApiContractTesting\OpenApiValidationResult; use Studio\OpenApiContractTesting\Validation\Response\ResponseBodyValidationResult; /** @@ -21,6 +23,10 @@ * the unvalidated body is not miscounted as a clean pass and the skip reason * reaches coverage tracking. * + * If a sibling validator (path / query / header / security) failed, the + * orchestrator builds a `failure()` instead and the `skipReason` is dropped + * — a genuine failure takes precedence over a skip. + * * This mirrors {@see ResponseBodyValidationResult} on the response side; * request-side coverage has no per-content-type dimension, so no * `matchedContentType` is carried. @@ -31,9 +37,21 @@ { /** * @param string[] $errors + * + * @throws InvalidArgumentException when `skipReason` is set alongside a + * non-empty `errors` list — a skip means the body was deliberately + * not checked, which is mutually exclusive with reporting errors. + * Mirrors the `failure([])` guard on {@see OpenApiValidationResult}. */ public function __construct( public array $errors, public ?string $skipReason = null, - ) {} + ) { + if ($skipReason !== null && $errors !== []) { + throw new InvalidArgumentException( + 'A skipped RequestBodyValidationResult cannot also carry errors: ' + . 'a skip means the body was not checked.', + ); + } + } } diff --git a/src/Validation/Request/RequestBodyValidator.php b/src/Validation/Request/RequestBodyValidator.php index e5d1f46..e468b89 100644 --- a/src/Validation/Request/RequestBodyValidator.php +++ b/src/Validation/Request/RequestBodyValidator.php @@ -123,6 +123,12 @@ public function validate( // body is not recorded as a clean pass. A non-JSON entry // with no `schema` has nothing to validate — stay // silently successful, as before. + // + // `isset` (not `array_key_exists`) is deliberate: an + // explicit `schema: null` is a degenerate entry, and the + // per-media-type malformed-schema guard above already + // rejected it loudly before this point — so it never + // reaches here as a silent "no schema" case. if (isset($content[$matchedKey]['schema'])) { return new RequestBodyValidationResult( [], diff --git a/src/Validation/Response/ResponseBodyValidationResult.php b/src/Validation/Response/ResponseBodyValidationResult.php index 5659994..919cf83 100644 --- a/src/Validation/Response/ResponseBodyValidationResult.php +++ b/src/Validation/Response/ResponseBodyValidationResult.php @@ -4,6 +4,9 @@ namespace Studio\OpenApiContractTesting\Validation\Response; +use InvalidArgumentException; +use Studio\OpenApiContractTesting\OpenApiValidationResult; + /** * Outcome of {@see ResponseBodyValidator::validate()}. * @@ -19,7 +22,8 @@ * JSON-Schema engine cannot evaluate (issue #254). `errors` stays empty in * that case — it is a skip, not a failure — and the orchestrator turns it * into an `OpenApiValidationResult::skipped()` so the unvalidated body is - * not miscounted as a clean pass. + * not miscounted as a clean pass. A skip always names the matched key, so + * `matchedContentType` is non-null whenever `skipReason` is set. * * Coverage tracking uses `matchedContentType` to record per-(status, media-type) * granularity instead of treating the whole endpoint as a single bucket. @@ -30,10 +34,30 @@ { /** * @param string[] $errors + * + * @throws InvalidArgumentException when `skipReason` is set alongside a + * non-empty `errors` list (a skip is mutually exclusive with + * reporting errors) or alongside a null `matchedContentType` + * (a skip is only reached after a media-type key matched). + * Mirrors the `failure([])` guard on {@see OpenApiValidationResult}. */ public function __construct( public array $errors, public ?string $matchedContentType, public ?string $skipReason = null, - ) {} + ) { + if ($skipReason !== null && $errors !== []) { + throw new InvalidArgumentException( + 'A skipped ResponseBodyValidationResult cannot also carry errors: ' + . 'a skip means the body was not checked.', + ); + } + + if ($skipReason !== null && $matchedContentType === null) { + throw new InvalidArgumentException( + 'A skipped ResponseBodyValidationResult must name the matched ' + . 'media-type key so coverage records the skip against it.', + ); + } + } } diff --git a/src/Validation/Response/ResponseBodyValidator.php b/src/Validation/Response/ResponseBodyValidator.php index 80a7cfe..1d1d030 100644 --- a/src/Validation/Response/ResponseBodyValidator.php +++ b/src/Validation/Response/ResponseBodyValidator.php @@ -83,6 +83,12 @@ public function validate( // body is not recorded as a clean pass. A non-JSON entry // with no `schema` has nothing to validate — stay // silently successful, as before. + // + // `isset` treats a degenerate `schema: null` as "no + // schema" (silent success). Unlike the request validator, + // this validator has no upstream malformed-schema guard, + // so a non-array `schema` is not surfaced as a loud spec + // error here — tracked separately (see #256). if (isset($content[$matchedKey]['schema'])) { return new ResponseBodyValidationResult( [], diff --git a/tests/Integration/ResponseValidationTest.php b/tests/Integration/ResponseValidationTest.php index 594f30d..aeb5cd0 100644 --- a/tests/Integration/ResponseValidationTest.php +++ b/tests/Integration/ResponseValidationTest.php @@ -162,8 +162,10 @@ public function non_json_endpoint_with_explicit_content_type_records_skipped(): } #[Test] - public function content_negotiation_non_json_response_succeeds_and_records_coverage(): void + public function content_negotiation_non_json_response_is_skipped_and_records_coverage(): void { + // The 409 `text/html` entry declares a `schema`, so the matched + // non-JSON Content-Type is Skipped (issue #254), not validated. $result = $this->validator->validate( 'petstore-3.0', 'POST', @@ -173,14 +175,26 @@ public function content_negotiation_non_json_response_succeeds_and_records_cover 'text/html', ); $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); + $this->assertSame('text/html', $result->matchedContentType()); $this->recordResult('petstore-3.0', 'POST', $result); $coverage = OpenApiCoverageTracker::computeCoverage('petstore-3.0'); $endpoints = $this->indexEndpoints($coverage['endpoints']); - // text/html is one of two declared 409 content-types; the other (application/json) + // text/html is one of two declared 409 content-types; it is recorded + // as skipped (not validated), and the other (application/json) // remains uncovered, so the endpoint as a whole is partial. $this->assertSame(EndpointCoverageState::Partial, $endpoints['POST /v1/pets']['state']); + + $textHtmlRow = null; + foreach ($endpoints['POST /v1/pets']['responses'] as $row) { + if ($row['statusKey'] === '409' && $row['contentTypeKey'] === 'text/html') { + $textHtmlRow = $row; + } + } + $this->assertNotNull($textHtmlRow); + $this->assertSame(ResponseCoverageState::Skipped, $textHtmlRow['state']); } #[Test] diff --git a/tests/Unit/OpenApiResponseValidatorTest.php b/tests/Unit/OpenApiResponseValidatorTest.php index ebad2ff..ee516db 100644 --- a/tests/Unit/OpenApiResponseValidatorTest.php +++ b/tests/Unit/OpenApiResponseValidatorTest.php @@ -667,8 +667,12 @@ public function v30_mixed_content_types_with_invalid_json_body_fails(): void // ======================================== #[Test] - public function v30_mixed_content_type_with_non_json_response_content_type_succeeds(): void + public function v30_mixed_content_type_with_non_json_response_content_type_is_skipped(): void { + // The 409 response declares both `application/json` and a `text/html` + // entry that carries a `schema`. A `text/html` response matches the + // latter — a non-JSON schema this engine cannot evaluate — so the + // result is Skipped (issue #254), not a clean Success. $result = $this->validator->validate( 'petstore-3.0', 'POST', @@ -679,7 +683,10 @@ public function v30_mixed_content_type_with_non_json_response_content_type_succe ); $this->assertTrue($result->isValid()); + $this->assertTrue($result->isSkipped()); $this->assertSame('/v1/pets', $result->matchedPath()); + $this->assertSame('text/html', $result->matchedContentType()); + $this->assertNotNull($result->skipReason()); } #[Test] @@ -904,6 +911,30 @@ public function non_json_response_content_type_without_a_schema_succeeds(): void $this->assertSame('/text-without-schema', $result->matchedPath()); } + #[Test] + public function non_json_body_skip_does_not_mask_a_missing_required_header(): void + { + // Regression guard for the `&& $headerErrors === []` gate on the + // issue #254 skip branch: the response body is skip-eligible (non-JSON + // `text/plain` with a schema), but the spec also requires an + // `X-Trace-Id` response header that the response omits. The header + // failure must still fail the result loudly — the body skip must not + // swallow it. + $result = $this->validator->validate( + 'non-json-content-schema', + 'GET', + '/text-with-schema-and-required-header', + 200, + 'plain body', + 'text/plain', + [], + ); + + $this->assertFalse($result->isValid()); + $this->assertFalse($result->isSkipped()); + $this->assertStringContainsString('X-Trace-Id', $result->errorMessage()); + } + // ======================================== // OAS 3.1 tests // ======================================== diff --git a/tests/Unit/ValidatesOpenApiSchemaAutoValidateRequestTest.php b/tests/Unit/ValidatesOpenApiSchemaAutoValidateRequestTest.php index 758dc49..94d617d 100644 --- a/tests/Unit/ValidatesOpenApiSchemaAutoValidateRequestTest.php +++ b/tests/Unit/ValidatesOpenApiSchemaAutoValidateRequestTest.php @@ -557,6 +557,42 @@ public function records_request_skip_reason_in_coverage_for_downgraded_failure() $this->assertStringContainsString('422', (string) $endpoint['requestSkipReason']); } + #[Test] + public function records_request_skip_reason_in_coverage_for_non_json_schema_body(): void + { + // Issue #254 end-to-end: a non-JSON request Content-Type that matched + // a spec media-type key declaring a `schema` is downgraded to Skipped + // by the request validator. The trait must forward that skip reason + // into coverage so the endpoint is recorded as request-reached with a + // skip reason rather than a clean validated request. + $GLOBALS['__openapi_testing_config']['openapi-contract-testing.auto_validate_request'] = true; + $GLOBALS['__openapi_testing_config']['openapi-contract-testing.default_spec'] = 'non-json-content-schema'; + + $request = Request::create( + '/text-with-schema', + 'POST', + [], + [], + [], + ['CONTENT_TYPE' => 'text/plain'], + 'plain body', + ); + + // text/plain with a schema → Skipped, not a failure: no exception. + $this->maybeAutoValidateOpenApiRequest($request, HttpMethod::POST, '/text-with-schema'); + + $state = OpenApiCoverageTracker::exportState(); + $endpoint = $state['specs']['non-json-content-schema']['POST /text-with-schema'] ?? null; + $this->assertNotNull($endpoint); + $this->assertTrue($endpoint['requestReached']); + $this->assertArrayHasKey('requestSkipReason', $endpoint); + $this->assertNotNull($endpoint['requestSkipReason']); + $this->assertStringContainsString( + 'JSON Schema engine only', + (string) $endpoint['requestSkipReason'], + ); + } + #[Test] public function request_validator_cache_invalidates_when_skip_codes_config_changes(): void { diff --git a/tests/Unit/Validation/Request/RequestBodyValidatorTest.php b/tests/Unit/Validation/Request/RequestBodyValidatorTest.php index 8dd6a63..690ba49 100644 --- a/tests/Unit/Validation/Request/RequestBodyValidatorTest.php +++ b/tests/Unit/Validation/Request/RequestBodyValidatorTest.php @@ -4,10 +4,12 @@ namespace Studio\OpenApiContractTesting\Tests\Unit\Validation\Request; +use InvalidArgumentException; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Studio\OpenApiContractTesting\DecodedBody; use Studio\OpenApiContractTesting\OpenApiVersion; +use Studio\OpenApiContractTesting\Validation\Request\RequestBodyValidationResult; use Studio\OpenApiContractTesting\Validation\Request\RequestBodyValidator; use Studio\OpenApiContractTesting\Validation\Support\SchemaValidatorRunner; @@ -606,4 +608,47 @@ public function validate_does_not_skip_non_json_content_type_without_a_schema(): $this->assertSame([], $result->errors); $this->assertNull($result->skipReason); } + + #[Test] + public function validate_skips_non_json_content_type_matched_via_wildcard_range_with_a_schema(): void + { + // Issue #254 skip detection keys off `findContentTypeKey()`, which + // also matches `/*` ranges. A non-JSON Content-Type that + // matches a wildcard spec key declaring a `schema` must skip too — + // not just exact-key matches. + $operation = [ + 'requestBody' => [ + 'content' => [ + 'application/*' => ['schema' => ['type' => 'string']], + ], + ], + ]; + + $result = $this->validator->validate( + 'spec', + 'POST', + '/blob', + $operation, + DecodedBody::present('binary-ish blob'), + 'application/octet-stream', + OpenApiVersion::V3_0, + ); + + $this->assertSame([], $result->errors); + $this->assertNotNull($result->skipReason); + $this->assertStringContainsString('application/*', $result->skipReason); + } + + #[Test] + public function result_rejects_a_skip_reason_alongside_errors(): void + { + // A skip means the body was deliberately not checked — that is + // mutually exclusive with reporting errors. The DTO guard makes the + // contradictory state unconstructable so a future producer bug fails + // loudly instead of silently miscounting an errored body as a skip. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('cannot also carry errors'); + + new RequestBodyValidationResult(['some error'], 'a skip reason'); + } } diff --git a/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php b/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php index acde5ef..766a954 100644 --- a/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php +++ b/tests/Unit/Validation/Response/ResponseBodyValidatorTest.php @@ -4,10 +4,12 @@ namespace Studio\OpenApiContractTesting\Tests\Unit\Validation\Response; +use InvalidArgumentException; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Studio\OpenApiContractTesting\DecodedBody; use Studio\OpenApiContractTesting\OpenApiVersion; +use Studio\OpenApiContractTesting\Validation\Response\ResponseBodyValidationResult; use Studio\OpenApiContractTesting\Validation\Response\ResponseBodyValidator; use Studio\OpenApiContractTesting\Validation\Support\SchemaValidatorRunner; @@ -213,6 +215,57 @@ public function validate_does_not_skip_non_json_content_type_without_a_schema(): $this->assertNull($result->skipReason); } + #[Test] + public function validate_skips_non_json_content_type_matched_via_wildcard_range_with_a_schema(): void + { + // Issue #254 skip detection keys off `findContentTypeKey()`, which + // also matches `/*` ranges. A non-JSON Content-Type that + // matches a wildcard spec key declaring a `schema` must skip too — + // not just exact-key matches. + $content = [ + 'application/*' => ['schema' => ['type' => 'string']], + ]; + + $result = $this->validator->validate( + 'spec', + 'GET', + '/blob', + 200, + $content, + DecodedBody::present('binary-ish blob'), + 'application/octet-stream', + OpenApiVersion::V3_0, + ); + + $this->assertSame([], $result->errors); + $this->assertSame('application/*', $result->matchedContentType); + $this->assertNotNull($result->skipReason); + } + + #[Test] + public function result_rejects_a_skip_reason_alongside_errors(): void + { + // A skip means the body was deliberately not checked — mutually + // exclusive with reporting errors. The DTO guard makes the + // contradictory state unconstructable. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('cannot also carry errors'); + + new ResponseBodyValidationResult(['some error'], 'text/plain', 'a skip reason'); + } + + #[Test] + public function result_rejects_a_skip_reason_without_a_matched_content_type(): void + { + // A skip is only reached after a media-type key matched, so a skip + // must always name that key — otherwise coverage would record the + // skip against the wildcard bucket instead of the real row. + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('must name the matched'); + + new ResponseBodyValidationResult([], null, 'a skip reason'); + } + #[Test] public function validate_preserves_spec_content_type_casing(): void { diff --git a/tests/fixtures/specs/non-json-content-schema.json b/tests/fixtures/specs/non-json-content-schema.json index a332c02..ced2f8e 100644 --- a/tests/fixtures/specs/non-json-content-schema.json +++ b/tests/fixtures/specs/non-json-content-schema.json @@ -39,6 +39,31 @@ } } }, + "/text-with-schema-and-required-header": { + "get": { + "operationId": "getTextWithSchemaAndRequiredHeader", + "responses": { + "200": { + "description": "text/plain WITH a schema AND a required response header — the body skip must not mask a missing-header failure", + "headers": { + "X-Trace-Id": { + "required": true, + "schema": { + "type": "string" + } + } + }, + "content": { + "text/plain": { + "schema": { + "type": "string" + } + } + } + } + } + } + }, "/text-without-schema": { "get": { "operationId": "getTextWithoutSchema",