Skip to content

Commit 310a8e0

Browse files
Copiloteiriktsarpalis
Copilot
and
eiriktsarpalis
committed
Address reviewer feedback: Refactor Union struct and Parse method
- Move Union struct to bottom of Converter class - Make Parse method a static member of Converter (not Union) - Replace while(reader.Read()) with while(true) pattern - Add Debug.Assert for reader.Read() success (converters get fully buffered objects) - Add Debug.Assert for PropertyName token type - Add System.Diagnostics using for Debug class - All 1035 tests still pass Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
1 parent ea7bc33 commit 310a8e0

1 file changed

Lines changed: 105 additions & 103 deletions

File tree

‎src/ModelContextProtocol.Core/Protocol/JsonRpcMessage.cs‎

Lines changed: 105 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using ModelContextProtocol.Server;
22
using System.ComponentModel;
3+
using System.Diagnostics;
34
using System.Text.Json;
45
using System.Text.Json.Nodes;
56
using System.Text.Json.Serialization;
@@ -73,112 +74,10 @@ public sealed class Converter : JsonConverter<JsonRpcMessage>
7374
{
7475
private const string JsonRpcVersion = "2.0";
7576

76-
/// <summary>
77-
/// Private struct to hold parsed JSON-RPC message data during deserialization.
78-
/// </summary>
79-
private struct Union
80-
{
81-
/// <summary>The JSON-RPC protocol version (must be "2.0").</summary>
82-
public string JsonRpc;
83-
/// <summary>The message identifier for requests and responses.</summary>
84-
public RequestId Id;
85-
/// <summary>The method name for requests and notifications.</summary>
86-
public string? Method;
87-
/// <summary>The parameters for requests and notifications.</summary>
88-
public JsonNode? Params;
89-
/// <summary>The error details for error responses.</summary>
90-
public JsonRpcErrorDetail? Error;
91-
/// <summary>The result for successful responses.</summary>
92-
public JsonNode? Result;
93-
/// <summary>Indicates whether an 'id' property was present.</summary>
94-
public bool HasId;
95-
/// <summary>Indicates whether a 'method' property was present.</summary>
96-
public bool HasMethod;
97-
/// <summary>Indicates whether an 'error' property was present.</summary>
98-
public bool HasError;
99-
/// <summary>Indicates whether a 'result' property was present.</summary>
100-
public bool HasResult;
101-
102-
/// <summary>
103-
/// Manually parses a JSON-RPC message from the reader into the Union struct.
104-
/// </summary>
105-
public static Union Parse(ref Utf8JsonReader reader, JsonSerializerOptions options)
106-
{
107-
var union = new Union
108-
{
109-
JsonRpc = string.Empty // Initialize to avoid null reference warnings
110-
};
111-
112-
if (reader.TokenType != JsonTokenType.StartObject)
113-
{
114-
throw new JsonException("Expected StartObject token");
115-
}
116-
117-
while (reader.Read())
118-
{
119-
if (reader.TokenType == JsonTokenType.EndObject)
120-
{
121-
break;
122-
}
123-
124-
if (reader.TokenType != JsonTokenType.PropertyName)
125-
{
126-
throw new JsonException("Expected PropertyName token");
127-
}
128-
129-
string? propertyName = reader.GetString();
130-
if (propertyName is null)
131-
{
132-
throw new JsonException("Property name cannot be null");
133-
}
134-
135-
reader.Read();
136-
137-
switch (propertyName)
138-
{
139-
case "jsonrpc":
140-
union.JsonRpc = reader.GetString() ?? string.Empty;
141-
break;
142-
143-
case "id":
144-
union.Id = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<RequestId>());
145-
union.HasId = true;
146-
break;
147-
148-
case "method":
149-
union.Method = reader.GetString();
150-
union.HasMethod = union.Method is not null;
151-
break;
152-
153-
case "params":
154-
union.Params = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonNode>());
155-
break;
156-
157-
case "error":
158-
union.Error = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonRpcErrorDetail>());
159-
union.HasError = union.Error is not null;
160-
break;
161-
162-
case "result":
163-
union.Result = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonNode>());
164-
union.HasResult = true;
165-
break;
166-
167-
default:
168-
// Skip unknown properties
169-
reader.Skip();
170-
break;
171-
}
172-
}
173-
174-
return union;
175-
}
176-
}
177-
17877
/// <inheritdoc/>
17978
public override JsonRpcMessage? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
18079
{
181-
var union = Union.Parse(ref reader, options);
80+
var union = ParseUnion(ref reader, options);
18281

18382
// All JSON-RPC messages must have a jsonrpc property with value "2.0"
18483
if (union.JsonRpc != JsonRpcVersion)
@@ -276,5 +175,108 @@ public override void Write(Utf8JsonWriter writer, JsonRpcMessage value, JsonSeri
276175
throw new JsonException($"Unknown JSON-RPC message type: {value.GetType()}");
277176
}
278177
}
178+
179+
/// <summary>
180+
/// Manually parses a JSON-RPC message from the reader into the Union struct.
181+
/// </summary>
182+
private static Union ParseUnion(ref Utf8JsonReader reader, JsonSerializerOptions options)
183+
{
184+
var union = new Union
185+
{
186+
JsonRpc = string.Empty // Initialize to avoid null reference warnings
187+
};
188+
189+
if (reader.TokenType != JsonTokenType.StartObject)
190+
{
191+
throw new JsonException("Expected StartObject token");
192+
}
193+
194+
while (true)
195+
{
196+
bool success = reader.Read();
197+
Debug.Assert(success, "custom converters are guaranteed to be passed fully buffered objects");
198+
199+
if (reader.TokenType is JsonTokenType.EndObject)
200+
{
201+
break;
202+
}
203+
204+
Debug.Assert(reader.TokenType is JsonTokenType.PropertyName);
205+
206+
string? propertyName = reader.GetString();
207+
if (propertyName is null)
208+
{
209+
throw new JsonException("Property name cannot be null");
210+
}
211+
212+
success = reader.Read();
213+
Debug.Assert(success, "custom converters are guaranteed to be passed fully buffered objects");
214+
215+
switch (propertyName)
216+
{
217+
case "jsonrpc":
218+
union.JsonRpc = reader.GetString() ?? string.Empty;
219+
break;
220+
221+
case "id":
222+
union.Id = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<RequestId>());
223+
union.HasId = true;
224+
break;
225+
226+
case "method":
227+
union.Method = reader.GetString();
228+
union.HasMethod = union.Method is not null;
229+
break;
230+
231+
case "params":
232+
union.Params = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonNode>());
233+
break;
234+
235+
case "error":
236+
union.Error = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonRpcErrorDetail>());
237+
union.HasError = union.Error is not null;
238+
break;
239+
240+
case "result":
241+
union.Result = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<JsonNode>());
242+
union.HasResult = true;
243+
break;
244+
245+
default:
246+
// Skip unknown properties
247+
reader.Skip();
248+
break;
249+
}
250+
}
251+
252+
return union;
253+
}
254+
255+
/// <summary>
256+
/// Private struct to hold parsed JSON-RPC message data during deserialization.
257+
/// </summary>
258+
private struct Union
259+
{
260+
/// <summary>The JSON-RPC protocol version (must be "2.0").</summary>
261+
public string JsonRpc;
262+
/// <summary>The message identifier for requests and responses.</summary>
263+
public RequestId Id;
264+
/// <summary>The method name for requests and notifications.</summary>
265+
public string? Method;
266+
/// <summary>The parameters for requests and notifications.</summary>
267+
public JsonNode? Params;
268+
/// <summary>The error details for error responses.</summary>
269+
public JsonRpcErrorDetail? Error;
270+
/// <summary>The result for successful responses.</summary>
271+
public JsonNode? Result;
272+
/// <summary>Indicates whether an 'id' property was present.</summary>
273+
public bool HasId;
274+
/// <summary>Indicates whether a 'method' property was present.</summary>
275+
public bool HasMethod;
276+
/// <summary>Indicates whether an 'error' property was present.</summary>
277+
public bool HasError;
278+
/// <summary>Indicates whether a 'result' property was present.</summary>
279+
public bool HasResult;
280+
}
279281
}
280282
}

0 commit comments

Comments
 (0)