-
Notifications
You must be signed in to change notification settings - Fork 5k
Add CreateJson overloads to BinaryContent for simplified JSON serialization #50921
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
@copilot, continue working on this PR. I changed the credits |
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
sdk/core/System.ClientModel/tests/Message/BinaryContentTests.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/tests/Message/BinaryContentTests.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/tests/Message/BinaryContentTests.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/tests/Message/BinaryContentTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@copilot, continue working on this |
…eedback Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
BinaryData doesn't have a MediaType property - it's a basic byte container from the .NET BCL. Could you clarify how to get the MediaType from BinaryData? Should the Create(BinaryData) method continue to return null for MediaType as it currently does, or is there another mechanism to determine the content type? |
Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com>
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
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.
The CI fails because the new PAIs are not AOT compliant:
Found 2 additional warnings that were not expected:
//sdk/core/System.ClientModel/src/Message/BinaryContent.cs(91): Trim analysis warning IL2026: System.ClientModel.BinaryContent.CreateJson(T,JsonSerializerOptions): Using member 'System.BinaryData.FromObjectAsJson(T,JsonSerializerOptions)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. [D:\a_work\1\s\eng\scripts\compatibility\TempAotCompatFiles\aotcompatibility.csproj]
//sdk/core/System.ClientModel/src/Message/BinaryContent.cs(91): AOT analysis warning IL3050: System.ClientModel.BinaryContent.CreateJson(T,JsonSerializerOptions): Using member 'System.BinaryData.FromObjectAsJson(T,JsonSerializerOptions)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. [D:\a_work\1\s\eng\scripts\compatibility\TempAotCompatFiles\aotcompatibility.csproj]
Deleting test app files.
I think you need to apply AOT annotations and/or support context. See how BinaryData does it at
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Memory.Data/src/System/BinaryData.cs#L88
…nings Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
Added AOT compliance attributes (RequiresDynamicCode and RequiresUnreferencedCode) to the CreateJson(T, JsonSerializerOptions?) method to resolve the IL2026 and IL3050 warnings. The attributes follow the same pattern used by BinaryData.FromObjectAsJson. Updated API surface files accordingly. Commit b34f3f1 |
/check-enforcer override |
Summary
This PR adds two new
CreateJson
overloads toBinaryContent
that simplify the common pattern of creating binary content from JSON-serializable objects.Problem
Currently, creating
BinaryContent
from a JSON object requires a two-step process:Solution
This PR introduces two new static factory methods that provide a more convenient API:
API Changes
Added two new public methods to
BinaryContent
:Implementation Details
BinaryData.FromObjectAsJson
internally for consistent behaviorBinaryDataBinaryContent
internal classTesting
JsonSerializerOptions
Fixes #50920
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.