diff --git a/mycroft/configuration/config.py b/mycroft/configuration/config.py index 28a75334a5..c383604c57 100644 --- a/mycroft/configuration/config.py +++ b/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): - """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 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: - json.dump(self, f, indent=2) + if self.is_valid or force: + 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: {}" diff --git a/test/unittests/configuration/test_configuration.py b/test/unittests/configuration/test_configuration.py index 77cb376f18..dfe71271eb 100644 --- a/test/unittests/configuration/test_configuration.py +++ b/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()