diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index eb718df1b4e..9d2217bd257 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -17,6 +17,7 @@ from .models import Build, Conda, Mkdocs, Python, Sphinx, Submodules from .parser import ParseError, parse from .validation import ( + VALUE_NOT_FOUND, ValidationError, validate_bool, validate_choice, @@ -25,7 +26,6 @@ validate_file, validate_list, validate_string, - validate_value_exists, ) __all__ = ( @@ -54,6 +54,7 @@ PYTHON_INVALID = 'python-invalid' SUBMODULES_INVALID = 'submodules-invalid' INVALID_KEYS_COMBINATION = 'invalid-keys-combination' +INVALID_KEY = 'invalid-key' DOCKER_DEFAULT_IMAGE = 'readthedocs/build' DOCKER_DEFAULT_VERSION = '2.0' @@ -176,6 +177,41 @@ def catch_validation_error(self, key): source_position=self.source_position, ) + def pop(self, name, container, default, raise_ex): + """ + Search and pop a key inside a dict. + + This will pop the keys recursively if the container is empty. + + :param name: the key name in a list form (``['key', 'inner']``) + :param container: a dictionary that contains the key + :param default: default value to return if the key doesn't exists + :param raise_ex: if True, raises an exception when a key is not found + """ + key = name[0] + validate_dict(container) + if key in container: + if len(name) > 1: + value = self.pop(name[1:], container[key], default, raise_ex) + if not container[key]: + container.pop(key) + else: + value = container.pop(key) + return value + if raise_ex: + raise ValidationError(key, VALUE_NOT_FOUND) + return default + + def pop_config(self, key, default=None, raise_ex=False): + """ + Search and pop a key (recursively) from `self.raw_config`. + + :param key: the key name in a dotted form (``key.innerkey``) + :param default: Optionally, it can receive a default value + :param raise_ex: If True, raises an exception when the key is not found + """ + return self.pop(key.split('.'), self.raw_config, default, raise_ex) + def validate(self): raise NotImplementedError() @@ -594,6 +630,7 @@ def validate(self): # TODO: remove later self.validate_final_doc_type() self._config['submodules'] = self.validate_submodules() + self.validate_keys() def validate_formats(self): """ @@ -602,7 +639,7 @@ def validate_formats(self): The ``ALL`` keyword can be used to indicate that all formats are used. We ignore the default values here. """ - formats = self.raw_config.get('formats', []) + formats = self.pop_config('formats', []) if formats == ALL: return self.valid_formats with self.catch_validation_error('formats'): @@ -622,7 +659,7 @@ def validate_conda(self): conda = {} with self.catch_validation_error('conda.environment'): - environment = validate_value_exists('environment', raw_conda) + environment = self.pop_config('conda.environment', raise_ex=True) conda['environment'] = validate_file(environment, self.base_path) return conda @@ -637,7 +674,7 @@ def validate_build(self): validate_dict(raw_build) build = {} with self.catch_validation_error('build.image'): - image = raw_build.get('image', self.default_build_image) + image = self.pop_config('build.image', self.default_build_image) build['image'] = '{}:{}'.format( DOCKER_DEFAULT_IMAGE, validate_choice( @@ -674,7 +711,7 @@ def validate_python(self): python = {} with self.catch_validation_error('python.version'): - version = raw_python.get('version', 3) + version = self.pop_config('python.version', 3) if isinstance(version, six.string_types): try: version = int(version) @@ -690,7 +727,7 @@ def validate_python(self): with self.catch_validation_error('python.requirements'): requirements = self.defaults.get('requirements_file') - requirements = raw_python.get('requirements', requirements) + requirements = self.pop_config('python.requirements', requirements) if requirements != '' and requirements is not None: requirements = validate_file(requirements, self.base_path) python['requirements'] = requirements @@ -699,14 +736,16 @@ def validate_python(self): install = ( 'setup.py' if self.defaults.get('install_project') else None ) - install = raw_python.get('install', install) + install = self.pop_config('python.install', install) if install is not None: validate_choice(install, self.valid_install_options) python['install_with_setup'] = install == 'setup.py' python['install_with_pip'] = install == 'pip' with self.catch_validation_error('python.extra_requirements'): - extra_requirements = raw_python.get('extra_requirements', []) + extra_requirements = self.pop_config( + 'python.extra_requirements', [] + ) extra_requirements = validate_list(extra_requirements) if extra_requirements and not python['install_with_pip']: self.error( @@ -724,8 +763,8 @@ def validate_python(self): 'use_system_packages', False, ) - system_packages = raw_python.get( - 'system_packages', + system_packages = self.pop_config( + 'python.system_packages', system_packages, ) python['use_system_site_packages'] = validate_bool(system_packages) @@ -778,13 +817,13 @@ def validate_mkdocs(self): mkdocs = {} with self.catch_validation_error('mkdocs.configuration'): - configuration = raw_mkdocs.get('configuration') + configuration = self.pop_config('mkdocs.configuration', None) if configuration is not None: configuration = validate_file(configuration, self.base_path) mkdocs['configuration'] = configuration with self.catch_validation_error('mkdocs.fail_on_warning'): - fail_on_warning = raw_mkdocs.get('fail_on_warning', False) + fail_on_warning = self.pop_config('mkdocs.fail_on_warning', False) mkdocs['fail_on_warning'] = validate_bool(fail_on_warning) return mkdocs @@ -812,7 +851,7 @@ def validate_sphinx(self): sphinx = {} with self.catch_validation_error('sphinx.builder'): builder = validate_choice( - raw_sphinx.get('builder', 'html'), + self.pop_config('sphinx.builder', 'html'), self.valid_sphinx_builders.keys(), ) sphinx['builder'] = self.valid_sphinx_builders[builder] @@ -822,13 +861,15 @@ def validate_sphinx(self): # The default value can be empty if not configuration: configuration = None - configuration = raw_sphinx.get('configuration', configuration) + configuration = self.pop_config( + 'sphinx.configuration', configuration + ) if configuration is not None: configuration = validate_file(configuration, self.base_path) sphinx['configuration'] = configuration with self.catch_validation_error('sphinx.fail_on_warning'): - fail_on_warning = raw_sphinx.get('fail_on_warning', False) + fail_on_warning = self.pop_config('sphinx.fail_on_warning', False) sphinx['fail_on_warning'] = validate_bool(fail_on_warning) return sphinx @@ -870,7 +911,7 @@ def validate_submodules(self): submodules = {} with self.catch_validation_error('submodules.include'): - include = raw_submodules.get('include', []) + include = self.pop_config('submodules.include', []) if include != ALL: include = [ validate_string(submodule) @@ -880,7 +921,7 @@ def validate_submodules(self): with self.catch_validation_error('submodules.exclude'): default = [] if submodules['include'] else ALL - exclude = raw_submodules.get('exclude', default) + exclude = self.pop_config('submodules.exclude', default) if exclude != ALL: exclude = [ validate_string(submodule) @@ -902,11 +943,54 @@ def validate_submodules(self): ) with self.catch_validation_error('submodules.recursive'): - recursive = raw_submodules.get('recursive', False) + recursive = self.pop_config('submodules.recursive', False) submodules['recursive'] = validate_bool(recursive) return submodules + def validate_keys(self): + """ + Checks that we don't have extra keys (invalid ones). + + This should be called after all the validations are done + and all keys are popped from `self.raw_config`. + """ + msg = ( + 'Invalid configuration option: {}. ' + 'Make sure the key name is correct.' + ) + # The version key isn't popped, but it's + # validated in `load`. + self.pop_config('version', None) + wrong_key = '.'.join(self._get_extra_key(self.raw_config)) + if wrong_key: + self.error( + wrong_key, + msg.format(wrong_key), + code=INVALID_KEY, + ) + + def _get_extra_key(self, value): + """ + Get the extra keyname (list form) of a dict object. + + If there is more than one extra key, the first one is returned. + + Example:: + + { + 'key': { + 'name': 'inner', + } + } + + Will return `['key', 'name']`. + """ + if isinstance(value, dict) and value: + key_name = next(iter(value)) + return [key_name] + self._get_extra_key(value[key_name]) + return [] + @property def formats(self): return self._config['formats'] diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index e2bfc37bf46..4ba0b5c0982 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -4,20 +4,42 @@ import os import re import textwrap +from collections import OrderedDict import pytest from mock import DEFAULT, patch from pytest import raises from readthedocs.config import ( - ALL, BuildConfigV1, BuildConfigV2, ConfigError, - ConfigOptionNotSupportedError, InvalidConfig, ProjectConfig, load) + ALL, + BuildConfigV1, + BuildConfigV2, + ConfigError, + ConfigOptionNotSupportedError, + InvalidConfig, + ProjectConfig, + load, +) from readthedocs.config.config import ( - CONFIG_FILENAME_REGEX, CONFIG_NOT_SUPPORTED, CONFIG_REQUIRED, NAME_INVALID, - NAME_REQUIRED, PYTHON_INVALID, VERSION_INVALID) + CONFIG_FILENAME_REGEX, + CONFIG_NOT_SUPPORTED, + CONFIG_REQUIRED, + INVALID_KEY, + NAME_INVALID, + NAME_REQUIRED, + PYTHON_INVALID, + VERSION_INVALID, +) from readthedocs.config.models import Conda from readthedocs.config.validation import ( - INVALID_BOOL, INVALID_CHOICE, INVALID_LIST, INVALID_PATH, INVALID_STRING) + INVALID_BOOL, + INVALID_CHOICE, + INVALID_LIST, + INVALID_PATH, + INVALID_STRING, + VALUE_NOT_FOUND, + ValidationError, +) from .utils import apply_fs @@ -1702,3 +1724,83 @@ def test_submodules_recursive_explict_default(self): assert build.submodules.include == [] assert build.submodules.exclude == [] assert build.submodules.recursive is False + + @pytest.mark.parametrize('value,key', [ + ({'typo': 'something'}, 'typo'), + ( + { + 'pyton': { + 'version': 'another typo', + } + }, + 'pyton.version' + ), + ( + { + 'build': { + 'image': 'latest', + 'extra': 'key', + } + }, + 'build.extra' + ) + ]) + def test_strict_validation(self, value, key): + build = self.get_build_config(value) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == key + assert excinfo.value.code == INVALID_KEY + + def test_strict_validation_pops_all_keys(self): + build = self.get_build_config({ + 'version': 2, + 'python': { + 'version': 3, + }, + }) + build.validate() + assert build.raw_config == {} + + @pytest.mark.parametrize('value,expected', [ + ({}, []), + ({'one': 1}, ['one']), + ({'one': {'two': 3}}, ['one', 'two']), + (OrderedDict([('one', 1), ('two', 2)]), ['one']), + (OrderedDict([('one', {'two': 2}), ('three', 3)]), ['one', 'two']), + ]) + def test_get_extra_key(self, value, expected): + build = self.get_build_config({}) + assert build._get_extra_key(value) == expected + + def test_pop_config_single(self): + build = self.get_build_config({'one': 1}) + build.pop_config('one') + assert build.raw_config == {} + + def test_pop_config_nested(self): + build = self.get_build_config({'one': {'two': 2}}) + build.pop_config('one.two') + assert build.raw_config == {} + + def test_pop_config_nested_with_residue(self): + build = self.get_build_config({'one': {'two': 2, 'three': 3}}) + build.pop_config('one.two') + assert build.raw_config == {'one': {'three': 3}} + + def test_pop_config_default_none(self): + build = self.get_build_config({'one': {'two': 2, 'three': 3}}) + assert build.pop_config('one.four') is None + assert build.raw_config == {'one': {'two': 2, 'three': 3}} + + def test_pop_config_default(self): + build = self.get_build_config({'one': {'two': 2, 'three': 3}}) + assert build.pop_config('one.four', 4) == 4 + assert build.raw_config == {'one': {'two': 2, 'three': 3}} + + def test_pop_config_raise_exception(self): + build = self.get_build_config({'one': {'two': 2, 'three': 3}}) + with raises(ValidationError) as excinfo: + build.pop_config('one.four', raise_ex=True) + assert excinfo.value.value == 'four' + assert excinfo.value.code == VALUE_NOT_FOUND