diff --git a/RELEASE.md b/RELEASE.md new file mode 100644 index 00000000..76e47100 --- /dev/null +++ b/RELEASE.md @@ -0,0 +1,3 @@ +Release type: patch + +Fix incorrect parsing of parameters specified via `-e` / `--extra-settings` option flags (#2938). diff --git a/docs/settings.rst b/docs/settings.rst index 47852527..675e6b8f 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -9,11 +9,12 @@ line:: If you used the ``pelican-quickstart`` command, your primary settings file will be named ``pelicanconf.py`` by default. -You can also specify extra settings via ``-e`` / ``--extra-settings`` option -flags, which will override default settings as well as any defined within -settings files:: +You can also specify settings via ``-e`` / ``--extra-settings`` option +flags. It will override default settings as well as any defined within the +setting file. Note that values must follow JSON notation:: + + pelican content -e SITENAME='"A site"' READERS='{"html": null}' CACHE_CONTENT=true - pelican content -e DELETE_OUTPUT_DIRECTORY=true .. note:: diff --git a/pelican/__init__.py b/pelican/__init__.py index 99aa2776..9858dbd3 100644 --- a/pelican/__init__.py +++ b/pelican/__init__.py @@ -1,4 +1,5 @@ import argparse +import json import logging import multiprocessing import os @@ -24,7 +25,7 @@ from pelican.plugins import signals from pelican.plugins._utils import get_plugin_name, load_plugins from pelican.readers import Readers from pelican.server import ComplexHTTPRequestHandler, RootedHTTPServer -from pelican.settings import coerce_overrides, read_settings +from pelican.settings import read_settings from pelican.utils import (FileSystemWatcher, clean_output_dir, maybe_pluralize) from pelican.writers import Writer @@ -259,16 +260,29 @@ class PrintSettings(argparse.Action): parser.exit() -class ParseDict(argparse.Action): +class ParseOverrides(argparse.Action): def __call__(self, parser, namespace, values, option_string=None): - d = {} - if values: - for item in values: - split_items = item.split("=", 1) - key = split_items[0].strip() - value = split_items[1].strip() - d[key] = value - setattr(namespace, self.dest, d) + overrides = {} + for item in values: + try: + k, v = item.split("=", 1) + except ValueError: + raise ValueError( + 'Extra settings must be specified as KEY=VALUE pairs ' + f'but you specified {item}' + ) + try: + overrides[k] = json.loads(v) + except json.decoder.JSONDecodeError: + raise ValueError( + f'Invalid JSON value: {v}. ' + 'Values specified via -e / --extra-settings flags ' + 'must be in JSON notation. ' + 'Use -e KEY=\'"string"\' to specify a string value; ' + '-e KEY=null to specify None; ' + '-e KEY=false (or true) to specify False (or True).' + ) + setattr(namespace, self.dest, overrides) def parse_arguments(argv=None): @@ -366,13 +380,13 @@ def parse_arguments(argv=None): parser.add_argument('-e', '--extra-settings', dest='overrides', help='Specify one or more SETTING=VALUE pairs to ' - 'override settings. If VALUE contains spaces, ' - 'add quotes: SETTING="VALUE". Values other than ' - 'integers and strings can be specified via JSON ' - 'notation. (e.g., SETTING=none)', + 'override settings. VALUE must be in JSON notation: ' + 'specify string values as SETTING=\'"some string"\'; ' + 'booleans as SETTING=true or SETTING=false; ' + 'None as SETTING=null.', nargs='*', - action=ParseDict - ) + action=ParseOverrides, + default={}) args = parser.parse_args(argv) @@ -385,6 +399,8 @@ def parse_arguments(argv=None): def get_config(args): + """Builds a config dictionary based on supplied `args`. + """ config = {} if args.path: config['PATH'] = os.path.abspath(os.path.expanduser(args.path)) @@ -409,7 +425,7 @@ def get_config(args): if args.bind is not None: config['BIND'] = args.bind config['DEBUG'] = args.verbosity == logging.DEBUG - config.update(coerce_overrides(args.overrides)) + config.update(args.overrides) return config diff --git a/pelican/settings.py b/pelican/settings.py index ea3ee8eb..5b495e86 100644 --- a/pelican/settings.py +++ b/pelican/settings.py @@ -1,7 +1,6 @@ import copy import importlib.util import inspect -import json import locale import logging import os @@ -659,25 +658,3 @@ def configure_settings(settings): continue # setting not specified, nothing to do return settings - - -def coerce_overrides(overrides): - if overrides is None: - return {} - coerced = {} - types_to_cast = {int, str, bool} - for k, v in overrides.items(): - if k not in DEFAULT_CONFIG: - logger.warning('Override for unknown setting %s, ignoring', k) - continue - setting_type = type(DEFAULT_CONFIG[k]) - if setting_type not in types_to_cast: - coerced[k] = json.loads(v) - else: - try: - coerced[k] = setting_type(v) - except ValueError: - logger.debug('ValueError for %s override with %s, try to ' - 'load as json', k, v) - coerced[k] = json.loads(v) - return coerced diff --git a/pelican/tests/test_cli.py b/pelican/tests/test_cli.py new file mode 100644 index 00000000..13b307e7 --- /dev/null +++ b/pelican/tests/test_cli.py @@ -0,0 +1,72 @@ +import unittest + +from pelican import get_config, parse_arguments + + +class TestParseOverrides(unittest.TestCase): + def test_flags(self): + for flag in ['-e', '--extra-settings']: + args = parse_arguments([flag, 'k=1']) + self.assertDictEqual(args.overrides, {'k': 1}) + + def test_parse_multiple_items(self): + args = parse_arguments('-e k1=1 k2=2'.split()) + self.assertDictEqual(args.overrides, {'k1': 1, 'k2': 2}) + + def test_parse_valid_json(self): + json_values_python_values_map = { + '""': '', + 'null': None, + '"string"': 'string', + '["foo", 12, "4", {}]': ['foo', 12, '4', {}] + } + for k, v in json_values_python_values_map.items(): + args = parse_arguments(['-e', 'k=' + k]) + self.assertDictEqual(args.overrides, {'k': v}) + + def test_parse_invalid_syntax(self): + invalid_items = ['k= 1', 'k =1', 'k', 'k v'] + for item in invalid_items: + with self.assertRaises(ValueError): + parse_arguments(f'-e {item}'.split()) + + def test_parse_invalid_json(self): + invalid_json = { + '', 'False', 'True', 'None', 'some other string', + '{"foo": bar}', '[foo]' + } + for v in invalid_json: + with self.assertRaises(ValueError): + parse_arguments(['-e ', 'k=' + v]) + + +class TestGetConfigFromArgs(unittest.TestCase): + def test_overrides_known_keys(self): + args = parse_arguments([ + '-e', + 'DELETE_OUTPUT_DIRECTORY=false', + 'OUTPUT_RETENTION=["1.txt"]', + 'SITENAME="Title"' + ]) + config = get_config(args) + config_must_contain = { + 'DELETE_OUTPUT_DIRECTORY': False, + 'OUTPUT_RETENTION': ['1.txt'], + 'SITENAME': 'Title' + } + self.assertDictEqual(config, {**config, **config_must_contain}) + + def test_overrides_non_default_type(self): + args = parse_arguments([ + '-e', + 'DISPLAY_PAGES_ON_MENU=123', + 'PAGE_TRANSLATION_ID=null', + 'TRANSLATION_FEED_RSS_URL="someurl"' + ]) + config = get_config(args) + config_must_contain = { + 'DISPLAY_PAGES_ON_MENU': 123, + 'PAGE_TRANSLATION_ID': None, + 'TRANSLATION_FEED_RSS_URL': 'someurl' + } + self.assertDictEqual(config, {**config, **config_must_contain}) diff --git a/pelican/tests/test_settings.py b/pelican/tests/test_settings.py index 83203ae5..c407f7c8 100644 --- a/pelican/tests/test_settings.py +++ b/pelican/tests/test_settings.py @@ -7,7 +7,7 @@ from sys import platform from pelican.settings import (DEFAULT_CONFIG, DEFAULT_THEME, _printf_s_to_format_field, - coerce_overrides, configure_settings, + configure_settings, handle_deprecated_settings, read_settings) from pelican.tests.support import unittest @@ -304,18 +304,3 @@ class TestSettingsConfiguration(unittest.TestCase): [(r'C\+\+', 'cpp')] + self.settings['SLUG_REGEX_SUBSTITUTIONS']) self.assertNotIn('SLUG_SUBSTITUTIONS', settings) - - def test_coerce_overrides(self): - overrides = coerce_overrides({ - 'ARTICLE_EXCLUDES': '["testexcl"]', - 'READERS': '{"foo": "bar"}', - 'STATIC_EXCLUDE_SOURCES': 'true', - 'THEME_STATIC_DIR': 'theme', - }) - expected = { - 'ARTICLE_EXCLUDES': ["testexcl"], - 'READERS': {"foo": "bar"}, - 'STATIC_EXCLUDE_SOURCES': True, - 'THEME_STATIC_DIR': 'theme', - } - self.assertDictEqual(overrides, expected)