Bug: Inaccurate Error Messages When Dict Keys Require Complex Validation

[kennytrytek-wf]

>>> from voluptuous import *
>>> s = Schema({All('abc', Coerce(int)): 1})
>>> s({'abc': 1})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kennytrytek/virtual_env/skynet-cli/lib/python2.7/site-packages/voluptuous.py", line 333, in __call__
    return self._compiled([], data)
  File "/Users/kennytrytek/virtual_env/skynet-cli/lib/python2.7/site-packages/voluptuous.py", line 631, in validate_dict
    return base_validate(path, iteritems(data), out)
  File "/Users/kennytrytek/virtual_env/skynet-cli/lib/python2.7/site-packages/voluptuous.py", line 467, in validate_mapping
    raise MultipleInvalid(errors)
voluptuous.MultipleInvalid: extra keys not allowed @ data['abc']

The real error is that Coerce fails because 'abc' isn't a valid int, but it gets clobbered by the extra keys error.

[markgollnick-wf]

This also prevents errors raised by custom validators from being seen, which is not very helpful in discerning what went wrong with some data:

>>> import voluptuous as v
>>> def V(s='abc'):
...     def f(x):
...         if x == s:
...             return x
...         raise v.Invalid('String "{x}" is not "{s}"'.format(x=x, s=s))
...     return f
... 
>>> v.Schema({V(): '123'})({'abc': '123'})
{'abc': '123'}
>>> v.Schema({V(): '123'})({'abcd': '123'})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 333, in __call__
    return self._compiled([], data)
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 631, in validate_dict
    return base_validate(path, iteritems(data), out)
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 467, in validate_mapping
    raise MultipleInvalid(errors)
voluptuous.MultipleInvalid: extra keys not allowed @ data['abcd']

It works in the values case, however:

>>> v.Schema({'123': V()})({'123': 'abc'})
{'123': 'abc'}
>>> v.Schema({'123': V()})({'123': 'abcd'})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 333, in __call__
    return self._compiled([], data)
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 631, in validate_dict
    return base_validate(path, iteritems(data), out)
  File "/Users/markgollnick/.virtualenvs/py27/lib/python2.7/site-packages/voluptuous.py", line 467, in validate_mapping
    raise MultipleInvalid(errors)
voluptuous.MultipleInvalid: String "abcd" is not "abc" for dictionary value @ data['123']

[kennytrytek-wf]

Workaround:

class DictKeyInvalid(Invalid):
    """Dict key does not match the given schema."""


def DictKey(schema):
    """
    Dict key does not match the given schema.

    :type schema: Schema
    :param schema: Schema with which to validate each dict key.

    >>> schema = All(DictKey(All(str, Coerce(int))), {str: int})
    >>> schema({'123': 123, 'abc': 'abc'})
    voluptuous.CoerceInvalid: expected int
    >>> schema({'123': 123, '456': 456})
    {'123': 123, '456': 456}
    """
    def f(x):
        if not x:
            return x
        for k in x.keys():
            schema(k)
        return x
    return f

See DictKey docstring for examples. It's not pretty, but it does the job with an understandable error message.

[alecthomas]

This is not strictly speaking a bug. For a dictionary, Voluptuous will attempt to match each key against all keys in the schema. If multiple keys fail, which error should it report?

For example, in the following schema, there are two possible keys that could report errors. Voluptuous does not know which one to report, so it reports the most generic error.

>>> s = Schema({All(str, Coerce(int)): 1, 'bar': int})
>>> s({'foo': 1})
...
MultipleInvalid: extra keys not allowed @ data['asd']

In your specific case where there is only one key it would be possible to report a deeper error. But it's arguable whether that error is "more" correct than "extra keys not allowed".

I do understand that this is perhaps not what one would expect, but if you can propose a general solution that works well in all cases I'd be happy to consider it.

[alecthomas]

Also see #114 and probably others.

[kennytrytek-wf]

@alecthomas, thank you for your response. Is it possible to enumerate the varying cases you have in mind? I updated your example slightly to better illustrate what kind of response is currently given versus what I would expect to be the error message.

Current behavior:

>>> s = Schema({All(str, Coerce(int), msg='dict key must be str that coerces to int'): 1, 'bar': int})
>>> s({'foo': 1})
Traceback (most recent call last):
 ...
voluptuous.MultipleInvalid: extra keys not allowed @ data['foo']

Desired behavior:

>>> s = Schema({All(str, Coerce(int), msg='dict key must be str that coerces to int'): 1, 'bar': int})
>>> s({'foo': 1})
Traceback (most recent call last):
 ...
voluptuous.MultipleInvalid: dict key must be a str that coerces to int @ data['foo']

Or, with the original example, which doesn't provide exactly the right message, but is closer than "extra keys not allowed", and can be enhanced with a custom message, as above:

>>> s = Schema({All(str, Coerce(int)): 1, 'bar': int})
>>> s({'foo': 1})
Traceback (most recent call last):
 ...
voluptuous.MultipleInvalid: expected int @ data['foo']

Where "extra keys" makes sense:

>>> s = Schema({Optional('abc'): 1})
>>> s({'def': 1})
Traceback (most recent call last):
 ...
voluptuous.MultipleInvalid: extra keys not allowed @ data['def']

Do you think this behavior could be achieved? Is there a case I'm not seeing, or something I'm misunderstanding?

UPDATE:
Reading through this again, I think I see your point.

s = Schema({Coerce(int): 1, Coerce(str): 'a'})
s({True: 'a', False: 1})

Which error message should be raised? Expected int, or expected str? Since it doesn't match any dict keys, it makes sense for the message to read, "extra keys not allowed," since it's not known which key the user was attempting to match.

I think my use case is very narrow, and only applies to dicts that have a single schema for dict keys where extra keys are not allowed, e.g. s = Schema({Coerce(int): 1}). In that case, it is desirable that the deepest error message be raised, since there's no other possibility. However, if you don't feel like adding that special case, please close this issue (although I think #114 is a distinct issue, and only tangentially related in that both involve dict keys).

[alecthomas]

Yes, your update is exactly correct. It's tricky. The idea of returning the deepest error message found is what Voluptuous does actually try to do, but I think in this case it doesn't work because All() is an independent Schema (ie. it doesn't know about the nested "str, Coerce" validations). What might need to be done is pass the "depth" to the Invalid exception, or track it in some other way. Then when the Schema receives an exception with depth information it can track it correctly.