add UnmarshalJSON to Tool #35
Conversation
WalkthroughThe changes modify the Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
mcp/tools.go (1)
75-77: 💡 Verification agent🧩 Analysis chain
Consider updating the JSON tags as mentioned in PR description
The PR description mentioned updating the
InputSchemafield tag fromjson:"-"tojson:"inputSchema", but the field still hasjson:"-"in the code. This inconsistency should be addressed.The current implementation uses custom marshal/unmarshal methods to handle these fields, so changing the tag might require adjusting these methods. Let's verify the PR objective:
🏁 Script executed:
#!/bin/bash # Check the PR description to confirm intended changes to JSON tags echo "Looking for references to JSON tag changes in the repository..." git log -p --grep="input[Ss]chema" # Check for any related issues echo "Looking for related issues..." gh issue list --search "inputSchema json tag"Length of output: 253
Action Required: Update JSON Tag on InputSchema Field
- Update the tag for the
InputSchemafield fromjson:"-"tojson:"inputSchema"as specified in the PR description.- Verify that the custom marshal/unmarshal methods (in
mcp/tools.go) correctly process the updated tag to avoid any unintended side effects in JSON serialization.
🧹 Nitpick comments (2)
mcp/tools.go (2)
107-137: Missing validation for schema conflictThe existing
MarshalJSONmethod checks for conflicts betweenInputSchemaandRawInputSchema, but this newUnmarshalJSONmethod doesn't have similar validation.Consider adding validation to ensure both aren't set simultaneously, similar to the check in
MarshalJSON. This would ensure consistency in your codebase and prevent potential issues:func (t *Tool) UnmarshalJSON(b []byte) error { // Temporary structure for decoding var raw struct { Name string `json:"name"` Description string `json:"description"` InputSchema json.RawMessage `json:"inputSchema"` } // Unmarshal into the temporary structure if err := json.Unmarshal(b, &raw); err != nil { return err } // Assign name and description t.Name = raw.Name t.Description = raw.Description + // Clear both schema fields to avoid conflicts + t.InputSchema = ToolInputSchema{} + t.RawInputSchema = nil // Try to unmarshal InputSchema into structured format var schema ToolInputSchema if err := json.Unmarshal(raw.InputSchema, &schema); err == nil { // Successfully parsed structured schema t.InputSchema = schema } else { // Couldn't parse as structured schema, use as raw schema t.RawInputSchema = raw.InputSchema } return nil }
107-137: Missing test coverage for JSON unmarshalingAdding new functionality like this should be accompanied by unit tests to ensure correct behavior.
Consider adding tests that verify:
- Unmarshaling valid JSON with a structured schema
- Unmarshaling valid JSON with a raw schema
- Error handling for invalid JSON
- Marshaling and then unmarshaling produces equivalent objects (roundtrip test)
// Example test for UnmarshalJSON (to be placed in appropriate test file) func TestToolUnmarshalJSON(t *testing.T) { // Test case 1: Valid JSON with structured schema jsonData := `{ "name": "testTool", "description": "A test tool", "inputSchema": { "type": "object", "properties": { "param1": {"type": "string"} }, "required": ["param1"] } }` var tool Tool err := json.Unmarshal([]byte(jsonData), &tool) if err != nil { t.Fatalf("Failed to unmarshal valid JSON: %v", err) } // Verify the tool was populated correctly if tool.Name != "testTool" || tool.Description != "A test tool" { t.Errorf("Unexpected tool data: %+v", tool) } // Verify the schema was parsed correctly if tool.InputSchema.Type != "object" || tool.RawInputSchema != nil { t.Errorf("Schema not parsed correctly: %+v", tool.InputSchema) } // Additional test cases would follow... }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mcp/tools.go (1)
133-136: Consider error handling vs. fallback for invalid schema
Currently, if the raw input cannot be unmarshaled into a validToolInputSchema, the code silently falls back to storing raw JSON. If the MCP server is expected never to return unsanctioned raw schemas, you might consider returning an error or logging a warning instead, to catch potential mismatches or schema violations early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/tools.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server should only contain structured InputSchema and should not contain RawInputSchema. This is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format and returns an error if it cannot parse the schema.
mcp/tools.go (2)
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server should only contain structured InputSchema and should not contain RawInputSchema. This is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format and returns an error if it cannot parse the schema.
🔇 Additional comments (3)
mcp/tools.go (3)
107-109: Well-defined documentation block
The doc comment clearly states the purpose of theUnmarshalJSONmethod and aligns with the method's actual behavior.
110-120: Robust decoding structure
Defining a dedicated temporary struct for capturing raw input ensures a clean separation of concerns. It makes the code more understandable and reduces the likelihood of confusion or variable shadowing in the main struct.
126-132: Successful parse path works as expected
When the rawinputSchemacan be parsed into a validToolInputSchemawith a non-emptyType, it is correctly assigned toInputSchema, andRawInputSchemais nulled out. This approach follows the logic that a fully recognized schema should take precedence over raw data.
|
@xinwo Can you add a simple test that confirms the correct structure is produced by the new Unmarshal function? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
mcp/tools_test.go (2)
107-110: Consider using more precise assertion methods.While the
Subsetassertion works, it's not ideal for exact matching of properties as it only checks that a subset of expected keys/values exist.- assert.Subset(t, toolUnmarshalled.InputSchema.Properties["query"], map[string]interface{}{ - "type": "string", - "description": "Search query", - }) + queryProps := toolUnmarshalled.InputSchema.Properties["query"] + assert.Equal(t, "string", queryProps["type"]) + assert.Equal(t, "Search query", queryProps["description"])
77-144: Consider adding edge case tests for better coverage.While the current tests cover the basic functionality, adding tests for edge cases would enhance robustness.
Consider adding tests for:
- Invalid JSON input
- Missing required fields
- Schema validation failures
- Very large schemas that might approach serialization limits
Example test case for invalid JSON:
func TestUnmarshalToolWithInvalidJSON(t *testing.T) { invalidJSON := []byte(`{"name": "broken-tool", "inputSchema": {invalid}`) var tool Tool err := json.Unmarshal(invalidJSON, &tool) assert.Error(t, err) // Optionally verify specific error type/message if your implementation // returns custom errors }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp/tools.go(1 hunks)mcp/tools_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/tools.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in mark3labs/mcp-go handles both InputSchema and RawInputSchema formats. When unmarshaling JSON, it first tries to parse into a structured ToolInputSchema format, and if that fails, it falls back to using the raw schema format, providing symmetry with the MarshalJSON method.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
🔇 Additional comments (5)
mcp/tools_test.go (5)
77-118: Well-structured test for unmarshaling tool with raw schema.This test effectively validates that the Tool struct can correctly unmarshal a JSON representation that was originally created from a raw schema. The test covers important validation points including the proper reconstruction of nested properties and their values.
A few observations:
- The test provides good coverage for the new UnmarshalJSON functionality
- The assertions are thorough, checking nested properties and their types
- The use of Subset assertions is appropriate for validating complex nested structures
114-115: Be aware of potential floating-point precision issues in JSON parsing.When unmarshaling JSON numbers, they are decoded as float64 by default in Go, which is why the test is using 1.0 and 50.0 instead of integers.
This is the correct approach for the test, but it's worth noting that this behavior is something to be aware of when dealing with JSON data in production code. The JSON unmarshaling process in Go deserializes all numbers as float64 by default when using the map[string]interface{} approach.
120-144: Good test case for tools without raw schemas.This test appropriately verifies that a tool created with only structured schema (without raw schema) can be properly marshaled and unmarshaled, maintaining all its properties correctly.
The assertions effectively verify:
- Basic properties (name, description)
- Complex nested properties in the InputSchema
- Proper handling of empty fields (Required, RawInputSchema)
142-143: Thorough validation of empty fields.These assertions explicitly verify that both the Required array and RawInputSchema are empty after unmarshaling, which is important for confirming the symmetry between marshaling and unmarshaling operations.
This aligns with the learning that the Tool struct should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods.
77-144: Verify that the new tests align with the PR objectives.These tests effectively validate the UnmarshalJSON functionality for the Tool struct mentioned in the PR objectives. The tests ensure that both marshaling and unmarshaling of the Tool struct work correctly with the modified InputSchema field (now using json:"inputSchema" tag instead of json:"-").
The tests fulfill the request from user ezynda3 to add simple tests confirming that the new UnmarshalJSON function produces the correct structure.
|
Awesome! Thanks! |
#30
may also need to implement UnmarshalJSON method.
maybe restoring
to
also works.
related issue
#34
Summary by CodeRabbit
New Features
InputSchemafield is now included in the JSON output under the key "inputSchema".Tests