mcp: don't omit required fields in ImageContent and AudioContent #95
Conversation
I noticed this while working on #95. Seems Meta was accidentally skipped in the custom wire format struct.
|
@martinemde I think your suggestion to remove the 'omitempty' attrs is preferable: the missing 'mimeType' or 'data' field is really an error in business logic, not marshalling. For data, we should be careful to replace nil with an empty byte slice, as JSON null is not valid. Thanks for picking this up! |
mcp/content.go
Outdated
| } | ||
| if c.MIMEType == "" { | ||
| return nil, errors.New("ImageContent missing mimeType") | ||
| } | ||
| return json.Marshal(&wireContent{ |
There was a problem hiding this comment.
As noted, I think we should use an inline type rather than 'wireContent' here, so that we can make Data and MimeType not "omitempty". We should be sure to replace nil data with the empty slice though.
There was a problem hiding this comment.
Thanks, will get to it asap.
mcp/content.go
Outdated
| } | ||
| if c.MIMEType == "" { | ||
| return nil, errors.New("ImageContent missing mimeType") | ||
| } | ||
| return json.Marshal(&wireContent{ |
martinemde
force-pushed
the
martinemde/validate-required-content-fields
branch
from
3b9ca9b to
9091440
Compare
|
This is updated. Thanks for your patience while I got around to this. |
martinemde
changed the title
martinemde
force-pushed
the
martinemde/validate-required-content-fields
branch
from
9091440 to
ef96b43
Compare
…Content ImageContent and AudioContent now use custom MarshalJSON methods with inline structs to ensure data and mimeType fields are always included in JSON output, even when empty. This matches the approach used for TextContent.Text and follows TypeScript schema requirements. Updated tests to verify empty fields are serialized as empty strings rather than being omitted.
martinemde
force-pushed
the
martinemde/validate-required-content-fields
branch
from
ef96b43 to
c5b9f06
Compare
| @@ -58,13 +58,25 @@ type ImageContent struct { | |||
| } | |||
|
|
|||
| func (c *ImageContent) MarshalJSON() ([]byte, error) { | |||
| return json.Marshal(&wireContent{ | |||
| // Custom wire format to ensure required fields are always included, even when empty. | |||
There was a problem hiding this comment.
It looks like these two functions are almost identical. Factor out into a single function. It looks to me that that function's args are the type (image or audio) and the data.
There was a problem hiding this comment.
We can if you think it's important.
Two copies of something doesn't feel like a strong deduplication signal to me given that it will split the currently very clear schema mapping into a function that abstracts away half of the type.
If you think this is a blocker, I'll refactor, since I'd prefer the sdk is correct regardless of the code inside.
This is a follow-up related to #91 (though not requiring it).
Updates
ImageContentandAudioContentto use an inline wire format to ensure that required fields are not omitted during JSON marshaling, matching TypeScript schema requirements. This follows the TextContent approach.ImageContentrequiresdataandmimeTypefields.AudioContentrequiresdataandmimeTypefields.