Normalise/fix config field designators in validation messages

- Instead of "Service 'web' configuration key 'image'", just say
  "web.image"

- Fix the "Service 'services'" bug in the v2 file format

Signed-off-by: Aanand Prasad <[email protected]>

compose/config/validation.py

@@ -174,8 +174,8 @@ def validate_depends_on(service_config, service_names):
                 "undefined.".format(s=service_config, dep=dependency))
 
 
-def get_unsupported_config_msg(service_name, error_key):
-    msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key)
+def get_unsupported_config_msg(path, error_key):
+    msg = "Unsupported config option for {}: '{}'".format(path_string(path), error_key)
     if error_key in DOCKER_CONFIG_HINTS:
         msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key])
     return msg
@@ -191,7 +191,7 @@ def is_service_dict_schema(schema_id):
     return schema_id == 'fields_schema_v1.json' or schema_id == '#/properties/services'
 
 
-def handle_error_for_schema_with_id(error, service_name):
+def handle_error_for_schema_with_id(error, path):
     schema_id = error.schema['id']
 
     if is_service_dict_schema(schema_id) and error.validator == 'additionalProperties':
@@ -215,62 +215,64 @@ def handle_error_for_schema_with_id(error, service_name):
         # TODO: only applies to v1
         if 'image' in error.instance and context:
             return (
-                "Service '{}' has both an image and build path specified. "
+                "{} 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))
+                "image, not both.".format(path_string(path)))
         if 'image' not in error.instance and not context:
             return (
-                "Service '{}' has neither an image nor a build path "
-                "specified. At least one must be provided.".format(service_name))
+                "{} 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 (
-                "Service '{}' has both an image and alternate Dockerfile. "
+                "{} 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))
+                "image, not both.".format(path_string(path)))
 
     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)
+            return get_unsupported_config_msg(path, invalid_config_key)
 
 
-def handle_generic_service_error(error, service_name):
-    config_key = " ".join("'%s'" % k for k in error.path)
+def handle_generic_service_error(error, path):
     msg_format = None
     error_msg = error.message
 
     if error.validator == 'oneOf':
-        msg_format = "Service '{}' configuration key {} {}"
-        error_msg = _parse_oneof_validator(error)
+        msg_format = "{path} {msg}"
+        config_key, error_msg = _parse_oneof_validator(error)
+        if config_key:
+            path.append(config_key)
 
     elif error.validator == 'type':
-        msg_format = ("Service '{}' configuration key {} contains an invalid "
-                      "type, it should be {}")
+        msg_format = "{path} contains an invalid type, it should be {msg}"
         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, {}"
+        msg_format = "{path} is invalid, {msg}"
 
     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])
+
+        msg_format = "{path} is invalid: {msg}"
+        path.append(config_key)
         error_msg = "when defining '{}' you must set '{}' as well".format(
             config_key,
             required_keys)
 
     elif error.cause:
         error_msg = six.text_type(error.cause)
-        msg_format = "Service '{}' configuration key {} is invalid: {}"
+        msg_format = "{path} is invalid: {msg}"
 
     elif error.path:
-        msg_format = "Service '{}' configuration key {} value {}"
+        msg_format = "{path} value {msg}"
 
     if msg_format:
-        return msg_format.format(service_name, config_key, error_msg)
+        return msg_format.format(path=path_string(path), msg=error_msg)
 
     return error.message
 
@@ -279,6 +281,10 @@ def parse_key_from_error_msg(error):
     return error.message.split("'")[1]
 
 
+def path_string(path):
+    return ".".join(c for c in path if isinstance(c, six.string_types))
+
+
 def _parse_valid_types_from_validator(validator):
     """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.
@@ -304,52 +310,52 @@ def _parse_oneof_validator(error):
     for context in error.context:
 
         if context.validator == 'required':
-            return context.message
+            return (None, context.message)
 
         if context.validator == 'additionalProperties':
             invalid_config_key = parse_key_from_error_msg(context)
-            return "contains unsupported option: '{}'".format(invalid_config_key)
+            return (None, "contains unsupported option: '{}'".format(invalid_config_key))
 
         if context.path:
-            invalid_config_key = " ".join(
-                "'{}' ".format(fragment) for fragment in context.path
-                if isinstance(fragment, six.string_types)
+            return (
+                path_string(context.path),
+                "contains {}, which is an invalid type, it should be {}".format(
+                    json.dumps(context.instance),
+                    _parse_valid_types_from_validator(context.validator_value)),
             )
-            return "{}contains {}, which is an invalid type, it should be {}".format(
-                invalid_config_key,
-                # Always print the json repr of the invalid value
-                json.dumps(context.instance),
-                _parse_valid_types_from_validator(context.validator_value))
 
         if context.validator == 'uniqueItems':
-            return "contains non unique items, please remove duplicates from {}".format(
-                context.instance)
+            return (
+                None,
+                "contains non unique items, please remove duplicates from {}".format(
+                    context.instance),
+            )
 
         if context.validator == 'type':
             types.append(context.validator_value)
 
     valid_types = _parse_valid_types_from_validator(types)
-    return "contains an invalid type, it should be {}".format(valid_types)
+    return (None, "contains an invalid type, it should be {}".format(valid_types))
 
 
-def process_errors(errors, service_name=None):
+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.
     """
