Conversation
…lient (#108, PR 2/2) Closes #108. Builds on PR 1/2 which added local-filesystem ref support. The resolver now follows `$ref` paths to HTTP(S) URLs when the user opts in by providing a PSR-18 ClientInterface + PSR-17 RequestFactory and flipping `allowRemoteRefs: true`: $ref: 'https://example.com/schemas/pet.yaml' $ref: 'https://example.com/schemas/pet.yaml#/Pet' $ref: 'https://registry.example.com/Pet' # Schema Registry, no extension Without all three (client + factory + flag), HTTP refs reject up front with a precise reason so the misconfiguration surfaces immediately: - allowRemoteRefs:true + missing client/factory -> InvalidArgumentException at configure() time (caught before any spec is loaded) - allowRemoteRefs:false + ref to URL -> RemoteRefDisallowed - allowRemoteRefs:false + httpClient set -> E_USER_WARNING at configure() to flag the wired-but-disabled mistake (PHPUnit's failOnWarning elevates this) - allowRemoteRefs:true + 4xx/5xx/network error -> RemoteRefFetchFailed - URL with no .json/.yaml extension AND no JSON/YAML Content-Type -> UnsupportedExtension Implementation: - New `RefResolutionContext` DTO consolidates source file + HTTP wiring + flag into one immutable carrier. Replaces the 6-arg walk()/resolve() signatures that PR #119's review (S3) flagged as unwieldy. - New `Internal\HttpRefLoader` mirrors `ExternalRefLoader`: PSR-18 fetch, status check, format detection (URL extension first, then Content-Type), per-resolution cache keyed by canonical URL, exception mapping for ClientExceptionInterface and HTTP error codes. - New `Internal\SpecDocumentDecoder` extracts the JSON/YAML decoding logic that ExternalRefLoader and HttpRefLoader both need. - `OpenApiRefResolver::descendIntoLoadedDocument()` extracted: the fragment-vs-whole-file dispatch + cycle-key + walk recursion is identical for filesystem and HTTP loads, so both paths share it. - `OpenApiSpecLoader::configure()` gains optional client + factory + allowRemoteRefs parameters with the validation/warning rules above. `reset()` clears them so test isolation holds. - Cross-source refs work in both directions: local file -> HTTP URL, HTTP URL -> local file. Cycle detection's canonical chain key handles mixed-source loops. Three new `InvalidOpenApiSpecReason` cases: `RemoteRefDisallowed`, `RemoteRefFetchFailed`, `HttpClientNotConfigured`. The previous `RemoteRefNotImplemented` is marked @deprecated; kept for callers that branched on it. Test seam: - New `Tests\Helpers\FakeHttpClient` is a minimal PSR-18 implementation that returns pre-canned responses keyed by URL. - 13 new `HttpRefLoaderTest` cases covering format detection (URL extension + Content-Type fallbacks for `application/json`, `application/*+json`, `application/yaml`, `text/yaml`), per-call cache, 404/5xx/network errors, malformed body, non-mapping root. - 12 new `OpenApiRefResolverHttpRefsTest` cases covering the resolver's HTTP branch end-to-end: opt-in success, fragments, multi-hop chains, cross-file cycles, cross-source local<->HTTP chains, sibling fragment reuse, opt-in/wiring failure modes, JSON Pointer escapes, missing fragment, trailing-hash rejection. - 3 new `OpenApiSpecLoaderTest` cases pinning the configure-time guards (InvalidArgumentException, E_USER_WARNING, no-warning happy path). README: - Comparison table flips `External $ref auto-resolution` to checkmark for this library. - New "HTTP $ref resolution (opt-in)" section documents wiring, failure modes, format detection, and security stance. - Setup section rewritten: pre-bundling no longer required for internal/local refs. Redocly remains supported for users on the legacy bundled-spec workflow. Dependencies: - `require`: `psr/http-client`, `psr/http-factory`, `psr/http-message` (interface-only, ~zero size impact). - `require-dev`: `guzzlehttp/psr7` for tests. - `suggest`: `guzzlehttp/guzzle`, `symfony/http-client` as examples. Quality gates: - PHPUnit: 745 tests / 1481 assertions pass (+44 vs PR 1/2 baseline) - PHPStan level 6: 0 errors - PHP-CS-Fixer: 0 violations
Round 1 (Critical): - Stream errors during HTTP body read no longer silently surface as MalformedJson/Yaml. getBody()->getContents() inside try/catch re-wraps RuntimeException as RemoteRefFetchFailed with the underlying message, so the operator sees the I/O cause. - Relative refs inside HTTP-loaded documents now resolve against the source URL per RFC 3986. `$ref: './pet.yaml'` inside `https://example.com/openapi.json` correctly fetches `https://example.com/pet.yaml` instead of producing a confusing LocalRefNotFound. - FakeHttpClientUnexpectedRequest moved to its own file. PSR-4 autoloaders that import the class standalone (instead of transitively via FakeHttpClient) now work as expected. Round 2 (Important): - E_USER_WARNING for "client wired without flag" promoted to InvalidArgumentException. Warnings can be suppressed by error handlers / error_reporting masks; a hard exception cannot. The pairing invariant is now enforced symmetrically at configure time. - Yaml::parse() catch broadened from ParseException to Throwable so malformed-Unicode and OOM-during-parse errors land as MalformedYaml instead of propagating as raw stack traces. - 3xx redirect responses now produce a dedicated message including the Location header and a hint to configure the PSR-18 client to follow redirects (Guzzle does by default; Symfony does not). - userinfo (`https://user:pass@host/...`) is redacted from all error messages and exception `ref` fields so credentials don't leak into CI logs. - OpenApiSpecLoader::configure() now evicts the spec cache so a policy change (especially flipping allowRemoteRefs) takes effect on the next load() instead of silently serving the previously resolved spec. - Multi-hop HTTP fetch failures surface the resolution chain in the error message ("via $ref chain: A -> B -> C") so a failure 4 hops deep is debuggable from the leaf URL alone. - Content-Type detection now splits on `,` as well as `;`, so RFC-violating duplicate Content-Type headers (concatenated by PSR-7's getHeaderLine) still resolve correctly. - JSON depth-limit errors get a dedicated message (deeply recursive schemas hit JSON_ERROR_DEPTH; the message now distinguishes "your JSON is invalid" from "your nesting exceeds 512 levels"). Documentation: - README HTTP $ref section: install hint for Guzzle 7+, RFC 3986 base resolution example, "Schema Registry" wording rewritten as "URLs without a recognisable extension still work as long as the server sets a usable Content-Type" (no third-party trademark). - ExternalRef enum @deprecated docblock now points at the actual cases (RemoteRefDisallowed/HttpClientNotConfigured/etc.) instead of the now-also-deprecated RemoteRefNotImplemented. - OpenApiRefResolver::resolve() docblock corrected: "any non-internal $ref triggers LocalRefRequiresSourceFile" was inaccurate after this PR; it's actually "any filesystem-relative $ref" (HTTP refs go through resolveHttpRef). - canonicalizeUri docblock trimmed; long rationale moved to a one- liner. Tests added (10): - 3xx redirect rejection with Location-bearing message - URL extension wins over conflicting Content-Type - Empty Content-Type header - Duplicate Content-Type headers (RFC-violating, comma-separated) - userinfo redaction in error messages - Per-resolution cache scope (second resolve() re-fetches) - Relative ref inside HTTP doc resolves to URL (RFC 3986) - Parent-dir relative ref against HTTP base - Absolute-path ref inside HTTP doc resolves to same host - Multi-hop fetch failure includes chain in error message - configure() evicts cached specs (policy-change pin) - reset() clears HTTP client/factory state Round 3 (Type design): - New `Internal\LoadedDocument` readonly DTO replaces the divergent `array{absolutePath, decoded}` and `array{absoluteUri, decoded}` shapes returned by ExternalRefLoader and HttpRefLoader. Both loaders now return the same `LoadedDocument(canonicalIdentifier, decoded)` so the resolver's chain-key logic is uniform across filesystem and HTTP loads. - `RefResolutionContext` constructor made private. New named factories `filesystemOnly()` and `withRemoteRefs($client, $factory, $sourceFile)` enforce the pairing invariant at the type level — it is structurally impossible to construct a context with `allowRemoteRefs=true` and a null client. - Comment cleanup on canonicalizeUri docblock and test prose ("Schema Registry hybrid" → neutral phrasing). Suggestion deferred: - S2 LoadedDocument DTO consolidates loader returns; cache itself remains primitive `array<string, array<string, mixed>>` for now. A `LoadedDocumentCache` class would naturally pair with this DTO but adds churn that is out of scope for this PR. Quality gates: - PHPUnit: 757 tests / 1502 assertions pass (+12 vs first push) - PHPStan level 6: 0 errors - PHP-CS-Fixer: 0 violations
Collaborator
Author
Review feedback addressed (Round 1 + 2 + 3)Pushed commit Round 1 (Critical) — fixed
Round 2 (Important) — fixed
Plus S5 (extension-wins-over-Content-Type), S6 (empty Content-Type), and S7 (duplicate Content-Type comma split) all got dedicated tests. Round 3 (Type design) — fixed
Diff stats vs first push
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Closes #108. Builds on PR 1/2 (#119) which added local-filesystem ref support.
The resolver now follows
$refpaths to HTTP(S) URLs when the user opts in:Design decisions (chat-confirmed)
httpClientset withoutallowRemoteRefs: truetriggersE_USER_WARNINGatconfigure()time so the misconfiguration doesn't sit silent. Belt + suspenders prevents accidental network calls.RefResolutionContextDTO — bundled into this PR (S3 from feat(spec-loader): resolve local-filesystem external $refs (#108, PR 1/2) #119 review). Thewalk()/resolve()signatures already had 6 params; adding 3 more for HTTP would have crossed into unmaintainable territory.Failure modes — all surfaced precisely
allowRemoteRefs: true+ missing client/factoryInvalidArgumentExceptionatconfigure()httpClientset +allowRemoteRefs: falseE_USER_WARNINGatconfigure()$ref+allowRemoteRefs: falseRemoteRefDisallowed$ref+ flag on + missing client/factoryHttpClientNotConfigured$ref+ opt-in + 4xx/5xx/network errorRemoteRefFetchFailed$ref+ URL has no.json/.yamlAND no JSON/YAMLContent-TypeUnsupportedExtensionRemoteRefNotImplementedfrom PR 1/2 is@deprecated(kept for callers branching on it).Test plan
HttpRefLoaderTest(13 tests): URL-extension + Content-Type format detection (incl.application/*+json,application/yaml,text/yaml), per-call cache, 404/5xx/network error mapping, malformed body, non-mapping rootOpenApiRefResolverHttpRefsTest(12 tests): opt-in success path, fragments, multi-hop HTTP chains, cross-file HTTP cycles, cross-source local↔HTTP chains, sibling fragment URL reuse, opt-in/wiring failure modes, JSON Pointer escapes (~1/~0) in remote fragments, missing fragment, trailing-hash rejectionOpenApiSpecLoaderTest(3 new tests): configure-timeInvalidArgumentException,E_USER_WARNINGfor wired-but-disabled, no-warning happy pathFakeHttpClientreturns canned responses; no real network callsNew types / classes
RefResolutionContext(DTO, public) — immutable carrier for sourceFile + httpClient + requestFactory + allowRemoteRefsInternal\HttpRefLoader— PSR-18 fetch + decodeInternal\SpecDocumentDecoder— shared JSON/YAML decoder for bothExternalRefLoaderandHttpRefLoaderTests\Helpers\FakeHttpClient— minimal PSR-18 test doubleDependencies
require(interface-only packages, ~zero size impact):psr/http-client: ^1.0psr/http-factory: ^1.0psr/http-message: ^1.0 || ^2.0require-dev:guzzlehttp/psr7: ^2.0(test PSR-7 implementations)suggest:guzzlehttp/guzzle,symfony/http-clientas concrete client examples.README updates
External $ref auto-resolution❌ → ✅$refresolution (opt-in)" documents wiring, failure modes, format detection, and security stanceOut of scope (future issues)
allowRemoteRefs: falsedefault + spec author trust)ResponseValidationContextDTO from PR feat(response-validator): validate response headers against spec (#110) #120 review (separate concern)Closes
Closes #108