feat: Fix binary data handling in API GET command#740
Conversation
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
|
cbosdo
left a comment
cbosdo
left a comment
There was a problem hiding this comment.
The server doesn't handle the binary returns correctly so far, so I believe that this code has never been tested: this is a first reason to not accept it in the current state.
I would also prefer to not try to parse JSON and then convert to bytes if it fails as it can hide other issues. HTTP has a header to indicate the response content type: it should be used to distinguish when to output binary blob.
Along the other things to fix for all PRs:
- Add a copyright line for you in the file header
- Set a useful commit description rather than a single line comment (can be used as PR description too)
- Run the tests including the linter
| // For JSON/text data, it pretty-prints the JSON. | ||
| // For binary data, it writes raw bytes to stdout. | ||
| func outputResult(cmd *cobra.Command, res *api.APIResponse[interface{}]) error { | ||
| // Try to marshal as JSON first |
There was a problem hiding this comment.
Ideally we should rely on the HTTP Header indicating the content type rather than trying first.
| return err | ||
| } | ||
| // Fallback: try to write as string | ||
| _, err := fmt.Fprint(cmd.OutOrStdout(), res.Result) |
There was a problem hiding this comment.
A string can always be converted to bytes: I believe this fallback will never happen.
| } | ||
|
|
||
| // Check if output looks like binary (contains null bytes or non-printable chars) | ||
| if containsBinaryData(out) { |
There was a problem hiding this comment.
You are handling the binary data twice: if you come to this point, the data has been marshalled as JSON… there is no point to try to look for binary data.
| } | ||
|
|
||
| // isTextContentType checks if the content type indicates text-based data. | ||
| func isTextContentType(contentType string) bool { |
There was a problem hiding this comment.
This function is not used at all
2 participants
Git Commit Message
Fix binary data handling in API GET command
PR Description
What does this PR change?
Fixes the TODO comment in
mgrctl/cmd/api/get.goregarding binary data handling. The API GET command now properly handles both text/JSON responses and binary data.Changes:
outputResult()function that intelligently detects and handles response typescontainsBinaryData()to detect binary content (null bytes, control characters)isTextContentType()for content-type header validationCodespace
Check if you already have a running container clicking on
Test coverage
No tests: add explanation
No tests: already covered
[] Unit tests were added
DONE (tested with
go test ./mgrctl/cmd/api/...andgo build ./...)Links
Issue(s): #
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test(see below)Before you merge
Check How to branch and merge properly!