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 <dnephin@docker.com>

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

compose/config/config_schema_v1.json

@@ -167,8 +167,8 @@
     },
 
     "constraints": {
-      "services": {
+      "service": {
-        "id": "#/definitions/services/constraints",
+        "id": "#/definitions/constraints/service",
         "anyOf": [
           {
             "required": ["build"],

compose/config/config_schema_v2.0.json

@@ -312,8 +312,8 @@
     },
 
     "constraints": {
-      "services": {
+      "service": {
-        "id": "#/definitions/services/constraints",
+        "id": "#/definitions/constraints/service",
         "anyOf": [
           {"required": ["build"]},
           {"required": ["image"]}

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):
+def process_service_constraint_errors(error, service_name, version):
-    """jsonschema gives us an error tree full of information to explain what has
+    if version == V1:
-    gone wrong. Process each error and pull out relevant information and re-write
+        if 'image' in error.instance and 'build' in error.instance:
-    helpful error messages that are relevant.
+            return (
-    """
+                "Service {} has both an image and build path specified. "
-    path_prefix = path_prefix or []
+                "A service can either be built to image or use an existing "
+                "image, not both.".format(service_name))
 
-    def format_error_message(error):
+        if 'image' in error.instance and 'dockerfile' in error.instance:
-        path = path_prefix + list(error.path)
+            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:
+    if 'image' not in error.instance and 'build' not in error.instance:
-            error_msg = handle_error_for_schema_with_id(error, path)
+        return (
-            if error_msg:
+            "Service {} has neither an image nor a build context specified. "
-                return error_msg
+            "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(
+    schema = load_jsonschema(config_file.version)
-        config_file.config,
+    format_checker = FormatChecker(["ports", "expose", "bool-value-in-mapping"])
-        "config_schema_v{0}.json".format(config_file.version),
+    validator = Draft4Validator(
-        format_checker=["ports", "expose", "bool-value-in-mapping"],
+        schema,
-        filename=config_file.filename)
+        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:
+    def handler(errors):
-    pass
+        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(
+def get_schema_path():
-        config,
+    return os.path.dirname(os.path.abspath(__file__))
-        schema_filename,
-        format_checker=(),
-        path_prefix=None,
-        filename=None):
-    config_source_dir = 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 = "///"
+        scheme = "///"
-        config_source_dir = config_source_dir.replace('\\', '/')
+        # 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:
+def handle_errors(errors, format_error_func, filename):
-        schema = json.load(schema_fh)
+    """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
-    resolver = RefResolver(resolver_full_path, schema)
+    helpful error messages that are relevant.
-    validation_output = Draft4Validator(
+    """
-        schema,
+    errors = list(sorted(errors, key=str))
-        resolver=resolver,
-        format_checker=FormatChecker(format_checker))
-
-    errors = [error for error in sorted(validation_output.iter_errors(config), key=str)]
     if not errors:
         return
 
-    error_msg = process_errors(errors, path_prefix=path_prefix)
+    error_msg = '\n'.join(format_error_func(error) for error in errors)
-    file_msg = " in file '{}'".format(filename) if filename else ''
+    raise ConfigurationError(
-    raise ConfigurationError("Validation failed{}, reason(s):\n{}".format(
+        "Validation failed{file_msg}, reason(s):\n{error_msg}".format(
-        file_msg,
+            file_msg=" in file '{}'".format(filename) if filename else "",
-        error_msg))
+            error_msg=error_msg))

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'}},
+                    {invalid_name: {'image': 'busybox'}}))
-                    'working_dir',
-                    'filename.yml'))
             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(
+                config.load(build_config_details(
-                    build_config_details({
+                    {
                         'version': '2',
-                        'services': {invalid_name: {'image': 'busybox'}}
+                        'services': {invalid_name: {'image': 'busybox'}},
-                    }, 'working_dir', 'filename.yml')
+                    }))
-                )
             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()
         )