Conversation
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…where finding Agent-Logs-Url: https://github.com/microsoft/OpenAPI.NET/sessions/4f39d2a9-62e7-497f-be81-7f0efcb876c9 Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
…ce and rename V3_0 to V30 Agent-Logs-Url: https://github.com/microsoft/OpenAPI.NET/sessions/0fac65d3-0bbb-44db-b82e-7648c8fb8f86 Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
|
@copilot we're missing a policy to map the requestBody property to a in:body parameter for the path conversion to v2 |
…sion Agent-Logs-Url: https://github.com/microsoft/OpenAPI.NET/sessions/3e249ace-8d41-41c9-af08-288d036a4def Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
Added in c0d89d1. The new
|
|
@copilot the policies aren't 'rooted' in the document then they can have false positive matches. For example the V2ResponseContentUnwrappingPolicy can match an extension or schema that just happens to have a path that matches ./responses/[^/]/content/[^/]*. Example paths that can cause incorrect path conversions: #/paths/ Attempt to root the regexes where possible (dollar sign), add additional negative tests |
… in schema trees Agent-Logs-Url: https://github.com/microsoft/OpenAPI.NET/sessions/ade1e55d-3bd6-4d67-baee-32d89b1ad9f9 Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
Addressed in c063087. Two rooting strategies were applied:
Added negative tests covering schema-property false positives, extension-path false positives, and component-schema paths where only the rename applies. |
|
There was a problem hiding this comment.
Pull request overview
Adds an experimental path/pointer conversion helper so JSON Pointer paths produced by the v3-based walker/validator can be translated to equivalent pointers for older specs (notably OpenAPI v2), addressing issue #2806’s incorrect pointer output for v2 inputs.
Changes:
- Introduces
OpenApiPathHelperwith pluggable policies to transform or null-out pointers for unsupported constructs by target spec version. - Adds
OpenApiValidatorError.GetVersionedPointer(...)as a convenience API for converting validation error pointers. - Adds comprehensive unit tests and new documentation for experimental API usage, including an
ExperimentalAttributepolyfill for non-.NET8 targets.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.OpenApi.Tests/Services/OpenApiPathHelperTests.cs | Adds coverage for v2/v3 pointer conversion behaviors and edge cases. |
| src/Microsoft.OpenApi/Validations/OpenApiValidatorError.cs | Adds experimental GetVersionedPointer convenience method. |
| src/Microsoft.OpenApi/Services/OpenApiPathHelper.cs | Implements path conversion logic via spec-targeted policies. |
| src/Microsoft.OpenApi/Services/IOpenApiPathRepresentationPolicy.cs | Introduces internal policy interface for conversions. |
| src/Microsoft.OpenApi/PublicAPI.Unshipped.txt | Declares new public APIs for PublicAPI analyzer. |
| src/Microsoft.OpenApi/Attributes/ExperimentalAttribute.cs | Provides non-.NET8 polyfill for ExperimentalAttribute. |
| docs/experimental-apis.md | Documents experimental API diagnostics and the new path conversion APIs. |
| ### Supported Transformations (v2 target) | ||
|
|
||
| | v3 Path Pattern | v2 Equivalent | | ||
| |---|---| | ||
| | `#/components/schemas/{name}/**` | `#/definitions/{name}/**` | | ||
| | `#/components/parameters/{name}/**` | `#/parameters/{name}/**` | | ||
| | `#/components/responses/{name}/**` | `#/responses/{name}/**` | | ||
| | `#/components/securitySchemes/{name}/**` | `#/securityDefinitions/{name}/**` | | ||
| | `.../responses/{code}/content/{mediaType}/schema/**` | `.../responses/{code}/schema/**` | | ||
| | `.../headers/{name}/schema/**` | `.../headers/{name}/**` | | ||
|
|
||
| ### Paths With No v2 Equivalent (returns `null`) | ||
|
|
||
| - `#/servers/**` | ||
| - `#/webhooks/**` | ||
| - `.../callbacks/**` | ||
| - `.../links/**` | ||
| - `.../requestBody/**` | ||
| - `.../content/{mediaType}/encoding/**` | ||
| - `#/components/examples/**`, `#/components/headers/**`, `#/components/pathItems/**`, | ||
| `#/components/links/**`, `#/components/callbacks/**`, `#/components/requestBodies/**`, | ||
| `#/components/mediaTypes/**` |
There was a problem hiding this comment.
The documentation contradicts the implementation: .../requestBody/** is listed as having “no v2 equivalent (returns null)”, but OpenApiPathHelper currently maps operation-level requestBody paths to .../parameters/0/.... Either remove requestBody from the “returns null” list and document the mapping (and add it to the Supported Transformations table), or change the implementation to return null for requestBody paths.
| public static string? GetVersionedPath(string path, OpenApiSpecVersion targetVersion) | ||
| { | ||
| if (string.IsNullOrEmpty(path) || targetVersion == OpenApiSpecVersion.OpenApi3_2) | ||
| { | ||
| return path; | ||
| } |
There was a problem hiding this comment.
GetVersionedPath is implemented to accept null/empty (string.IsNullOrEmpty(path)), but its public signature is string path. This forces callers to use the null-forgiving operator to pass a possibly-null pointer (as the tests do). Consider changing the signature to string? path (and updating PublicAPI.Unshipped) or, if null isn’t intended to be supported, remove the null-handling branch/tests to keep the API contract consistent.
| // Walk through segments looking for v3-only constructs | ||
| for (var i = 1; i < segments.Length; i++) | ||
| { | ||
| var segment = segments[i]; | ||
|
|
||
| // Skip segments that are inside a schema property tree — they are property names, | ||
| // not structural OpenAPI keywords. | ||
| if (OpenApiPathHelper.IsSchemaContext(segments, i)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // servers, callbacks, links at any nested level | ||
| if (UnsupportedSegments.Contains(segment)) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
V2UnsupportedPathPolicy treats servers/callbacks/links segments as unsupported at any nested level (outside schema properties). For pointers that traverse extension payloads (e.g., #/x-foo/servers/0), this will return null even though extensions are version-agnostic and the path should typically be left unchanged. Consider adding an “extension context” guard (similar to IsSchemaContext) that disables v2 structural keyword handling after any segment starting with x- (OpenApiConstants.ExtensionFieldNamePrefix).
| // +1 because requestBody (1 segment) is replaced by parameters/0 (2 segments) | ||
| var buffer = new string[segments.Length + 1]; | ||
| var written = 0; | ||
|
|
||
| // Copy segments before requestBody | ||
| for (var i = 0; i < requestBodyIndex; i++) | ||
| { | ||
| buffer[written++] = segments[i]; | ||
| } | ||
|
|
||
| // Replace requestBody with parameters/0 | ||
| buffer[written++] = OpenApiConstants.Parameters; | ||
| buffer[written++] = "0"; | ||
|
|
||
| // Skip content/{mediaType} if both segments immediately follow requestBody | ||
| var next = requestBodyIndex + 1; | ||
| if (next + 1 < segments.Length && | ||
| string.Equals(segments[next], OpenApiConstants.Content, StringComparison.Ordinal)) | ||
| { | ||
| next += 2; // skip "content" and "{mediaType}" | ||
| } | ||
|
|
||
| // Copy remaining segments | ||
| for (var i = next; i < segments.Length; i++) | ||
| { | ||
| buffer[written++] = segments[i]; | ||
| } | ||
|
|
There was a problem hiding this comment.
V2RequestBodyToBodyParameterPolicy hard-codes requestBody → parameters/0. In Swagger v2, the body parameter can appear at any index within the operation’s parameters array, so .../parameters/0 may point at the wrong parameter (e.g., when query/path parameters come first). If the conversion can’t reliably determine the correct index without document context, consider returning null (no equivalent) for requestBody paths, or redesigning this conversion to be document-aware so it can locate the actual body parameter index.
| // +1 because requestBody (1 segment) is replaced by parameters/0 (2 segments) | |
| var buffer = new string[segments.Length + 1]; | |
| var written = 0; | |
| // Copy segments before requestBody | |
| for (var i = 0; i < requestBodyIndex; i++) | |
| { | |
| buffer[written++] = segments[i]; | |
| } | |
| // Replace requestBody with parameters/0 | |
| buffer[written++] = OpenApiConstants.Parameters; | |
| buffer[written++] = "0"; | |
| // Skip content/{mediaType} if both segments immediately follow requestBody | |
| var next = requestBodyIndex + 1; | |
| if (next + 1 < segments.Length && | |
| string.Equals(segments[next], OpenApiConstants.Content, StringComparison.Ordinal)) | |
| { | |
| next += 2; // skip "content" and "{mediaType}" | |
| } | |
| // Copy remaining segments | |
| for (var i = next; i < segments.Length; i++) | |
| { | |
| buffer[written++] = segments[i]; | |
| } | |
| // In Swagger v2, the body parameter can appear at any index within the | |
| // operation's parameters array. This path-only conversion policy does not | |
| // have enough document context to determine the correct index reliably, so | |
| // it must not guess by hard-coding parameters/0. | |
| return false; |
fixes #2806