From 263e57e5e6fe1577379c4b5b915aa6179f9c6d73 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Sat, 18 Jan 2020 21:28:02 -0600 Subject: [PATCH 1/4] config: verify before saving a modification Close #3192 --- dvc/config.py | 20 +++++++++++++------- tests/func/test_config.py | 8 ++++---- tests/func/test_remote.py | 12 ++++++++++-- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 0fd312aa14..13965264bd 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -252,7 +252,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes def __init__(self, dvc_dir=None, validate=True): self.dvc_dir = dvc_dir - self.validate = validate + self.should_validate = validate if not dvc_dir: try: @@ -382,14 +382,10 @@ def load(self): c = self._lower(c) self.config.merge(c) - if not self.validate: + if not self.should_validate: return - d = self.config.dict() - try: - d = self.COMPILED_SCHEMA(d) - except Invalid as exc: - raise ConfigError(str(exc)) from exc + d = self.validate() self.config = configobj.ConfigObj(d, write_empty_values=True) def save(self, config=None): @@ -427,6 +423,14 @@ def _save(config): raise config.write() + def validate(self, config=None): + d = (config or self.config).dict() + + try: + return self.COMPILED_SCHEMA(d) + except Invalid as exc: + raise ConfigError(str(exc)) from exc + def unset(self, section, opt=None, level=None, force=False): """Unsets specified option and/or section in the config. @@ -491,6 +495,8 @@ def set(self, section, opt, value, level=None, force=True): ) config[section][opt] = value + + self.validate(config) self.save(config) def get(self, section, opt=None, level=None): diff --git a/tests/func/test_config.py b/tests/func/test_config.py index f6cae78d3c..e4581dd270 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -33,11 +33,11 @@ def test_root(self): self.assertEqual(ret, 0) def _do_test(self, local=False): - section = "setsection" - field = "setfield" + section = "core" + field = "analytics" section_field = "{}.{}".format(section, field) - value = "setvalue" - newvalue = "setnewvalue" + value = "True" + newvalue = "False" base = ["config"] if local: diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index c6d1cffe9e..2bbe5088f3 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -32,7 +32,7 @@ def test(self): self.assertEqual(main(["remote", "list"]), 0) self.assertEqual( - main(["remote", "modify", remotes[0], "option", "value"]), 0 + main(["remote", "modify", remotes[0], "checksum_jobs", "1"]), 0 ) self.assertEqual(main(["remote", "remove", remotes[0]]), 0) @@ -113,7 +113,7 @@ def test(self): class TestRemoteRemove(TestDvc): def test(self): - ret = main(["config", "core.jobs", "1"]) + ret = main(["config", "core.checksum_jobs", "1"]) self.assertEqual(ret, 0) remote = "mys3" @@ -257,3 +257,11 @@ def test_modify_missing_remote(dvc): with pytest.raises(ConfigError, match=r"unable to find remote section"): remote_config.modify("myremote", "gdrive_client_id", "xxx") + + +def test_modify_invalid_key(dvc): + remote_config = RemoteConfig(dvc.config) + remote_config.add("example", "https://example.com") + + with pytest.raises(ConfigError, match=r"extra keys not allowed"): + remote_config.modify("example", "random_key", "value") From efcddb6f5bf15ed785ca3a2b7f37451925678f18 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Sun, 19 Jan 2020 01:56:24 -0600 Subject: [PATCH 2/4] remote: fix merging two different config levels --- dvc/config.py | 7 +++++-- tests/func/test_remote.py | 7 +++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index 13965264bd..cb0fd8e702 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -424,10 +424,13 @@ def _save(config): config.write() def validate(self, config=None): - d = (config or self.config).dict() + ret = copy.deepcopy(self.config) + + if config: + ret.merge(config) try: - return self.COMPILED_SCHEMA(d) + return self.COMPILED_SCHEMA(ret.dict()) except Invalid as exc: raise ConfigError(str(exc)) from exc diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 2bbe5088f3..78730d6dad 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -265,3 +265,10 @@ def test_modify_invalid_key(dvc): with pytest.raises(ConfigError, match=r"extra keys not allowed"): remote_config.modify("example", "random_key", "value") + + +def test_merging_two_levels(tmp_dir, dvc): + assert 0 == main(["remote", "add", "test", "https://example.com"]) + assert 0 == main(["remote", "modify", "--local", "test", "password", "1"]) + assert "example.com" in (tmp_dir / ".dvc" / "config").read_text() + assert "password" in (tmp_dir / ".dvc" / "config.local").read_text() From 566cc6c8622ffe63f11c78c890fadb4a7cac679b Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Sun, 19 Jan 2020 02:27:51 -0600 Subject: [PATCH 3/4] tests: move tests from remote to config --- tests/func/test_config.py | 12 ++++++++++++ tests/func/test_remote.py | 15 --------------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/tests/func/test_config.py b/tests/func/test_config.py index e4581dd270..7fa4a45718 100644 --- a/tests/func/test_config.py +++ b/tests/func/test_config.py @@ -1,6 +1,8 @@ +import pytest import configobj from dvc.main import main +from dvc.config import Config, ConfigError from tests.basic_env import TestDvc @@ -83,3 +85,13 @@ def test_non_existing(self): ret = main(["config", "core.non_existing_field", "-u"]) self.assertEqual(ret, 251) + + +def test_set_invalid_key(dvc): + with pytest.raises(ConfigError, match=r"extra keys not allowed"): + dvc.config.set("core", "invalid.key", "value") + + +def test_merging_two_levels(dvc): + dvc.config.set('remote "test"', "url", "https://example.com") + dvc.config.set('remote "test"', "password", "1", level=Config.LEVEL_LOCAL) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 78730d6dad..0e3eb347bd 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -257,18 +257,3 @@ def test_modify_missing_remote(dvc): with pytest.raises(ConfigError, match=r"unable to find remote section"): remote_config.modify("myremote", "gdrive_client_id", "xxx") - - -def test_modify_invalid_key(dvc): - remote_config = RemoteConfig(dvc.config) - remote_config.add("example", "https://example.com") - - with pytest.raises(ConfigError, match=r"extra keys not allowed"): - remote_config.modify("example", "random_key", "value") - - -def test_merging_two_levels(tmp_dir, dvc): - assert 0 == main(["remote", "add", "test", "https://example.com"]) - assert 0 == main(["remote", "modify", "--local", "test", "password", "1"]) - assert "example.com" in (tmp_dir / ".dvc" / "config").read_text() - assert "password" in (tmp_dir / ".dvc" / "config.local").read_text() From 230e5c86d832cb75c40ca7c2e08e1af14a870847 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 27 Jan 2020 16:37:36 +0200 Subject: [PATCH 4/4] config: use validate() more explicitly --- dvc/config.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/dvc/config.py b/dvc/config.py index cb0fd8e702..40f3052eb5 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -385,7 +385,7 @@ def load(self): if not self.should_validate: return - d = self.validate() + d = self.validate(self.config) self.config = configobj.ConfigObj(d, write_empty_values=True) def save(self, config=None): @@ -423,14 +423,9 @@ def _save(config): raise config.write() - def validate(self, config=None): - ret = copy.deepcopy(self.config) - - if config: - ret.merge(config) - + def validate(self, config): try: - return self.COMPILED_SCHEMA(ret.dict()) + return self.COMPILED_SCHEMA(config.dict()) except Invalid as exc: raise ConfigError(str(exc)) from exc @@ -499,7 +494,10 @@ def set(self, section, opt, value, level=None, force=True): config[section][opt] = value - self.validate(config) + result = copy.deepcopy(self.config) + result.merge(config) + self.validate(result) + self.save(config) def get(self, section, opt=None, level=None):