Skip to content

Propagate fix from AIFunctionFactory to TemporaryAIFunctionFactory #215

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

Merged
merged 1 commit into from
Apr 5, 2025
Merged

Propagate fix from AIFunctionFactory to TemporaryAIFunctionFactory #215

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
4 changes: 2 additions & 2 deletions src/ModelContextProtocol/Server/TemporaryAIFunctionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,9 +568,7 @@ static bool IsAsyncMethod(MethodInfo method)
throw new ArgumentException("Parameter is missing a name.", nameof(parameter));
}

// Resolve the contract used to marshal the value from JSON -- can throw if not supported or not found.
Type parameterType = parameter.ParameterType;
JsonTypeInfo typeInfo = serializerOptions.GetTypeInfo(parameterType);
Copy link
Contributor

@eiriktsarpalis eiriktsarpalis Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pretty sure the same issue also manifested for return parameters. I take it you dropped the fix for that intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a larger set of changes, it's much less of an issue (as opposed to parameters that are things like IMcpServer and CancellationToken that have no chance of being in the JSO), and this code will (hopefully) all be deleted tomorrow, so I chose to let it be. We can bring it over if you think it's worthwhile, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine if we wait. I also decided against backporting the fix for the same reason.


// For CancellationToken parameters, we always bind to the token passed directly to InvokeAsync.
if (parameterType == typeof(CancellationToken))
Expand Down Expand Up @@ -606,6 +604,8 @@ static bool IsAsyncMethod(MethodInfo method)
}

// For all other parameters, create a marshaller that tries to extract the value from the arguments dictionary.
// Resolve the contract used to marshal the value from JSON -- can throw if not supported or not found.
JsonTypeInfo typeInfo = serializerOptions.GetTypeInfo(parameterType);
return (arguments, _) =>
{
// If the parameter has an argument specified in the dictionary, return that argument.
Expand Down