Skip to content

Prepend built-in metadata for SystemTextJsonFormatter - #1558

Open
Eric Erhardt (eerhardt) wants to merge 3 commits into
microsoft:mainfrom
eerhardt:eerhardt-requestid-source-generation
Open

Eric Erhardt (eerhardt) wants to merge 3 commits into
microsoft:mainfrom
eerhardt:eerhardt-requestid-source-generation

Conversation

@eerhardt

@eerhardt Eric Erhardt (eerhardt) commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Prepend StreamJsonRpc's source-generated STJ metadata directly in MassageUserDataSerializerOptions when the formatter is constructed or assigned options. Cancellation parameters work even when the application's context omits RequestId.
  • Preserve default reflection serialization when no resolver was supplied, without adding a reflection resolver to explicitly configured source-only options.
  • Use ordinary order-based configuration: built-in metadata precedes existing resolvers, callers can prepend a custom resolver afterward, and subsequently replacing TypeInfoResolver removes the built-in registration. No Lazy, custom locking, or deferred options preparation.
  • Update NativeAOT guidance and samples to assign fully configured options with JsonSerializerOptions = new() { TypeInfoResolver = MyContext.Default }.
  • Cover eager registration, resolver precedence and replacement, caller-option isolation, missing application metadata, cancellation delivery, and NativeAOT execution.

Related to #1407. This PR contains only automatic metadata registration. Making RequestIdSTJsonConverter public and testing direct consumer source generation is isolated in #1557. The converter remains internal here; both changes target main independently.

Validation

  • Release test-project build: zero warnings/errors.
  • 789 targeted formatter/RPC tests passed across net8.0, net9.0, and net472, excluding known unstable tests (TestCategory=FailsInCloudTest).
  • Updated samples build and shipping-library package/API validation passed with zero warnings/errors.
  • Windows x64 NativeAOT publish and smoke-program execution passed with the application context omitting RequestId.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 00:11
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is focused, documented, and comprehensively covers the relevant resolver, cancellation, and NativeAOT scenarios.

Review effort: Balanced
Findings: None

What changed in this PR

Adds deferred built-in System.Text.Json metadata fallback, enabling cancellation serialization with application-defined source-generation contexts while preserving resolver precedence.

Changes:

  • Lazily appends built-in metadata and freezes serializer options on first use.
  • Adds formatter, cancellation, and NativeAOT regression coverage.
  • Documents resolver-chain and reflection-fallback behavior.
File Description
src/​StreamJsonRpc/​SystemTextJsonFormatter.cs Implements deferred built-in metadata fallback.
test/​StreamJsonRpc.Tests/​SystemTextJsonFormatterTests.cs Tests resolver precedence and options behavior.
test/​StreamJsonRpc.Tests/​JsonRpcSystemTextJsonHeadersTests.cs Tests remote cancellation with generated metadata.
test/​NativeAOTCompatibility.Test/​SystemTextJson.cs Adds NativeAOT RequestId coverage.
docfx/​docs/​nativeAOT.md Documents metadata resolver requirements.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/StreamJsonRpc/SystemTextJsonFormatter.cs Outdated
Remove lazy serializer options preparation and allow subsequent resolver changes to override registration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:13
@eerhardt Eric Erhardt (eerhardt) changed the title Provide built-in metadata fallback for SystemTextJsonFormatter Prepend built-in metadata for SystemTextJsonFormatter Oct 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is consistent with the documented resolver semantics and has comprehensive targeted coverage.

Review effort: Balanced
Findings: None

Comment thread samples/cs/NativeAOT/SystemTextJson.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 1, 2026 16:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation is focused, correctly documented, and comprehensively tested across resolver and NativeAOT scenarios.

Review effort: Balanced
Findings: None

@AArnott

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@AArnott Andrew Arnott (AArnott) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recalling my prior approval because I now see that my concern about a breaking change is proven by your having to change the tests and sample so they keep working.
I'm still evaluating.

@AArnott Andrew Arnott (AArnott) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I was mistaken about the breaking change, as I thought the change was in the library when it was only in a sample.
Can you resolve the merge conflict though?

@AArnott
Andrew Arnott (AArnott) enabled auto-merge (squash) October 2, 2026 23:06

This branch has not been deployed

No deployments
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.

4 participants