Merge ~jslarraz/review-tools:rework-extract-file into review-tools:master

[Jorge Sancho Larraz]

rework BaseContainer:_extract_file and add relevant tests

[Alex Murray (alexmurray)]

LGTM - nice use of type hints too!

review: Approve

diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index 173036a..3cd069f 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -1,6 +1,7 @@
 import os
 import shutil
 import magic
+from typing import Union
 from tempfile import mkdtemp
 
 from reviewtools.common import (
@@ -9,7 +10,6 @@ from reviewtools.common import (
     debug,
     unpack_pkg,
     recursive_rm,
-    open_file_read,
 )
 
 
@@ -104,15 +104,26 @@ class BaseContainer:
         if os.path.exists(self.tmp_dir):
             recursive_rm(self.tmp_dir)
 
-    def _extract_file(self, fn):
-        """Extract file"""
-        if not fn.startswith("/"):
-            raise ContainerException("_extract_file() expects absolute path")
-        rel = os.path.relpath(fn, self.unpack_dir)
+    def get_file(self, fn: str) -> Union[str, None]:
+        """
+        Reads and return the raw content of the specified file.
+
+        Args:
+            fn (str): file path relative to the container root (i.e. unpack_dir)
+
+        Returns:
+            str: raw content of the specified file. The output will need to be
+                decoded() to use it as a string
+
+        Raises:
+            ContainerException: Calling this method will raise an exception if
+            the requested file is not found.
+        """
+        if not os.path.exists(os.path.join(self.unpack_dir, fn)):
+            raise ContainerException("Could not find '%s'" % fn)
 
-        if not os.path.isfile(fn):
-            raise ContainerException("Could not find '%s'" % rel)
-        return open_file_read(fn)
+        with open(os.path.join(self.unpack_dir, fn), "rb") as fd:
+            return fd.read()
 
     def get_files_list(self):
         """List all files included in this package."""
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index 518e9a2..aeea216 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -1982,11 +1982,10 @@ class SnapReviewLint(SnapReview):
                 appnames.append("%s.%s" % (self.snap_yaml["name"], app))
 
         try:
-            fh = self.snap._extract_file(fn)
+            file = self.snap.get_file(fn)
         except ContainerException as e:
             error(str(e))
-        lines = fh.readlines()
-        fh.close()
+        lines = file.decode().split("\n")
 
         # For now, just check Exec=, Icon= and bools since:
         # - snapd strips out anything it doesn't understand
@@ -2130,7 +2129,7 @@ class SnapReviewLint(SnapReview):
         for f in self.pkg_files:
             fn = os.path.relpath(f, self._get_unpack_dir())
             if fn.startswith("meta/gui/") and fn.endswith(".desktop"):
-                self._verify_desktop_file(f)
+                self._verify_desktop_file(fn)
                 has_desktop_files = True
                 break
 
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
index 4d0563f..976c74a 100644
--- a/reviewtools/tests/containers/test_base_container.py
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -12,9 +12,7 @@ class TestBaseContainer(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
         cls.tmp_dir = mkdtemp()
-        cls.fn = make_snap2(
-            output_dir=cls.tmp_dir,
-        )
+        cls.fn = make_snap2(output_dir=cls.tmp_dir, yaml="Test")
 
     @classmethod
     def tearDownClass(cls):
@@ -25,7 +23,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_initialization_container__happy(self):
         try:
             BaseContainer(self.fn)
-        except Exception:
+        except ContainerException:
             self.fail(
                 "An unexpected error occurred during BaseContainer initialization with container file"
             )
@@ -37,3 +35,18 @@ class TestBaseContainer(unittest.TestCase):
                 "does not exists or has incorrect permissions"
                 in context.exception.value
             )
+
+    # Test get file
+    def test_get_file__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            snap_yaml = container.get_file("meta/snap.yaml")
+            self.assertEqual(snap_yaml.decode(), "Test")
+        except ContainerException:
+            self.fail("An unexpected error occurred while getting meta/snap.yaml")
+
+    def test_get_file__missing_file(self):
+        with self.assertRaises(ContainerException) as context:
+            container = BaseContainer(self.fn)
+            container.get_file("non/existent/file")
+            self.assertTrue("Could not find" in context.exception.value)