-
Notifications
You must be signed in to change notification settings - Fork 620
Hide experimental properties from the JSON source generator to avoid MCPEXP001 diagnostics #1260
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
Open
MackinnonBuck
wants to merge
1
commit into
main
Choose a base branch
from
mbuck/experimental-json-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+320
−7
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
53 changes: 53 additions & 0 deletions
53
src/ModelContextProtocol.Core/ExperimentalJsonConverter.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
| using System.Text.Json.Serialization.Metadata; | ||
|
|
||
| namespace ModelContextProtocol; | ||
|
|
||
| /// <summary> | ||
| /// A JSON converter that handles serialization of experimental MCP types through <c>object?</c> backing fields. | ||
| /// </summary> | ||
| /// <typeparam name="T">The experimental type to serialize/deserialize.</typeparam> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This converter is used on internal <c>object?</c> backing fields that shadow public experimental properties | ||
| /// marked with <see cref="ExperimentalAttribute"/>. By declaring the backing field | ||
| /// as <c>object?</c>, the System.Text.Json source generator does not walk the experimental type graph, preventing | ||
| /// MCPEXP diagnostics from being emitted in generated code in consuming projects. | ||
| /// </para> | ||
| /// <para> | ||
| /// Serialization delegates to <see cref="McpJsonUtilities.DefaultOptions"/>, which already contains source-generated | ||
| /// contracts for all experimental types. | ||
| /// </para> | ||
| /// </remarks> | ||
| internal sealed class ExperimentalJsonConverter<T> : JsonConverter<object?> where T : class | ||
| { | ||
| private static JsonTypeInfo<T> TypeInfo => (JsonTypeInfo<T>)McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(T)); | ||
|
|
||
| public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| if (reader.TokenType == JsonTokenType.Null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return JsonSerializer.Deserialize(ref reader, TypeInfo); | ||
| } | ||
|
|
||
| public override void Write(Utf8JsonWriter writer, object? value, JsonSerializerOptions options) | ||
| { | ||
| if (value is null) | ||
| { | ||
| writer.WriteNullValue(); | ||
| return; | ||
| } | ||
|
|
||
| if (value is not T typed) | ||
| { | ||
| throw new JsonException($"Expected value of type '{typeof(T).Name}' but got '{value.GetType().Name}'."); | ||
| } | ||
|
|
||
| JsonSerializer.Serialize(writer, typed, TypeInfo); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
186 changes: 186 additions & 0 deletions
186
tests/ModelContextProtocol.Tests/ExperimentalJsonConverterTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| using ModelContextProtocol.Protocol; | ||
| using System.Text.Json; | ||
|
|
||
| namespace ModelContextProtocol.Tests; | ||
|
|
||
| public static class ExperimentalJsonConverterTests | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ModelContextProtocol.Tests currently suppresses experimental warnings. Do we need any testing checking that applying the source generator on MCP types doesn't trigger experimental warnings? I foresee that this could easily regress in the future without us noticing. |
||
| { | ||
| [Fact] | ||
| public static void Tool_WithExecution_RoundTrips() | ||
| { | ||
| var original = new Tool | ||
| { | ||
| Name = "test-tool", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Optional } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<Tool>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.Equal("test-tool", deserialized.Name); | ||
| Assert.NotNull(deserialized.Execution); | ||
| Assert.Equal(ToolTaskSupport.Optional, deserialized.Execution.TaskSupport); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void Tool_WithNullExecution_RoundTrips() | ||
| { | ||
| var original = new Tool { Name = "simple-tool" }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<Tool>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.Null(deserialized.Execution); | ||
| Assert.DoesNotContain("execution", json); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void ServerCapabilities_WithTasks_RoundTrips() | ||
| { | ||
| var original = new ServerCapabilities | ||
| { | ||
| Tasks = new McpTasksCapability() | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<ServerCapabilities>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.NotNull(deserialized.Tasks); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void ServerCapabilities_WithNullTasks_OmitsProperty() | ||
| { | ||
| var original = new ServerCapabilities(); | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.DoesNotContain("tasks", json); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void ClientCapabilities_WithTasks_RoundTrips() | ||
| { | ||
| var original = new ClientCapabilities | ||
| { | ||
| Tasks = new McpTasksCapability() | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<ClientCapabilities>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.NotNull(deserialized.Tasks); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void CallToolResult_WithTask_RoundTrips() | ||
| { | ||
| var original = new CallToolResult | ||
| { | ||
| Task = new McpTask | ||
| { | ||
| TaskId = "task-123", | ||
| Status = McpTaskStatus.Working, | ||
| CreatedAt = DateTimeOffset.UtcNow, | ||
| LastUpdatedAt = DateTimeOffset.UtcNow, | ||
| } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<CallToolResult>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.NotNull(deserialized.Task); | ||
| Assert.Equal("task-123", deserialized.Task.TaskId); | ||
| Assert.Equal(McpTaskStatus.Working, deserialized.Task.Status); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void CallToolRequestParams_WithTask_RoundTrips() | ||
| { | ||
| var original = new CallToolRequestParams | ||
| { | ||
| Name = "my-tool", | ||
| Task = new McpTaskMetadata { TimeToLive = TimeSpan.FromMinutes(5) } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<CallToolRequestParams>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.Equal("my-tool", deserialized.Name); | ||
| Assert.NotNull(deserialized.Task); | ||
| Assert.Equal(TimeSpan.FromMinutes(5), deserialized.Task.TimeToLive); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void CreateMessageRequestParams_WithTask_RoundTrips() | ||
| { | ||
| var original = new CreateMessageRequestParams | ||
| { | ||
| Messages = [], | ||
| MaxTokens = 100, | ||
| Task = new McpTaskMetadata { TimeToLive = TimeSpan.FromMinutes(10) } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<CreateMessageRequestParams>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.NotNull(deserialized.Task); | ||
| Assert.Equal(TimeSpan.FromMinutes(10), deserialized.Task.TimeToLive); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void ElicitRequestParams_WithTask_RoundTrips() | ||
| { | ||
| var original = new ElicitRequestParams | ||
| { | ||
| Message = "test prompt", | ||
| Task = new McpTaskMetadata { TimeToLive = TimeSpan.FromMinutes(15) } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(original, McpJsonUtilities.DefaultOptions); | ||
| var deserialized = JsonSerializer.Deserialize<ElicitRequestParams>(json, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.NotNull(deserialized); | ||
| Assert.NotNull(deserialized.Task); | ||
| Assert.Equal(TimeSpan.FromMinutes(15), deserialized.Task.TimeToLive); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void Tool_WithExecution_JsonPropertyNameIsCorrect() | ||
| { | ||
| var tool = new Tool | ||
| { | ||
| Name = "test", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Required } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(tool, McpJsonUtilities.DefaultOptions); | ||
|
|
||
| Assert.Contains("\"execution\"", json); | ||
| Assert.Contains("\"taskSupport\"", json); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void Tool_WriteIndented_IsRespected() | ||
| { | ||
| var tool = new Tool | ||
| { | ||
| Name = "test", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Optional } | ||
| }; | ||
|
|
||
| // Use caller options with WriteIndented = true | ||
| var options = new JsonSerializerOptions(McpJsonUtilities.DefaultOptions) { WriteIndented = true }; | ||
| string json = JsonSerializer.Serialize(tool, options); | ||
|
|
||
| // The output should be indented because WriteIndented is controlled by the writer | ||
| Assert.Contains("\n", json); | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Today,
JsonIncludeagainst inaccessible members only works with reflection but not the source generator. We'd want to mark this field as public and additionallyEditorBrowsable.Never. BecauseMcpTaskMetadatais itself marked as experimental this will likely cause experimental warnings to resurface on account of it producingnew ExperimentalJsonConverter<McpTaskMetadata>()expressions.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.
Thanks for pointing this out. This actually made me realize that the backing field pattern may introduce binary breaking changes regardless of how we structure it. When the experimental API stabilizes, removing the backing field and its converter breaks consumer source-generated code that references them, even for consumers who never used the experimental feature. The only way to avoid that is to keep the backing field and converter around indefinitely, which isn't ideal.
I'm starting to think a new feature in
System.Text.Jsonmight be the best way to solve this. One option is to introduce a new attribute like[JsonSourceGenerationIgnoreByDefault]that can be applied to a type. The source generator would skip generating serialization logic for that type unless the consumer explicitly registers it via[JsonSerializable(typeof(T))]on theirJsonSerializerContext. We'd apply it to all experimental DTO types (e.g.,McpTaskMetadata,McpTasksCapability). Consumers who don't use Tasks get no diagnostics and no generated code for those types. Consumers who do use Tasks can opt-in explicitly by adding the[JsonSerializable]entry, accepting the instability risk. When the type stabilizes, we remove the attribute, and the type becomes source-gen-visible by default, which is additive rather than breaking. TheJsonContextinMcpJsonUtilitieswould explicitly opt-in experimental types for serialization so that everything "just works" when using the SDK-providedJsonSerializerOptions. Does something like this seem feasible, @eiriktsarpalis? The main drawback I see is that it might not be obvious to developers why experimental properties aren't getting serialized by default when using a customJsonSerializerContext.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.
When it comes to improving the STJ generator itself, we have many options. The problem with that approach of course is that it won't be available to this library before the next LTS version of .NET gets released.