Refactor process_errors into smaller functions

So that it passed new max-complexity requirement

Signed-off-by: Daniel Nephin <dnephin@docker.com>

compose/config/validation.py

@@ -109,189 +109,169 @@ def anglicize_validator(validator):
     return 'a ' + validator
 
 
-def process_errors(errors, service_name=None):
+def handle_error_for_schema_with_id(error, service_name):
-    """
+    schema_id = error.schema['id']
-    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
+    if schema_id == 'fields_schema.json' and error.validator == 'additionalProperties':
-    helpful error messages that are relevant.
+        return "Invalid service name '{}' - only {} characters are allowed".format(
-    """
+            # The service_name is the key to the json object
-    def _parse_key_from_error_msg(error):
+            list(error.instance)[0],
+            VALID_NAME_CHARS)
+
+    if schema_id == '#/definitions/constraints':
+        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))
+        if 'image' not in error.instance and 'build' not in error.instance:
+            return (
+                "Service '{}' has neither an image nor a build path "
+                "specified. Exactly one must be provided.".format(service_name))
+        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 schema_id == '#/definitions/service':
+        if error.validator == 'additionalProperties':
+            invalid_config_key = parse_key_from_error_msg(error)
+            return get_unsupported_config_msg(service_name, invalid_config_key)
+
+
+def handle_generic_service_error(error, service_name):
+    config_key = " ".join("'%s'" % k for k in error.path)
+    msg_format = None
+    error_msg = error.message
+
+    if error.validator == 'oneOf':
+        msg_format = "Service '{}' configuration key {} {}"
+        error_msg = _parse_oneof_validator(error)
+
+    elif error.validator == 'type':
+        msg_format = ("Service '{}' configuration key {} contains an invalid "
+                      "type, it should be {}")
+        error_msg = _parse_valid_types_from_validator(error.validator_value)
+
+    # TODO: no test case for this branch, there are no config options
+    # which exercise this branch
+    elif error.validator == 'required':
+        msg_format = "Service '{}' configuration key '{}' is invalid, {}"
+
+    elif error.validator == 'dependencies':
+        msg_format = "Service '{}' configuration key '{}' is invalid: {}"
+        config_key = list(error.validator_value.keys())[0]
+        required_keys = ",".join(error.validator_value[config_key])
+        error_msg = "when defining '{}' you must set '{}' as well".format(
+            config_key,
+            required_keys)
+
+    elif error.path:
+        msg_format = "Service '{}' configuration key {} value {}"
+
+    if msg_format:
+        return msg_format.format(service_name, config_key, error_msg)
+
+    return error.message
+
+
+def parse_key_from_error_msg(error):
     return error.message.split("'")[1]
 
-    def _clean_error_message(message):
-        return message.replace("u'", "'")
 
-    def _parse_valid_types_from_validator(validator):
+def _parse_valid_types_from_validator(validator):
-        """
+    """A validator value can be either an array of valid types or a string of
-        A validator value can be either an array of valid types or a string of
     a valid type. Parse the valid types and prefix with the correct article.
     """
-        if isinstance(validator, list):
+    if not isinstance(validator, list):
-            if len(validator) >= 2:
+        return anglicize_validator(validator)
-                first_type = anglicize_validator(validator[0])
-                last_type = anglicize_validator(validator[-1])
-                types_from_validator = ", ".join([first_type] + validator[1:-1])
 
-                msg = "{} or {}".format(
+    if len(validator) == 1:
-                    types_from_validator,
+        return anglicize_validator(validator[0])
-                    last_type
-                )
-            else:
-                msg = "{}".format(anglicize_validator(validator[0]))
-        else:
-            msg = "{}".format(anglicize_validator(validator))
 
-        return msg
+    return "{}, or {}".format(
+        ", ".join([anglicize_validator(validator[0])] + validator[1:-1]),
+        anglicize_validator(validator[-1]))
 
-    def _parse_oneof_validator(error):
+
-        """
+def _parse_oneof_validator(error):
-        oneOf has multiple schemas, so we need to reason about which schema, sub
+    """oneOf has multiple schemas, so we need to reason about which schema, sub
     schema or constraint the validation is failing on.
     Inspecting the context value of a ValidationError gives us information about
     which sub schema failed and which kind of error it is.
     """
-        required = [context for context in error.context if context.validator == 'required']
+    types = []
-        if required:
+    for context in error.context:
-            return required[0].message
 
-        additionalProperties = [context for context in error.context if context.validator == 'additionalProperties']
+        if context.validator == 'required':
-        if additionalProperties:
+            return context.message
-            invalid_config_key = _parse_key_from_error_msg(additionalProperties[0])
+
+        if context.validator == 'additionalProperties':
+            invalid_config_key = parse_key_from_error_msg(context)
             return "contains unsupported option: '{}'".format(invalid_config_key)
 
