Repository navigation
Python: Fix replacing binary content data URIs - #14605
Captain (po-et) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix reuses established parsing behavior, includes targeted regression coverage, and has no identified blocking issues.
0 open findings
What changed in this PR
Fixes replacement of existing Python binary content through data_uri, keeping the payload, format, and MIME type consistent.
Changes:
- Uses the existing URI parser for every assignment.
- Adds eight regression cases covering replacements, invalid-input preservation, and image request serialization.
| File | Description |
|---|---|
| python/tests/unit/contents/test_image_content.py | Verifies image replacement and request serialization. |
| python/tests/unit/contents/test_binary_content.py | Covers replacement formats and invalid-input preservation. |
| python/semantic_kernel/contents/binary_content.py | Parses replacement URIs before assigning content. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community review. This does not clear the merge gate.
The fix works and I think it is correct. I found one small gap, described last.
What I ran (upstream/main at 7a92cd4, a separate worktree, venv at C:/skv2, PR diff applied with git apply):
python/tests/unit/contentson main with the PR's tests: 8 failed, 394 passed. With thebinary_content.pychange: 402 passed. The failing tests on main includetest_replace_image_data_uriandtest_replace_data_uri_invalid_preserves_existing_data.- A probe of my own on main:
ImageContent(data=b"old_image", ...)thenimage.data_uri = "data:image/png;base64,bmV3X2ltYWdl"raisesbinascii.Error: Only base64 data is allowed. On the PR the same code givesmime_type == "image/png",data == b"new_image", andto_dict()returns the new URI. That matches the description. - The existing
test_update_data_uri(settingdata_urion an emptyBinaryContent) still passes, since that path already usedDataUri.from_data_uri.
The invalid-URI test passes because from_data_uri runs before _data_uri is reassigned, so the old content stays. I read that in the diff and the test confirms it.
The gap: the setter does self.metadata.update(self._data_uri.parameters), which merges and never removes parameters from the previous URI. Probe on the PR: start from BinaryContent(data_uri="data:text/plain;foo=old;base64,b2xk"), then set data_uri = "data:image/jpeg;base64,bmV3". Result: mime_type is image/jpeg and data is b'new', but metadata is still {'foo': 'old'} and the new data_uri string is data:image/jpeg;foo=old;base64,bmV3. So a replacement URI with no parameters inherits the old URI's parameters. The PR's test_replace_data_uri always replaces foo=old with foo=new, so it does not catch this. Whether metadata given at construction (the test's {"source": "test"}) should survive is a design choice, so I would only drop the keys that came from the old URI's parameters, if the maintainers care about this case. I did not try a fix.
I did not run the full Python suite or the multimodal connectors that consume these URIs.
|
Captain (@po-et) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Motivation and Context
Replacing
ImageContent.data_uriafter the object already contains data can raise a base64 error or store the entire URI as the payload. The MIME type also stays at its previous value. In addition, parameters from the old URI remain in metadata and get serialized into the replacement URI even when the replacement omits them. These affect applications that update existing image content before sending it to a multimodal model.Description
Parse a replacement URI with
DataUri.from_data_uribefore changing state. Replace the payload, format and MIME type together, removing old URI parameter entries whose metadata values still match their old values. Preserve unrelated caller metadata and values the caller has since changed, including non-string values. New URI parameters override matching metadata keys.An invalid replacement leaves the existing content and metadata intact. The separate
datasetter and the data URI parser are unchanged.Regression coverage includes base64, bytes and ndarray-backed content; base64 and plain-text replacements with or without parameters; caller metadata values of string, None, ndarray and dict; invalid replacement atomicity; and the
ImageContent.to_dict()payload. The original eight regressions failed on the original base. Two parameter-removal cases also failed on the earlier PR head before this follow-up.Validation
git diff --checkpassed.Optional-provider suites and live model calls were not run.
Contribution Checklist
AI disclosure: OpenAI Codex assisted with investigation, implementation, tests and independent review.