[Files] - fixes and improvements (#4081)

* [Files] - support automatic reading when reading http + uts

* support reading xif as text files

* fixed bug when unicoding files from memory

* changelog

* exceptions docstring

* arrange exceptions

* trigger retry on connection errors

* update exceptions

* add reading file errors uts

* handle exceptions of __read_git_file

* changelog update

* update pr num

* changelog change-name

* naming convensions

* changelog

.changelog/4081.yml

@@ -0,0 +1,6 @@
+changes:
+- description: Fixed an issue where SDK would crash when trying to read files which are not encoded with utf-8 directly from memory.
+  type: fix
+- description: Improved error handling when reading files locally, remotely or from git.
+  type: feature
+pr_number: 4081

demisto_sdk/commands/common/files/errors.py

@@ -1,37 +1,78 @@
 from pathlib import Path
+from typing import Optional
 
 
 class FileReadError(IOError):
-    pass
+    """
+    Base class for errors reading files, catch this error to catch any error related to reading files
+    """
+
+    def __init__(self, message: str, exc: Exception):
+        super().__init__(message)
+        self.original_exc = exc
 
 
 class FileWriteError(IOError):
     def __init__(self, path: Path, exc: Exception):
+        self.original_exc = exc
         super().__init__(f"could not write file {path}, full error:\n{exc}")
 
 
 class LocalFileReadError(FileReadError):
+    """
+    Raised when there was an issue reading files from local file system.
+    """
+
     def __init__(self, path: Path, exc: Exception):
-        self.original_exc = exc
-        super().__init__(f"file {path} could not be read, full error:\n{exc}")
+        super().__init__(f"file {path} could not be read, error:\n{exc}", exc=exc)
+
 
+class MemoryFileReadError(FileReadError):
+    """
+    Raised when there was an issue reading files from memory (as bytes)
+    """
 
-class FileContentReadError(FileReadError):
     def __init__(self, exc: Exception):
-        super().__init__(f"file could not be read, full error:\n{exc}")
+        super().__init__(f"file could not be read, error:\n{exc}", exc=exc)
+
+
+class FileLoadError(FileReadError):
+    """
+    Raised when there was an issue to load a file into the requested object, e.g (as text, json, yml and so on)
+    """
+
+    def __init__(self, exc: Exception, class_name: str, path: Optional[Path] = None):
+        if path:
+            message = f"Could not load file {path} as {class_name}"
+        else:
+            message = f"Could not load file as {class_name}"
+        super().__init__(message, exc=exc)
 
 
 class GitFileReadError(FileReadError):
+    """
+    Raised when there was an issue reading files from git (from remote/locally)
+    """
+
     def __init__(self, path: Path, tag: str, exc: Exception):
         super().__init__(
-            f"Could not get file {path} from branch/commit {tag}, full_error:\n{exc}"
+            f"Could not get file {path} from branch/commit {tag}, error:\n{exc}",
+            exc=exc,
         )
 
 
-class HttpFileReadError(IOError):
+class HttpFileReadError(FileReadError):
+    """
+    Raised when there was an issue retrieving a file via http-request / api call
+    """
+
     def __init__(self, url: str, exc: Exception):
-        super().__init__(f"Could not read file from {url}, full error:\n{exc}")
+        super().__init__(f"Could not read file from {url}, full error:\n{exc}", exc=exc)
 
 
 class UnknownFileError(ValueError):
+    """
+    Raised when failing to automatically detect subclass of File object dynamically by file's suffix/name
+    """
+
     pass

demisto_sdk/commands/common/files/file.py

@@ -18,11 +18,12 @@
     urljoin,
 )
 from demisto_sdk.commands.common.files.errors import (
-    FileContentReadError,
+    FileLoadError,
     FileReadError,
     GitFileReadError,
     HttpFileReadError,
     LocalFileReadError,
+    MemoryFileReadError,
     UnknownFileError,
 )
 from demisto_sdk.commands.common.git_content_config import GitContentConfig
@@ -37,6 +38,16 @@ class File(ABC):
     def path(self) -> Path:
         return getattr(self, "_path")
 
+    @property
+    def safe_path(self) -> Union[Path, None]:
+        """
+        Returns the path of the file if exists, otherwise returns None
+        """
+        try:
+            return self.path
+        except AttributeError:
+            return None
+
     @cached_property
     def file_content(self) -> bytes:
         return self.path.read_bytes()
