Repository: scoutapp/scout_apm_python
Pull request #564 (2020-09-01 16:00:55)

Simplify and fully test core agent manifest parsing
Don't log a full exception when the manifest file is missing, which is the default on a new system. Most users won't use debug-level logs, but when they do we don't want to scare them with a stacktrace for an expected exception.


Patch:
diff --git a/src/scout_apm/compat.py b/src/scout_apm/compat.py
index b252812..1edef4c 100644
--- a/src/scout_apm/compat.py
+++ b/src/scout_apm/compat.py
@@ -25,6 +25,12 @@ else:
         return dictionary.iteritems()  # noqa: B301
 
 
+# open() raises OSError, or a specific subclass, on Python 3+
+if sys.version_info >= (3, 3):
+    CouldNotOpenFile = OSError
+else:
+    CouldNotOpenFile = IOError
+
 if sys.version_info >= (3, 2):
     from contextlib import ContextDecorator
 else:
diff --git a/src/scout_apm/core/core_agent_manager.py b/src/scout_apm/core/core_agent_manager.py
index 31fa57a..e17ecb9 100644
--- a/src/scout_apm/core/core_agent_manager.py
+++ b/src/scout_apm/core/core_agent_manager.py
@@ -1,6 +1,8 @@
 # coding=utf-8
+# coding=utf-8
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+import errno
 import hashlib
 import json
 import logging
@@ -11,7 +13,7 @@ import time
 
 from urllib3.exceptions import HTTPError
 
-from scout_apm.compat import urllib3_cert_pool_manager
+from scout_apm.compat import CouldNotOpenFile, text_type, urllib3_cert_pool_manager
 from scout_apm.core.config import scout_config
 
 logger = logging.getLogger(__name__)
@@ -120,8 +122,8 @@ class CoreAgentManager(object):
             return []
 
     def verify(self):
-        manifest = CoreAgentManifest(self.core_agent_dir + "/manifest.json")
-        if not manifest.is_valid():
+        manifest = parse_manifest(self.core_agent_dir + "/manifest.json")
+        if manifest is None:
             logger.debug(
                 "Core Agent verification failed: CoreAgentManifest is not valid."
             )
@@ -129,7 +131,7 @@ class CoreAgentManager(object):
             self.core_agent_bin_version = None
             return False
 
-        bin_path = self.core_agent_dir + "/" + manifest.bin_name
+        bin_path = os.path.join(self.core_agent_dir, manifest.bin_name)
         if sha256_digest(bin_path) == manifest.sha256:
             self.core_agent_bin_path = bin_path
             self.core_agent_bin_version = manifest.bin_version
@@ -230,33 +232,50 @@ class CoreAgentDownloader(object):
         return scout_config.value("download_url")
 
 
+def parse_manifest(path):
+    try:
+        manifest_file = open(path)
+    except CouldNotOpenFile as exc:
+        if exc.errno == errno.ENOENT:
+            logger.debug("Core Agent Manifest does not exist at %s", path)
+        else:
+            logger.debug("Error opening Core Agent Manifest at %s", path, exc_info=exc)
+        return None
+
+    try:
+        with manifest_file:
+            data = json.load(manifest_file)
+            logger.debug("Core Agent manifest json: %s", data)
+
+            bin_name = data["core_agent_binary"]
+            if not isinstance(bin_name, text_type):
+                raise TypeError("core_agent_binary should be a string.")
+            bin_version = data["core_agent_version"]
+            if not isinstance(bin_version, text_type):
+                raise TypeError("core_agent_version should be a string.")
+            sha256 = data["core_agent_binary_sha256"]
+            if not isinstance(sha256, text_type):
+                raise TypeError("core_agent_binary_sha256 should be a string.")
+
+            return CoreAgentManifest(
+                bin_name=bin_name,
+                bin_version=bin_version,
+                sha256=sha256,
+            )
+
+    # IOError => OSError on Python 3
+    except (KeyError, ValueError, TypeError, OSError, IOError) as exc:  # noqa: B014
+        logger.debug("Error parsing Core Agent Manifest", exc_info=exc)
+        return None
+
+
 class CoreAgentManifest(object):
-    def __init__(self, path):
-        self.manifest_path = path
-        self.bin_name = None
-        self.bin_version = None
-        self.sha256 = None
-        self.valid = False
-        try:
-            self.parse()
-        # noqa for this issue: https://github.com/PyCQA/flake8-bugbear/issues/110
-        except (ValueError, TypeError, OSError, IOError) as exc:  # noqa: B014
-            logger.debug("Error parsing Core Agent Manifest", exc_info=exc)
-
-    def parse(self):
-        logger.debug("Parsing Core Agent manifest path: %s", self.manifest_path)
-        with open(self.manifest_path) as manifest_file:
-            self.raw = manifest_file.read()
-            self.json = json.loads(self.raw)
-            self.version = self.json["version"]
-            self.bin_version = self.json["core_agent_version"]
-            self.bin_name = self.json["core_agent_binary"]
-            self.sha256 = self.json["core_agent_binary_sha256"]
-            self.valid = True
-            logger.debug("Core Agent manifest json: %s", self.json)
-
-    def is_valid(self):
-        return self.valid
+    __slots__ = ("bin_name", "bin_version", "sha256")
+
+    def __init__(self, bin_name, bin_version, sha256):
+        self.bin_name = bin_name
+        self.bin_version = bin_version
+        self.sha256 = sha256
 
 
 def sha256_digest(filename, block_size=65536):