-    def format_error_message(error, service_name):
-        if not service_name and error.path:
-            # field_schema errors will have service name on the path
-            service_name = error.path.popleft()
+    path_prefix = path_prefix or []
+
+    def format_error_message(error):
+        path = path_prefix + list(error.path)
 
         if 'id' in error.schema:
-            error_msg = handle_error_for_schema_with_id(error, service_name)
+            error_msg = handle_error_for_schema_with_id(error, path)
             if error_msg:
                 return error_msg
 
-        return handle_generic_service_error(error, service_name)
+        return handle_generic_service_error(error, path)
 
-    return '\n'.join(format_error_message(error, service_name) for error in errors)
+    return '\n'.join(format_error_message(error) for error in errors)
 
 
 def validate_against_fields_schema(config_file):
@@ -366,14 +372,14 @@ def validate_against_service_schema(config, service_name, version):
         config,
         "service_schema_v{0}.json".format(version),
         format_checker=["ports"],
-        service_name=service_name)
+        path_prefix=[service_name])
 
 
 def _validate_against_schema(
         config,
         schema_filename,
         format_checker=(),
-        service_name=None,
+        path_prefix=None,
         filename=None):
     config_source_dir = os.path.dirname(os.path.abspath(__file__))
 
@@ -399,7 +405,7 @@ def _validate_against_schema(
     if not errors:
         return
 
-    error_msg = process_errors(errors, service_name)
+    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,

tests/unit/config/config_test.py

@@ -253,15 +253,31 @@ class ConfigTest(unittest.TestCase):
             assert 'Invalid service name \'%s\'' % invalid_name in exc.exconly()
 
     def test_load_with_invalid_field_name(self):
-        config_details = build_config_details(
-            {'web': {'image': 'busybox', 'name': 'bogus'}},
-            'working_dir',
-            'filename.yml')
         with pytest.raises(ConfigurationError) as exc:
-            config.load(config_details)
-        error_msg = "Unsupported config option for 'web' service: 'name'"
-        assert error_msg in exc.exconly()
-        assert "Validation failed in file 'filename.yml'" in exc.exconly()
+            config.load(build_config_details(
+                {
+                    'version': 2,
+                    'services': {
+                        'web': {'image': 'busybox', 'name': 'bogus'},
+                    }
+                },
+                'working_dir',
+                'filename.yml',
+            ))
+
+        assert "Unsupported config option for services.web: 'name'" in exc.exconly()
+
+    def test_load_with_invalid_field_name_v1(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details(
+                {
+                    'web': {'image': 'busybox', 'name': 'bogus'},
+                },
+                'working_dir',
+                'filename.yml',
+            ))
+
+        assert "Unsupported config option for web: 'name'" in exc.exconly()
 
     def test_load_invalid_service_definition(self):
         config_details = build_config_details(
@@ -645,6 +661,39 @@ class ConfigTest(unittest.TestCase):
             }, 'tests/fixtures/build-ctx'))
             assert "Service 'Foo' contains uppercase characters" in exc.exconly()
 