@@ -165,9 +176,9 @@ def read_from_file_content(
 
         try:
             return cls.as_default(encoding=encoding, handler=handler).load(file_content)
-        except LocalFileReadError as e:
+        except FileLoadError as error:
             logger.error(f"Could not read file content as {cls.__name__} file")
-            raise FileContentReadError(exc=e.original_exc)
+            raise MemoryFileReadError(error.original_exc) from error
 
     @classmethod
     def as_path(cls, path: Path, **kwargs):
@@ -229,11 +240,11 @@ def read_from_local_path(
     def __read_local_file(self):
         try:
             return self.load(self.file_content)
-        except FileReadError:
+        except FileLoadError as error:
             logger.error(
                 f"Could not read file {self.path} as {self.__class__.__name__} file"
             )
-            raise
+            raise LocalFileReadError(self.path, exc=error.original_exc) from error
 
     @classmethod
     @lru_cache
@@ -286,17 +297,17 @@ def __read_git_file(self, tag: str, from_remote: bool = True) -> Any:
                     self.path, commit_or_branch=tag, from_remote=from_remote
                 )
             )
-        except Exception as e:
+        except Exception as error:
             if from_remote:
                 tag = f"{DEMISTO_GIT_UPSTREAM}:{tag}"
             logger.error(
-                f"Could not read git file {self.path} from {tag} as {self.__class__.__name__} file"
+                f"Could not read git file {self.path} from branch/commit {tag} as {self.__class__.__name__} file"
             )
             raise GitFileReadError(
                 self.path,
                 tag=tag,
-                exc=e,
-            )
+                exc=error.original_exc if isinstance(error, FileReadError) else error,
+            ) from error
 
     @classmethod
     def read_from_github_api(
@@ -440,10 +451,6 @@ def read_from_http_request(
             Any: the file content in the desired format
 
         """
-        if cls is File:
-            raise ValueError(
-                "when reading from file content please specify concrete class"
-            )
         if clear_cache:
             cls.read_from_http_request.cache_clear()
         try:
@@ -454,8 +461,15 @@ def read_from_http_request(
                 timeout=timeout,
                 headers={key: value for key, value in headers} if headers else None,
             )
+            if response.status_code == requests.codes.not_found:
+                raise FileNotFoundError(f"file in {url} does not exist")
             response.raise_for_status()
         except RequestException as e:
+            if isinstance(e, (ConnectionError, Timeout)):
+                logger.debug(
+                    f"Could not retrieve file from {url} due to a connection issue"
+                )
+                raise
             logger.exception(f"Could not retrieve file from {url}")
             raise HttpFileReadError(url, exc=e)
 
@@ -465,6 +479,6 @@ def read_from_http_request(
             return _cls.read_from_file_content(
                 response.content, encoding=encoding, handler=handler
             )
-        except FileContentReadError as e:
-            logger.error(f"Could not read file from {url} as {_cls.__name__} file")
-            raise HttpFileReadError(url, exc=e)
+        except MemoryFileReadError as e:
+            logger.error(f"Could not load file from URL {url} as {_cls.__name__} file")
+            raise HttpFileReadError(url, exc=e.original_exc) from e

demisto_sdk/commands/common/files/ini_file.py

@@ -3,6 +3,7 @@
 from typing import Any
 
 from demisto_sdk.commands.common.constants import PACKS_PACK_IGNORE_FILE_NAME
+from demisto_sdk.commands.common.files.errors import FileLoadError
 from demisto_sdk.commands.common.files.text_file import TextFile
 
 
@@ -16,7 +17,14 @@ def is_model_type_by_path(cls, path: Path) -> bool:
 
     def load(self, file_content: bytes) -> ConfigParser:
         config_parser = ConfigParser(allow_no_value=True)
-        config_parser.read_string(super().load(file_content))
+        file_content_as_text = super().load(file_content)
+        try:
+            config_parser.read_string(file_content_as_text)
+        except Exception as error:
+            raise FileLoadError(
+                error, class_name=self.__class__.__name__, path=self.safe_path
+            )
+
         return config_parser
 
     def _do_write(self, data: Any, path: Path, **kwargs):

demisto_sdk/commands/common/files/structured_file.py

@@ -3,7 +3,7 @@
 from pathlib import Path
 from typing import Any, Optional, Type, Union
 
-from demisto_sdk.commands.common.files.errors import FileWriteError
+from demisto_sdk.commands.common.files.errors import FileLoadError, FileWriteError
 from demisto_sdk.commands.common.files.text_file import TextFile
 from demisto_sdk.commands.common.handlers.xsoar_handler import XSOAR_Handler
 from demisto_sdk.commands.common.logger import logger
@@ -30,7 +30,13 @@ def as_default(cls, **kwargs):
         return instance
 
     def load(self, file_content: bytes) -> Any:
-        return self.handler.load(StringIO(super().load(file_content)))
+        file_content_as_text = super().load(file_content)
+        try:
+            return self.handler.load(StringIO(file_content_as_text))
+        except Exception as error:
+            raise FileLoadError(
+                error, class_name=self.__class__.__name__, path=self.safe_path
+            )
 
     @classmethod
     def write(