Skip to content

Fix data-URL strings decoding as binary values - #295

Open
EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/preserve-data-url-json-strings
Open

EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/preserve-data-url-json-strings

Conversation

@EhsanAzish80

Copy link
Copy Markdown

Fixes #277. JSON strings that look like data URLs now stay strings, so tool text is preserved when it passes through Value. This is an independently tested alternative to #278.

Tests: 554 Swift tests passed locally. AI was used to write the tests. I ran them locally.

Copy link
Copy Markdown

This fixes the accidental data-URL coercion, but it also makes the public Value.data case non-round-trippable through its own Codable conformance.

Value.data still encodes as a plain data-URL string:

case let .data(mimeType, value):
    try container.encode(value.dataURLEncoded(mimeType: mimeType))

while this PR now decodes every JSON string as .string. So:

let original = Value.data(mimeType: "application/octet-stream", Data([0, 1, 2]))
let decoded = try JSONDecoder().decode(Value.self, from: JSONEncoder().encode(original))

changes type from .data to .string. The new test explicitly codifies that asymmetry.

If generic JSON strings must never be auto-promoted to binary (which makes sense), could Value.data use an explicit tagged representation for encoding/decoding, or otherwise be removed/documented as non-Codable-lossless? As it stands, persistence or forwarding of a Value.data through JSON silently loses the enum case.

AI-assisted review; checked the current PR head and Value Codable paths before posting.

@EhsanAzish80

Copy link
Copy Markdown
Author

I kept JSON strings as strings and documented that Value.data is not lossless through generic JSON decoding. The new test shows how to recover the bytes with Data.parseDataURL. All 554 Swift tests passed locally. AI helped write the test; I ran it. Could a maintainer review this choice and approve CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Value decoding changes data-URL-looking JSON strings into Value.data

2 participants