From 4666236c6036c41fcc1ae780762270bb25d8e66e Mon Sep 17 00:00:00 2001 From: Maxime MICHEL Date: Wed, 23 Sep 2026 14:32:01 +0200 Subject: [PATCH] :bug: Resolve dollar-ref chains and disambiguate component sections OpenApiParser.resolveRef previously keyed resolved components by a ref's trailing name segment only, ignoring which components.
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. --- CHANGELOG.md | 9 + .../networkmock/core/openapi/OpenApiParser.kt | 128 +++++++-- .../repository/MockConfigRepositoryTest.kt | 262 +++++++++++++++--- docs/modules/networkmock-core.md | 6 +- 4 files changed, 330 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d89652..6f58a7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (`devview-networkmock-ktor`, #89) ### Fixed +- NetworkMock: `OpenApiParser`'s `$ref` resolution now disambiguates by the full + `components.
` a fragment names, not just its trailing name — a `$ref` whose fragment + points at an unexpected section (e.g. `components/parameters/Foo` where a `components/responses` + entry was expected) is rejected with a clear error instead of being silently resolved against + whatever section 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` chains — an entry that itself + declares another `$ref` — are now followed until a non-ref entry is reached (previously only one + level deep), guarded against cycles: a circular `$ref` chain now fails with a clear error instead + of hanging. (`devview-networkmock-core`) - NetworkMock: replaced ~35 unconditional `println` calls in `MockConfigRepository` and `NetworkMockPlugin` with gated [Kermit](https://github.com/touchlab/Kermit) logging (tag `DevViewNetworkMock`), consolidating the plugin's multi-line per-request trace into one diff --git a/devview-networkmock-core/src/commonMain/kotlin/com/worldline/devview/networkmock/core/openapi/OpenApiParser.kt b/devview-networkmock-core/src/commonMain/kotlin/com/worldline/devview/networkmock/core/openapi/OpenApiParser.kt index d7a0dd8..c664b86 100644 --- a/devview-networkmock-core/src/commonMain/kotlin/com/worldline/devview/networkmock/core/openapi/OpenApiParser.kt +++ b/devview-networkmock-core/src/commonMain/kotlin/com/worldline/devview/networkmock/core/openapi/OpenApiParser.kt @@ -33,9 +33,12 @@ internal data class ResolvedResponse( * - Response bodies are sourced only from `examples..externalValue`; an example * declared with an inline `value` is skipped, since this library keeps response bodies as * external files (see the epic's format decisions). - * - `$ref` and `externalValue` both resolve relative to the file that declares them, exactly - * one level deep, into `#/components//` — a `$ref` chain (a component that - * itself points at another `$ref`) is not followed. + * - `$ref` and `externalValue` both resolve relative to the file that declares them, into + * `#/components/
/` — a `$ref` chain (a component that itself points at + * another `$ref`) is followed until a non-ref entry is reached, guarded against cycles. + * Each hop's fragment must declare the section the caller expects (e.g. a response `$ref` + * must point into `components/responses`), so a same-named entry in a different section + * is never silently conflated with the one actually referenced. * - No schema resolution of any kind — this parser mocks, it does not validate or synthesize * bodies (see #82/#83/#84, explicitly out of scope for 0.2.0). */ @@ -154,7 +157,13 @@ internal object OpenApiParser { document: OpenApiDocument ): ParameterObject { val ref = raw.ref ?: return raw - return resolveRef(ref = ref, document = document) { it.components.parameters } + return resolveRef( + ref = ref, + document = document, + section = "parameters", + componentsOf = { it.components.parameters }, + refOf = { it.ref } + ) } suspend fun resolveResponseIndex( @@ -167,8 +176,11 @@ internal object OpenApiParser { val response = if (rawResponse.ref != null) { resolveRef( ref = rawResponse.ref, - document = document - ) { it.components.responses } + document = document, + section = "responses", + componentsOf = { it.components.responses }, + refOf = { it.ref } + ) } else { rawResponse } @@ -181,8 +193,11 @@ internal object OpenApiParser { val example = if (rawExample.ref != null) { resolveRef( ref = rawExample.ref, - document = document - ) { it.components.examples } + document = document, + section = "examples", + componentsOf = { it.components.examples }, + refOf = { it.ref } + ) } else { rawExample } @@ -209,7 +224,13 @@ internal object OpenApiParser { ): Map = raw .mapNotNull { (name, rawHeader) -> val header = if (rawHeader.ref != null) { - resolveRef(ref = rawHeader.ref, document = document) { it.components.headers } + resolveRef( + ref = rawHeader.ref, + document = document, + section = "headers", + componentsOf = { it.components.headers }, + refOf = { it.ref } + ) } else { rawHeader } @@ -218,33 +239,84 @@ internal object OpenApiParser { /** * Resolves a `$ref` string to its target, either locally (within [document]) or in - * another file, exactly one level deep — the resolved object's own `$ref` (if any) - * is not followed further. + * another file, following a chain of `$ref`s — an entry that itself declares a `$ref` + * is resolved again — until a non-ref entry is reached. + * + * [section] is the `components.
` key every hop's fragment must declare (e.g. + * `"responses"`); a fragment naming a different section (`#/components/schemas/Foo` + * when a `"responses"` entry was expected) is rejected, so a same-named entry in a + * different section is never silently conflated with the one actually referenced. + * [componentsOf] selects the matching `components.
` map from a document, and + * [refOf] extracts a resolved entry's own `$ref` (if any) so the chain can continue. + * + * @throws IllegalStateException if a `$ref` cannot be resolved, names an unexpected + * section, or the chain revisits a `(document, fragment)` pair already seen (a cycle). */ @Suppress("DocumentationOverPrivateFunction") private suspend fun resolveRef( ref: String, document: OpenApiDocument, - componentsOf: (OpenApiDocument) -> Map + section: String, + componentsOf: (OpenApiDocument) -> Map, + refOf: (T) -> String? ): T { - val (targetDocument, fragment) = if (ref.startsWith(prefix = "#/")) { - document to ref.removePrefix(prefix = "#/") - } else { - val filePath = ref.substringBefore(delimiter = "#") - val fragment = ref - .substringAfter( - delimiter = "#", - missingDelimiterValue = "" - ).removePrefix(prefix = "/") - loadExternalDocument(filePath = filePath) to fragment - } + val visited = mutableSetOf>() + var currentDocument = document + var currentRef = ref + + while (true) { + val (targetDocument, fragment) = locate( + ref = currentRef, + document = currentDocument + ) + + if (!visited.add(element = targetDocument to fragment)) { + error( + message = "Unresolvable \$ref '$ref': cyclic reference detected — " + + "'$currentRef' revisits an already-resolved fragment." + ) + } + + val segments = fragment.split("/") + val name = segments.lastOrNull() + ?: error( + message = "Unresolvable \$ref '$currentRef': fragment has no component name." + ) + val actualSection = segments.getOrNull(index = segments.size - 2) + ?: error( + message = "Unresolvable \$ref '$currentRef': fragment has no component section." + ) + if (actualSection != section) { + error( + message = "Unresolvable \$ref '$currentRef': expected a '$section' entry " + + "but the fragment points into '$actualSection'." + ) + } - val segments = fragment.split("/") - val name = segments.lastOrNull() - ?: error(message = "Unresolvable \$ref '$ref': fragment has no component name.") + val entry = componentsOf(targetDocument)[name] + ?: error( + message = "Unresolvable \$ref '$currentRef': no such entry in components.$section." + ) - return componentsOf(targetDocument)[name] - ?: error(message = "Unresolvable \$ref '$ref': no such entry in components.") + val nestedRef = refOf(entry) ?: return entry + currentDocument = targetDocument + currentRef = nestedRef + } + } + + /** Splits [ref] into the document it targets and its fragment, loading an external file if needed. */ + @Suppress("DocumentationOverPrivateFunction") + private suspend fun locate( + ref: String, + document: OpenApiDocument + ): Pair = if (ref.startsWith(prefix = "#/")) { + document to ref.removePrefix(prefix = "#/") + } else { + val filePath = ref.substringBefore(delimiter = "#") + val fragment = ref + .substringAfter(delimiter = "#", missingDelimiterValue = "") + .removePrefix(prefix = "/") + loadExternalDocument(filePath = filePath) to fragment } private suspend fun loadExternalDocument(filePath: String): OpenApiDocument { diff --git a/devview-networkmock-core/src/commonTest/kotlin/com/worldline/devview/networkmock/core/repository/MockConfigRepositoryTest.kt b/devview-networkmock-core/src/commonTest/kotlin/com/worldline/devview/networkmock/core/repository/MockConfigRepositoryTest.kt index 9bb550d..889aa73 100644 --- a/devview-networkmock-core/src/commonTest/kotlin/com/worldline/devview/networkmock/core/repository/MockConfigRepositoryTest.kt +++ b/devview-networkmock-core/src/commonTest/kotlin/com/worldline/devview/networkmock/core/repository/MockConfigRepositoryTest.kt @@ -8,6 +8,7 @@ import io.kotest.matchers.collections.shouldContainExactly import io.kotest.matchers.collections.shouldHaveSize import io.kotest.matchers.nulls.shouldBeNull import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain import kotlin.test.Test import kotlinx.coroutines.test.runTest @@ -32,7 +33,8 @@ class MockConfigRepositoryTest { @Test fun `loadConfiguration uses cache and avoids second file read`() = runTest { val loader = RecordingResourceLoader(resources = baseResources()) - val repository = MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) + val repository = + MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) repository.loadConfiguration().getOrThrow() repository.loadConfiguration().getOrThrow() @@ -43,7 +45,8 @@ class MockConfigRepositoryTest { @Test fun `invalidate forces loadConfiguration to re-read the spec file`() = runTest { val loader = RecordingResourceLoader(resources = baseResources()) - val repository = MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) + val repository = + MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) repository.loadConfiguration().getOrThrow() repository.invalidate() @@ -55,7 +58,8 @@ class MockConfigRepositoryTest { @Test fun `invalidate then loadConfiguration reflects a changed spec file`() = runTest { val loader = MutableResourceLoader(resources = baseResources()) - val repository = MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) + val repository = + MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = loader) val before = repository.loadConfiguration().getOrThrow() before.specs[0].operations.map { it.operationId } shouldContainExactly @@ -252,8 +256,9 @@ class MockConfigRepositoryTest { } @Test - fun `findMatchingMock picks the first spec that has a matching operation when hosts collide`() = runTest { - val firstSpec = """ + fun `findMatchingMock picks the first spec that has a matching operation when hosts collide`() = + runTest { + val firstSpec = """ { "info": { "title": "First" }, "servers": [ { "url": "https://api.example.com" } ], @@ -264,7 +269,7 @@ class MockConfigRepositoryTest { } } """.trimIndent() - val secondSpec = """ + val secondSpec = """ { "info": { "title": "Second" }, "servers": [ { "url": "https://api.example.com" } ], @@ -275,23 +280,26 @@ class MockConfigRepositoryTest { } } """.trimIndent() - val loader = RecordingResourceLoader( - resources = mapOf("specs/first.json" to firstSpec, "specs/second.json" to secondSpec) - ) - val repository = MockConfigRepository( - specPaths = listOf("specs/first.json", "specs/second.json"), - resourceLoader = loader - ) + val loader = RecordingResourceLoader( + resources = mapOf( + "specs/first.json" to firstSpec, + "specs/second.json" to secondSpec + ) + ) + val repository = MockConfigRepository( + specPaths = listOf("specs/first.json", "specs/second.json"), + resourceLoader = loader + ) - val match = repository.findMatchingMock( - host = "api.example.com", - path = "/api/only-in-first", - method = "GET" - ) + val match = repository.findMatchingMock( + host = "api.example.com", + path = "/api/only-in-first", + method = "GET" + ) - match?.specId shouldBe "first" - match?.operationId shouldBe "onlyInFirst" - } + match?.specId shouldBe "first" + match?.operationId shouldBe "onlyInFirst" + } @Test fun `findMatchingMock falls through to the next spec when the matched host has no matching operation`() = @@ -319,14 +327,21 @@ class MockConfigRepositoryTest { } """.trimIndent() val loader = RecordingResourceLoader( - resources = mapOf("specs/first.json" to firstSpec, "specs/second.json" to secondSpec) + resources = mapOf( + "specs/first.json" to firstSpec, + "specs/second.json" to secondSpec + ) ) val repository = MockConfigRepository( specPaths = listOf("specs/first.json", "specs/second.json"), resourceLoader = loader ) - val match = repository.findMatchingMock(host = "api.example.com", path = "/api/target", method = "GET") + val match = repository.findMatchingMock( + host = "api.example.com", + path = "/api/target", + method = "GET" + ) match?.specId shouldBe "second" match?.operationId shouldBe "target" @@ -371,8 +386,9 @@ class MockConfigRepositoryTest { } @Test - fun `x-devview failureRate is parsed as an operation-level field with no spec-wide default`() = runTest { - val spec = """ + fun `x-devview failureRate is parsed as an operation-level field with no spec-wide default`() = + runTest { + val spec = """ { "info": { "title": "Example" }, "servers": [ { "url": "https://api.example.com" } ], @@ -391,15 +407,15 @@ class MockConfigRepositoryTest { } } """.trimIndent() - val repository = createRepository(resources = mapOf(SPEC_PATH to spec)) + val repository = createRepository(resources = mapOf(SPEC_PATH to spec)) - val config = repository.loadConfiguration().getOrThrow() - val operations = config.specs[0].operations.associateBy { it.operationId } + val config = repository.loadConfiguration().getOrThrow() + val operations = config.specs[0].operations.associateBy { it.operationId } - // Unlike delayMs, a document-root failureRate is not a spec-wide default. - operations.getValue("flaky").failureRate shouldBe 0.1 - operations.getValue("steady").failureRate shouldBe null - } + // Unlike delayMs, a document-root failureRate is not a spec-wide default. + operations.getValue("flaky").failureRate shouldBe 0.1 + operations.getValue("steady").failureRate shouldBe null + } @Test fun `operation version is extracted from a v-n path segment`() = runTest { @@ -471,7 +487,12 @@ class MockConfigRepositoryTest { resources = mapOf(SPEC_PATH to spec, "responses/getUser-200.json" to """{"id":1}""") ) - val responses = repository.discoverResponseFiles(key = OperationKey(specId = "example", operationId = "getUser")) + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) responses shouldHaveSize 1 responses.single().statusCode shouldBe 200 @@ -521,17 +542,151 @@ class MockConfigRepositoryTest { ) ) - val responses = repository.discoverResponseFiles(key = OperationKey(specId = "example", operationId = "getUser")) + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) + + responses shouldHaveSize 1 + responses.single().content shouldBe """{"id":1}""" + } + + @Test + fun `dollar-ref naming the wrong components section is rejected even if a same-named entry exists there`() = + runTest { + val spec = """ + { + "info": { "title": "Example" }, + "servers": [ { "url": "https://api.example.com" } ], + "paths": { + "/api/users/{userId}": { + "get": { + "operationId": "getUser", + "responses": { + "200": { "${'$'}ref": "#/components/parameters/UserOk" } + } + } + } + }, + "components": { + "parameters": { + "UserOk": { "name": "userOk", "in": "query", "example": "not-a-response" } + }, + "responses": { + "UserOk": { + "content": { + "application/json": { + "examples": { + "default": { "externalValue": "/responses/getUser-200.json" } + } + } + } + } + } + } + } + """.trimIndent() + val repository = createRepository( + resources = mapOf(SPEC_PATH to spec, "responses/getUser-200.json" to """{"id":1}""") + ) + + val result = repository.loadConfiguration() + + result.isFailure shouldBe true + result.exceptionOrNull()?.message.orEmpty() shouldContain "expected a 'responses' entry" + } + + @Test + fun `local dollar-ref chain of two hops resolves to the final non-ref entry`() = runTest { + val spec = """ + { + "info": { "title": "Example" }, + "servers": [ { "url": "https://api.example.com" } ], + "paths": { + "/api/users/{userId}": { + "get": { + "operationId": "getUser", + "responses": { + "200": { "${'$'}ref": "#/components/responses/A" } + } + } + } + }, + "components": { + "responses": { + "A": { "${'$'}ref": "#/components/responses/B" }, + "B": { + "content": { + "application/json": { + "examples": { + "default": { "externalValue": "/responses/getUser-200.json" } + } + } + } + } + } + } + } + """.trimIndent() + val repository = createRepository( + resources = mapOf(SPEC_PATH to spec, "responses/getUser-200.json" to """{"id":1}""") + ) + + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) responses shouldHaveSize 1 responses.single().content shouldBe """{"id":1}""" } + @Test + fun `cyclic dollar-ref chain fails clearly instead of hanging`() = runTest { + val spec = """ + { + "info": { "title": "Example" }, + "servers": [ { "url": "https://api.example.com" } ], + "paths": { + "/api/users/{userId}": { + "get": { + "operationId": "getUser", + "responses": { + "200": { "${'$'}ref": "#/components/responses/A" } + } + } + } + }, + "components": { + "responses": { + "A": { "${'$'}ref": "#/components/responses/B" }, + "B": { "${'$'}ref": "#/components/responses/A" } + } + } + } + """.trimIndent() + val repository = createRepository(resources = mapOf(SPEC_PATH to spec)) + + val result = repository.loadConfiguration() + + result.isFailure shouldBe true + result.exceptionOrNull()?.message.orEmpty() shouldContain "cyclic reference detected" + } + @Test fun `discoverResponseFiles returns responses sorted by status code`() = runTest { val repository = createRepository(resources = baseResources()) - val responses = repository.discoverResponseFiles(key = OperationKey(specId = "example", operationId = "getUser")) + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) responses.map { it.statusCode } shouldBe listOf(200, 404) } @@ -540,7 +695,12 @@ class MockConfigRepositoryTest { fun `discoverResponseFiles discovers named example variants`() = runTest { val repository = createRepository(resources = multiExampleResources()) - val responses = repository.discoverResponseFiles(key = OperationKey(specId = "example", operationId = "getUser")) + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) responses shouldHaveSize 3 responses.map { it.exampleName } shouldContain "detailed" @@ -550,21 +710,28 @@ class MockConfigRepositoryTest { fun `discoverResponseFiles preserves declared example order within a status code`() = runTest { val repository = createRepository(resources = multiExampleResources()) - val responses = repository.discoverResponseFiles(key = OperationKey(specId = "example", operationId = "getUser")) + val responses = repository.discoverResponseFiles( + key = OperationKey( + specId = "example", + operationId = "getUser" + ) + ) - responses.filter { it.statusCode == 404 }.map { it.exampleName }.toSet() shouldBe setOf("default", "detailed") + responses.filter { it.statusCode == 404 }.map { it.exampleName } + .toSet() shouldBe setOf("default", "detailed") } @Test - fun `discoverResponseFiles returns empty list when operation declares no responses`() = runTest { - val repository = createRepository(resources = baseResources()) + fun `discoverResponseFiles returns empty list when operation declares no responses`() = + runTest { + val repository = createRepository(resources = baseResources()) - val responses = repository.discoverResponseFiles( - key = OperationKey(specId = "example", operationId = "doesNotExist") - ) + val responses = repository.discoverResponseFiles( + key = OperationKey(specId = "example", operationId = "doesNotExist") + ) - responses shouldBe emptyList() - } + responses shouldBe emptyList() + } @Test fun `loadMockResponse returns parsed response when example exists`() = runTest { @@ -693,7 +860,10 @@ class MockConfigRepositoryTest { } private fun createRepository(resources: Map): MockConfigRepository = - MockConfigRepository(specPaths = listOf(SPEC_PATH), resourceLoader = RecordingResourceLoader(resources)) + MockConfigRepository( + specPaths = listOf(SPEC_PATH), + resourceLoader = RecordingResourceLoader(resources) + ) private class RecordingResourceLoader( private val resources: Map diff --git a/docs/modules/networkmock-core.md b/docs/modules/networkmock-core.md index c2b5441..999c87c 100644 --- a/docs/modules/networkmock-core.md +++ b/docs/modules/networkmock-core.md @@ -127,7 +127,11 @@ Parameters, responses, examples, and headers may be declared via `$ref` instead - **Local**: `"$ref": "#/components/parameters/UserId"` resolves against the same document's `components`. - **External**: `"$ref": "./common.json#/components/responses/Error"` loads another file (relative to the spec's own location) via the same `NetworkMockResourceLoader`. -Refs resolve one level deep — a referenced component's own `$ref` (if any) is not followed further. +Refs are followed as a chain — a referenced component's own `$ref` (if any) is resolved again, +until a non-ref entry is reached — with a cycle guard that fails clearly instead of hanging on a +circular reference. Each ref's fragment must name the section its context expects (e.g. a response +`$ref` must point into `components/responses`), so a same-named entry in a different section is +never resolved by mistake. ### x-devview extension