Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define a custom exception class instead of raising generic exceptions #40

Closed
dgelessus opened this issue Feb 25, 2020 · 2 comments · Fixed by #80
Closed

Define a custom exception class instead of raising generic exceptions #40

dgelessus opened this issue Feb 25, 2020 · 2 comments · Fixed by #80

Comments

@dgelessus
Copy link
Contributor

Currently, when a Kaitai Struct parser detects that the input data is invalid, it can raise a variety of exceptions: ValueError, EOFError, generic Exception, and possibly some others. This makes it very difficult for callers to specifically catch parse errors.

It would be helpful to have a custom exception class (e. g. kaitaistruct.ParseError) that is consistently raised for all invalid input data. This would allow the calling code to catch and handle that exception specifically, without catching other unrelated errors by accident.

@armijnhemel
Copy link

Throwing a generic Exception seems to happen in three places in kaitaistruct.py:

read_bytes_term(), ensure_fixed_contents() and process_rotate_left(). My guess is that introducing custom exceptions will probably not break existing code.

@generalmimon
Copy link
Member

@dgelessus:

It would be helpful to have a custom exception class (e. g. kaitaistruct.ParseError) that is consistently raised for all invalid input data.

Makes sense, but instead of kaitaistruct.ParseError, I'll probably reuse the existing KaitaiStructError:

class KaitaiStructError(Exception):
"""Common ancestor for all error originating from Kaitai Struct usage.
Stores KSY source path, pointing to an element supposedly guilty of
an error.
"""
def __init__(self, msg, src_path):
super(KaitaiStructError, self).__init__("%s: %s" % (src_path, msg))
self.src_path = src_path

Currently, it's only used by UndecidedEndiannessError and ValidationFailedError (which is a common ancestor for all specific Validation*Errors). Both of these types of exceptions can be thrown from both the parsing process and the serialization process, so it's a good thing that they don't extend any class that has "Parse" in its name.

In general, I don't think a user needs to be able to distinguish exceptions raised during parsing from exceptions raised during serialization via the type system. After all, they start the process of parsing/serialization themselves by calling _read() (+ instance getters) or _write(), so they can just use a minimal try..except block enclosing just these, which makes it easy to recognize where the exception was thrown from.

The problem with KaitaiStructError is that it currently expects the src_path argument with the KSY source path, which is only known for exceptions thrown directly from the generated code (i.e. again UndecidedEndiannessError and subclasses of ValidationFailedError). For other exceptions, it'll be unknown (unless we wanted to pass it when calling any runtime library method as shown in kaitai-io/kaitai_struct_php_runtime#6 (comment), which is disgusting). I guess that's fine, though - in Python, we can just pass None whenever we don't know it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants