-
Notifications
You must be signed in to change notification settings - Fork 627
Hide experimental types from external source generators using internal property pattern #1301
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| using System.Text.Json.Serialization; | ||
| using ModelContextProtocol.Protocol; | ||
|
|
||
| namespace ModelContextProtocol.ExperimentalApiRegressionTest; | ||
|
|
||
| /// <summary> | ||
| /// This file validates that the System.Text.Json source generator does not produce | ||
| /// MCPEXP001 diagnostics for MCP protocol types with experimental properties. | ||
| /// </summary> | ||
| [JsonSerializable(typeof(Tool))] | ||
| [JsonSerializable(typeof(ServerCapabilities))] | ||
| [JsonSerializable(typeof(ClientCapabilities))] | ||
| [JsonSerializable(typeof(CallToolResult))] | ||
| [JsonSerializable(typeof(CallToolRequestParams))] | ||
| [JsonSerializable(typeof(CreateMessageRequestParams))] | ||
| [JsonSerializable(typeof(ElicitRequestParams))] | ||
| internal partial class ExperimentalPropertyRegressionContext : JsonSerializerContext; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net10.0;net9.0;net8.0</TargetFrameworks> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <IsPackable>false</IsPackable> | ||
|
|
||
| <!-- | ||
| This project validates that MCPEXP001 diagnostics are NOT emitted by the | ||
| System.Text.Json source generator for types with experimental properties. | ||
| tests/Directory.Build.props suppresses MCPEXP001 globally for all test projects, | ||
| but this project must NOT suppress it: its successful compilation IS the test. | ||
| --> | ||
| <NoWarn>$(NoWarn.Replace('MCPEXP001',''))</NoWarn> | ||
|
|
||
| <!-- | ||
| [JsonInclude] internal members trigger this warning only because the MCP C# SDK | ||
| is a project reference here. NuGet consumers won't see it since Roslyn doesn't import | ||
| internal members from metadata. | ||
| --> | ||
| <NoWarn>$(NoWarn);SYSLIB1038</NoWarn> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\ModelContextProtocol.Core\ModelContextProtocol.Core.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| using System.Reflection; | ||
| using System.Text.Json.Serialization; | ||
| using ModelContextProtocol.Protocol; | ||
|
|
||
| namespace ModelContextProtocol.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Validates the internal property pattern used for experimental MCP properties. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Experimental properties on stable protocol types must use an internal serialization | ||
|
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. How can we make sure this is being adequately advertised to users? The fact that we're silently dropping experimental properties only for externally defined source generators (but not externally defined reflection-based JSOs) seems to violate the principle of least astonishment, particularly in the face of us documenting that STJ has the same behavior regardless of what contract resolver is being used. Given how STJ is being used typically in AOT apps, users could be seeing regressions trigger in unexpected manner, consider the following example: JsonSerializerOptions options = new()
{
TypeInfoResolverchain = { MyContext.Default, McpJsonUtilities.DefaultOptions.Resolver }
};
[JsonSerializable(typeof(MyCustomType)]
//[JsonSerializable(typeof(CallToolRequestParams)]
partial class MyContext : JsonSerializerContext;Either uncommenting
Collaborator
Author
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. Yeah, that's the unfortunate footgun that this approach has. The brittle configuration is the main tradeoff this PR makes compared to #1260. I would think that at a minimum, we'd need to document somewhere that anyone serializing MCP protocol types via We could also add some kind of A more involved option could be to emit a compiler diagnostic if we detect that any protocol types are either directly or transitively included in a
Collaborator
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. The only other thing that's coming to mind for me is for us to implement an analyzer that detects if the JSON source generator is being used, understand this pattern we have, and raise diagnostics for the footgun. But I suggest we start by documenting this adequately and explore the analyzer separately (not blocking 1.0.0 GA). |
||
| /// property to hide the experimental type from external source generators. The public | ||
| /// property is marked [Experimental][JsonIgnore] and delegates to an internal *Core | ||
| /// property marked [JsonInclude][JsonPropertyName]. | ||
| /// </remarks> | ||
| public class ExperimentalInternalPropertyTests | ||
| { | ||
| [Fact] | ||
| public void ExperimentalProperties_MustBeHiddenFromSourceGenerator() | ||
MackinnonBuck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| // [Experimental] properties on stable protocol types must use the internal property | ||
| // pattern so the STJ source generator does not reference experimental types in | ||
| // generated code (which would trigger MCPEXP001 for consumers). | ||
| // | ||
| // Required pattern: | ||
| // 1. Mark the public property [Experimental][JsonIgnore] | ||
| // 2. Add an internal *Core property with [JsonInclude][JsonPropertyName] | ||
| // that the public property delegates to | ||
| // | ||
| // To stabilize: | ||
| // 1. Remove [Experimental] and [JsonIgnore] from the public property | ||
| // 2. Add [JsonPropertyName] to the public property | ||
| // 3. Convert to auto-property | ||
| // 4. Remove the internal *Core property | ||
|
|
||
| foreach (var (type, prop) in GetExperimentalPropertiesOnStableTypes()) | ||
| { | ||
| Assert.True( | ||
| prop.GetCustomAttribute<JsonIgnoreAttribute>() is not null, | ||
| $"{type.Name}.{prop.Name} is [Experimental] but missing [JsonIgnore]."); | ||
|
|
||
| Assert.True( | ||
| prop.GetCustomAttribute<JsonPropertyNameAttribute>() is null, | ||
| $"{type.Name}.{prop.Name} is [Experimental] and must not have [JsonPropertyName]."); | ||
| } | ||
| } | ||
|
|
||
| private static IEnumerable<(Type Type, PropertyInfo Property)> GetExperimentalPropertiesOnStableTypes() | ||
| { | ||
| var protocolTypes = typeof(Tool).Assembly.GetTypes() | ||
| .Where(t => t.Namespace == "ModelContextProtocol.Protocol" && t.IsClass && !t.IsAbstract); | ||
|
|
||
| foreach (var type in protocolTypes) | ||
| { | ||
| if (type.GetCustomAttributes().Any(a => a.GetType().Name == "ExperimentalAttribute")) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| foreach (var prop in type.GetProperties(BindingFlags.Public | BindingFlags.Instance)) | ||
| { | ||
| if (prop.GetCustomAttributes().Any(a => a.GetType().Name == "ExperimentalAttribute")) | ||
| { | ||
| yield return (type, prop); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
| using ModelContextProtocol.Protocol; | ||
|
|
||
| namespace ModelContextProtocol.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Validates that the internal property pattern used for experimental properties | ||
| /// produces the expected serialization behavior for SDK consumers using source generators. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// Experimental properties (e.g. <see cref="Tool.Execution"/>, <see cref="ServerCapabilities.Tasks"/>) | ||
| /// use an internal <c>*Core</c> property for serialization. A consumer's source-generated | ||
| /// <see cref="JsonSerializerContext"/> cannot see internal members, so experimental data is | ||
| /// silently dropped unless the consumer chains the SDK's resolver into their options. | ||
| /// </para> | ||
| /// <para> | ||
| /// These tests depend on <see cref="Tool.Execution"/> and <see cref="ServerCapabilities.Tasks"/> | ||
| /// being experimental. When those APIs stabilize, update these tests to reference whatever | ||
| /// experimental properties exist at that time, or remove them entirely if no experimental | ||
| /// APIs remain. | ||
| /// </para> | ||
| /// </remarks> | ||
| public class ExperimentalPropertySerializationTests | ||
| { | ||
| [Fact] | ||
| public void ExperimentalProperties_Dropped_WithConsumerContextOnly() | ||
| { | ||
| var options = new JsonSerializerOptions | ||
| { | ||
| TypeInfoResolverChain = { ConsumerJsonContext.Default } | ||
| }; | ||
|
|
||
| var tool = new Tool | ||
| { | ||
| Name = "test-tool", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Optional } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(tool, options); | ||
| Assert.DoesNotContain("\"execution\"", json); | ||
| Assert.Contains("\"name\"", json); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ExperimentalProperties_IgnoredOnDeserialize_WithConsumerContextOnly() | ||
| { | ||
| string json = JsonSerializer.Serialize( | ||
| new Tool | ||
| { | ||
| Name = "test-tool", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Optional } | ||
| }, | ||
| McpJsonUtilities.DefaultOptions); | ||
| Assert.Contains("\"execution\"", json); | ||
|
|
||
| var options = new JsonSerializerOptions | ||
| { | ||
| TypeInfoResolverChain = { ConsumerJsonContext.Default } | ||
| }; | ||
| var deserialized = JsonSerializer.Deserialize<Tool>(json, options)!; | ||
| Assert.Equal("test-tool", deserialized.Name); | ||
| Assert.Null(deserialized.Execution); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ExperimentalProperties_RoundTrip_WhenSdkResolverIsChained() | ||
| { | ||
| var options = new JsonSerializerOptions | ||
| { | ||
| TypeInfoResolverChain = | ||
| { | ||
| McpJsonUtilities.DefaultOptions.TypeInfoResolver!, | ||
| ConsumerJsonContext.Default, | ||
| } | ||
| }; | ||
|
|
||
| var tool = new Tool | ||
| { | ||
| Name = "test-tool", | ||
| Execution = new ToolExecution { TaskSupport = ToolTaskSupport.Optional } | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(tool, options); | ||
| Assert.Contains("\"execution\"", json); | ||
| Assert.Contains("\"name\"", json); | ||
|
|
||
| var deserialized = JsonSerializer.Deserialize<Tool>(json, options)!; | ||
| Assert.Equal("test-tool", deserialized.Name); | ||
| Assert.NotNull(deserialized.Execution); | ||
| Assert.Equal(ToolTaskSupport.Optional, deserialized.Execution.TaskSupport); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ExperimentalProperties_RoundTrip_WithDefaultOptions() | ||
| { | ||
| var capabilities = new ServerCapabilities | ||
| { | ||
| Tasks = new McpTasksCapability() | ||
| }; | ||
|
|
||
| string json = JsonSerializer.Serialize(capabilities, McpJsonUtilities.DefaultOptions); | ||
| Assert.Contains("\"tasks\"", json); | ||
|
|
||
| var deserialized = JsonSerializer.Deserialize<ServerCapabilities>(json, McpJsonUtilities.DefaultOptions)!; | ||
| Assert.NotNull(deserialized.Tasks); | ||
| } | ||
| } | ||
|
|
||
| [JsonSerializable(typeof(Tool))] | ||
| [JsonSerializable(typeof(ServerCapabilities))] | ||
| [JsonSerializable(typeof(ClientCapabilities))] | ||
| [JsonSerializable(typeof(CallToolResult))] | ||
| [JsonSerializable(typeof(CallToolRequestParams))] | ||
| [JsonSerializable(typeof(CreateMessageRequestParams))] | ||
| [JsonSerializable(typeof(ElicitRequestParams))] | ||
| internal partial class ConsumerJsonContext : JsonSerializerContext; |
Uh oh!
There was an error while loading. Please reload this page.