fix/no-overwrite-config-badjson (mycroft-core/pull/2881)

cherry pick of MycroftAI#2881

Do not overwrite configs with malformed json

If a config file is loaded and is invalid saves to that file will not be
possible until it's manually restored. The store accepts a force flag to
override this protection.

The method returns True if the store succeeded so call sites can
verbally report the issue as well.

Separate tests of LocalConf into new test class

mycroft/configuration/config.py

@@ -84,6 +84,7 @@ class LocalConf(dict):
 
     def __init__(self, path):
         super(LocalConf, self).__init__()
+        self.is_valid = True  # is loaded json valid, updated when load occurs
         if path:
             self.path = path
             self.load_local(path)
@@ -104,18 +105,37 @@ def load_local(self, path):
             except Exception as e:
                 LOG.error("Error loading configuration '{}'".format(path))
                 LOG.error(repr(e))
+                self.is_valid = False
         else:
             LOG.debug("Configuration '{}' not defined, skipping".format(path))
 
-    def store(self, path=None):
-        """Cache the received settings locally.
+    def store(self, path=None, force=False):
+        """Save config to disk.
 
         The cache will be used if the remote is unreachable to load settings
         that are as close to the user's as possible.
+
+        path (str): path to store file to, if missing will use the path from
+                    where the config was loaded.
+        force (bool): Set to True if writing should occur despite the original
+                      was malformed.
+
+        Returns:
+            (bool) True if save was successful, else False.
         """
+        result = False
         path = path or self.path
-        with open(path, 'w') as f:
-            json.dump(self, f, indent=2)
+        if self.is_valid or force:
+            # Only write settings
+            with open(path, 'w') as f:
+                json.dump(self, f, indent=2)
+            result = True
+        else:
+            LOG.warning(('"{}" was not a valid config file when loaded, '
+                         'will not save config. Please correct the json or '
+                         'remove it to allow updates.').format(path))
+            result = False
+        return result
 
     def merge(self, conf):
         merge_dict(self, conf)
@@ -170,7 +190,7 @@ def __init__(self, cache=None):
 
                 for key in config:
                     self.__setitem__(key, config[key])
-                self.store(cache)
+                self.store(cache, force=True)
 
         except RequestException as e:
             LOG.error("RequestException fetching remote configuration: {}"

test/unittests/configuration/test_configuration.py

@@ -32,26 +32,60 @@ def test_remote(self, mock_api):
         self.assertTrue(rc['test_config'])
         self.assertEqual(rc['location']['city']['name'], 'Stockholm')
 
-    @patch('json.dump')
-    @patch('mycroft.configuration.config.exists')
-    @patch('mycroft.configuration.config.isfile')
-    @patch('mycroft.configuration.config.load_commented_json')
-    def test_local(self, mock_json_loader, mock_isfile, mock_exists,
-                   mock_json_dump):
+    @patch('mycroft.configuration.config.RemoteConf')
+    @patch('mycroft.configuration.config.LocalConf')
+    def test_update(self, mock_remote, mock_local):
+        mock_remote.return_value = {}
+        mock_local.return_value = {'a': 1}
+        c = mycroft.configuration.Configuration.get()
+        self.assertEqual(c, {'a': 1})
+
+        mock_local.return_value = {'a': 2}
+        mycroft.configuration.Configuration.updated('message')
+        self.assertEqual(c, {'a': 2})
+
+    def tearDown(self):
+        mycroft.configuration.Configuration.load_config_stack([{}], True)
+
+
+@patch('mycroft.configuration.config.exists')
+@patch('mycroft.configuration.config.isfile')
+@patch('mycroft.configuration.config.load_commented_json')
+class TestLocalConf(TestCase):
+    """Test cases for LocalConf class."""
+    def test_create(self, mock_json_loader, mock_isfile, mock_exists):
+        """Test that initialization and creation works as expected."""
         local_conf = {'answer': 42, 'falling_objects': ['flower pot', 'whale']}
         mock_exists.return_value = True
         mock_isfile.return_value = True
         mock_json_loader.return_value = local_conf
         lc = mycroft.configuration.LocalConf('test')
         self.assertEqual(lc, local_conf)
 
-        # Test merge method
+    def test_merge(self, mock_json_loader, mock_isfile, mock_exists):
+        """Check that configurations are merged correctly."""
+        local_conf = {'answer': 42, 'falling_objects': ['flower pot', 'whale']}
+        mock_exists.return_value = True
+        mock_isfile.return_value = True
+        mock_json_loader.return_value = local_conf
+        lc = mycroft.configuration.LocalConf('test')
+
         merge_conf = {'falling_objects': None, 'has_towel': True}
+
         lc.merge(merge_conf)
         self.assertEqual(lc['falling_objects'], None)
         self.assertEqual(lc['has_towel'], True)
 
-        # test store
+    @patch('json.dump')
+    def test_store(self, mock_json_dump, mock_json_loader, mock_isfile,
+                   mock_exists):
+        """Check that the config is stored correctly."""
+        local_conf = {'answer': 42, 'falling_objects': ['flower pot', 'whale']}
+        mock_exists.return_value = True
+        mock_isfile.return_value = True
+        mock_json_loader.return_value = local_conf
+        lc = mycroft.configuration.LocalConf('test')
+
         lc.store('test_conf.json')
         self.assertEqual(mock_json_dump.call_args[0][0], lc)
         # exists but is not file
@@ -64,17 +98,19 @@ def test_local(self, mock_json_loader, mock_isfile, mock_exists,
         lc = mycroft.configuration.LocalConf('test')
         self.assertEqual(lc, {})
 
-    @patch('mycroft.configuration.config.RemoteConf')
-    @patch('mycroft.configuration.config.LocalConf')
-    def test_update(self, mock_remote, mock_local):
-        mock_remote.return_value = {}
-        mock_local.return_value = {'a': 1}
-        c = mycroft.configuration.Configuration.get()
-        self.assertEqual(c, {'a': 1})
+    @patch('json.dump')
+    def test_store_invalid(self, mock_json_dump, mock_json_loader, mock_isfile,
+                           mock_exists):
+        """Storing shouldn't happen when config content is invalid."""
+        mock_exists.return_value = True
+        mock_isfile.return_value = True
 
-        mock_local.return_value = {'a': 2}
-        mycroft.configuration.Configuration.updated('message')
-        self.assertEqual(c, {'a': 2})
+        def raise_error(*arg, **kwarg):
+            raise Exception('Test exception')
 
-    def tearDown(self):
-        mycroft.configuration.Configuration.load_config_stack([{}], True)
+        mock_json_loader.side_effect = raise_error
+
+        lc = mycroft.configuration.LocalConf('invalid')
+        self.assertFalse(lc.is_valid)
+        self.assertFalse(lc.store())
+        mock_json_dump.assert_not_called()