Make sure only difference to default config is persisted for plugins
This change should make sure that only those settings actually make it into config.yaml per plugin that do differ from the default settings. Up until now the full dictionary as received e.g. from the REST API was saved, regardless whether it contained (only) default values or not. That led to a lot of "null" entries in the config.yaml file, which for one looked horrible and also led to problems when a plugin actually wanted to change its default values. With this commit, two things are done. One, upon saving a configuration the default implementation of the save routine in the settings plugin now first diffs the new data against the defaults and only saves what is different. Two, upon server startup a cleanup method is now called on settings plugins whose default implementation fetches the current data stored in config.yaml, diffs that against the defaults and only writes back the diff result. If that is empty the entry is completely deleted from config.yaml.
This commit is contained in:
Gina Häußge
parent
66fbd1fc6a
commit
d70fe32750
5 changed files with 228 additions and 40 deletions
|
|
@ -295,19 +295,16 @@ class PluginSettings(object):
|
|||
self.set_preprocessors = dict(plugins=dict())
|
||||
self.set_preprocessors["plugins"][plugin_key] = set_preprocessors
|
||||
|
||||
def prefix_path(path):
|
||||
return ['plugins', self.plugin_key] + path
|
||||
|
||||
def prefix_path_in_args(args, index=0):
|
||||
result = []
|
||||
if index == 0:
|
||||
result.append(prefix_path(args[0]))
|
||||
result.append(self._prefix_path(args[0]))
|
||||
result.extend(args[1:])
|
||||
else:
|
||||
args_before = args[:index - 1]
|
||||
args_after = args[index + 1:]
|
||||
result.extend(args_before)
|
||||
result.append(prefix_path(args[index]))
|
||||
result.append(self._prefix_path(args[index]))
|
||||
result.extend(args_after)
|
||||
return result
|
||||
|
||||
|
|
@ -326,6 +323,7 @@ class PluginSettings(object):
|
|||
return kwargs
|
||||
|
||||
self.access_methods = dict(
|
||||
has =("has", prefix_path_in_args, add_getter_kwargs),
|
||||
get =("get", prefix_path_in_args, add_getter_kwargs),
|
||||
get_int =("getInt", prefix_path_in_args, add_getter_kwargs),
|
||||
get_float =("getFloat", prefix_path_in_args, add_getter_kwargs),
|
||||
|
|
@ -333,7 +331,8 @@ class PluginSettings(object):
|
|||
set =("set", prefix_path_in_args, add_setter_kwargs),
|
||||
set_int =("setInt", prefix_path_in_args, add_setter_kwargs),
|
||||
set_float =("setFloat", prefix_path_in_args, add_setter_kwargs),
|
||||
set_boolean=("setBoolean", prefix_path_in_args, add_setter_kwargs)
|
||||
set_boolean=("setBoolean", prefix_path_in_args, add_setter_kwargs),
|
||||
remove =("remove", prefix_path_in_args)
|
||||
)
|
||||
self.deprecated_access_methods = dict(
|
||||
getInt ="get_int",
|
||||
|
|
@ -344,6 +343,17 @@ class PluginSettings(object):
|
|||
setBoolean="set_boolean"
|
||||
)
|
||||
|
||||
def _prefix_path(self, path=None):
|
||||
if path is None:
|
||||
path = list()
|
||||
return ['plugins', self.plugin_key] + path
|
||||
|
||||
def global_has(self, path, **kwargs):
|
||||
return self.settings.has(path, **kwargs)
|
||||
|
||||
def global_remove(self, path, **kwargs):
|
||||
return self.settings.remove(path, **kwargs)
|
||||
|
||||
def global_get(self, path, **kwargs):
|
||||
"""
|
||||
Getter for retrieving settings not managed by the plugin itself from the core settings structure. Use this
|
||||
|
|
@ -437,6 +447,12 @@ class PluginSettings(object):
|
|||
os.makedirs(path)
|
||||
return path
|
||||
|
||||
def get_all_data(self, **kwargs):
|
||||
return self.settings.get(self._prefix_path(), **kwargs)
|
||||
|
||||
def clean_all_data(self):
|
||||
self.settings.remove(self._prefix_path())
|
||||
|
||||
def __getattr__(self, item):
|
||||
all_access_methods = self.access_methods.keys() + self.deprecated_access_methods.keys()
|
||||
if item in all_access_methods:
|
||||
|
|
|
|||
|
|
@ -1189,6 +1189,9 @@ class SettingsPlugin(OctoPrintPlugin):
|
|||
the plugin core system upon initialization of the implementation.
|
||||
"""
|
||||
|
||||
config_version_key = "_config_version"
|
||||
"""Key of the field in the settings that holds the configuration format version."""
|
||||
|
||||
def on_settings_load(self):
|
||||
"""
|
||||
Loads the settings for the plugin, called by the Settings API view in order to retrieve all settings from
|
||||
|
|
@ -1207,8 +1210,8 @@ class SettingsPlugin(OctoPrintPlugin):
|
|||
:return: the current settings of the plugin, as a dictionary
|
||||
"""
|
||||
data = self._settings.get([], asdict=True, merged=True)
|
||||
if "_config_version" in data:
|
||||
del data["_config_version"]
|
||||
if self.config_version_key in data:
|
||||
del data[self.config_version_key]
|
||||
return data
|
||||
|
||||
def on_settings_save(self, data):
|
||||
|
|
@ -1220,7 +1223,8 @@ class SettingsPlugin(OctoPrintPlugin):
|
|||
.. note::
|
||||
|
||||
The default implementation will persist your plugin's settings as is, so just in the structure and in the
|
||||
types that were received by the Settings API view.
|
||||
types that were received by the Settings API view. Values identical to the default settings values
|
||||
will *not* be persisted.
|
||||
|
||||
If you need more granular control here, e.g. over the used data types, you'll need to override this method
|
||||
and iterate yourself over all your settings, retrieving them (if set) from the supplied received ``data``
|
||||
|
|
@ -1228,15 +1232,27 @@ class SettingsPlugin(OctoPrintPlugin):
|
|||
|
||||
Arguments:
|
||||
data (dict): The settings dictionary to be saved for the plugin
|
||||
|
||||
Returns:
|
||||
dict: The settings that differed from the defaults and were actually saved.
|
||||
"""
|
||||
import octoprint.util
|
||||
|
||||
if "_config_version" in data:
|
||||
del data["_config_version"]
|
||||
if self.config_version_key in data:
|
||||
del data[self.config_version_key]
|
||||
|
||||
current = self._settings.get([], asdict=True, merged=True)
|
||||
merged = octoprint.util.dict_merge(current, data)
|
||||
self._settings.set([], merged)
|
||||
# determine diff dict that contains minimal set of changes against the
|
||||
# default settings - we only want to persist that, not everything
|
||||
diff = octoprint.util.dict_diff(self.get_settings_defaults(), data)
|
||||
|
||||
version = self.get_settings_version()
|
||||
|
||||
to_persist = dict(diff)
|
||||
if version:
|
||||
to_persist[self.config_version_key] = version
|
||||
self._settings.set([], to_persist)
|
||||
|
||||
return diff
|
||||
|
||||
def get_settings_defaults(self):
|
||||
"""
|
||||
|
|
@ -1319,6 +1335,52 @@ class SettingsPlugin(OctoPrintPlugin):
|
|||
"""
|
||||
pass
|
||||
|
||||
def on_settings_cleanup(self):
|
||||
"""
|
||||
Called after migration and initialization but before call to :func:`on_settings_initialized`.
|
||||
|
||||
Plugins may overwrite this method to perform additional clean up tasks.
|
||||
|
||||
The default implementation just minimizes the data persisted on disk to only contain
|
||||
the differences to the defaults (in case the current data was persisted with an older
|
||||
version of OctoPrint that still duplicated default data).
|
||||
"""
|
||||
import octoprint.util
|
||||
from octoprint.settings import NoSuchSettingsPath
|
||||
|
||||
try:
|
||||
# let's fetch the current persisted config (so only the data on disk,
|
||||
# without the defaults)
|
||||
config = self._settings.get([], asdict=True, incl_defaults=False, error_on_path=True)
|
||||
except NoSuchSettingsPath:
|
||||
# no config persisted, nothing to do => get out of here
|
||||
return
|
||||
|
||||
if config is None:
|
||||
# config is set to None, that doesn't make sense, kill it and leave
|
||||
self._settings.clean_all_data()
|
||||
return
|
||||
|
||||
if self.config_version_key in config and config[self.config_version_key] is None:
|
||||
# delete None entries for config version - it's the default, no need
|
||||
del config[self.config_version_key]
|
||||
|
||||
# calculate a minimal diff between the settings and the current config -
|
||||
# anything already in the settings will be removed from the persisted
|
||||
# config, no need to duplicate it
|
||||
defaults = self.get_settings_defaults()
|
||||
diff = octoprint.util.dict_diff(defaults, config)
|
||||
|
||||
if not diff:
|
||||
# no diff to defaults, no need to have anything persisted
|
||||
self._settings.clean_all_data()
|
||||
else:
|
||||
# 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
|
||||
|
|
|
|||
|
|
@ -224,7 +224,7 @@ class Server(object):
|
|||
set_preprocessors=set_preprocessors)
|
||||
return dict(settings=plugin_settings)
|
||||
|
||||
def settings_plugin_config_migration(name, implementation):
|
||||
def settings_plugin_config_migration_and_cleanup(name, implementation):
|
||||
if not isinstance(implementation, octoprint.plugin.SettingsPlugin):
|
||||
return
|
||||
|
||||
|
|
@ -232,12 +232,13 @@ class Server(object):
|
|||
settings_migrator = implementation.on_settings_migrate
|
||||
|
||||
if settings_version is not None and settings_migrator is not None:
|
||||
stored_version = implementation._settings.get_int(["_config_version"])
|
||||
stored_version = implementation._settings.get_int([octoprint.plugin.SettingsPlugin.config_version_key])
|
||||
if stored_version is None or stored_version < settings_version:
|
||||
settings_migrator(settings_version, stored_version)
|
||||
implementation._settings.set_int(["_config_version"], settings_version)
|
||||
implementation._settings.set_int([octoprint.plugin.SettingsPlugin.config_version_key], settings_version)
|
||||
implementation._settings.save()
|
||||
|
||||
implementation.on_settings_cleanup()
|
||||
implementation.on_settings_initialized()
|
||||
|
||||
pluginManager.implementation_inject_factories=[octoprint_plugin_inject_factory, settings_plugin_inject_factory]
|
||||
|
|
@ -246,11 +247,11 @@ class Server(object):
|
|||
settingsPlugins = pluginManager.get_implementations(octoprint.plugin.SettingsPlugin)
|
||||
for implementation in settingsPlugins:
|
||||
try:
|
||||
settings_plugin_config_migration(implementation._identifier, implementation)
|
||||
settings_plugin_config_migration_and_cleanup(implementation._identifier, implementation)
|
||||
except:
|
||||
self._logger.exception("Error while trying to migrate settings for plugin {}, ignoring it".format(implementation._identifier))
|
||||
|
||||
pluginManager.implementation_post_inits=[settings_plugin_config_migration]
|
||||
pluginManager.implementation_post_inits=[settings_plugin_config_migration_and_cleanup]
|
||||
|
||||
pluginManager.log_all_plugins()
|
||||
|
||||
|
|
|
|||
|
|
@ -312,6 +312,11 @@ default_settings = {
|
|||
valid_boolean_trues = [True, "true", "yes", "y", "1"]
|
||||
""" Values that are considered to be equivalent to the boolean ``True`` value, used for type conversion in various places."""
|
||||
|
||||
|
||||
class NoSuchSettingsPath(BaseException):
|
||||
pass
|
||||
|
||||
|
||||
class Settings(object):
|
||||
"""
|
||||
The :class:`Settings` class allows managing all of OctoPrint's settings. It takes care of initializing the settings
|
||||
|
|
@ -832,13 +837,13 @@ class Settings(object):
|
|||
stat = os.stat(self._configfile)
|
||||
return stat.st_mtime
|
||||
|
||||
#~~ getter
|
||||
##~~ Internal getter
|
||||
|
||||
def get(self, path, asdict=False, config=None, defaults=None, preprocessors=None, merged=False, incl_defaults=True):
|
||||
def _get_value(self, path, asdict=False, config=None, defaults=None, preprocessors=None, merged=False, incl_defaults=True):
|
||||
import octoprint.util as util
|
||||
|
||||
if len(path) == 0:
|
||||
return None
|
||||
raise NoSuchSettingsPath()
|
||||
|
||||
if config is None:
|
||||
config = self._config
|
||||
|
|
@ -856,7 +861,7 @@ class Settings(object):
|
|||
config = {}
|
||||
defaults = defaults[key]
|
||||
else:
|
||||
return None
|
||||
raise NoSuchSettingsPath()
|
||||
|
||||
if preprocessors and isinstance(preprocessors, dict) and key in preprocessors:
|
||||
preprocessors = preprocessors[key]
|
||||
|
|
@ -880,7 +885,7 @@ class Settings(object):
|
|||
elif incl_defaults and key in defaults:
|
||||
value = defaults[key]
|
||||
else:
|
||||
value = None
|
||||
raise NoSuchSettingsPath()
|
||||
|
||||
if preprocessors and isinstance(preprocessors, dict) and key in preprocessors and callable(preprocessors[key]):
|
||||
value = preprocessors[key](value)
|
||||
|
|
@ -898,8 +903,34 @@ class Settings(object):
|
|||
else:
|
||||
return results
|
||||
|
||||
def getInt(self, path, config=None, defaults=None, preprocessors=None, incl_defaults=True):
|
||||
value = self.get(path, config=config, defaults=defaults, preprocessors=preprocessors, incl_defaults=incl_defaults)
|
||||
#~~ has
|
||||
|
||||
def has(self, path, **kwargs):
|
||||
try:
|
||||
self._get_value(path, **kwargs)
|
||||
except NoSuchSettingsPath:
|
||||
return False
|
||||
else:
|
||||
return True
|
||||
|
||||
#~~ getter
|
||||
|
||||
def get(self, path, **kwargs):
|
||||
error_on_path = kwargs.get("error_on_path", False)
|
||||
new_kwargs = dict(kwargs)
|
||||
if "error_on_path" in new_kwargs:
|
||||
del new_kwargs["error_on_path"]
|
||||
|
||||
try:
|
||||
return self._get_value(path, **new_kwargs)
|
||||
except NoSuchSettingsPath:
|
||||
if error_on_path:
|
||||
raise
|
||||
else:
|
||||
return None
|
||||
|
||||
def getInt(self, path, **kwargs):
|
||||
value = self.get(path, **kwargs)
|
||||
if value is None:
|
||||
return None
|
||||
|
||||
|
|
@ -909,8 +940,8 @@ class Settings(object):
|
|||
self._logger.warn("Could not convert %r to a valid integer when getting option %r" % (value, path))
|
||||
return None
|
||||
|
||||
def getFloat(self, path, config=None, defaults=None, preprocessors=None, incl_defaults=True):
|
||||
value = self.get(path, config=config, defaults=defaults, preprocessors=preprocessors, incl_defaults=incl_defaults)
|
||||
def getFloat(self, path, **kwargs):
|
||||
value = self.get(path, **kwargs)
|
||||
if value is None:
|
||||
return None
|
||||
|
||||
|
|
@ -920,8 +951,8 @@ class Settings(object):
|
|||
self._logger.warn("Could not convert %r to a valid integer when getting option %r" % (value, path))
|
||||
return None
|
||||
|
||||
def getBoolean(self, path, config=None, defaults=None, preprocessors=None, incl_defaults=True):
|
||||
value = self.get(path, config=config, defaults=defaults, preprocessors=preprocessors, incl_defaults=incl_defaults)
|
||||
def getBoolean(self, path, **kwargs):
|
||||
value = self.get(path, **kwargs)
|
||||
if value is None:
|
||||
return None
|
||||
if isinstance(value, bool):
|
||||
|
|
@ -974,6 +1005,23 @@ class Settings(object):
|
|||
|
||||
return script
|
||||
|
||||
#~~ remove
|
||||
|
||||
def remove(self, path, config=None):
|
||||
if config is None:
|
||||
config = self._config
|
||||
|
||||
while len(path) > 1:
|
||||
key = path.pop(0)
|
||||
if not isinstance(config, dict) or key not in config:
|
||||
return
|
||||
config = config[key]
|
||||
|
||||
key = path.pop(0)
|
||||
if isinstance(config, dict) and key in config:
|
||||
del config[key]
|
||||
self._dirty = True
|
||||
|
||||
#~~ setter
|
||||
|
||||
def set(self, path, value, force=False, defaults=None, config=None, preprocessors=None):
|
||||
|
|
@ -1020,9 +1068,9 @@ class Settings(object):
|
|||
config[key] = value
|
||||
self._dirty = True
|
||||
|
||||
def setInt(self, path, value, force=False, defaults=None, config=None, preprocessors=None):
|
||||
def setInt(self, path, value, **kwargs):
|
||||
if value is None:
|
||||
self.set(path, None, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, None, **kwargs)
|
||||
return
|
||||
|
||||
try:
|
||||
|
|
@ -1031,11 +1079,11 @@ class Settings(object):
|
|||
self._logger.warn("Could not convert %r to a valid integer when setting option %r" % (value, path))
|
||||
return
|
||||
|
||||
self.set(path, intValue, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, intValue, **kwargs)
|
||||
|
||||
def setFloat(self, path, value, force=False, defaults=None, config=None, preprocessors=None):
|
||||
def setFloat(self, path, value, **kwargs):
|
||||
if value is None:
|
||||
self.set(path, None, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, None, **kwargs)
|
||||
return
|
||||
|
||||
try:
|
||||
|
|
@ -1044,15 +1092,15 @@ class Settings(object):
|
|||
self._logger.warn("Could not convert %r to a valid integer when setting option %r" % (value, path))
|
||||
return
|
||||
|
||||
self.set(path, floatValue, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, floatValue, **kwargs)
|
||||
|
||||
def setBoolean(self, path, value, force=False, defaults=None, config=None, preprocessors=None):
|
||||
def setBoolean(self, path, value, **kwargs):
|
||||
if value is None or isinstance(value, bool):
|
||||
self.set(path, value, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, value, **kwargs)
|
||||
elif value.lower() in valid_boolean_trues:
|
||||
self.set(path, True, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, True, **kwargs)
|
||||
else:
|
||||
self.set(path, False, config=config, force=force, defaults=defaults, preprocessors=preprocessors)
|
||||
self.set(path, False, **kwargs)
|
||||
|
||||
def setBaseFolder(self, type, path, force=False):
|
||||
if type not in default_settings["folder"].keys():
|
||||
|
|
|
|||
|
|
@ -432,6 +432,67 @@ def dict_clean(a, b):
|
|||
return result
|
||||
|
||||
|
||||
def dict_diff(a, b):
|
||||
"""
|
||||
Recursively calculates the minimal dict that would be needed to be deep merged with
|
||||
a in order to produce the same result as deep merging a and b.
|
||||
|
||||
Example::
|
||||
|
||||
>>> a = dict(foo=dict(a=1, b=2), bar=dict(c=3, d=4))
|
||||
>>> b = dict(bar=dict(c=3, d=5), fnord=None)
|
||||
>>> c = dict_diff(a, b)
|
||||
>>> c == dict(bar=dict(d=5), fnord=None)
|
||||
True
|
||||
>>> dict_merge(a, c) == dict_merge(a, b)
|
||||
True
|
||||
|
||||
Arguments:
|
||||
a (dict): Source dictionary
|
||||
b (dict): Dictionary to compare to source dictionary and derive diff for
|
||||
|
||||
Returns:
|
||||
dict: The minimal dictionary to deep merge on a to get the same result
|
||||
as deep merging b on a.
|
||||
"""
|
||||
|
||||
if not isinstance(a, dict) or not isinstance(b, dict):
|
||||
raise ValueError("a and b must be dictionaries")
|
||||
|
||||
if a == b:
|
||||
# shortcut: if both are equal, we return an empty dict as result
|
||||
return dict()
|
||||
|
||||
from copy import deepcopy
|
||||
|
||||
all_keys = set(a.keys() + b.keys())
|
||||
result = dict()
|
||||
for k in all_keys:
|
||||
if k not in b:
|
||||
# key not contained in b => not contained in result
|
||||
continue
|
||||
|
||||
if k in a:
|
||||
# key is present in both dicts, we have to take a look at the value
|
||||
value_a = a[k]
|
||||
value_b = b[k]
|
||||
|
||||
if value_a != value_b:
|
||||
# we only need to look further if the values are not equal
|
||||
|
||||
if isinstance(value_a, dict) and isinstance(value_b, dict):
|
||||
# both are dicts => deeper down it goes into the rabbit hole
|
||||
result[k] = dict_diff(value_a, value_b)
|
||||
else:
|
||||
# new b wins over old a
|
||||
result[k] = deepcopy(value_b)
|
||||
|
||||
else:
|
||||
# key is new, add it
|
||||
result[k] = deepcopy(b[k])
|
||||
return result
|
||||
|
||||
|
||||
def dict_contains_keys(a, b):
|
||||
"""
|
||||
Recursively deep-checks if ``a`` contains all keys found in ``b``.
|
||||
|
|
|
|||
Loading…
Reference in a new issue