diff --git a/src/octoprint/plugin/types.py b/src/octoprint/plugin/types.py index 71fb2d2b..bb193c9f 100644 --- a/src/octoprint/plugin/types.py +++ b/src/octoprint/plugin/types.py @@ -1209,7 +1209,7 @@ class SettingsPlugin(OctoPrintPlugin): :return: the current settings of the plugin, as a dictionary """ - data = self._settings.get([], asdict=True, merged=True) + data = self._settings.get_all_data() if self.config_version_key in data: del data[self.config_version_key] return data @@ -1240,6 +1240,8 @@ class SettingsPlugin(OctoPrintPlugin): # get the current data current = self._settings.get_all_data() + if current is None: + current = dict() # merge our new data on top of it new_current = octoprint.util.dict_merge(current, data) @@ -1255,7 +1257,11 @@ class SettingsPlugin(OctoPrintPlugin): to_persist = dict(diff) if version: to_persist[self.config_version_key] = version - self._settings.set([], to_persist) + + if to_persist: + self._settings.set([], to_persist) + else: + self._settings.clean_all_data() return diff @@ -1383,9 +1389,6 @@ class SettingsPlugin(OctoPrintPlugin): # diff => persist only that self._settings.set([], diff) - # finally save everything - self._settings.save() - def on_settings_initialized(self): """ Called after the settings have been initialized and - if necessary - also been migrated through a call to diff --git a/src/octoprint/server/__init__.py b/src/octoprint/server/__init__.py index 973a1472..9ad2e635 100644 --- a/src/octoprint/server/__init__.py +++ b/src/octoprint/server/__init__.py @@ -236,9 +236,10 @@ class Server(object): if stored_version is None or stored_version < settings_version: settings_migrator(settings_version, stored_version) implementation._settings.set_int([octoprint.plugin.SettingsPlugin.config_version_key], settings_version) - implementation._settings.save() implementation.on_settings_cleanup() + implementation._settings.save() + implementation.on_settings_initialized() pluginManager.implementation_inject_factories=[octoprint_plugin_inject_factory, settings_plugin_inject_factory] diff --git a/tests/plugin/test_types_settings.py b/tests/plugin/test_types_settings.py new file mode 100644 index 00000000..31a53df7 --- /dev/null +++ b/tests/plugin/test_types_settings.py @@ -0,0 +1,313 @@ +import unittest +import mock + +import octoprint.plugin + +class TestSettingsPlugin(unittest.TestCase): + + def setUp(self): + self.settings = mock.MagicMock() + + self.plugin = octoprint.plugin.SettingsPlugin() + self.plugin._settings = self.settings + + def test_on_settings_cleanup(self): + """Tests that after cleanup only minimal config is left in storage.""" + + ### setup + + # settings defaults + defaults = dict( + foo=dict( + a=1, + b=2, + l1=["some", "list"], + l2=["another", "list"] + ), + bar=True, + fnord=None + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + # stored config, containing one redundant entry (bar=True, same as default) + in_config = dict( + foo=dict( + l1=["some", "other", "list"], + l2=["another", "list"], + l3=["a", "third", "list"] + ), + bar=True, + fnord=dict( + c=3, + d=4 + ) + ) + self.settings.get_all_data.return_value = in_config + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # minimal config (current without redundant value) should have been set + expected = dict( + foo=dict( + l1=["some", "other", "list"], + l3=["a", "third", "list"] + ), + fnord=dict( + c=3, + d=4 + ) + ) + self.settings.set.assert_called_once_with([], expected) + + def test_on_settings_cleanup_configversion(self): + """Tests that set config version is always left stored.""" + + ### setup + + defaults = dict( + foo="fnord" + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + in_config = dict( + _config_version=1, + foo="fnord" + ) + self.settings.get_all_data.return_value = in_config + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # minimal config incl. config version should have been set + self.settings.set.assert_called_once_with([], dict(_config_version=1)) + + def test_on_settings_cleanup_noconfigversion(self): + """Tests that config versions of None are cleaned from stored data.""" + + ### setup + + defaults = dict( + foo="bar" + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + # stored config version is None + in_config = dict( + _config_version=None, + foo="fnord" + ) + self.settings.get_all_data.return_value = in_config + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # minimal config without config version should have been set + self.settings.set.assert_called_once_with([], dict(foo="fnord")) + + def test_on_settings_cleanup_emptydiff(self): + """Tests that settings are cleaned up if the diff data <-> defaults is empty.""" + + ### setup + + defaults = dict( + foo="bar" + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + # current stored config, same as defaults + in_config = dict( + foo="bar" + ) + self.settings.get_all_data.return_value = in_config + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # should have been cleared + self.settings.clean_all_data.assert_called_once_with() + + def test_on_settings_cleanup_nosuchpath(self): + """Tests that no processing is done if nothing is stored in settings.""" + + from octoprint.settings import NoSuchSettingsPath + + ### setup + + # simulate no settings stored in config.yaml + self.settings.get_all_data.side_effect = NoSuchSettingsPath() + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # only get_all_data should have been called + self.settings.get_all_data.assert_called_once_with(merged=False, incl_defaults=False, error_on_path=True) + self.assertTrue(len(self.settings.method_calls) == 1) + + def test_on_settings_cleanup_none(self): + """Tests the None entries in config get cleaned up.""" + + ### setup + + # simulate None entry in config.yaml + self.settings.get_all_data.return_value = None + + ### execute + + self.plugin.on_settings_cleanup() + + ### assert + + # should have been cleaned + self.settings.clean_all_data.assert_called_once_with() + + def test_on_settings_save(self): + """Tests that only the diff is saved.""" + + ### setup + + current = dict( + foo="bar" + ) + self.settings.get_all_data.return_value = current + + defaults = dict( + foo="foo", + bar=dict( + a=1, + b=2 + ) + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + ### execute + + data = dict( + foo="fnord", + bar=dict( + a=1, + b=2 + ) + ) + diff = self.plugin.on_settings_save(data) + + ### assert + + # the minimal diff should have been saved + expected = dict( + foo="fnord" + ) + self.settings.set.assert_called_once_with([], expected) + + self.assertEquals(diff, expected) + + def test_on_settings_save_nodiff(self): + """Tests that data is cleaned if there's not difference between data and defaults.""" + + ### setup + + self.settings.get_all_data.return_value = None + + defaults = dict( + foo="bar", + bar=dict( + a=1, + b=2, + l=["some", "list"] + ) + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + ### execute + + data = dict(foo="bar") + diff = self.plugin.on_settings_save(data) + + ### assert + + self.settings.clean_all_data.assert_called_once_with() + self.assertEquals(diff, dict()) + + def test_on_settings_save_configversion(self): + """Tests that saved data gets stripped config version and set correct one.""" + + ### setup + + self.settings.get_all_data.return_value = None + + defaults = dict( + foo="bar" + ) + self.plugin.get_settings_defaults = mock.MagicMock() + self.plugin.get_settings_defaults.return_value = defaults + + version = 1 + self.plugin.get_settings_version = mock.MagicMock() + self.plugin.get_settings_version.return_value = version + + ### execute + + data = dict(_config_version=None, foo="bar") + diff = self.plugin.on_settings_save(data) + + ### assert + + expected_diff = dict() + expected_set = dict(_config_version=version) + + # while there was no diff, we should still have saved the new config version + self.settings.set.assert_called_once_with([], expected_set) + + self.assertEquals(diff, expected_diff) + + def test_on_settings_load(self): + """Tests that on_settings_load returns what's stored in the config, without config version.""" + + ### setup + + # current data incl. config version + current = dict( + _config_version=3, + foo="bar", + fnord=dict( + a=1, + b=2, + l=["some", "list"] + ) + ) + + # expected is current without _config_version - we make the copy now + # since our current dict will be modified by the test + expected = dict(current) + del expected["_config_version"] + + self.settings.get_all_data.return_value = expected + + ### execute + + result = self.plugin.on_settings_load() + + ### assert + + self.assertEquals(result, expected)