-
Notifications
You must be signed in to change notification settings - Fork 131
Refactor JSON processing to exclude "method" in serialization #157
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?
Conversation
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.
Pull Request Overview
This PR refactors JSON processing to ensure the "method" field is excluded during serialization and properly handled during deserialization for both Request and Notification objects. This addresses a bug where the method field was being incorrectly included in the serialized JSON parameters.
- Modifies JSON serialization to exclude "method" field from parameters during encoding
- Updates deserialization logic to reconstruct the method field from the JSON-RPC method property
- Consolidates notification serialization to use the standardized toJSON() function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
types.kt | Updates Request and Notification serialization/deserialization to properly handle method field exclusion and reconstruction |
Protocol.kt | Refactors notification sending to use the standardized toJSON() method instead of inline serialization |
Comments suppressed due to low confidence (3)
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt:128
- [nitpick] The variable name
encoded
is misleading since it represents a modified JSON object with the method field removed, not the full encoded representation. Consider renaming toparamsWithoutMethod
orfilteredParams
for clarity.
val encoded = JsonObject(McpJson.encodeToJsonElement(this).jsonObject.minus("method"))
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt:142
- [nitpick] The variable name
requestData
could be more descriptive. Consider renaming toparamsWithMethod
orreconstructedParams
to clearly indicate what this variable represents.
val requestData = JsonObject(params.jsonObject.plus("method" to JsonPrimitive(method)))
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt:183
- [nitpick] The variable name
data
is too generic. Consider using a more descriptive name likeparamsWithMethod
to match the pattern used in the Request deserialization function.
val data = JsonObject(params.jsonObject.plus("method" to JsonPrimitive(method)))
internal fun JSONRPCRequest.fromJSON(): Request { | ||
val requestData = JsonObject(params.jsonObject.plus("method" to JsonPrimitive(method))) | ||
|
||
val deserializer = selectRequestDeserializer(method) | ||
return McpJson.decodeFromJsonElement(deserializer, requestData) |
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.
The function signature changed from returning Request?
to Request
, removing null safety. This is a breaking change that could cause issues if callers were handling null returns. Consider maintaining the nullable return type or ensuring all call sites are updated to handle potential exceptions.
internal fun JSONRPCRequest.fromJSON(): Request { | |
val requestData = JsonObject(params.jsonObject.plus("method" to JsonPrimitive(method))) | |
val deserializer = selectRequestDeserializer(method) | |
return McpJson.decodeFromJsonElement(deserializer, requestData) | |
internal fun JSONRPCRequest.fromJSON(): Request? { | |
val requestData = JsonObject(params.jsonObject.plus("method" to JsonPrimitive(method))) | |
val deserializer = selectRequestDeserializer(method) | |
return try { | |
McpJson.decodeFromJsonElement(deserializer, requestData) | |
} catch (e: Exception) { | |
null | |
} |
Copilot uses AI. Check for mistakes.
#56
Motivation and Context
fix bug
How Has This Been Tested?
unit tests and manual testing
Breaking Changes
Types of changes
Checklist