Don't overwrite invalid json in config (#2881)

* Separate tests of LocalConf into new test class

* 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.

mycroft/configuration/config.py

@@ -97,6 +97,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)
@@ -117,23 +118,41 @@ class LocalConf(dict):
             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):
+    def store(self, path=None, force=False):
-        """Cache the received settings locally.
+        """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
         with self._lock:
             path = path or self.path
             config_dir = dirname(path)
             if not exists(config_dir):
                 os.makedirs(config_dir)
 
-            with open(path, 'w') as f:
+            if self.is_valid or force:
-                json.dump(self, f, indent=2)
+                with open(path, 'w') as f:
+                    json.dump(self, f, indent=2)
+                result = True
+            else:
+                LOG.warning((f'"{path}" was not a valid config file when '
+                             'loaded, will not save config. Please correct '
+                             'the json or remove it to allow updates.'))
+                result = False
+        return result
 
     def merge(self, conf):
         merge_dict(self, conf)
@@ -175,7 +194,7 @@ class RemoteConf(LocalConf):
             translate_remote(config, setting)
             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,38 +32,6 @@ class TestConfiguration(TestCase):
         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):
-        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
-        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
-        lc.store('test_conf.json')
-        self.assertEqual(mock_json_dump.call_args[0][0], lc)
-        # exists but is not file
-        mock_isfile.return_value = False
-        lc = mycroft.configuration.LocalConf('test')
-        self.assertEqual(lc, {})
-
-        # does not exist
-        mock_exists.return_value = False
-        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):
@@ -78,3 +46,71 @@ class TestConfiguration(TestCase):
 
     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)
+
+    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)
+
+    @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
+        mock_isfile.return_value = False
+        lc = mycroft.configuration.LocalConf('test')
+        self.assertEqual(lc, {})
+
+        # does not exist
+        mock_exists.return_value = False
+        lc = mycroft.configuration.LocalConf('test')
+        self.assertEqual(lc, {})
+
+    @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
+
+        def raise_error(*arg, **kwarg):
+            raise Exception('Test exception')
+
+        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()