refactor(mcp): remove redundant type field from Content implementations#27
refactor(mcp): remove redundant type field from Content implementations#27
Conversation
- The type field and associated methods were redundant in Content implementations (TextContent, ImageContent, EmbeddedResource) as the type information is already handled by Jackson's polymorphic type handling via @JsonSubTypes annotation. - Added comprehensive unit tests for McpSchema to validate serialization/deserialization behavior of all schema components. Resolves #26 Resolve spring-projects/spring-ai#2350 Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
chemicL
left a comment
chemicL
left a comment
There was a problem hiding this comment.
Despite the triviality of the fix, the tests are comprehensive and useful so I reviewed them carefully. I left a few comments.
| public sealed interface Content permits TextContent, ImageContent, EmbeddedResource { | ||
|
|
||
| String type(); | ||
| // String type(); |
There was a problem hiding this comment.
Actually I've decided to add a default String type() implementation to comply with the MCP Schema, were the type in the content exist.
| String value = mapper.writeValueAsString(request); | ||
| assertEquals( | ||
| """ | ||
| {"protocolVersion":"2024-11-05","capabilities":{"roots":{"listChanged":true},"sampling":{}},"clientInfo":{"name":"test-client","version":"1.0.0"}}""", |
There was a problem hiding this comment.
This will break as soon as we update McpSchema.LATEST_PROTOCOL_VERSION. It should embed that value in the expected string.
There was a problem hiding this comment.
Same comment applies to the other tests with the version.
There was a problem hiding this comment.
Alternatively, instead of using the constant, specify a value inside the test and then the tests will work regardless of the versioning evolution.
There was a problem hiding this comment.
you're right. I will remove the constant from the fixture.
| // Use Jackson to parse both the expected and actual JSON to compare them as | ||
| // objects | ||
| // This ignores the order of properties in JSON objects |
There was a problem hiding this comment.
Why is there a need for this in one test and not in others?
There was a problem hiding this comment.
It shouldn't. will remove the comment
- Enhance Content interface with type() implementation:
- Add default implementation that returns the appropriate type string
based on the implementing class (text, image, or resource)
- Remove unused JsonIgnore import
- Improve test assertions and coverage:
- Replace JUnit Jupiter assertions with AssertJ for more readable assertions
- Add comprehensive deserialization tests for all content types
- Add negative test case for invalid content type deserialization
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
| public sealed interface Content permits TextContent, ImageContent, EmbeddedResource { | ||
|
|
||
| String type(); | ||
| default String type() { |
There was a problem hiding this comment.
Since you are dealing with a sealed interface, you can use a pattern switch on this and have a complete coverage without the default case.
There was a problem hiding this comment.
pattern matching is available in Java 21+. The MCP project at the moment is configured for 17.
There was a problem hiding this comment.
Oh, it was a preview feature back then. My bad, sorry!
Replace direct string assertions with json-unit-assertj in McpSchemaTests to make tests more robust against non-functional changes in JSON formatting. The new approach ignores array order and extra array items, reducing test brittleness while maintaining functional validation. Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
|
Addressed review comments |
2 participants
Resolves #26
Resolve spring-projects/spring-ai#2350