From eb9070aef3e5ec8d730e082087ddd987e248ec66 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 15 Aug 2022 15:53:34 +0200 Subject: config: Discard prior mutables before applying If we only clear existing mutables *after* applying, we get into an inconsistent state if there was an error in one of the config values: The improper value lingers around in self._mutables, and then gets returned when get_mutable_obj() (or update_mutables()) gets called the next time. Reproducer: qutebrowser --debug --temp-basedir \ ':config-dict-add content.javascript.log_message.levels example.org bla' \ ':later 1000 config-dict-add content.javascript.log_message.levels example.org bla' Results in: ERROR: Invalid value 'bla' - expected a value of type list but got str. ERROR: example.org already exists in content.javascript.log_message - use --replace to overwrite! Fixes the second part of #7343. nb: As before, the mutable updating actually gets interrupted by a failing update, instead of it e.g. collecting all errors but carrying on. With this change, the remaining updates will thus also be discarded, but that does not seem to be a problem with how mutables are currently used. Ideally, we should get rid of the mutable handling entirely anyways, at least for qutebrowser internal code - see #4344. (cherry picked from commit 8eecf3af83fc9a4e465744a83e86856fe1c6df10) --- qutebrowser/config/config.py | 5 +++-- tests/unit/config/test_config.py | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 834709ae6..de210d50d 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -549,11 +549,12 @@ class Config(QObject): Here, we check all those saved copies for mutations, and if something mutated, we call set_obj again so we save the new value. """ - for name, (old_value, new_value) in self._mutables.items(): + mutables = self._mutables.items() + self._mutables = {} + for name, (old_value, new_value) in mutables: if old_value != new_value: log.config.debug("{} was mutated, updating".format(name)) self.set_obj(name, new_value, save_yaml=save_yaml) - self._mutables = {} def dump_userconfig(self) -> str: """Get the part of the config which was changed by the user. diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index b88bc2f8d..73941e9cb 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -613,6 +613,19 @@ class TestConfig: expected = {'X-Foo': 'fooval', 'X-Bar': 'barval'} assert conf.get_obj(option) == expected + def test_get_mutable_invalid_value(self, conf): + """Make sure invalid values aren't stored in mutables.""" + option = 'keyhint.blacklist' + obj = conf.get_mutable_obj(option) + assert obj == [] + obj.append(42) + + with pytest.raises(configexc.ValidationError): + conf.update_mutables() + + obj = conf.get_mutable_obj(option) + assert obj == [] + def test_get_obj_unknown_mutable(self, conf): """Make sure we don't have unknown mutable types.""" with pytest.raises(AssertionError): -- cgit v1.2.3-54-g00ecf