kaitai-io / kaitai_struct_python_runtime

Kaitai Struct: runtime for Python
MIT License
93 stars 30 forks source link

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

Closed dgelessus closed 1 month ago

dgelessus commented 4 years ago

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 commented 2 years ago

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 commented 1 month ago

@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:

https://github.com/kaitai-io/kaitai_struct_python_runtime/blob/d0058acd7a1fbf59cad39f4d665d1e6e9b997585/kaitaistruct.py#L842-L849

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 https://github.com/kaitai-io/kaitai_struct_php_runtime/pull/6#discussion_r384790125, which is disgusting). I guess that's fine, though - in Python, we can just pass None whenever we don't know it.