CASMPET-6723 catch UnicodeDecodeError from os/run_command (#54)

* CASMPET-6723 raise a UnicodeDecodeError from run_command function

* CASMPET-6723 fix test for unicode run_command error

* CSAMPET-6723 remove trailing whitespace

* CASMPET-6723 remove whitespace in hsm_components_url

* Make decoding a parameter

Make the `decode` an optional, and just return the string as bytes
unless the developer has passed an encoding.

It is false to assume an encoding, and auto-resolving the encoding is
unreliable.

* CASMPET-6723 catch jsonDecodeError if request returns bad json

* CASMPET-6723 fixed 'response' spelling

* CASMPET-6723 fixing python lint

* CASMPET-6723 change JSONDecodeError to ValueError

* CASMPET-6723 reformat printed strings

* CASMPET-6723 fix line length and indentation

---------

Co-authored-by: Russell Bunch <[email protected]>

libcsm/bss/api.py

@@ -57,12 +57,12 @@ def get_bss_bootparams(self, xname: str) -> str:
                                                 "Content-Type": "application/json"},
                                     data=json.dumps(body))
         except requests.exceptions.RequestException as ex:
-            raise requests.exceptions.RequestException(f'ERROR exception: \
-                {type(ex).__name__} when trying to get bootparameters')
+            raise requests.exceptions.RequestException(f'ERROR exception:' \
+                f'{type(ex).__name__} when trying to get bootparameters')
         if bss_response.status_code != http.HTTPStatus.OK:
-            raise requests.exceptions.RequestException(f'ERROR Failed to get BSS \
-                bootparameters for {xname}. Recieved http response:\
-                {bss_response.status_code} from  BSS.')
+            raise requests.exceptions.RequestException(f'ERROR Failed to get BSS' \
+                f'bootparameters for {xname}. Recieved http response:' \
+                f'{bss_response.status_code} from  BSS.')
         return bss_response.json()[0]
 
 
@@ -76,12 +76,12 @@ def patch_bss_bootparams(self, xname : str, bss_json) -> None:
                                         "Content-Type": "application/json"},
                                 data=json.dumps(bss_json))
         except requests.exceptions.RequestException as ex:
-            raise requests.exceptions.RequestException(f'ERROR exception: \
-                {type(ex).__name__} when trying to patch bootparameters')
+            raise requests.exceptions.RequestException(f'ERROR exception:' \
+                f'{type(ex).__name__} when trying to patch bootparameters')
         if patch_response.status_code != http.HTTPStatus.OK:
-            raise requests.exceptions.RequestException(f'ERROR Failed to patch BSS \
-                bootparameters for {xname}. Recieved {patch_response.status_code} \
-                from as BSS response.')
+            raise requests.exceptions.RequestException(f'ERROR Failed to patch BSS' \
+                f'bootparameters for {xname}. Recieved {patch_response.status_code}' \
+                f'from as BSS response.')
         print('BSS entry patched')
 
     def set_bss_image(self, xname: str, image_dict: dict) -> None:
@@ -93,23 +93,24 @@ def set_bss_image(self, xname: str, image_dict: dict) -> None:
         """
 
         if 'initrd' not in image_dict or 'kernel' not in image_dict or 'rootfs' not in image_dict:
-            raise ValueError(f"ERROR set_bss_image has inputs 'xname' and 'image_dictonary' where \
-            'image_dictionary' is a dictionary containing values for 'initrd', 'kernel', and \
-            'rootfs'. The inputs recieved were xname:{xname}, image_dictionary:{image_dict}")
+            raise ValueError(f"ERROR set_bss_image has inputs 'xname' and 'image_dictonary' where" \
+                f"'image_dictionary' is a dictionary containing values for 'initrd', 'kernel', " \
+                f"and 'rootfs'. The inputs recieved were xname:{xname}, " \
+                f"image_dictionary:{image_dict}")
 
         bss_json = self.get_bss_bootparams(xname)
         if 'initrd' not in bss_json or 'kernel' not in bss_json:
-            raise KeyError(f"BSS bootparams did not have the expected keys 'initrd' or 'kernel'. \
-            Boot parameters recieved: {bss_json}")
+            raise KeyError(f"BSS bootparams did not have the expected keys 'initrd' or 'kernel'." \
+                f"Boot parameters recieved: {bss_json}")
         # set new images
         bss_json['initrd'] = image_dict['initrd']
         bss_json['kernel'] = image_dict['kernel']
         params = bss_json['params']
         try:
             current_rootfs = params.split("metal.server=", 1)[1].split(" ",1)[0]
         except Exception as exc:
-            raise KeyError(f"ERROR could not find current metal.server image in {xname} \
-                bss params") from exc
+            raise KeyError(f"ERROR could not find current metal.server image in {xname}" \
+                f"bss params") from exc
 
         bss_json['params'] = params.replace(current_rootfs, image_dict['rootfs'])
 

libcsm/hsm/components.py

@@ -40,8 +40,8 @@ def get_components(role_subrole: str, api_gateway_address="api-gw-service-nmn.lo
     auth = api.Auth()
     auth.refresh_token()
     session = get_session()
-    hsm_components_url = f'https://{api_gateway_address}/\
-        apis/smd/hsm/v2/State/Components'
+    hsm_components_url = f'https://{api_gateway_address}/'\
+        f'apis/smd/hsm/v2/State/Components'
     # get components
     if role_subrole not in ROLE_SUBROLES:
         raise KeyError(f'ERROR {role_subrole} is not a valid role_subrole')
@@ -51,9 +51,9 @@ def get_components(role_subrole: str, api_gateway_address="api-gw-service-nmn.lo
             f'?role=Management&subrole={subrole}',
             headers={'Authorization': f'Bearer {auth.token}'})
     except requests.exceptions.RequestException as ex:
-        raise requests.exceptions.RequestException(f'ERROR exception: \
-            {type(ex).__name__} when trying to get components')
+        raise requests.exceptions.RequestException(f'ERROR exception:' \
+            f'{type(ex).__name__} when trying to get components')
     if components_response.status_code != http.HTTPStatus.OK:
-        raise requests.exceptions.RequestException(f'ERROR Failed \
-            to get components with subrole {subrole}')
+        raise requests.exceptions.RequestException(f'ERROR Failed' \
+            f'to get components with subrole {subrole}')
     return components_response

libcsm/os.py

@@ -45,8 +45,8 @@ class _CLI:
     An object to abstract the return result from ``run_command``.
 
     """
