Refactor validation out

Move validation out into its own file without causing circular
import errors.

Fix some of the tests to import from the right place.

Also fix tests that were not using valid test data, as the validation
schema is now firing telling you that you couldn't "just" have this
dict without a build/image config key.

Signed-off-by: Mazz Mosley <[email protected]>

compose/config/config.py

@@ -3,9 +3,6 @@ import os
 import sys
 import yaml
 from collections import namedtuple
-import json
-from jsonschema import Draft4Validator, FormatChecker, ValidationError
-
 import six
 
 from compose.cli.utils import find_candidates_in_parent_dirs
@@ -16,10 +13,9 @@ from .errors import (
     CircularReference,
     ComposeFileNotFound,
 )
+from .validation import validate_against_schema
 
 
-VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]'
-
 DOCKER_CONFIG_KEYS = [
     'cap_add',
     'cap_drop',
@@ -69,22 +65,6 @@ ALLOWED_KEYS = DOCKER_CONFIG_KEYS + [
     'name',
 ]
 
-DOCKER_CONFIG_HINTS = {
-    'cpu_share': 'cpu_shares',
-    'add_host': 'extra_hosts',
-    'hosts': 'extra_hosts',
-    'extra_host': 'extra_hosts',
-    'device': 'devices',
-    'link': 'links',
-    'memory_swap': 'memswap_limit',
-    'port': 'ports',
-    'privilege': 'privileged',
-    'priviliged': 'privileged',
-    'privilige': 'privileged',
-    'volume': 'volumes',
-    'workdir': 'working_dir',
-}
-
 
 SUPPORTED_FILENAMES = [
     'docker-compose.yml',
@@ -135,122 +115,18 @@ def get_config_path(base_dir):
     return os.path.join(path, winner)
 
 
[email protected]_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted."))
-def format_ports(instance):
-    def _is_valid(port):
-        if ':' in port or '/' in port:
-            return True
-        try:
-            int(port)
-            return True
-        except ValueError:
-            return False
-        return False
-
-    if isinstance(instance, list):
-        for port in instance:
-            if not _is_valid(port):
-                return False
-        return True
-    elif isinstance(instance, str):
-        return _is_valid(instance)
-    return False
-
-
-def get_unsupported_config_msg(service_name, error_key):
-    msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key)
-    if error_key in DOCKER_CONFIG_HINTS:
-        msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key])
-    return msg
-
-
-def process_errors(errors):
-    """
-    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.
-    """
-    def _parse_key_from_error_msg(error):
-        return error.message.split("'")[1]
-
-    root_msgs = []
-    invalid_keys = []
-    required = []
-    type_errors = []
-
-    for error in errors:
-        # handle root level errors
-        if len(error.path) == 0:
-            if error.validator == 'type':
-                msg = "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level."
-                root_msgs.append(msg)
-            elif error.validator == 'additionalProperties':
-                invalid_service_name = _parse_key_from_error_msg(error)
-                msg = "Invalid service name '{}' - only {} characters are allowed".format(invalid_service_name, VALID_NAME_CHARS)
-                root_msgs.append(msg)
-            else:
-                root_msgs.append(error.message)
-
-        else:
-            # handle service level errors
-            service_name = error.path[0]
-
-            if error.validator == 'additionalProperties':
-                invalid_config_key = _parse_key_from_error_msg(error)
-                invalid_keys.append(get_unsupported_config_msg(service_name, invalid_config_key))
-            elif error.validator == 'anyOf':
-                if 'image' in error.instance and 'build' in error.instance:
-                    required.append("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))
-                elif 'image' not in error.instance and 'build' not in error.instance:
-                    required.append("Service '{}' has neither an image nor a build path specified. Exactly one must be provided.".format(service_name))
-                else:
-                    required.append(error.message)
-            elif error.validator == 'type':
-                msg = "a"
-                if error.validator_value == "array":
-                    msg = "an"
-
-                try:
-                    config_key = error.path[1]
-                    type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value))
-                except IndexError:
-                    config_key = error.path[0]
-                    root_msgs.append("Service '{}' doesn\'t have any configuration options. All top level keys in your docker-compose.yml must map to a dictionary of configuration options.'".format(config_key))
-            elif error.validator == 'required':
-                config_key = error.path[1]
-                required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message))
-            elif error.validator == 'dependencies':
-                dependency_key = error.validator_value.keys()[0]
-                required_keys = ",".join(error.validator_value[dependency_key])
-                required.append("Invalid '{}' configuration for '{}' service: when defining '{}' you must set '{}' as well".format(
-                    dependency_key, service_name, dependency_key, required_keys))
-
-    return "\n".join(root_msgs + invalid_keys + required + type_errors)
-
-
-def validate_against_schema(config):
-    config_source_dir = os.path.dirname(os.path.abspath(__file__))
-    schema_file = os.path.join(config_source_dir, "schema.json")
-
-    with open(schema_file, "r") as schema_fh:
-        schema = json.load(schema_fh)
-
-    validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"]))
-
-    errors = [error for error in sorted(validation_output.iter_errors(config), key=str)]
-    if errors:
-        error_msg = process_errors(errors)
-        raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg))
-
-
 def load(config_details):
     config, working_dir, filename = config_details
