Skip to content

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

Merged
merged 11 commits into from
Jul 1, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 26, 2025

Summary

This PR adds two new CreateJson overloads to BinaryContent 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:

BinaryContent content = BinaryContent.Create(BinaryData.FromObjectAsJson(
    new {
        model = "gpt-4.1",
        prompt = new {
            id = "pmpt_abc123",
            version = "2",
            variables = new {
                customer_name = "Jane Doe",
                product = "40oz juice box"
            }
        }
    }
));

Solution

This PR introduces two new static factory methods that provide a more convenient API:

// Simple case with default serialization options
BinaryContent content = BinaryContent.CreateJson(
    new {
        model = "gpt-4.1",
        prompt = new {
            id = "pmpt_abc123",
            version = "2",
            variables = new {
                customer_name = "Jane Doe",
                product = "40oz juice box"
            }
        }
    }
);

// With custom JsonSerializerOptions
BinaryContent content = BinaryContent.CreateJson(obj, options);

// With JsonTypeInfo for AOT scenarios
BinaryContent content = BinaryContent.CreateJson(obj, jsonTypeInfo);

API Changes

Added two new public methods to BinaryContent:

public static BinaryContent CreateJson<T>(T jsonSerializable, JsonSerializerOptions? options = default)
public static BinaryContent CreateJson<T>(T jsonSerializable, JsonTypeInfo<T> jsonTypeInfo)

Implementation Details

  • Uses existing BinaryData.FromObjectAsJson internally for consistent behavior
  • Reuses the existing BinaryDataBinaryContent internal class
  • Added proper XML documentation and pragma directives to handle linter rules
  • Updated API surface files for both .NET Standard 2.0 and .NET 8.0
  • Added comprehensive tests covering various scenarios

Testing

  • All existing tests continue to pass (68/68)
  • Added new tests for both overloads with different scenarios:
    • Basic JSON serialization
    • Custom JsonSerializerOptions
    • Verification that results match the existing pattern
    • Both sync and async operations

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.

@KrzysztofCwalina
Copy link
Member

@copilot, continue working on this PR. I changed the credits

Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] [FEATURE REQ] Add more Create overloads to BinaryContent Add CreateJson overloads to BinaryContent for simplified JSON serialization Jun 26, 2025
Copilot finished work on behalf of KrzysztofCwalina June 26, 2025 23:31
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jun 26, 2025

@copilot, continue working on this PR. I changed the credits

Completed all requested changes including adding MediaType property and consolidating tests. All tests are passing. Commit 72e14a1

Copilot finished work on behalf of KrzysztofCwalina June 26, 2025 23:58
@Copilot Copilot AI requested a review from KrzysztofCwalina June 26, 2025 23:58
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@Copilot Copilot AI requested a review from KrzysztofCwalina June 27, 2025 00:09
Copilot finished work on behalf of KrzysztofCwalina June 27, 2025 00:09
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@KrzysztofCwalina
Copy link
Member

@copilot, continue working on this

…eedback

Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@KrzysztofCwalina KrzysztofCwalina marked this pull request as ready for review June 27, 2025 21:40
@KrzysztofCwalina KrzysztofCwalina requested a review from a team as a code owner June 27, 2025 21:40
Copy link
Contributor Author

Copilot AI commented Jun 27, 2025

lastly, public static BinaryContent Create(BinaryData value); needs to copy the MediaType from BinaryData to BinaryContent.

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?

Copilot finished work on behalf of KrzysztofCwalina June 27, 2025 21:44
Co-authored-by: jsquire <913445+jsquire@users.noreply.github.com>
@Copilot Copilot AI requested a review from jsquire June 27, 2025 21:59
Copilot finished work on behalf of jsquire June 27, 2025 21:59
Co-authored-by: KrzysztofCwalina <9724236+KrzysztofCwalina@users.noreply.github.com>
@Copilot Copilot AI requested a review from KrzysztofCwalina June 27, 2025 23:03
Copilot finished work on behalf of KrzysztofCwalina June 27, 2025 23:03
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina left a 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>
Copy link
Contributor Author

Copilot AI commented Jun 30, 2025

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 IL...

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

@Copilot Copilot AI requested a review from KrzysztofCwalina June 30, 2025 16:39
Copilot finished work on behalf of KrzysztofCwalina June 30, 2025 16:39
@KrzysztofCwalina KrzysztofCwalina enabled auto-merge (squash) June 30, 2025 17:26
@KrzysztofCwalina
Copy link
Member

/check-enforcer override

@KrzysztofCwalina KrzysztofCwalina merged commit 1d1f122 into main Jul 1, 2025
38 checks passed
@KrzysztofCwalina KrzysztofCwalina deleted the copilot/fix-50920 branch July 1, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQ] Add more Create overloads to BinaryContent
4 participants