-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Update media type for JSON Patch #62988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use `Content-Type: application/json-patch+json` for JSON patch. - Fix some code analyzer suggestions. - Fix broken `build.cmd` for JsonPatch. Adapted from dotnet#62057. Resolves dotnet#61956.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the media type for JSON Patch requests from the default application/json
to the RFC 6902 compliant application/json-patch+json
. The implementation adds IEndpointParameterMetadataProvider
to the JsonPatchDocument classes to automatically configure the correct content type in OpenAPI documentation and ASP.NET Core endpoints.
Key changes:
- Implements
IEndpointParameterMetadataProvider
in JsonPatchDocument classes to set proper content type metadata - Updates code to use modern C# patterns (collection expressions, object initializers)
- Fixes the build script path for the JsonPatch project
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
JsonPatchDocumentOfT.cs (both versions) | Implements metadata provider interface and modernizes code syntax |
JsonPatchDocument.cs (both versions) | Implements metadata provider interface and modernizes code syntax |
Sample project files | Adds JsonPatch dependency and test endpoint for demonstration |
OpenAPI test snapshots | Shows updated content type in generated documentation |
Project files | Adds required HTTP abstractions dependency |
build.cmd | Fixes incorrect repository root path |
"content": { | ||
"application/json-patch+json": { | ||
"schema": { | ||
"$ref": "#/components/schemas/JsonPatchDocumentOfParentObject" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we need a custom JSON convert to model the schema for JSON path document. I assume the schema looks something like:
{
type: "object",
properties: {
"op": "string",
"path": "string",
"value": "string"
}
}
@mikekistler Sanity checking this by you. Does it seem sufficient to use the same schema for all JsonPatchDocument types or should we figure out some way to capture the properties of the model that you're patchinmg in the schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If needed I could look into doing that in a follow-up, as that's existing behaviour anyway without the media type change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the JsonPatchDocumentOfParentObject
is just this (line 624):
"JsonPatchDocumentOfParentObject": { },
I think we can tackle the issue of the patch body schema in a separate PR. We should probably open an issue for this. I don't have an issue with using the same schema for all patch operations, but it should be more like the schema you show above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sounds good. We can probably add special handling for the JsonPatchDocument
type the same way we do for IFormFile
and PipeReader
. Hopefully, there's no referemce issues adding a reference to the patching library in the OpenAPI lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM sans schema follow-up!
Pick up changes from dotnet#62988. Resolves dotnet#63073.
Pick up changes from dotnet#62988. Resolves dotnet#63073.
Update media type for JSON Patch
Use
Content-Type: application/json-patch+json
for JSON patch.Description
Content-Type: application/json-patch+json
for JSON patch.build.cmd
for JsonPatch.Adapted from #62057.
Fixes #61956.