-    _stdout = ''
-    _stderr = ''
+    _stdout = b''
+    _stderr = b''
     _return_code = None
     _duration = None
 
@@ -86,9 +86,15 @@ def _run(self) -> None:
             self._return_code = error.errno
             LOG.error('Could not find command for given args: %s', self.args)
         else:
-            self._stdout = stdout.decode('utf8')
-            self._stderr = stderr.decode('utf8')
-            self._return_code = command.returncode
+            try:
+                self._stdout = stdout
+                self._stderr = stderr
+                self._return_code = command.returncode
+            except UnicodeDecodeError as error:
+                self._stderr = error
+                self._return_code = 1
+                LOG.error('Could not decode stdout or stderr recieved from given args: %s. \
+stdout: %s, stderr %s', self.args, stdout, stderr)
         self._duration = time() - start_time
         if self._return_code and self._duration:
             LOG.info(
@@ -99,14 +105,14 @@ def _run(self) -> None:
             )
 
     @property
-    def stdout(self) -> str:
+    def stdout(self) -> [str, bytes]:
         """
         ``stdout`` from the command.
         """
         return self._stdout
 
     @property
-    def stderr(self) -> str:
+    def stderr(self) -> [str, bytes]:
         """
         ``stderr`` from the command.
         """
@@ -126,6 +132,26 @@ def duration(self) -> float:
         """
         return self._duration
 
+    def decode(self, charset: str) -> None:
+        """
+        Decodes ``self._stdout`` and ``self._stderr`` with the given ``charset``.
+        :param charset: The character set to decode with.
+        """
+        if not isinstance(self._stdout, str):
+            try:
+                self._stdout = self._stdout.decode(charset)
+            except ValueError as error:
+                LOG.error("Command output was requested to be decoded as"
+                          " %s but failed: %s", charset, error)
+                raise error
+        if not isinstance(self._stderr, str):
+            try:
+                self._stderr = self._stderr.decode(charset)
+            except ValueError as error:
+                LOG.error("Command output was requested to be decoded as"
+                          " %s but failed: %s", charset, error)
+                raise error
+
 
 @contextmanager
 def chdir(directory: str, create: bool = False) -> None:
@@ -165,7 +191,9 @@ def chdir(directory: str, create: bool = False) -> None:
 def run_command(
     args: [list, str],
     in_shell: bool = False,
-    silence: bool = False, ) -> _CLI:
+    silence: bool = False,
+    charset: str = None,
+) -> _CLI:
     """
     Runs a given command or list of commands by instantiating a ``CLI`` object.
 