+    if not isinstance(config, dict):
+        raise ConfigurationError(
+            "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level."
+        )
+
     config = interpolate_environment_variables(config)
+    validate_against_schema(config)
 
     service_dicts = []
 
-    validate_against_schema(config)
-
     for service_name, service_dict in list(config.items()):
         loader = ServiceLoader(working_dir=working_dir, filename=filename)
         service_dict = loader.make_service_dict(service_name, service_dict)
@@ -347,13 +223,6 @@ def validate_extended_service_dict(service_dict, filename, service):
 
 
 def process_container_options(service_dict, working_dir=None):
-    for k in service_dict:
-        if k not in ALLOWED_KEYS:
-            msg = "Unsupported config option for %s service: '%s'" % (service_dict['name'], k)
-            if k in DOCKER_CONFIG_HINTS:
-                msg += " (did you mean '%s'?)" % DOCKER_CONFIG_HINTS[k]
-            raise ConfigurationError(msg)
-
     service_dict = service_dict.copy()
 
     if 'volumes' in service_dict and service_dict.get('volume_driver') is None:

compose/config/validation.py

@@ -0,0 +1,134 @@
+import os
+
+import json
+from jsonschema import Draft4Validator, FormatChecker, ValidationError
+
+from .errors import ConfigurationError
+
+
+DOCKER_CONFIG_HINTS = {
+    'cpu_share': 'cpu_shares',
+    'add_host': 'extra_hosts',
+    'hosts': 'extra_hosts',
+    'extra_host': 'extra_hosts',
+    'device': 'devices',
+    'link': 'links',
+    'memory_swap': 'memswap_limit',
+    'port': 'ports',
+    'privilege': 'privileged',
+    'priviliged': 'privileged',
+    'privilige': 'privileged',
+    'volume': 'volumes',
+    'workdir': 'working_dir',
+}
+
+
+VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]'
+
+
[email protected]_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted."))
+def format_ports(instance):
+    def _is_valid(port):
+        if ':' in port or '/' in port:
+            return True
+        try:
+            int(port)
+            return True
+        except ValueError:
+            return False
+        return False
+
+    if isinstance(instance, list):
+        for port in instance:
+            if not _is_valid(port):
+                return False
+        return True
+    elif isinstance(instance, str):
+        return _is_valid(instance)
+    return False
+
+
+def get_unsupported_config_msg(service_name, error_key):
+    msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key)
+    if error_key in DOCKER_CONFIG_HINTS:
+        msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key])
+    return msg
+
+
+def process_errors(errors):
+    """
+    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.
+    """
+    def _parse_key_from_error_msg(error):
+        return error.message.split("'")[1]
+
+    root_msgs = []
+    invalid_keys = []
+    required = []
+    type_errors = []
+
+    for error in errors:
+        # handle root level errors
+        if len(error.path) == 0:
+            if error.validator == 'type':
+                msg = "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level."
+                root_msgs.append(msg)
+            elif error.validator == 'additionalProperties':
+                invalid_service_name = _parse_key_from_error_msg(error)
+                msg = "Invalid service name '{}' - only {} characters are allowed".format(invalid_service_name, VALID_NAME_CHARS)
+                root_msgs.append(msg)
+            else:
+                root_msgs.append(error.message)
+
+        else:
+            # handle service level errors
+            service_name = error.path[0]
+
+            if error.validator == 'additionalProperties':
+                invalid_config_key = _parse_key_from_error_msg(error)
+                invalid_keys.append(get_unsupported_config_msg(service_name, invalid_config_key))
+            elif error.validator == 'anyOf':
+                if 'image' in error.instance and 'build' in error.instance:
+                    required.append("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))
+                elif 'image' not in error.instance and 'build' not in error.instance:
+                    required.append("Service '{}' has neither an image nor a build path specified. Exactly one must be provided.".format(service_name))
+                else:
+                    required.append(error.message)
+            elif error.validator == 'type':
+                msg = "a"
+                if error.validator_value == "array":
+                    msg = "an"
+
+                try:
+                    config_key = error.path[1]
+                    type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value))
+                except IndexError:
+                    config_key = error.path[0]
+                    root_msgs.append("Service '{}' doesn\'t have any configuration options. All top level keys in your docker-compose.yml must map to a dictionary of configuration options.'".format(config_key))
+            elif error.validator == 'required':
+                config_key = error.path[1]
+                required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message))
+            elif error.validator == 'dependencies':
+                dependency_key = error.validator_value.keys()[0]
+                required_keys = ",".join(error.validator_value[dependency_key])
+                required.append("Invalid '{}' configuration for '{}' service: when defining '{}' you must set '{}' as well".format(
+                    dependency_key, service_name, dependency_key, required_keys))
+
+    return "\n".join(root_msgs + invalid_keys + required + type_errors)
+
+
+def validate_against_schema(config):
+    config_source_dir = os.path.dirname(os.path.abspath(__file__))
+    schema_file = os.path.join(config_source_dir, "schema.json")
+
+    with open(schema_file, "r") as schema_fh:
+        schema = json.load(schema_fh)
+
+    validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"]))
+
+    errors = [error for error in sorted(validation_output.iter_errors(config), key=str)]
+    if errors:
+        error_msg = process_errors(errors)
+        raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg))

