-
Notifications
You must be signed in to change notification settings - Fork 41
feat: Fix binary data handling in API GET command #740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ package api | |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "mime" | ||
| "strings" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
|
|
@@ -18,7 +19,7 @@ import ( | |
| "github.com/uyuni-project/uyuni-tools/shared/utils" | ||
| ) | ||
|
|
||
| func runGet(_ *types.GlobalFlags, flags *apiFlags, _ *cobra.Command, args []string) error { | ||
| func runGet(_ *types.GlobalFlags, flags *apiFlags, cmd *cobra.Command, args []string) error { | ||
| log.Debug().Msgf("Running GET command %s", args[0]) | ||
| client, err := api.Init(&flags.ConnectionDetails) | ||
| if err == nil && (client.Details.User != "" || client.Details.InSession) { | ||
|
|
@@ -35,13 +36,90 @@ func runGet(_ *types.GlobalFlags, flags *apiFlags, _ *cobra.Command, args []stri | |
| return utils.Errorf(err, L("error in query '%s'"), path) | ||
| } | ||
|
|
||
| // TODO do this only when result is JSON or TEXT. Watchout for binary data | ||
| // Decode JSON to the string and pretty print it | ||
| // Check if the result is binary data by examining content type or trying JSON marshaling | ||
| if err := outputResult(cmd, res); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // outputResult handles outputting the API response appropriately based on content type. | ||
| // 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 | ||
| out, err := json.MarshalIndent(res.Result, "", " ") | ||
| if err != nil { | ||
| // If JSON marshaling fails, treat as binary data | ||
| log.Debug().Msg(L("Result is not JSON-serializable, treating as binary data")) | ||
|
|
||
| // Convert result to bytes and write directly | ||
| if res.Result != nil { | ||
| if data, ok := res.Result.([]byte); ok { | ||
| _, err := cmd.OutOrStdout().Write(data) | ||
| return err | ||
| } | ||
| // Fallback: try to write as string | ||
| _, err := fmt.Fprint(cmd.OutOrStdout(), res.Result) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A string can always be converted to bytes: I believe this fallback will never happen. |
||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Check if output looks like binary (contains null bytes or non-printable chars) | ||
| if containsBinaryData(out) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| log.Debug().Msg(L("Detected binary data in response")) | ||
| _, err := cmd.OutOrStdout().Write(out) | ||
| return err | ||
| } | ||
| fmt.Print(string(out)) | ||
|
|
||
| return nil | ||
| // Output JSON with newline | ||
| _, err = fmt.Fprintln(cmd.OutOrStdout(), string(out)) | ||
| return err | ||
| } | ||
|
|
||
| // containsBinaryData checks if the data contains binary content. | ||
| func containsBinaryData(data []byte) bool { | ||
| // Check for null bytes which indicate binary data | ||
| for _, b := range data { | ||
| if b == 0 { | ||
| return true | ||
| } | ||
| // Check for high frequency of non-printable characters | ||
| if b < 32 && b != '\n' && b != '\r' && b != '\t' { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // isTextContentType checks if the content type indicates text-based data. | ||
| func isTextContentType(contentType string) bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is not used at all |
||
| if contentType == "" { | ||
| return false | ||
| } | ||
|
|
||
| mediaType, _, err := mime.ParseMediaType(contentType) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Text-based content types | ||
| textTypes := []string{ | ||
| "application/json", | ||
| "text/plain", | ||
| "text/html", | ||
| "text/xml", | ||
| "application/xml", | ||
| "application/javascript", | ||
| } | ||
|
|
||
| for _, t := range textTypes { | ||
| if mediaType == t { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should rely on the HTTP Header indicating the content type rather than trying first.