@@ -179,6 +207,9 @@ def run_command(
     :param args: List of arguments to run, can also be a string. If a string,
     :param in_shell: Whether to use a shell when invoking the command.
     :param silence: Tells this not to output the command to console.
+    :param charset: Returns the command ``stdout`` and ``stderr`` as a
+                    string instead of bytes, and decoded with the given
+                    ``charset``.
     """
     args_string = [str(x) for x in args]
     if not silence:
@@ -187,4 +218,7 @@ def run_command(
             ' '.join(args_string),
             in_shell
         )
-    return _CLI(args_string, shell=in_shell)
+    result = _CLI(args_string, shell=in_shell)
+    if charset:
+        result.decode(charset)
+    return result

libcsm/sls/api.py

@@ -54,43 +54,51 @@ def get_management_components_from_sls(self) -> requests.Response:
                 'search/hardware?extra_properties.Role=Management',
                 headers={'Authorization': f'Bearer {self._auth.token}'})
         except requests.exceptions.RequestException as ex:
-            raise requests.exceptions.RequestException(f'ERROR exception: {type(ex).__name__} \
-                when trying to get management components from SLS') from ex
+            raise requests.exceptions.RequestException(f'ERROR exception: {type(ex).__name__}' \
+                f'when trying to get management components from SLS') from ex
         if components_response.status_code != http.HTTPStatus.OK:
-            raise requests.exceptions.RequestException(f'ERROR Bad response \
-                recieved from SLS. Recived: {components_response}')
+            raise requests.exceptions.RequestException(f'ERROR Bad response' \
+                f'recieved from SLS. Recived: {components_response}')
 
         return components_response
 
     def get_xname(self, hostname: str) -> str:
         """
         Function to get the xname of a node from SLS based on a provided hostname.
         """
-        components_response = self.get_management_components_from_sls()
+        try:
+            components_response = (self.get_management_components_from_sls()).json()
+        except ValueError as error:
+            raise ValueError(f'ERROR did not get valid json for management components' \
+                f'from sls. {error}') from error
 
-        for node in components_response.json():
+        for node in components_response:
             try:
                 if hostname in node['ExtraProperties']['Aliases']:
                     return node['Xname']
             except KeyError as error:
-                raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the \
-                response from sls. These fields are expected in the json response. \
-                The resonponse was {components_response.json()}') from error
+                raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the' \
+                    f'response from sls. These fields are expected in the json response.' \
+                    f'The response was {components_response}') from error
         raise ValueError(f'ERROR hostname:{hostname} was not found in management nodes.')
 
     def get_hostname(self, xname: str) -> str:
         """
         Function to get the hostname of a management node from SLS based on a provided xname.
         """
-        components_response = self.get_management_components_from_sls()
+        try:
+            components_response = (self.get_management_components_from_sls()).json()
+        except ValueError as error:
+            raise ValueError(f'ERROR did not get valid json for management components' \
+                f'from sls. {error}') from error
 
-        for node in components_response.json():
+        for node in components_response:
             try:
                 if xname == node['Xname']:
                     # assumes the hostname is the first entry in ['ExtraProperties']['Aliases']
                     return node['ExtraProperties']['Aliases'][0]
             except KeyError as error:
-                raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the \
-                    response from sls. These fields are expected in the json response. \
-                    The resonponse was {components_response.json()}') from error
+                raise KeyError(f'ERROR [ExtraProperties][Aliases] was not in the' \
+                    f'response from sls. These fields are expected in the json response.' \
+                    f'The response was {components_response}') from error
         raise ValueError(f'ERROR xname:{xname} was not found in management nodes.')

libcsm/tests/test_os.py

@@ -25,6 +25,10 @@
 Tests for the ``os`` module.
 """
 from os import getcwd
+from subprocess import Popen
+
+import pytest
+import mock
 
 from libcsm.os import run_command
 from libcsm.os import chdir
@@ -53,6 +57,49 @@ def test_run_command(self) -> None:
             assert isinstance(result.return_code, int)
             assert result.stdout or result.stderr
 
+    def test_run_command_decode(self) -> None:
+        """
+        Assert that a ``_CLI`` object successfully decodes its ``stdout`` and
+        ``stderr``.
+        """
+        command = ['ls', '-l']
+        shell_command = 'ls -l'
+        bad_command = 'foo!!!'
+        no_shell_result = run_command(shell_command)
+        shell_result = run_command(
+            shell_command,
+            in_shell=True,
+            charset='utf-8'
+        )
+        bad_result = run_command(bad_command)
+        command_result = run_command(command)
+        for result in [no_shell_result, shell_result, bad_result,
+                       command_result]:
+            assert result.duration > 0.0
+            assert isinstance(result.return_code, int)
+            assert result.stdout or result.stderr
+
+    def test_run_command_decode_unicode_error(self) -> None:
+        """
+        Assert that ``UnicodeDecodeErrors`` are caught by the ``run_command``
+        and a non-zero return code is received.
+        """
+        # Test using a CP1252 (windows ostensibly) character "œ"
+        byte_string = b'\x9c'
+        with mock.patch.object(
+            Popen,
+            'communicate',
+            return_value=(byte_string, byte_string)
+        ):
+            result = run_command(['ls', '-l'])
+            assert result.duration > 0.0
+            assert isinstance(result.return_code, int)
+            assert result.stdout or result.stderr
+            assert isinstance(result.stdout, bytes)
+            assert isinstance(result.stderr, bytes)
+            with pytest.raises(UnicodeDecodeError):
+                run_command(['ls', '-l'], charset='utf-8')
+
     def test_chdir(self) -> None:
         """
         Assert that our context manager will change directories and