compose/service.py

@@ -12,7 +12,7 @@ from docker.errors import APIError
 from docker.utils import create_host_config, LogConfig
 
 from . import __version__
-from .config import DOCKER_CONFIG_KEYS, merge_environment, VALID_NAME_CHARS
+from .config import DOCKER_CONFIG_KEYS, merge_environment
 from .const import (
     DEFAULT_TIMEOUT,
     LABEL_CONTAINER_NUMBER,
@@ -26,6 +26,7 @@ from .container import Container
 from .legacy import check_for_legacy_containers
 from .progress_stream import stream_output, StreamOutputError
 from .utils import json_hash, parallel_execute
+from .config.validation import VALID_NAME_CHARS
 
 log = logging.getLogger(__name__)
 

tests/unit/config_test.py

@@ -5,6 +5,7 @@ import tempfile
 from .. import unittest
 
 from compose.config import config
+from compose.config.errors import ConfigurationError
 
 
 def make_service_dict(name, service_dict, working_dir):
@@ -43,7 +44,7 @@ class ConfigTest(unittest.TestCase):
         )
 
     def test_load_throws_error_when_not_dict(self):
-        with self.assertRaises(config.ConfigurationError):
+        with self.assertRaises(ConfigurationError):
             config.load(
                 config.ConfigDetails(
                     {'web': 'busybox:latest'},
@@ -52,15 +53,8 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-    def test_config_validation(self):
-        self.assertRaises(
-            config.ConfigurationError,
-            lambda: make_service_dict('foo', {'port': ['8000']}, 'tests/')
-        )
-        make_service_dict('foo', {'ports': ['8000']}, 'tests/')
-
     def test_config_invalid_service_names(self):
-        with self.assertRaises(config.ConfigurationError):
+        with self.assertRaises(ConfigurationError):
             for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']:
                 config.load(
                     config.ConfigDetails(
@@ -81,7 +75,7 @@ class ConfigTest(unittest.TestCase):
             )
 
     def test_config_invalid_ports_format_validation(self):
-        with self.assertRaises(config.ConfigurationError):
+        with self.assertRaises(ConfigurationError):
             for invalid_ports in [{"1": "8000"}, "whatport"]:
                 config.load(
                     config.ConfigDetails(
@@ -104,7 +98,7 @@ class ConfigTest(unittest.TestCase):
 
     def test_config_hint(self):
         expected_error_msg = "(did you mean 'privileged'?)"
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     {
@@ -117,7 +111,7 @@ class ConfigTest(unittest.TestCase):
 
     def test_invalid_config_build_and_image_specified(self):
         expected_error_msg = "Service 'foo' has both an image and build path specified."
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     {
@@ -130,7 +124,7 @@ class ConfigTest(unittest.TestCase):
 
     def test_invalid_config_type_should_be_an_array(self):
         expected_error_msg = "Service 'foo' has an invalid value for 'links', it should be an array"
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     {
@@ -143,7 +137,7 @@ class ConfigTest(unittest.TestCase):
 
     def test_invalid_config_not_a_dictionary(self):
         expected_error_msg = "Top level object needs to be a dictionary."
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     ['foo', 'lol'],
@@ -198,7 +192,7 @@ class InterpolationTest(unittest.TestCase):
         os.environ['VOLUME_PATH'] = '/host/path'
         d = config.load(
             config.ConfigDetails(
-                config={'foo': {'volumes': ['${VOLUME_PATH}:/container/path']}},
+                config={'foo': {'build': '.', 'volumes': ['${VOLUME_PATH}:/container/path']}},
                 working_dir='.',
                 filename=None,
             )
@@ -422,7 +416,7 @@ class MemoryOptionsTest(unittest.TestCase):
         a mem_limit
         """
         expected_error_msg = "Invalid 'memswap_limit' configuration for 'foo' service: when defining 'memswap_limit' you must set 'mem_limit' as well"
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     {
@@ -465,7 +459,7 @@ class EnvTest(unittest.TestCase):
         self.assertEqual(config.parse_environment(environment), environment)
 
     def test_parse_environment_invalid(self):
-        with self.assertRaises(config.ConfigurationError):
+        with self.assertRaises(ConfigurationError):
             config.parse_environment('a=b')
 
     def test_parse_environment_empty(self):
@@ -519,7 +513,7 @@ class EnvTest(unittest.TestCase):
     def test_env_nonexistent_file(self):
         options = {'env_file': 'nonexistent.env'}
         self.assertRaises(
-            config.ConfigurationError,
+            ConfigurationError,
             lambda: make_service_dict('foo', options, 'tests/fixtures/env'),
         )
 
@@ -545,7 +539,7 @@ class EnvTest(unittest.TestCase):
 
         service_dict = config.load(
             config.ConfigDetails(
-                config={'foo': {'volumes': ['$HOSTENV:$CONTAINERENV']}},
+                config={'foo': {'build': '.', 'volumes': ['$HOSTENV:$CONTAINERENV']}},
                 working_dir="tests/fixtures/env",
                 filename=None,
             )
@@ -554,7 +548,7 @@ class EnvTest(unittest.TestCase):
 
         service_dict = config.load(
             config.ConfigDetails(
-                config={'foo': {'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']}},
+                config={'foo': {'build': '.', 'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']}},
                 working_dir="tests/fixtures/env",
                 filename=None,
             )
@@ -652,7 +646,7 @@ class ExtendsTest(unittest.TestCase):
             )
 
     def test_extends_validation_empty_dictionary(self):
-        with self.assertRaisesRegexp(config.ConfigurationError, 'service'):
+        with self.assertRaisesRegexp(ConfigurationError, 'service'):
             config.load(
                 config.ConfigDetails(
                     {
@@ -664,7 +658,7 @@ class ExtendsTest(unittest.TestCase):
             )
 
     def test_extends_validation_missing_service_key(self):
-        with self.assertRaisesRegexp(config.ConfigurationError, "u'service' is a required property"):
+        with self.assertRaisesRegexp(ConfigurationError, "u'service' is a required property"):
             config.load(
                 config.ConfigDetails(
                     {
@@ -677,7 +671,7 @@ class ExtendsTest(unittest.TestCase):
 
     def test_extends_validation_invalid_key(self):
         expected_error_msg = "Unsupported config option for 'web' service: 'rogue_key'"
-        with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg):
+        with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
             config.load(
                 config.ConfigDetails(
                     {
@@ -701,7 +695,7 @@ class ExtendsTest(unittest.TestCase):
         def load_config():
             return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
 
-        self.assertRaisesRegexp(config.ConfigurationError, 'file', load_config)
+        self.assertRaisesRegexp(ConfigurationError, 'file', load_config)
 
     def test_extends_validation_valid_config(self):
         service = config.load(
@@ -750,19 +744,19 @@ class ExtendsTest(unittest.TestCase):
                 }
             }, '.')
 
-        with self.assertRaisesRegexp(config.ConfigurationError, 'links'):
+        with self.assertRaisesRegexp(ConfigurationError, 'links'):
             other_config = {'web': {'links': ['db']}}
 
             with mock.patch.object(config, 'load_yaml', return_value=other_config):
                 print load_config()
 
-        with self.assertRaisesRegexp(config.ConfigurationError, 'volumes_from'):
+        with self.assertRaisesRegexp(ConfigurationError, 'volumes_from'):
             other_config = {'web': {'volumes_from': ['db']}}
 
             with mock.patch.object(config, 'load_yaml', return_value=other_config):
                 print load_config()
 
-        with self.assertRaisesRegexp(config.ConfigurationError, 'net'):
+        with self.assertRaisesRegexp(ConfigurationError, 'net'):
             other_config = {'web': {'net': 'container:db'}}
 
             with mock.patch.object(config, 'load_yaml', return_value=other_config):
@@ -804,7 +798,7 @@ class BuildPathTest(unittest.TestCase):
         self.abs_context_path = os.path.join(os.getcwd(), 'tests/fixtures/build-ctx')
 
     def test_nonexistent_path(self):
-        with self.assertRaises(config.ConfigurationError):
+        with self.assertRaises(ConfigurationError):
             config.load(
                 config.ConfigDetails(
                     {