Skip to content

Fix: Handle JsonSerializerOptions without TypeInfoResolver #1203

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.

Sign up for GitHub

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

Closed
GeorgiPopovIT wants to merge 1 commit into from
+19 −1
Closed

Fix: Handle JsonSerializerOptions without TypeInfoResolver #1203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ private static AIFunctionFactoryOptions CreateAIFunctionFactoryOptions(
Name = options?.Name ?? method.GetCustomAttribute<McpServerToolAttribute>()?.Name ?? DeriveName(method),
Description = options?.Description,
MarshalResult = static (result, _, cancellationToken) => new ValueTask<object?>(result),
SerializerOptions = options?.SerializerOptions ?? McpJsonUtilities.DefaultOptions,
SerializerOptions = GetSerializerOptions(options?.SerializerOptions),
JsonSchemaCreateOptions = options?.SchemaCreateOptions,
ConfigureParameterBinding = pi =>
{
Expand Down Expand Up @@ -580,4 +580,22 @@ private static CallToolResult ConvertAIContentEnumerableToCallToolResult(IEnumer
IsError = allErrorContent && hasAny
};
}

private static JsonSerializerOptions GetSerializerOptions(JsonSerializerOptions? customOptions)
{
if (customOptions is null)
{
return McpJsonUtilities.DefaultOptions;
}

if (customOptions.TypeInfoResolver is not null)
{
return customOptions;
}

customOptions.TypeInfoResolverChain.Add(McpJsonUtilities.JsonContext.Default);
customOptions.TypeInfoResolverChain.Add(AIJsonUtilities.DefaultOptions.TypeInfoResolver!);
Copy link
Contributor

@stephentoub stephentoub Jan 29, 2026

Choose a reason for hiding this comment

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

@eiriktsarpalis, is it desirable to mutate the externally-provided JsonSerializerOptions like this?

And is this thread-safe? What would happen if this JSO were passed to two McpServerTool.Create calls concurrently?

Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Νο. And even if it was, it would fail assuming the passed instance were read-only.

Copy link
Contributor

@stephentoub stephentoub Jan 29, 2026

Choose a reason for hiding this comment

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

@eiriktsarpalis, thanks. What is the right answer for #1150? We should support someone passing in new JsonSerializerOptions() { ... }.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 29, 2026

Choose a reason for hiding this comment

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

It's using the copy constructor like the PR was doing originally. That does have the side-effect of creating a new JSO and repopulating caches every time the method is invoked.

Copy link
Contributor

@stephentoub stephentoub Jan 29, 2026

Choose a reason for hiding this comment

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

How does JsonSerializer.Serialize(..., jso) avoid that? Or does it also create a new instance on each call?

Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 29, 2026

Choose a reason for hiding this comment

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

Reading #1150 more closely, I believe the user expects that we replicate the semantics of the non-AOT friendly JsonSerializer.Serialize(JsonSerializerOptions) methods which do silently mutate the JsonSerializerOptions. We can approximate that behavior while preserving AOT compatibility as follows:

if (options.TypeInfoResolver is null)
{
    Debug.Assert(!options.IsReadOnly, "If no resolver is present then the options must still be editable");
    options.TypeInfoResolver = McpJsonUtilities.DefaultOptions.TypeInfoResolver;
    options.MakeReadOnly();
}

Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 29, 2026

Choose a reason for hiding this comment

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

How does JsonSerializer.Serialize(..., jso) avoid that? Or does it also create a new instance on each call?

Just saw this but I think my last response should asnwer your question.


return customOptions;
}
}