Add 3-arg RequestContext constructor and obsolete 2-arg to eliminate null-forgiving operator usage#1462
Add 3-arg RequestContext constructor and obsolete 2-arg to eliminate null-forgiving operator usage#1462
Conversation
…e old constructor - Add new RequestContext(McpServer, JsonRpcRequest, TParams) constructor - Obsolete the old parameterless constructor - Change Params property from TParams? to TParams - Update all production code to use non-nullable Params - Update RequestHandlers.Set to use non-null TParams - Update InvokeHandlerAsync to use new constructor Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/a857ef9f-506e-4046-a2c2-d268cfc65701
Update all test files to use the new 3-arg RequestContext constructor (server, jsonRpcRequest, params) instead of the old 2-arg constructor with property initializer for Params. Changes: - Move Params from object initializer to constructor argument - Change .Params?. to .Params. on RequestContext variables - Fix request?.Params?.X to request?.Params.X - Remove dead null checks on non-nullable value type Level - Add required Name/Uri properties to default params instances Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/a857ef9f-506e-4046-a2c2-d268cfc65701
When a client omits the "params" field from a JSON-RPC message, JsonSerializer.Deserialize returns null. Since RequestContext.Params is now non-nullable, we need to deserialize from an empty JSON object instead to get a valid default TParams instance. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/1e9aed1a-2748-4117-987c-611e23751bea
…HandlerAsync constructor change The previous commits removed null-conditional operators (?.) from handler lambdas in McpServerImpl.cs, but at runtime TParams can still be null when JSON-RPC params is missing. Also reverts the EmptyJsonObject fallback in RequestHandlers.cs which failed for types with required constructor params. Restores defensive null-conditional access in all handler lambdas while keeping the desired API changes: TParams (non-nullable) InvokeHandlerAsync signature and the new 3-arg RequestContext constructor. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/74aa9308-d4cf-4669-86b4-1a680d0676b4
…constructor - Rename the `@params` constructor parameter to `parameters` in the 3-arg RequestContext<TParams> constructor. - Add MCP9003 diagnostic ID to Obsoletions.cs for the obsolete 2-arg constructor, following the established pattern (Message, DiagnosticId, UrlFormat). - Update the [Obsolete] attribute to use the centralized constants with DiagnosticId and UrlFormat. - Document MCP9003 in docs/list-of-diagnostics.md. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/6e17cd63-6cb0-4d77-ba6c-1f2a616fa03f
…s returning CallToolResult (#1425) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…1438) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Stephen Toub <stoub@microsoft.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…null-forgiving operator usage Co-authored-by: halter73 <54385+halter73@users.noreply.github.com> Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/43f266c7-392e-40c4-a808-759fb46d04d5
halter73
changed the title
@params to parameters and add proper obsoletion for 2-arg RequestContext constructor…d-requestcontext-const # Conflicts: # tests/ModelContextProtocol.Tests/Server/McpServerToolTests.cs
| public RequestContext(McpServer server, JsonRpcRequest jsonRpcRequest) | ||
| : base(server, jsonRpcRequest) | ||
| { | ||
| Params = default!; |
There was a problem hiding this comment.
I understand the goal, but the code using the ctor and the code using Params are typically in totally different places. Isn't it lying now that the property is non-nullable even though all existing usage will still be setting it to null? Is the thinking that no one outside of these MCP libraries is actually constructing these?
There was a problem hiding this comment.
Is the thinking that no one outside of these MCP libraries is actually constructing these?
Exactly.
I generally hate lying about nullability too, but at least this is only via an obsolete API that's (hopefully) rarely used. And 99.9% of the usage of the API seems to be either doing Params!... or Params?... on a subproperty that is nullable anyway rather than initializing a RequestContext. This is true even internally in the SDK if you look at places like AIFunctionMcpServerResource. The docs in the recent MRTR PR (#1458) really show how unnecessarily ugly this can be.
If in the future there becomes a new request type that really could have a null value, couldn't you make the generic parameter nullable? Even if not, I'm not super worried. We can cross that bridge when we get there, making it nullable again wouldn't be binary breaking at least. And presumably, for all the existing usage for specific known TParams today, it will be non-null.
I think it's better to make the code cleaner today rather than worry about an unlikely future we could probably handle fine anyway.
There was a problem hiding this comment.
Makes me uncomfortable. But if you really think it's unlikely folks are using this, and searching around (e.g. grep.app) helps to at least bear that out a little, ok. I see at least some tests using it (https://github.com/microsoft/mcp/blob/230297c5af1c99ebe73206c145c2aa206d2c00fb/core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/Commands/Runtime/McpRuntimeTests.cs#L50-L65), though those are also ok uses as they immediately assign the property.
src/ModelContextProtocol.Core/Server/AIFunctionMcpServerPrompt.cs
Outdated
Show resolved
Hide resolved
…in src/ Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/57676fa6-299c-40eb-ba0b-a7cbedbcef91 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…-conditionals) Agent-Logs-Url: https://github.com/modelcontextprotocol/csharp-sdk/sessions/e4a931bf-4543-4726-8897-54dcc66e354d Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
RequestContextconstructor📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.