Fix: Handle JsonSerializerOptions without TypeInfoResolver#1203
Fix: Handle JsonSerializerOptions without TypeInfoResolver#1203GeorgiPopovIT wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs
Outdated
Show resolved
Hide resolved
f2b2deb to
4c0cdec
Compare
Added a helper method which ensures that custom options are properly configured with required TypeInfoResolvers. Fixes modelcontextprotocol#1150
4c0cdec to
61cb96a
Compare
| } | ||
|
|
||
| customOptions.TypeInfoResolverChain.Add(McpJsonUtilities.JsonContext.Default); | ||
| customOptions.TypeInfoResolverChain.Add(AIJsonUtilities.DefaultOptions.TypeInfoResolver!); |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Νο. And even if it was, it would fail assuming the passed instance were read-only.
There was a problem hiding this comment.
@eiriktsarpalis, thanks. What is the right answer for #1150? We should support someone passing in new JsonSerializerOptions() { ... }.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How does JsonSerializer.Serialize(..., jso) avoid that? Or does it also create a new instance on each call?
There was a problem hiding this comment.
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();
}There was a problem hiding this comment.
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.
|
Thanks for the efforts here. Replaced by #1221. |
Added
GetSerializerOptions()helper method inAIFunctionMcpServerTooland ensure JsonSerializerOptions have proper TypeInfoResolver configuration.Executed tests and they passed.
Fixes #1150