From dc3a5ce624fb0a0bf2d0c701aaa34df63b6fc5bd Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 10 Feb 2016 16:05:15 -0500 Subject: [PATCH] Refactor config validation to support constraints in the same jsonschema Reworked the two schema validation functions to read from the same schema but use different parts of it. Error handling is now split as well by the schema that is being used to validate. Signed-off-by: Daniel Nephin --- compose/config/config.py | 2 +- compose/config/config_schema_v1.json | 4 +- compose/config/config_schema_v2.0.json | 4 +- compose/config/validation.py | 153 ++++++++++++------------- tests/unit/config/config_test.py | 19 ++- 5 files changed, 87 insertions(+), 95 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 3994a3323d..850af31c90 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -31,8 +31,8 @@ from .types import ServiceLink from .types import VolumeFromSpec from .types import VolumeSpec from .validation import match_named_volumes -from .validation import validate_config_section from .validation import validate_against_config_schema +from .validation import validate_config_section from .validation import validate_depends_on from .validation import validate_extends_file_path from .validation import validate_network_mode diff --git a/compose/config/config_schema_v1.json b/compose/config/config_schema_v1.json index affc19f0fc..cde8c8e56e 100644 --- a/compose/config/config_schema_v1.json +++ b/compose/config/config_schema_v1.json @@ -167,8 +167,8 @@ }, "constraints": { - "services": { - "id": "#/definitions/services/constraints", + "service": { + "id": "#/definitions/constraints/service", "anyOf": [ { "required": ["build"], diff --git a/compose/config/config_schema_v2.0.json b/compose/config/config_schema_v2.0.json index e8ceb4c2b0..54bfc978dd 100644 --- a/compose/config/config_schema_v2.0.json +++ b/compose/config/config_schema_v2.0.json @@ -312,8 +312,8 @@ }, "constraints": { - "services": { - "id": "#/definitions/services/constraints", + "service": { + "id": "#/definitions/constraints/service", "anyOf": [ {"required": ["build"]}, {"required": ["image"]} diff --git a/compose/config/validation.py b/compose/config/validation.py index 07ec04ef7d..4eafe7b5c9 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -14,6 +14,7 @@ from jsonschema import FormatChecker from jsonschema import RefResolver from jsonschema import ValidationError +from ..const import COMPOSEFILE_V1 as V1 from .errors import ConfigurationError from .errors import VERSION_EXPLANATION from .sort_services import get_service_name_from_network_mode @@ -209,7 +210,7 @@ def anglicize_json_type(json_type): def is_service_dict_schema(schema_id): - return schema_id == 'fields_schema_v1.json' or schema_id == '#/properties/services' + return schema_id in ('config_schema_v1.json', '#/properties/services') def handle_error_for_schema_with_id(error, path): @@ -221,35 +222,6 @@ def handle_error_for_schema_with_id(error, path): list(error.instance)[0], VALID_NAME_CHARS) - if schema_id == '#/definitions/constraints': - # Build context could in 'build' or 'build.context' and dockerfile could be - # in 'dockerfile' or 'build.dockerfile' - context = False - dockerfile = 'dockerfile' in error.instance - if 'build' in error.instance: - if isinstance(error.instance['build'], six.string_types): - context = True - else: - context = 'context' in error.instance['build'] - dockerfile = dockerfile or 'dockerfile' in error.instance['build'] - - # TODO: only applies to v1 - if 'image' in error.instance and context: - return ( - "{} has both an image and build path specified. " - "A service can either be built to image or use an existing " - "image, not both.".format(path_string(path))) - if 'image' not in error.instance and not context: - return ( - "{} has neither an image nor a build path specified. " - "At least one must be provided.".format(path_string(path))) - # TODO: only applies to v1 - if 'image' in error.instance and dockerfile: - return ( - "{} has both an image and alternate Dockerfile. " - "A service can either be built to image or use an existing " - "image, not both.".format(path_string(path))) - if error.validator == 'additionalProperties': if schema_id == '#/definitions/service': invalid_config_key = parse_key_from_error_msg(error) @@ -259,7 +231,7 @@ def handle_error_for_schema_with_id(error, path): return '{}\n{}'.format(error.message, VERSION_EXPLANATION) -def handle_generic_service_error(error, path): +def handle_generic_error(error, path): msg_format = None error_msg = error.message @@ -365,71 +337,94 @@ def _parse_oneof_validator(error): return (None, "contains an invalid type, it should be {}".format(valid_types)) -def process_errors(errors, path_prefix=None): - """jsonschema gives us an error tree full of information to explain what has - gone wrong. Process each error and pull out relevant information and re-write - helpful error messages that are relevant. - """ - path_prefix = path_prefix or [] +def process_service_constraint_errors(error, service_name, version): + if version == V1: + if 'image' in error.instance and 'build' in error.instance: + return ( + "Service {} has both an image and build path specified. " + "A service can either be built to image or use an existing " + "image, not both.".format(service_name)) - def format_error_message(error): - path = path_prefix + list(error.path) + if 'image' in error.instance and 'dockerfile' in error.instance: + return ( + "Service {} has both an image and alternate Dockerfile. " + "A service can either be built to image or use an existing " + "image, not both.".format(service_name)) - if 'id' in error.schema: - error_msg = handle_error_for_schema_with_id(error, path) - if error_msg: - return error_msg + if 'image' not in error.instance and 'build' not in error.instance: + return ( + "Service {} has neither an image nor a build context specified. " + "At least one must be provided.".format(service_name)) - return handle_generic_service_error(error, path) - return '\n'.join(format_error_message(error) for error in errors) +def process_config_schema_errors(error): + path = list(error.path) + + if 'id' in error.schema: + error_msg = handle_error_for_schema_with_id(error, path) + if error_msg: + return error_msg + + return handle_generic_error(error, path) def validate_against_config_schema(config_file): - _validate_against_schema( - config_file.config, - "config_schema_v{0}.json".format(config_file.version), - format_checker=["ports", "expose", "bool-value-in-mapping"], - filename=config_file.filename) + schema = load_jsonschema(config_file.version) + format_checker = FormatChecker(["ports", "expose", "bool-value-in-mapping"]) + validator = Draft4Validator( + schema, + resolver=RefResolver(get_resolver_path(), schema), + format_checker=format_checker) + handle_errors( + validator.iter_errors(config_file.config), + process_config_schema_errors, + config_file.filename) def validate_service_constraints(config, service_name, version): - # TODO: - pass + def handler(errors): + return process_service_constraint_errors(errors, service_name, version) + + schema = load_jsonschema(version) + validator = Draft4Validator(schema['definitions']['constraints']['service']) + handle_errors(validator.iter_errors(config), handler, None) -def _validate_against_schema( - config, - schema_filename, - format_checker=(), - path_prefix=None, - filename=None): - config_source_dir = os.path.dirname(os.path.abspath(__file__)) +def get_schema_path(): + return os.path.dirname(os.path.abspath(__file__)) + +def load_jsonschema(version): + filename = os.path.join( + get_schema_path(), + "config_schema_v{0}.json".format(version)) + + with open(filename, "r") as fh: + return json.load(fh) + + +def get_resolver_path(): + schema_path = get_schema_path() if sys.platform == "win32": - file_pre_fix = "///" - config_source_dir = config_source_dir.replace('\\', '/') + scheme = "///" + # TODO: why is this necessary? + schema_path = schema_path.replace('\\', '/') else: - file_pre_fix = "//" + scheme = "//" + return "file:{}{}/".format(scheme, schema_path) - resolver_full_path = "file:{}{}/".format(file_pre_fix, config_source_dir) - schema_file = os.path.join(config_source_dir, schema_filename) - with open(schema_file, "r") as schema_fh: - schema = json.load(schema_fh) - - resolver = RefResolver(resolver_full_path, schema) - validation_output = Draft4Validator( - schema, - resolver=resolver, - format_checker=FormatChecker(format_checker)) - - errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] +def handle_errors(errors, format_error_func, filename): + """jsonschema returns an error tree full of information to explain what has + gone wrong. Process each error and pull out relevant information and re-write + helpful error messages that are relevant. + """ + errors = list(sorted(errors, key=str)) if not errors: return - error_msg = process_errors(errors, path_prefix=path_prefix) - file_msg = " in file '{}'".format(filename) if filename else '' - raise ConfigurationError("Validation failed{}, reason(s):\n{}".format( - file_msg, - error_msg)) + error_msg = '\n'.join(format_error_func(error) for error in errors) + raise ConfigurationError( + "Validation failed{file_msg}, reason(s):\n{error_msg}".format( + file_msg=" in file '{}'".format(filename) if filename else "", + error_msg=error_msg)) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 11bc7f0b73..f8f224a08f 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -342,20 +342,17 @@ class ConfigTest(unittest.TestCase): for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: with pytest.raises(ConfigurationError) as exc: config.load(build_config_details( - {invalid_name: {'image': 'busybox'}}, - 'working_dir', - 'filename.yml')) + {invalid_name: {'image': 'busybox'}})) assert 'Invalid service name \'%s\'' % invalid_name in exc.exconly() - def test_config_invalid_service_names_v2(self): + def test_load_config_invalid_service_names_v2(self): for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: with pytest.raises(ConfigurationError) as exc: - config.load( - build_config_details({ + config.load(build_config_details( + { 'version': '2', - 'services': {invalid_name: {'image': 'busybox'}} - }, 'working_dir', 'filename.yml') - ) + 'services': {invalid_name: {'image': 'busybox'}}, + })) assert 'Invalid service name \'%s\'' % invalid_name in exc.exconly() def test_load_with_invalid_field_name(self): @@ -1317,7 +1314,7 @@ class ConfigTest(unittest.TestCase): }) with pytest.raises(ConfigurationError) as exc: config.load(config_details) - assert 'one.build is invalid, context is required.' in exc.exconly() + assert 'has neither an image nor a build context' in exc.exconly() class NetworkModeTest(unittest.TestCase): @@ -2269,7 +2266,7 @@ class ExtendsTest(unittest.TestCase): with pytest.raises(ConfigurationError) as exc: load_from_filename('tests/fixtures/extends/service-with-invalid-schema.yml') assert ( - "myweb has neither an image nor a build path specified" in + "myweb has neither an image nor a build context specified" in exc.exconly() )