keleshev / schema

Schema validation just got Pythonic
MIT License
2.87k stars 214 forks source link

General changes to error handling in schema #241

Open kmaork opened 4 years ago

kmaork commented 4 years ago

Problem

In my opinion, the way SchemaErrors are produced can be improved. There are a few problems with the current implementation:

  1. Error string formatting logic is duplicated throughout the code, does not exist in all needed cases (e.g. callable validators).
  2. SchemaError objects do not contain a reference to the data that validation failed on, which could be useful to users catching the exception.
  3. Many functions throw SchemaErrors in a context in which a SchemaError is handled, which causes a long traceback containing while handling this exception, another exception has occured: .... This stack trace doesn't contain any useful information because we collect the autos and errors from the handled SchemaError exceptions anyway.
  4. SchemaError's interface is hard to understand at first (mainly because of autos and errors) for anyone wanting to implement a custom validator that can throw a SchemaError'.

Possible solution

I would like to offer a new implementation of SchemaError that will hopefully solve all of the above. Pseudocode:

# The new SchemaError class
class SchemaError(Exception):
    def __init__(self, generated_error_msg, user_error_msg, invalid_data):
        super(Schema, self).__init__()
        self.generated_error_msg = generated_error_msg
        self.user_error_msg = user_error_msg
        self.invalid_data = invalid_data

    def __str__(self):
        return '{}: {}'.format(self.generated_error_msg, self.user_error_msg.format(self.invalid_data))

# Usage example
try:
    s.validate(data)
except SchemaError as e:
    six.raise_from(SchemaError(auto, user_msg, data), e)

This solves the problems above:

  1. Formatting is handled in one place.
  2. The exception contains the data.
  3. We don't collect autos and errors anymore, and let python's exception chaining do the job instead.
  4. IMO the new interface is much simpler and more pythonic.

Drawbacks

Some users might have raise SchemaError(...) in their code. Such code will break if we change the interface of SchemaError.__init__. We could:

skorokithakis commented 4 years ago

I quite like this proposal, thanks! I'm not very enthusiastic about breaking backwards compatibility, though. How much work do you think it would be to make the interface backwards-compatible? Would you be willing/available to give this a shot?

Nightreaver commented 1 year ago

I guess its still a thing, sad to see we didnt get a proper errors handling yet. Wasted my time trying to get a useful error out the current implementation.

I wouldn't mind if the new error-class has different, but it might help avoid complex work on backwards compatiblity.