fix: resolve dollar-ref chains and disambiguate component sections - #131
MaxMichel2 wants to merge 3 commits into
Conversation
OpenApiParser.resolveRef previously keyed resolved components by a ref's trailing name segment only, ignoring which components.<section> it named, and followed a chain exactly one level deep. This fixes both: a ref's fragment must now declare the section its call site expects (parameters/responses/examples/headers), so a same-named entry in a different section can never be silently conflated with the one actually referenced; and ref chains are followed until a non-ref entry is reached, guarded by a visited-set of (document, fragment) pairs that fails clearly on a cycle instead of hanging.
158fa62 to
4666236
Compare
Momik-jpg
left a comment
There was a problem hiding this comment.
Reviewed the diff at 4666236. The new section check still only inspects the final two fragment segments. For example, a response ref #/not-components/responses/UserOk (or #/responses/UserOk) passes actualSection == "responses" and resolves components.responses["UserOk"], even though the pointer does not identify that location.
Could you validate the complete supported fragment shape components/<expected-section>/<name> before the lookup, and add a regression case using an invalid prefix with an existing same-named response? The current wrong-section test does not exercise this path.
CI checks are green at this head; this finding is from source inspection, not a local runtime reproduction.
The previous section-disambiguation check only inspected the final two fragment segments. Fragments like "#/not-components/responses/UserOk" or "#/responses/UserOk" would pass the section check and incorrectly resolve to components.responses["UserOk"] despite not identifying that location. Now require the fragment to be exactly "components/<section>/<name>" (per the OpenAPI Reference Object convention), validating the structure before any section/name extraction. Update KDoc to reflect the enforced shape. Add two regression tests exercising malformed fragments that slip through the old check but fail on the new one. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Hey @Momik-jpg! Thanks for taking the time to identify this. Indeed, the specification was not 100% respected... I've pushed a new commit that should hopefully fix this. If not, what we'll do is merge the pending PRs and that should allow you to contribute a fix that fits your needs better :D |
|
Thanks! I reviewed ead88d9: it fixes both malformed-fragment cases, and CI is green. I found another issue with external reference chains: For example, Could you track each document’s source path, resolve each hop relative to that document, and add a regression test crossing directories? This finding is from source inspection; I haven’t reproduced it locally. |
OpenApiParser.ParseContext held a single baseDir (the root spec's), so every external $ref was loaded relative to the root spec even when it appeared in another file: specs/shared/a.json -> ./b.json loaded specs/b.json instead of specs/shared/b.json. Track each document's source path (SourceDocument) and resolve every hop relative to the document containing the $ref. resolveRef now also returns the document the final entry was found in, so nested local $refs (headers, examples) resolve against that document's components, and a relative externalValue resolves against its containing document rather than the root. The cycle guard now keys on (path, fragment) instead of structurally comparing whole documents. Add regression tests crossing directories (sibling and parent hops, with decoy files at the old root-relative location), nested local header refs, externalValue in an external document, and a cross-document cycle.
Summary
Stacked on #126 (PR chain: #123 → #124 → #127 → #128 → #129 → #126 → this) — merge in order.
OpenApiParser.resolveRefkeyed resolved$reftargets by a fragment's trailing name segment only, ignoring whichcomponents.<section>it actually named, and followed a$refchain exactly one level deep (a resolved entry's own$ref, if any, was not followed further). Real specs'componentssections cross-reference and chain more than this parser could handle — this is a prerequisite for #82/#84 (schema synthesis), which will need to resolve$refchains throughcomponents.schemasthe same way.What changed
OpenApiParser.resolveRef(private, insideParseContext): now takes an explicitsectionparameter ("parameters"/"responses"/"examples"/"headers") and validates a fragment's declared section against it — a$refnaming an unexpected section is rejected with a clear error instead of being silently resolved against whatever map the call site happened to expect, so a same-named entry in a different section can never be conflated with the one actually referenced.$ref(via a newrefOfparameter), until a non-ref entry is reached — a$refchain is now followed to completion instead of stopping after one hop.visited: MutableSet<Pair<OpenApiDocument, String>>tracks every(document, fragment)hop; revisiting one throws a clearIllegalStateExceptioninstead of looping forever.resolveParameter, and the response/example/header refs insideresolveResponseIndex/resolveHeaders) and the class-level KDoc's "Scope decisions" section to match.docs/modules/networkmock-core.md's "$refresolution" section updated — it documented the now-fixed "one level deep" limitation.Public API
None — the
openapipackage staysinternal(see #73's pure-seam requirement). Noapi.txtdiff.Tests
Added to
MockConfigRepositoryTest.kt, following the existing$reftest fixtures' style:`dollar-ref naming the wrong components section is rejected even if a same-named entry exists there`— a response$refpointing atcomponents/parameters/...instead ofcomponents/responses/..., with a same-named entry in both sections, now fails clearly instead of resolving against whichever map the call site expected.`local dollar-ref chain of two hops resolves to the final non-ref entry`—A→B→ literal content.`cyclic dollar-ref chain fails clearly instead of hanging`—A→B→A.Verification
All green, including a full repo-wide
testAndroidHostTestrun to confirm no fallout outside the networkmock modules.🤖 Generated with GitHub Copilot