+    def test_invalid_config_v1(self):
+        with pytest.raises(ConfigurationError) as excinfo:
+            config.load(
+                build_config_details(
+                    {
+                        'foo': {'image': 1},
+                    },
+                    'tests/fixtures/extends',
+                    'filename.yml'
+                )
+            )
+
+        assert "foo.image contains an invalid type, it should be a string" \
+            in excinfo.exconly()
+
+    def test_invalid_config_v2(self):
+        with pytest.raises(ConfigurationError) as excinfo:
+            config.load(
+                build_config_details(
+                    {
+                        'version': 2,
+                        'services': {
+                            'foo': {'image': 1},
+                        },
+                    },
+                    'tests/fixtures/extends',
+                    'filename.yml'
+                )
+            )
+
+        assert "services.foo.image contains an invalid type, it should be a string" \
+            in excinfo.exconly()
+
     def test_invalid_config_build_and_image_specified_v1(self):
         with pytest.raises(ConfigurationError) as excinfo:
             config.load(
@@ -657,7 +706,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'foo' has both an image and build path specified." in excinfo.exconly()
+        assert "foo has both an image and build path specified." in excinfo.exconly()
 
     def test_invalid_config_type_should_be_an_array(self):
         with pytest.raises(ConfigurationError) as excinfo:
@@ -671,7 +720,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'foo' configuration key 'links' contains an invalid type, it should be an array" \
+        assert "foo.links contains an invalid type, it should be an array" \
             in excinfo.exconly()
 
     def test_invalid_config_not_a_dictionary(self):
@@ -713,7 +762,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'web' configuration key 'command' contains 1, which is an invalid type, it should be a string" \
+        assert "web.command contains 1, which is an invalid type, it should be a string" \
             in excinfo.exconly()
 
     def test_load_config_dockerfile_without_build_raises_error_v1(self):
@@ -725,7 +774,7 @@ class ConfigTest(unittest.TestCase):
                 }
             }))
 
-        assert "Service 'web' has both an image and alternate Dockerfile." in exc.exconly()
+        assert "web has both an image and alternate Dockerfile." in exc.exconly()
 
     def test_config_extra_hosts_string_raises_validation_error(self):
         with pytest.raises(ConfigurationError) as excinfo:
@@ -740,7 +789,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'web' configuration key 'extra_hosts' contains an invalid type" \
+        assert "web.extra_hosts contains an invalid type" \
             in excinfo.exconly()
 
     def test_config_extra_hosts_list_of_dicts_validation_error(self):
@@ -759,7 +808,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-        assert "key 'extra_hosts' contains {\"somehost\": \"162.242.195.82\"}, " \
+        assert "web.extra_hosts contains {\"somehost\": \"162.242.195.82\"}, " \
                "which is an invalid type, it should be a string" \
             in excinfo.exconly()
 
@@ -781,7 +830,7 @@ class ConfigTest(unittest.TestCase):
                 'working_dir',
                 'filename.yml'))
 
-        assert "Service 'web' configuration key 'ulimits' 'nofile' contains unsupported option: 'not_soft_or_hard'" \
+        assert "web.ulimits.nofile contains unsupported option: 'not_soft_or_hard'" \
             in exc.exconly()
 
     def test_config_ulimits_required_keys_validation_error(self):
@@ -795,7 +844,7 @@ class ConfigTest(unittest.TestCase):
                 },
                 'working_dir',
                 'filename.yml'))
-        assert "Service 'web' configuration key 'ulimits' 'nofile'" in exc.exconly()
+        assert "web.ulimits.nofile" in exc.exconly()
         assert "'hard' is a required property" in exc.exconly()
 
     def test_config_ulimits_soft_greater_than_hard_error(self):
@@ -896,7 +945,7 @@ class ConfigTest(unittest.TestCase):
                     'extra_hosts': "www.example.com: 192.168.0.17",
                 }
             }))
-        assert "'extra_hosts' contains an invalid type" in exc.exconly()
+        assert "web.extra_hosts contains an invalid type" in exc.exconly()
 
     def test_validate_extra_hosts_invalid_list(self):
         with pytest.raises(ConfigurationError) as exc:
@@ -1593,7 +1642,7 @@ class MemoryOptionsTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'foo' configuration key 'memswap_limit' is invalid: when defining " \
+        assert "foo.memswap_limit is invalid: when defining " \
                "'memswap_limit' you must set 'mem_limit' as well" \
             in excinfo.exconly()
 
@@ -1905,7 +1954,7 @@ class ExtendsTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'web' configuration key 'extends' contains unsupported option: 'rogue_key'" \
+        assert "web.extends contains unsupported option: 'rogue_key'" \
             in excinfo.exconly()
 
     def test_extends_validation_sub_property_key(self):
@@ -1926,7 +1975,7 @@ class ExtendsTest(unittest.TestCase):
                 )
             )
 
-        assert "Service 'web' configuration key 'extends' 'file' contains 1, which is an invalid type, it should be a string" \
+        assert "web.extends.file contains 1, which is an invalid type, it should be a string" \
             in excinfo.exconly()
 
     def test_extends_validation_no_file_key_no_filename_set(self):
@@ -1956,7 +2005,7 @@ class ExtendsTest(unittest.TestCase):
         with pytest.raises(ConfigurationError) as exc:
             load_from_filename('tests/fixtures/extends/service-with-invalid-schema.yml')
         assert (
-            "Service 'myweb' has neither an image nor a build path specified" in
+            "myweb has neither an image nor a build path specified" in
             exc.exconly()
         )