diff --git a/CHANGELOG.md b/CHANGELOG.md index 19d97b9..9cd45a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Fixes**: * [#539](https://github.com/alecthomas/voluptuous/pull/539): Raise `Invalid` instead of leaking `TypeError`/`ValueError` for non-numeric input to the `Number` validator +* [#484](https://github.com/alecthomas/voluptuous/issues/484): Emit `required key not provided` errors in a stable schema-declaration order ## [0.16.0] diff --git a/voluptuous/schema_builder.py b/voluptuous/schema_builder.py index e51dceb..f13ec69 100644 --- a/voluptuous/schema_builder.py +++ b/voluptuous/schema_builder.py @@ -240,23 +240,25 @@ def _compile_mapping(self, schema, invalid_msg=None): """Create validator for given mapping.""" invalid_msg = invalid_msg or 'mapping value' - # Keys that may be required - all_required_keys = set( - key + # Keys that may be required. A dict is used as an insertion-ordered set + # so that "required key not provided" errors are emitted in a stable, + # schema-declaration order rather than an arbitrary set order (#484). + all_required_keys = { + key: None for key in schema if key is not Extra and ( (self.required and not isinstance(key, (Optional, Remove))) or isinstance(key, Required) ) - ) + } # Complex required keys that need special validation - complex_required_keys = set( - key + complex_required_keys = { + key: None for key in all_required_keys if isinstance(key, Required) and key.is_complex_key - ) + } # Keys that may have defaults all_default_keys = set( @@ -325,7 +327,7 @@ def validate_mapping(path, iterable, out): errors.append(er.RequiredFieldInvalid(msg, path + [complex_key])) else: # If at least one candidate key is present, mark this complex requirement as satisfied - required_keys.discard(complex_key) + required_keys.pop(complex_key, None) for key, value in key_value_map.items(): key_path = path + [key] remove_key = False @@ -376,12 +378,12 @@ def validate_mapping(path, iterable, out): # key, this means that the key was provided. # Discard the required key so it does not # create an additional, noisy exception. - required_keys.discard(skey) + required_keys.pop(skey, None) break # Key and value okay, mark as found in case it was # a Required() field. - required_keys.discard(skey) + required_keys.pop(skey, None) break else: diff --git a/voluptuous/tests/tests.py b/voluptuous/tests/tests.py index 5c96369..771d14b 100644 --- a/voluptuous/tests/tests.py +++ b/voluptuous/tests/tests.py @@ -95,6 +95,21 @@ def test_required(): schema({}) +def test_required_key_error_order_is_stable(): + """Missing-required-key errors follow schema declaration order (#484). + + Previously the required keys were stored in a set, so the emitted + ``MultipleInvalid`` errors came out in an arbitrary, hash-seed-dependent + order. Using several keys makes an accidental correct ordering on the old + code astronomically unlikely. + """ + keys = ['value1', 'value2', 'value3', 'value4', 'value5'] + schema = Schema({Required(k): str for k in keys}) + with pytest.raises(MultipleInvalid) as ctx: + schema({}) + assert [e.path[0] for e in ctx.value.errors] == keys + + def test_extra_with_required(): """Verify that Required does not break Extra.""" schema = Schema({Required('toaster'): str, Extra: object})