From c3efe1b90e0804dbf35cef5deecbd6d5f7534035 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sun, 29 Oct 2023 19:02:28 -0700 Subject: [PATCH] Only parse "--override" values as complex data types when they're for options of those types (#779). --- NEWS | 2 + borgmatic/config/override.py | 57 +++++++++++++++++------ borgmatic/config/validate.py | 2 +- tests/integration/config/test_override.py | 36 +++++++++----- tests/unit/config/test_override.py | 51 ++++++++++++++++---- 5 files changed, 112 insertions(+), 36 deletions(-) diff --git a/NEWS b/NEWS index 8bb80ab..e093afc 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ 1.8.5.dev0 * #779: Add a "--match-archives" flag to the "check" action for selecting the archives to check, overriding the existing "archive_name_format" and "match_archives" options in configuration. + * #779: Only parse "--override" values as complex data types when they're for options of those + types. 1.8.4 * #715: Add a monitoring hook for sending backup status to a variety of monitoring services via the diff --git a/borgmatic/config/override.py b/borgmatic/config/override.py index 05173d2..22365ec 100644 --- a/borgmatic/config/override.py +++ b/borgmatic/config/override.py @@ -22,13 +22,19 @@ def set_values(config, keys, value): set_values(config[first_key], keys[1:], value) -def convert_value_type(value): +def convert_value_type(value, option_type): ''' - Given a string value, determine its logical type (string, boolean, integer, etc.), and return it - converted to that type. + Given a string value and its schema type as a string, determine its logical type (string, + boolean, integer, etc.), and return it converted to that type. + + If the option type is a string, leave the value as a string so that special characters in it + don't get interpreted as YAML during conversion. Raise ruamel.yaml.error.YAMLError if there's a parse issue with the YAML. ''' + if option_type == 'string': + return value + return ruamel.yaml.YAML(typ='safe').load(io.StringIO(value)) @@ -46,11 +52,32 @@ def strip_section_names(parsed_override_key): return parsed_override_key -def parse_overrides(raw_overrides): +def type_for_option(schema, option_keys): ''' - Given a sequence of configuration file override strings in the form of "option.suboption=value", - parse and return a sequence of tuples (keys, values), where keys is a sequence of strings. For - instance, given the following raw overrides: + Given a configuration schema and a sequence of keys identifying an option, e.g. + ('extra_borg_options', 'init'), return the schema type of that option as a string. + + Return None if the option or its type cannot be found in the schema. + ''' + option_schema = schema + + for key in option_keys: + try: + option_schema = option_schema['properties'][key] + except KeyError: + return None + + try: + return option_schema['type'] + except KeyError: + return None + + +def parse_overrides(raw_overrides, schema): + ''' + Given a sequence of configuration file override strings in the form of "option.suboption=value" + and a configuration schema dict, parse and return a sequence of tuples (keys, values), where + keys is a sequence of strings. For instance, given the following raw overrides: ['my_option.suboption=value1', 'other_option=value2'] @@ -71,10 +98,13 @@ def parse_overrides(raw_overrides): for raw_override in raw_overrides: try: raw_keys, value = raw_override.split('=', 1) + keys = strip_section_names(tuple(raw_keys.split('.'))) + option_type = type_for_option(schema, keys) + parsed_overrides.append( ( - strip_section_names(tuple(raw_keys.split('.'))), - convert_value_type(value), + keys, + convert_value_type(value, option_type), ) ) except ValueError: @@ -87,12 +117,13 @@ def parse_overrides(raw_overrides): return tuple(parsed_overrides) -def apply_overrides(config, raw_overrides): +def apply_overrides(config, schema, raw_overrides): ''' - Given a configuration dict and a sequence of configuration file override strings in the form of - "option.suboption=value", parse each override and set it the configuration dict. + Given a configuration dict, a corresponding configuration schema dict, and a sequence of + configuration file override strings in the form of "option.suboption=value", parse each override + and set it into the configuration dict. ''' - overrides = parse_overrides(raw_overrides) + overrides = parse_overrides(raw_overrides, schema) for keys, value in overrides: set_values(config, keys, value) diff --git a/borgmatic/config/validate.py b/borgmatic/config/validate.py index c13329f..8024bc6 100644 --- a/borgmatic/config/validate.py +++ b/borgmatic/config/validate.py @@ -109,7 +109,7 @@ def parse_configuration(config_filename, schema_filename, overrides=None, resolv except (ruamel.yaml.error.YAMLError, RecursionError) as error: raise Validation_error(config_filename, (str(error),)) - override.apply_overrides(config, overrides) + override.apply_overrides(config, schema, overrides) logs = normalize.normalize(config_filename, config) if resolve_env: environment.resolve_env_variables(config) diff --git a/tests/integration/config/test_override.py b/tests/integration/config/test_override.py index cfcd339..bcf3d09 100644 --- a/tests/integration/config/test_override.py +++ b/tests/integration/config/test_override.py @@ -4,19 +4,24 @@ from borgmatic.config import override as module @pytest.mark.parametrize( - 'value,expected_result', + 'value,expected_result,option_type', ( - ('thing', 'thing'), - ('33', 33), - ('33b', '33b'), - ('true', True), - ('false', False), - ('[foo]', ['foo']), - ('[foo, bar]', ['foo', 'bar']), + ('thing', 'thing', 'string'), + ('33', 33, 'integer'), + ('33', '33', 'string'), + ('33b', '33b', 'integer'), + ('33b', '33b', 'string'), + ('true', True, 'boolean'), + ('false', False, 'boolean'), + ('true', 'true', 'string'), + ('[foo]', ['foo'], 'array'), + ('[foo]', '[foo]', 'string'), + ('[foo, bar]', ['foo', 'bar'], 'array'), + ('[foo, bar]', '[foo, bar]', 'string'), ), ) -def test_convert_value_type_coerces_values(value, expected_result): - assert module.convert_value_type(value) == expected_result +def test_convert_value_type_coerces_values(value, expected_result, option_type): + assert module.convert_value_type(value, option_type) == expected_result def test_apply_overrides_updates_config(): @@ -25,16 +30,23 @@ def test_apply_overrides_updates_config(): 'other_section.thing=value2', 'section.nested.key=value3', 'new.foo=bar', + 'new.mylist=[baz]', + 'new.nonlist=[quux]', ] config = { 'section': {'key': 'value', 'other': 'other_value'}, 'other_section': {'thing': 'thing_value'}, } + schema = { + 'properties': { + 'new': {'properties': {'mylist': {'type': 'array'}, 'nonlist': {'type': 'string'}}} + } + } - module.apply_overrides(config, raw_overrides) + module.apply_overrides(config, schema, raw_overrides) assert config == { 'section': {'key': 'value1', 'other': 'other_value', 'nested': {'key': 'value3'}}, 'other_section': {'thing': 'value2'}, - 'new': {'foo': 'bar'}, + 'new': {'foo': 'bar', 'mylist': ['baz'], 'nonlist': '[quux]'}, } diff --git a/tests/unit/config/test_override.py b/tests/unit/config/test_override.py index 38b07e4..f0c506a 100644 --- a/tests/unit/config/test_override.py +++ b/tests/unit/config/test_override.py @@ -44,6 +44,24 @@ def test_set_values_with_multiple_keys_updates_hierarchy(): assert config == {'option': {'key': 'value', 'other': 'other_value'}} +@pytest.mark.parametrize( + 'schema,option_keys,expected_type', + ( + ({'properties': {'foo': {'type': 'array'}}}, ('foo',), 'array'), + ( + {'properties': {'foo': {'properties': {'bar': {'type': 'array'}}}}}, + ('foo', 'bar'), + 'array', + ), + ({'properties': {'foo': {'type': 'array'}}}, ('other',), None), + ({'properties': {'foo': {'description': 'stuff'}}}, ('foo',), None), + ({}, ('foo',), None), + ), +) +def test_type_for_option_grabs_type_if_found_in_schema(schema, option_keys, expected_type): + assert module.type_for_option(schema, option_keys) == expected_type + + @pytest.mark.parametrize( 'key,expected_key', ( @@ -63,51 +81,64 @@ def test_strip_section_names_passes_through_key_without_section_name(key, expect def test_parse_overrides_splits_keys_and_values(): flexmock(module).should_receive('strip_section_names').replace_with(lambda value: value) - flexmock(module).should_receive('convert_value_type').replace_with(lambda value: value) + flexmock(module).should_receive('type_for_option').and_return('string') + flexmock(module).should_receive('convert_value_type').replace_with( + lambda value, option_type: value + ) raw_overrides = ['option.my_option=value1', 'other_option=value2'] expected_result = ( (('option', 'my_option'), 'value1'), (('other_option'), 'value2'), ) - module.parse_overrides(raw_overrides) == expected_result + module.parse_overrides(raw_overrides, schema={}) == expected_result def test_parse_overrides_allows_value_with_equal_sign(): flexmock(module).should_receive('strip_section_names').replace_with(lambda value: value) - flexmock(module).should_receive('convert_value_type').replace_with(lambda value: value) + flexmock(module).should_receive('type_for_option').and_return('string') + flexmock(module).should_receive('convert_value_type').replace_with( + lambda value, option_type: value + ) raw_overrides = ['option=this===value'] expected_result = ((('option',), 'this===value'),) - module.parse_overrides(raw_overrides) == expected_result + module.parse_overrides(raw_overrides, schema={}) == expected_result def test_parse_overrides_raises_on_missing_equal_sign(): flexmock(module).should_receive('strip_section_names').replace_with(lambda value: value) - flexmock(module).should_receive('convert_value_type').replace_with(lambda value: value) + flexmock(module).should_receive('type_for_option').and_return('string') + flexmock(module).should_receive('convert_value_type').replace_with( + lambda value, option_type: value + ) raw_overrides = ['option'] with pytest.raises(ValueError): - module.parse_overrides(raw_overrides) + module.parse_overrides(raw_overrides, schema={}) def test_parse_overrides_raises_on_invalid_override_value(): flexmock(module).should_receive('strip_section_names').replace_with(lambda value: value) + flexmock(module).should_receive('type_for_option').and_return('string') flexmock(module).should_receive('convert_value_type').and_raise(ruamel.yaml.parser.ParserError) raw_overrides = ['option=[in valid]'] with pytest.raises(ValueError): - module.parse_overrides(raw_overrides) + module.parse_overrides(raw_overrides, schema={}) def test_parse_overrides_allows_value_with_single_key(): flexmock(module).should_receive('strip_section_names').replace_with(lambda value: value) - flexmock(module).should_receive('convert_value_type').replace_with(lambda value: value) + flexmock(module).should_receive('type_for_option').and_return('string') + flexmock(module).should_receive('convert_value_type').replace_with( + lambda value, option_type: value + ) raw_overrides = ['option=value'] expected_result = ((('option',), 'value'),) - module.parse_overrides(raw_overrides) == expected_result + module.parse_overrides(raw_overrides, schema={}) == expected_result def test_parse_overrides_handles_empty_overrides(): - module.parse_overrides(raw_overrides=None) == () + module.parse_overrides(raw_overrides=None, schema={}) == ()