From 5c178a1ccb8d6cbc7570ca0719128c2803aed974 Mon Sep 17 00:00:00 2001 From: Victor Skvortsov Date: Wed, 20 Oct 2021 16:17:31 +0500 Subject: [PATCH] Fix #2938 Get rid of the coerce_overrides() function. Add the ParseOverrides argparse.Action to parse overrides. Treat all extra settings values strictly as json values. Test overrides. Edit docs and cli help. --- RELEASE.md | 3 ++ docs/settings.rst | 9 +++--- pelican/__init__.py | 50 ++++++++++++++++++++++------------ pelican/settings.py | 23 ---------------- pelican/tests/test_settings.py | 17 +----------- 5 files changed, 42 insertions(+), 60 deletions(-) create mode 100644 RELEASE.md diff --git a/RELEASE.md b/RELEASE.md new file mode 100644 index 000000000..76e47100d --- /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 478525278..675e6b8f5 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 99aa2776b..9858dbd3d 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._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 @@ def __call__(self, parser, namespace, values, option_string): 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 ea3ee8eb7..5b495e863 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_settings.py b/pelican/tests/test_settings.py index 83203ae52..c407f7c88 100644 --- a/pelican/tests/test_settings.py +++ b/pelican/tests/test_settings.py @@ -7,7 +7,7 @@ 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 @@ def test_deprecated_slug_substitutions_from_file(self): [(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)