diff --git a/tox.ini b/tox.ini
index 43297c1..9a463c0 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,11 +1,11 @@
 [tox]
 isolated_build = True
 envlist =
-    py38-codestyle,
-    {py27,py34,py35}-django18,
-    {py27,py34,py35,py36}-django111,
-    {py34,py35,py36,py37,py38}-django20,
-    {py35,py36,py37,py38}-django21,
+    py38-codestyle
+    {py27,py34,py35}-django18
+    {py27,py34,py35,py36}-django111
+    {py34,py35,py36,py37,py38}-django20
+    {py35,py36,py37,py38}-django21
     {py35,py36,py37,py38}-django22
     {py36,py37,py38}-django30
 


Test patch:
diff --git a/tests/unit/core/test_core_agent_manager.py b/tests/unit/core/test_core_agent_manager.py
new file mode 100644
index 0000000..6aa3d8c
--- /dev/null
+++ b/tests/unit/core/test_core_agent_manager.py
@@ -0,0 +1,111 @@
+# coding=utf-8
+from __future__ import absolute_import, division, print_function, unicode_literals
+
+import errno
+import logging
+import sys
+
+from scout_apm.core.core_agent_manager import parse_manifest
+from tests.compat import mock
+
+
+class TestParseManifest(object):
+    def test_fail_does_not_exist(self, caplog, tmp_path):
+        filename = str(tmp_path / "does-not-exist.json")
+
+        result = parse_manifest(filename)
+
+        assert result is None
+        assert caplog.record_tuples == [
+            (
+                "scout_apm.core.core_agent_manager",
+                logging.DEBUG,
+                "Core Agent Manifest does not exist at " + filename,
+            )
+        ]
+
+    def test_fail_other_open_error(self, caplog, tmp_path):
+        filename = str(tmp_path / "does-not-exist.json")
+        if sys.version_info[0] == 2:
+            error = IOError("Woops", errno.EACCES)
+        else:
+            error = OSError(errno.EACCES)
+        mock_open = mock.patch(
+            "scout_apm.core.core_agent_manager.open",
+            create=True,
+            side_effect=error,
+        )
+
+        with mock_open:
+            result = parse_manifest(filename)
+
+        assert result is None
+        assert caplog.record_tuples == [
+            (
+                "scout_apm.core.core_agent_manager",
+                logging.DEBUG,
+                "Error opening Core Agent Manifest at " + filename,
+            )
+        ]
+
+    def test_fail_core_agent_binary_not_string(self, caplog, tmp_path):
+        manifest = tmp_path / "manifest.json"
+        manifest.write_text('{"core_agent_binary": 1}')
+
+        result = parse_manifest(str(manifest))
+
+        assert result is None
+        assert len(caplog.record_tuples) == 2
+        assert caplog.record_tuples[1] == (
+            "scout_apm.core.core_agent_manager",
+            logging.DEBUG,
+            "Error parsing Core Agent Manifest",
+        )
+
+    def test_fail_core_agent_version_not_string(self, caplog, tmp_path):
+        manifest = tmp_path / "manifest.json"
+        manifest.write_text('{"core_agent_binary": "", "core_agent_version": 1}')
+
+        result = parse_manifest(str(manifest))
+
+        assert result is None
+        assert len(caplog.record_tuples) == 2
+        assert caplog.record_tuples[1] == (
+            "scout_apm.core.core_agent_manager",
+            logging.DEBUG,
+            "Error parsing Core Agent Manifest",
+        )
+
+    def test_fail_core_agent_binary_sha256_not_string(self, caplog, tmp_path):
+        manifest = tmp_path / "manifest.json"
+        manifest.write_text(
+            '{"core_agent_binary": "", "core_agent_version": "", '
+            + '"core_agent_binary_sha256": 1}'
+        )
+
+        result = parse_manifest(str(manifest))
+
+        assert result is None
+        assert len(caplog.record_tuples) == 2
+        assert caplog.record_tuples[1] == (
+            "scout_apm.core.core_agent_manager",
+            logging.DEBUG,
+            "Error parsing Core Agent Manifest",
+        )
+
+    def test_success(self, caplog, tmp_path):
+        manifest = tmp_path / "manifest.json"
+        manifest.write_text(
+            '{"core_agent_binary": "bin", "core_agent_version": "1.2.3", '
+            + '"core_agent_binary_sha256": "abc"}'
+        )
+
+        result = parse_manifest(str(manifest))
+
+        assert result.bin_name == "bin"
+        assert result.bin_version == "1.2.3"
+        assert result.sha256 == "abc"
+        logger, level, message = caplog.record_tuples[0]
+        assert logger == "scout_apm.core.core_agent_manager"
+        assert level == logging.DEBUG
+        assert message.startswith("Core Agent manifest json: ")