Skip to content

Strict validation in configuration file (v2 only) #4607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Oct 4, 2018
Merged

Strict validation in configuration file (v2 only) #4607

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0bbbc6a
Tests
stsewd Sep 5, 2018
573c878
Validation
stsewd Sep 5, 2018
2fc02e7
Refactor
stsewd Sep 6, 2018
28ccce3
Better docs
stsewd Sep 6, 2018
06b40f8
Refactor
stsewd Sep 10, 2018
1187efb
Isort
stsewd Sep 27, 2018
67d0420
One more test
stsewd Oct 1, 2018
c60f59a
Better docstrings
stsewd Oct 1, 2018
a2c6c9d
More tests
stsewd Oct 3, 2018
7ce0202
And more tests
stsewd Oct 3, 2018
d33854a
Isort
stsewd Oct 3, 2018
997b5e2
Typo
stsewd Oct 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 102 additions & 18 deletions readthedocs/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -25,7 +26,6 @@
validate_file,
validate_list,
validate_string,
validate_value_exists,
)

__all__ = (
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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):
"""
Expand All @@ -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'):
Expand All @@ -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

Expand All @@ -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(
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always pass the version key, but that's is already validated from outside

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 are more the one key, the first one is returned.
Copy link
Member

@humitos humitos Oct 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"more than one extra key"


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']
Expand Down
112 changes: 107 additions & 5 deletions readthedocs/config/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may worth to have some unit test for the specific methods that do the trick: _get_extra_key and validate_keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular the one that is recursive with different start/stop conditions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_keys is already called when running validate. _get_extra_key is validated here https://github.com/stsewd/readthedocs.org/blob/strict-validation/readthedocs/config/tests/test_config.py#L1718-L1743

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added one more test, but we are already covered anyway with the other tests (they don't throw an exception).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not throwing an exception doesn't test that _get_extra_key returns the correct keys. That's the test I wanted.

The same for validate_keys to check that if there is extra keys, it fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually test_strict_validation test that, an error is thrown and key and code are the correct in the error. We test with validate as entrypoint, as that's the way to use the config module (all tests were changed to test like that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you say. Although, I'd like to have a unit test for the methods I mentioned before for these cases:

  • _get_extra_key
    • returns no extra keys
    • a couple of cases where it returns extra keys
  • validate_keys
    • a couple of cases where it raises
    • the same for no raising the exception

The test_strict_validation is testing only one case and using the complete flow. I'd like to test the inner pieces, in particular the than that it's recursive (it's very easy to make a mistake on initial conditions)

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