Repository navigation
fix: handle relative schema references without fragment and prevent UriFormatException (#3148) - #3153
Open
Hardik K (hardikkaurani) wants to merge 5 commits into
Conversation
…icrosoft#3148) Fix GetReferenceIdAndExternalResource in OpenApiV31Deserializer and OpenApiV32Deserializer so that references to external documents without a fragment (e.g., schemas/uuid.schema.json or an absolute URL) correctly populate ExternalResource and mark IsExternal = true. Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
…icrosoft#3148) Update OpenApiDocument.ResolveReference and OpenApiWorkspace.ToLocationUrl to resolve relative references safely against BaseUri or external document URIs using Uri.TryCreate, preventing unhandled UriFormatException and ArgumentNullException when resolving relative references without fragments. Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The changes break external-component resolution and valid discriminator mappings, alter fragmentless relative references during serialization, and misinterpret root-relative workspace URLs as file URIs.
4 open issues
What changed in this PR
This PR addresses #3148 by recognizing fragmentless external schema references in the OpenAPI 3.1 and 3.2 readers and making URI resolution safer in the document and workspace layers.
Changes:
- Classifies fragmentless schema references as external resources.
- Uses safe URI construction for reference resolution and supports absolute workspace locations without a base URL.
- Adds regression tests for deserialization, parsing, and workspace-backed resolution.
| File | Description |
|---|---|
test/Microsoft.OpenApi.Tests/Workspaces/OpenApiWorkspaceTests.cs |
Tests absolute URI lookup without a configured base URL. |
test/Microsoft.OpenApi.Tests/Models/OpenApiDocumentTests.cs |
Tests fragmentless relative references with and without registered targets. |
test/Microsoft.OpenApi.Readers.Tests/V32Tests/OpenApiSchemaReferenceDeserializerTests.cs |
Tests fragmentless external references in OpenAPI 3.2. |
test/Microsoft.OpenApi.Readers.Tests/V31Tests/RelativeReferenceTests.cs |
Adds a parsing regression test for a relative schema reference. |
test/Microsoft.OpenApi.Readers.Tests/V31Tests/OpenApiSchemaReferenceDeserializerTests.cs |
Tests fragmentless external references in OpenAPI 3.1. |
src/Microsoft.OpenApi/Services/OpenApiWorkspace.cs |
Updates absolute and relative URI construction for workspace lookup. |
src/Microsoft.OpenApi/Reader/V32/OpenApiV32Deserializer.cs |
Recognizes external resources without fragments. |
src/Microsoft.OpenApi/Reader/V31/OpenApiV31Deserializer.cs |
Recognizes external resources without fragments. |
src/Microsoft.OpenApi/Models/OpenApiDocument.cs |
Handles standalone external references and safely constructs resolution URIs. |
Review effort: Balanced
… mappings, and workspace URLs (microsoft#3148) - Preserve component fragments in OpenApiDocument.ResolveReference when external resource and component names collide (e.g. common.json#/components/schemas/common.json). - Preserve authored fragmentless relative pointers in BaseOpenApiReference.SetJsonPointerPath and GetExternalReferenceV3 to maintain exact round trips during serialization. - Preserve bare schema names in OpenApiDiscriminatorDeserializer.LoadMapping so local schemas in discriminator mapping and defaultMapping resolve properly. - Resolve root-relative locations against BaseUrl in OpenApiWorkspace.ToLocationUrl before fallback to absolute URI parsing. - Catch loader errors gracefully in OpenApiWorkspaceLoader to report diagnostics without aborting document resolution. - Add regression unit tests for colliding external references, discriminator bare schema mappings, workspace root-relative URI resolution, and round-trip serialization. Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
Contributor
Author
|
I've pushed an update addressing all Copilot review findings in commit [d0eeafad](d0eeafa). Key updates:
Added regression tests covering all four scenarios. Thank you for the detailed feedback. Whenever convenient, please take another look! |
Member
|
Hardik K (@hardikkaurani) this pull request is conflicting, please address the conflicts. |
There was a problem hiding this comment.
🟡 Changes recommended
Active issues include dropped component fragments that break reference serialization and resolution, filename-based discriminator misclassification, and swallowed cancellation exceptions.
3 open issues
4 resolved since last review
Review effort: Balanced
Comment on lines
+257
to
+260
| if (string.Equals(ExternalResource, Id, StringComparison.Ordinal) && !Id.Contains("#")) | ||
| { | ||
| return ExternalResource; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit fb6b8eb8.
- Added an internal
IsStandaloneflag toBaseOpenApiReferenceto explicitly distinguish standalone document references from component references, eliminating the equality check inGetExternalReferenceV3that caused fragments to be dropped when component names matched. - Updated all reference readers across V3, V3.1, and V3.2, including responses, parameters, headers, callbacks, examples, links, request bodies, security schemes, and media types, to preserve authored pointers through
SetJsonPointerPath. - Added regression tests in
OpenApiDocumentTestscovering colliding non-schema references and inRelativeReferenceTestsverifying round-trip preservation.
| } | ||
| } | ||
| if (result.Document != null) | ||
| catch (Exception ex) |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit fb6b8eb8.
Excluded OperationCanceledException from the catch block using an exception filter (catch (Exception ex) when (ex is not OperationCanceledException)), ensuring cancellation requests propagate correctly to the caller instead of being handled as regular errors.
Added regression tests in OpenApiWorkspaceStreamTests covering both loader-triggered and cancellation-token-triggered scenarios.
Comment on lines
+78
to
+80
| && !pointer.EndsWith(".json", StringComparison.OrdinalIgnoreCase) | ||
| && !pointer.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase) | ||
| && !pointer.EndsWith(".yml", StringComparison.OrdinalIgnoreCase); |
Contributor
Author
There was a problem hiding this comment.
Addressed in commit fb6b8eb8.
Removed the filename suffix checks (.json, .yaml, .yml) from IsBareSchemaName in both V3.1 and V3.2 deserializers. Identifiers without path separators (/) or URI fragments (#) are now treated as local component names, preserving local resolution for file-suffixed and forward-declared schemas in both mapping and defaultMapping.
Added regression tests in V31Tests/OpenApiDiscriminatorTests and V32Tests/OpenApiDiscriminatorTests covering these scenarios.
…thout-fragment-uri-format # Conflicts: # test/Microsoft.OpenApi.Tests/Models/OpenApiDocumentTests.cs
…cellation, and preserve file-suffixed discriminator schemas (microsoft#3148) - Exclude OperationCanceledException from OpenApiWorkspaceLoader catch handler to preserve caller cancellation token semantics. - Retain authored component fragments and reference pointers across non-schema reference kinds (responses, parameters, headers, callbacks, examples, links, request bodies, security schemes, media types). - Distinguish standalone references from component references via IsStandalone on BaseOpenApiReference, preventing colliding component names from dropping their fragment. - Preserve local schema resolution in OpenApiDiscriminatorDeserializer for bare schema names including file-suffixed (.json, .yaml, .yml) and forward-declared schemas. - Add regression test coverage for colliding non-schema references, loader cancellation, and file-suffixed/forward-declared discriminator mappings. Signed-off-by: Hardik K <hardikkaurani1@gmail.com>
Contributor
Author
|
I've resolved the merge conflicts with
All 2,013 unit and integration tests pass locally. Whenever you have a moment, I'd appreciate another look. Thank you! |
This branch has not been deployed
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.
Resolves #3148
Summary of Changes
External Resource Deserialization (OpenAPI 3.1 & 3.2):
OpenApiV31Deserializer.GetReferenceIdAndExternalResourceandOpenApiV32Deserializer.GetReferenceIdAndExternalResource, removed the&& pointer.Contains('#')constraint when determiningexternalResource.schemas/uuid.schema.json,./schemas/uuid.schema.json, or absolute schema URLs) now properly setExternalResourceand evaluateIsExternal = true, aligning behavior withOpenApiV3Deserializerand the OpenAPI 3.1/3.2 specification.Safe Relative URI Resolution:
OpenApiDocument.ResolveReference, standalone external references (reference.IsExternal && reference.ExternalResource == id) are routed directly touriLocation = id, avoiding an unintended#/components/schemas/{id}relative fragment suffix.new Uri(uriLocation)with safe URI construction usingUri.TryCreate. Relative references now combine safely againstBaseUrior the registered external document URI in the workspace without throwing unhandledUriFormatExceptionorArgumentNullException. When no base URI is available, the resolution returnsnullgracefully.OpenApiWorkspace.ToLocationUrl, added direct support for resolving absolute URI locations even whenBaseUrlis not configured, while safely falling back toBaseUrlfor relative locations viaUri.TryCreate.Regression Tests:
OpenApiSchemaReferenceDeserializerTests(V31 and V32) covering external reference pointers without fragments (schemas/uuid.schema.json,./schemas/uuid.schema.json,uuid.schema.json, and absolute URLs).RelativeReferenceTests(V31) verifying that documents containing relative schema references without fragments parse and pass validation without throwingUriFormatException.OpenApiDocumentTestsverifying safe reference resolution with and without workspace targets.OpenApiWorkspaceTestsverifying resolution of absolute URI references.Commits
fix(v31): recognize external resource in schema ref without fragment (#3148)fix(models): resolve relative reference URIs safely against BaseUri (#3148)