-        constraint = [context for context in error.context if len(context.path) > 0]
+        if context.path:
-        if constraint:
+            invalid_config_key = " ".join(
-            valid_types = _parse_valid_types_from_validator(constraint[0].validator_value)
+                "'{}' ".format(fragment) for fragment in context.path
-            invalid_config_key = "".join(
-                "'{}' ".format(fragment) for fragment in constraint[0].path
                 if isinstance(fragment, six.string_types)
             )
-            msg = "{}contains {}, which is an invalid type, it should be {}".format(
+            return "{}contains {}, which is an invalid type, it should be {}".format(
                 invalid_config_key,
-                constraint[0].instance,
+                context.instance,
-                valid_types
+                _parse_valid_types_from_validator(context.validator_value))
-            )
-            return msg
 
-        uniqueness = [context for context in error.context if context.validator == 'uniqueItems']
+        if context.validator == 'uniqueItems':
-        if uniqueness:
+            return "contains non unique items, please remove duplicates from {}".format(
-            msg = "contains non unique items, please remove duplicates from {}".format(
+                context.instance)
-                uniqueness[0].instance
+
-            )
+        if context.validator == 'type':
-            return msg
+            types.append(context.validator_value)
 
-        types = [context.validator_value for context in error.context if context.validator == 'type']
     valid_types = _parse_valid_types_from_validator(types)
+    return "contains an invalid type, it should be {}".format(valid_types)
 
-        msg = "contains an invalid type, it should be {}".format(valid_types)
 
-        return msg
+def process_errors(errors, service_name=None):
-
+    """jsonschema gives us an error tree full of information to explain what has
-    root_msgs = []
+    gone wrong. Process each error and pull out relevant information and re-write
-    invalid_keys = []
+    helpful error messages that are relevant.
-    required = []
+    """
-    type_errors = []
+    def format_error_message(error, service_name):
-    other_errors = []
+        if not service_name and error.path:
-
-    for error in errors:
-        # handle root level errors
-        if len(error.path) == 0 and not service_name:
-            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(_clean_error_message(error.message))
-
-        else:
-            if not service_name:
             # field_schema errors will have service name on the path
-                service_name = error.path[0]
+            service_name = error.path.popleft()
-                error.path.popleft()
-            else:
-                # service_schema errors have the service name passed in, as that
-                # is not available on error.path or necessarily error.instance
-                service_name = service_name
 
-            if error.validator == 'additionalProperties':
+        if 'id' in error.schema:
-                invalid_config_key = _parse_key_from_error_msg(error)
+            error_msg = handle_error_for_schema_with_id(error, service_name)
-                invalid_keys.append(get_unsupported_config_msg(service_name, invalid_config_key))
+            if error_msg:
-            elif error.validator == 'anyOf':
+                return error_msg
-                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))
-                elif 'image' in error.instance and 'dockerfile' in error.instance:
-                    required.append(
-                        "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))
-                else:
-                    required.append(_clean_error_message(error.message))
-            elif error.validator == 'oneOf':
-                config_key = error.path[0]
-                msg = _parse_oneof_validator(error)
 
-                type_errors.append("Service '{}' configuration key '{}' {}".format(
+        return handle_generic_service_error(error, service_name)
-                    service_name, config_key, msg)
-                )
-            elif error.validator == 'type':
-                msg = _parse_valid_types_from_validator(error.validator_value)
 
-                if len(error.path) > 0:
+    return '\n'.join(format_error_message(error, service_name) for error in errors)
-                    config_key = " ".join(["'%s'" % k for k in error.path])
-                    type_errors.append(
-                        "Service '{}' configuration key {} contains an invalid "
-                        "type, it should be {}".format(
-                            service_name,
-                            config_key,
-                            msg))
-                else:
-                    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(service_name))
-            elif error.validator == 'required':
-                config_key = error.path[0]
-                required.append(
-                    "Service '{}' option '{}' is invalid, {}".format(
-                        service_name,
-                        config_key,
-                        _clean_error_message(error.message)))
-            elif error.validator == 'dependencies':
-                dependency_key = list(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))
-            else:
-                config_key = " ".join(["'%s'" % k for k in error.path])
-                err_msg = "Service '{}' configuration key {} value {}".format(service_name, config_key, error.message)
-                other_errors.append(err_msg)
-
-    return "\n".join(root_msgs + invalid_keys + required + type_errors + other_errors)
 
 
 def validate_against_fields_schema(config):
-    schema_filename = "fields_schema.json"
+    return _validate_against_schema(
-    format_checkers = ["ports", "environment"]
+        config,
-    return _validate_against_schema(config, schema_filename, format_checkers)
+        "fields_schema.json",
+        ["ports", "environment"])
 
 
 def validate_against_service_schema(config, service_name):
-    schema_filename = "service_schema.json"
+    return _validate_against_schema(
-    format_checkers = ["ports"]
+        config,
-    return _validate_against_schema(config, schema_filename, format_checkers, service_name)
+        "service_schema.json",
+        ["ports"],
+        service_name)
 
 
-def _validate_against_schema(config, schema_filename, format_checker=[], service_name=None):
+def _validate_against_schema(
+        config,
+        schema_filename,
+        format_checker=(),
+        service_name=None):
     config_source_dir = os.path.dirname(os.path.abspath(__file__))
 
     if sys.platform == "win32":

tests/unit/config/config_test.py

@@ -867,7 +867,7 @@ class MemoryOptionsTest(unittest.TestCase):
         a mem_limit
         """
         expected_error_msg = (
-            "Invalid 'memswap_limit' configuration for 'foo' service: when "
+            "Service 'foo' configuration key 'memswap_limit' is invalid: when "
             "defining 'memswap_limit' you must set 'mem_limit' as well"
         )
         with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):