Handle empty stdout properly in MinioAdmin #1099
Conversation
| @@ -50,7 +50,7 @@ def _run(self, args, multiline=False): | |||
| check=True, | |||
There was a problem hiding this comment.
Add text=True get proc.stdout as string and treat it as string later.
|
@cbows
On which admin API do you get this error? |
|
I got it on |
| @@ -48,9 +48,10 @@ def _run(self, args, multiline=False): | |||
| capture_output=True, | |||
| timeout=self._timeout, | |||
| check=True, | |||
| text=True, | |||
| ) | |||
There was a problem hiding this comment.
Add a fix generically to avoid any error on json.loads() here
if not proc.stdout:
return [] if multiline else {}
minio/minioadmin.py
Outdated
| if multiline: | ||
| return [json.loads(line) for line in proc.stdout.split("\n")] | ||
| return [json.loads(line) for line in proc.stdout.split("\n") if line] |
There was a problem hiding this comment.
No change required in this line.
There was a problem hiding this comment.
The error I had actually occurs when proc.stdout is not empty, but has an empty b"" after split.
There was a problem hiding this comment.
Here is an example:
The response:
'{"status":"success","policy":"diagnostics","isGroup":false}\n{"status":"success","policy":"readonly","isGroup":false}\n{"status":"success","policy":"readwrite","isGroup":false}\n{"status":"success","policy":"writeonly","isGroup":false}\n'
becomes
['{"status":"success","policy":"diagnostics","isGroup":false}',
'{"status":"success","policy":"readonly","isGroup":false}',
'{"status":"success","policy":"readwrite","isGroup":false}',
'{"status":"success","policy":"writeonly","isGroup":false}',
'']which fails at the very end because the trailing new-line results in empty string.
There was a problem hiding this comment.
you could also do something like proc.stdout.strip().split("\n") to get rid of any leading/trailing whitespace.
There was a problem hiding this comment.
Use .splitlines() instead of .split("\n")
There was a problem hiding this comment.
That still doesn't handle cases where response is "a\n\nb\nc\n", but it's good enough for me.
balamurugana
changed the title
To avoid empty strings after splitting `proc.stdout`, `splitlines` is used instead of `split`. To avoid errors when `proc.stdout` itself is empty, an additional guard has been added. In this case, an empty list or dictionary is returned, depending on `multiline`. Also, `text=True` is passed `subprocess.run` to ensure `proc.stdout` is a string and not bytes object.
json.loads(line)would throw an error if line is empty (e.g.line == b'').To fix it, simply check if line is truthy (i.e. not empty).
Also, since
proc.stdoutis of typebytesthe argument passed toproc.stdout.splitwas changed tob"\n".