From 32cd404c8c1bb31d04aa2845ca7fa15deae9b00b Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 8 Sep 2015 11:54:03 +0100 Subject: [PATCH 1/4] Remove redundant oneOf definitions For simple definitions where a field can be multiple types, we can specify the allowed types in an array. It's simpler and clearer. This is only applicable to *simple* definitions, like number, string, list, object without any other constraints. Signed-off-by: Mazz Mosley --- compose/config/fields_schema.json | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index 5c7322517..6277b57d6 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -24,12 +24,7 @@ ] }, "container_name": {"type": "string"}, - "cpu_shares": { - "oneOf": [ - {"type": "number"}, - {"type": "string"} - ] - }, + "cpu_shares": {"type": ["number", "string"]}, "cpuset": {"type": "string"}, "devices": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, "dns": {"$ref": "#/definitions/string_or_list"}, @@ -74,18 +69,8 @@ "log_opt": {"type": "object"}, "mac_address": {"type": "string"}, - "mem_limit": { - "oneOf": [ - {"type": "number"}, - {"type": "string"} - ] - }, - "memswap_limit": { - "oneOf": [ - {"type": "number"}, - {"type": "string"} - ] - }, + "mem_limit": {"type": ["number", "string"]}, + "memswap_limit": {"type": ["number", "string"]}, "name": {"type": "string"}, "net": {"type": "string"}, "pid": {"type": ["string", "null"]}, From 418ec5336b0c7848087b38b3d2617b2ce340c67d Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 8 Sep 2015 12:01:47 +0100 Subject: [PATCH 2/4] Improved messaging for simple type validator English language is a tricky old thing and I've pulled out the validator type parsing so that we can prefix our validator types with the correct article, 'an' or 'a'. Doing a bit of extra hard work to ensure the error message is clear and well constructed english. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 41 ++++++++++++++++++++++++++++++------ tests/unit/config_test.py | 15 +++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 632bdf03b..44763fda3 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -117,6 +117,38 @@ def process_errors(errors, service_name=None): else: return str(schema['type']) + 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. + """ + pre_msg_type_prefix = "a" + last_msg_type_prefix = "a" + types_requiring_an = ["array", "object"] + + if isinstance(validator, list): + last_type = validator.pop() + types_from_validator = ", ".join(validator) + + if validator[0] in types_requiring_an: + pre_msg_type_prefix = "an" + + if last_type in types_requiring_an: + last_msg_type_prefix = "an" + + msg = "{} {} or {} {}".format( + pre_msg_type_prefix, + types_from_validator, + last_msg_type_prefix, + last_type + ) + else: + if validator in types_requiring_an: + pre_msg_type_prefix = "an" + msg = "{} {}".format(pre_msg_type_prefix, validator) + + return msg + root_msgs = [] invalid_keys = [] required = [] @@ -176,19 +208,16 @@ def process_errors(errors, service_name=None): service_name, config_key, valid_type_msg) ) elif error.validator == 'type': - msg = "a" - if error.validator_value == "array": - msg = "an" + msg = _parse_valid_types_from_validator(error.validator_value) if len(error.path) > 0: config_key = " ".join(["'%s'" % k for k in error.path]) type_errors.append( "Service '{}' configuration key {} contains an invalid " - "type, it should be {} {}".format( + "type, it should be {}".format( service_name, config_key, - msg, - error.validator_value)) + msg)) else: root_msgs.append( "Service '{}' doesn\'t have any configuration options. " diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 870adcf81..90d7a6a26 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -269,6 +269,21 @@ class ConfigTest(unittest.TestCase): ) self.assertEqual(service[0]['entrypoint'], entrypoint) + def test_validation_message_for_invalid_type_when_multiple_types_allowed(self): + expected_error_msg = "Service 'web' configuration key 'mem_limit' contains an invalid type, it should be a number or a string" + + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + {'web': { + 'image': 'busybox', + 'mem_limit': ['incorrect'] + }}, + 'working_dir', + 'filename.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) From cf7b59538572c3cb49c8e54aec3e7a7f0da06b42 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 9 Sep 2015 13:18:52 +0100 Subject: [PATCH 3/4] Improve error messages from oneOf schema errors oneOf schema ValidationError takes a little more work to parse and pull out more detail so we can give a better error message back to the user. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 51 +++++++++++++++++++++++++----------- tests/unit/config_test.py | 5 ++-- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 44763fda3..971cfe371 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -107,16 +107,6 @@ def process_errors(errors, service_name=None): def _clean_error_message(message): return message.replace("u'", "'") - def _parse_valid_types_from_schema(schema): - """ - Our defined types using $ref in the schema require some extra parsing - retrieve a helpful type for error message display. - """ - if '$ref' in schema: - return schema['$ref'].replace("#/definitions/", "").replace("_", " ") - else: - return str(schema['type']) - def _parse_valid_types_from_validator(validator): """ A validator value can be either an array of valid types or a string of @@ -149,6 +139,39 @@ def process_errors(errors, service_name=None): return msg + def _parse_oneof_validator(error): + """ + 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. + """ + constraint = [context for context in error.context if len(context.path) > 0] + if constraint: + valid_types = _parse_valid_types_from_validator(constraint[0].validator_value) + msg = "contains {}, which is an invalid type, it should be {}".format( + constraint[0].instance, + valid_types + ) + return msg + + uniqueness = [context for context in error.context if context.validator == 'uniqueItems'] + if uniqueness: + msg = "contains non unique items, please remove duplicates from {}".format( + uniqueness[0].instance + ) + return msg + + types = [context.validator_value for context in error.context if context.validator == 'type'] + if len(types) == 1: + valid_types = _parse_valid_types_from_validator(types[0]) + else: + valid_types = _parse_valid_types_from_validator(types) + + msg = "contains an invalid type, it should be {}".format(valid_types) + + return msg + root_msgs = [] invalid_keys = [] required = [] @@ -200,12 +223,10 @@ def process_errors(errors, service_name=None): required.append(_clean_error_message(error.message)) elif error.validator == 'oneOf': config_key = error.path[0] + msg = _parse_oneof_validator(error) - valid_types = [_parse_valid_types_from_schema(schema) for schema in error.schema['oneOf']] - valid_type_msg = " or ".join(valid_types) - - type_errors.append("Service '{}' configuration key '{}' contains an invalid type, valid types are {}".format( - service_name, config_key, valid_type_msg) + type_errors.append("Service '{}' configuration key '{}' {}".format( + service_name, config_key, msg) ) elif error.validator == 'type': msg = _parse_valid_types_from_validator(error.validator_value) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 90d7a6a26..f55789207 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -183,7 +183,8 @@ class ConfigTest(unittest.TestCase): ) def test_invalid_list_of_strings_format(self): - expected_error_msg = "'command' contains an invalid type, valid types are string or array" + expected_error_msg = "Service 'web' configuration key 'command' contains 1" + expected_error_msg += ", which is an invalid type, it should be a string" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( @@ -222,7 +223,7 @@ class ConfigTest(unittest.TestCase): ) def test_config_extra_hosts_list_of_dicts_validation_error(self): - expected_error_msg = "Service 'web' configuration key 'extra_hosts' contains an invalid type" + expected_error_msg = "key 'extra_hosts' contains {'somehost': '162.242.195.82'}, which is an invalid type, it should be a string" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( From 1007ad0f868e00c61267e7b9eb059b5b811a84d9 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 14 Sep 2015 17:23:05 +0100 Subject: [PATCH 4/4] Refactor to simplify _parse_valid_types Signed-off-by: Mazz Mosley --- compose/config/validation.py | 43 +++++++++++++++--------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 971cfe371..dc630adf2 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -95,6 +95,12 @@ def get_unsupported_config_msg(service_name, error_key): return msg +def anglicize_validator(validator): + if validator in ["array", "object"]: + return 'an ' + validator + return 'a ' + validator + + def process_errors(errors, service_name=None): """ jsonschema gives us an error tree full of information to explain what has @@ -112,30 +118,20 @@ def process_errors(errors, service_name=None): 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. """ - pre_msg_type_prefix = "a" - last_msg_type_prefix = "a" - types_requiring_an = ["array", "object"] - if isinstance(validator, list): - last_type = validator.pop() - types_from_validator = ", ".join(validator) + if len(validator) >= 2: + first_type = anglicize_validator(validator[0]) + last_type = anglicize_validator(validator[-1]) + types_from_validator = "{}{}".format(first_type, ", ".join(validator[1:-1])) - if validator[0] in types_requiring_an: - pre_msg_type_prefix = "an" - - if last_type in types_requiring_an: - last_msg_type_prefix = "an" - - msg = "{} {} or {} {}".format( - pre_msg_type_prefix, - types_from_validator, - last_msg_type_prefix, - last_type - ) + msg = "{} or {}".format( + types_from_validator, + last_type + ) + else: + msg = "{}".format(anglicize_validator(validator[0])) else: - if validator in types_requiring_an: - pre_msg_type_prefix = "an" - msg = "{} {}".format(pre_msg_type_prefix, validator) + msg = "{}".format(anglicize_validator(validator)) return msg @@ -163,10 +159,7 @@ def process_errors(errors, service_name=None): return msg types = [context.validator_value for context in error.context if context.validator == 'type'] - if len(types) == 1: - valid_types = _parse_valid_types_from_validator(types[0]) - else: - valid_types = _parse_valid_types_from_validator(types) + valid_types = _parse_valid_types_from_validator(types) msg = "contains an invalid type, it should be {}".format(valid_types)