-
Notifications
You must be signed in to change notification settings - Fork 555
Audit protocol types #892
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?
Audit protocol types #892
Conversation
| { | ||
| Blob = dataContent.Base64Data.ToString(), | ||
| MimeType = dataContent.MediaType, | ||
| Uri = string.Empty, |
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.
This was implicitly string.Empty before, but now it's explicit. This PR doesn't introduce a behavioral change here, but we should decide whether the empty string should really be used here, as it's not actually a valid URI. It would also be strange to use dataContent.Uri, because then the data is specified in two places (once in Blob and once in Uri).
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.
then the data is specified in two places
It doesn't sound bad, DataContent.Uri is just $"data:{MediaType};base64,{base64}" while MimeType and Blob are there to access those parts without having to parse the Uri. It would also align with DataCotent.
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.
Realizing that there's also TextResourceContents, I wonder if Uri should be abstract and Blob and Text types should provide an implementation.
public sealed class TextContent
{
public override string Uri => $"data:text/plain;base64,{Base64Url.EncodeToString(Encoding.UTF8.GetBytes(Text))}"
}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.
then the data is specified in two places
It doesn't sound bad, DataContent.Uri is just
$"data:{MediaType};base64,{base64}"while MimeType and Blob are there to access those parts without having to parse the Uri. It would also align with DataCotent.
The DataContent type in Microsoft.Extensions.AI is different in that the Uri can only be a data URI, and the Data property is [JsonIgnore]d, so the duplication only exists in-memory. However, the MCP BlobResourceContents type has both uri and blob as separate, required properties. This means that if you treat BlobResourceContents.uri as a data URI, you'll be including the data twice in JSON, which would be unfortunate IMO.
The spec also doesn't seem to acknowledge the possibility of the uri property being a data URI (although it doesn't explicitly disallow it). The examples all show file://, and only https://, file://, and git:// are listed as examples of common URI schemes.
So, the purpose of BlobResourceContents.uri seems to be identifying the data, which is different than DataContent.Uri, whose purposes is containing the data itself. This is what creates the dilemma when mapping between these two types.
Realizing that there's also TextResourceContents, I wonder if Uri should be abstract and Blob and Text types should provide an implementation
This would disallow TextResourceContents and BlobResourceContents from having anything other than a data URI, right?
For context, the presence of |
tests/ModelContextProtocol.Tests/Server/McpServerResourceTests.cs
Outdated
Show resolved
Hide resolved
What are our principles around this? I know JsonNode is mutable and JsonElement is not, but why is JsonNode use for params and results and JsonElement or |
Exactly what I'm wondering too 🙂 Maybe we could get an STJ expert to weigh in on this? Edit: @eiriktsarpalis , do you have any thoughts about this? |
|
@MackinnonBuck I've opened a new pull request, #923, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
* Initial plan * Add DefaultSamplingMaxTokens property to McpServerOptions Co-authored-by: MackinnonBuck <[email protected]> * Add test to verify DefaultSamplingMaxTokens is respected Co-authored-by: MackinnonBuck <[email protected]> * Merge test into existing SampleAsync_Messages_Forwards_To_McpServer_SendRequestAsync Co-authored-by: MackinnonBuck <[email protected]> * Update src/ModelContextProtocol.Core/Server/McpServerOptions.cs --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: MackinnonBuck <[email protected]> Co-authored-by: Mackinnon Buck <[email protected]>
|
Thanks for the clarification, @eiriktsarpalis! I'll leave the properties representing JSON objects as-is for this PR, then. Nothing jumps out at me as obviously using the wrong type. |
Summary
Audits and standardizes MCP protocol types for consistency.
Fixes #519
Description
There are several inconsistencies across protocol types, including:
initvs.setrequiredvs. default property valuesenumvs.stringJsonNodevs.JsonElementvs.IDictionary<string, JsonElement>vs.IDictionary<string, object>This PR applies the following set of conventions to improve consistency:
Property Mutability
Guideline: Always use
set- noinit-only properties.Rationale:
setorinit, it's easier to standardize onsetbecause doing so is non-breakinginitmeans that if we do want to mutate a property after initialization, we need to clone the its containing object, which can be error-prone and isn't great for perfRequired Properties
Guideline: Use
requiredwhen the spec indicates a property is required, unless a clear, safe default exists.Rationale:
Safe defaults include:
Avoid defaults such as:
"") - this is rarely the expected value for a "required" propertyCollections
Guideline: Prefer
IList<T>/IDictionary<TKey, TValue>overIReadOnlyList<T>/IReadOnlyDictionary<TKey, TValue>.Rationale:
Open Questions
The following inconsistencies remain for further discussion:
enumvs.string: propertiesThe MCP schema defines some properties as unions of string values.
enumoffers strong typingstringis more future-proof (e.g., if new values are added)Existing examples of each:
enum:ContextInclusion,Rolestring:ElicitResult.ActionJSON-like Structures:
The use of
JsonNode,JsonElement,IDictionary<string, JsonElement>, andIDictionary<string, object>varies.Additional Notes