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

Redesign error handling in the decode module #65

Merged
merged 6 commits into from
Jul 18, 2022
Merged

Redesign error handling in the decode module #65

merged 6 commits into from
Jul 18, 2022

Conversation

partim
Copy link
Member

@partim partim commented Jul 1, 2022

This PR changes the entire way that errors are handled while decoding.

Sources now provide errors related to them failing the get more data when there should be more via the associated Source::Error type. This has been renamed from Source::Err for consistency with other such associated types.

A new type ContentError is introduced for errors where incorrectly encoded data is encountered, be actual encoding errors or data that isn't following the ASN.1 definition or informal profiles. This type wraps an error message that currently can be either a static str or a boxed Display trait object but can be extended if necessary later.

Since both source and content errors can happen during decoding, the compound type DecodeError wraps both these types. For content errors it also stores where in source the error happened to facilitate debugging. Currently, this type only allows displaying the error. It can be extended if additional access to the internally stored error is necessary.

The various content decoding methods now return a DecodeError.

In order to implement all these changes, the Source trait had to be adjusted. First, it needs to be able to provide the current position. This meant that it couldn’t be implemented on Bytes and &[u8] directly anymore. Therefore, the new trait IntoSource allows to convert a type into its Source implementation.

Finally, the trait's methods got cleaned up a bit. Specifically, Source::advance now only allows advancing as far as the length most recently returned by Source::request which also means it cannot fail anymore but needs to panic if the length is too large. This consistent to how Source::bytes already behaves.

partim added 5 commits June 24, 2022 11:16
This commit changes the entire way that errors are handled while
decoding.

Sources now provide errors related to them failing the get more data when
there should be more via the associated Source::Error type. This has been
renamed from Source::Err for consistency with other such associated types.

A new type ContentError is introduced for errors where incorrectly encoded
data is encountered, be actual encoding errors or data that isn't following
the ASN.1 definition or informal profiles. This type wraps an error message
that currently can be either a static str or a boxed Display trait object
but can be extended if necessary later.

Since both source and content errors can happen during decoding, the
compound type DecodeError wraps both these types. For content errors it also
stores where in source the error happened to facilitate debugging.
Currently, this type only allows displaying the error. It can be extended if
additional access to the internally stored error is necessary.

The various content decoding methods now return a DecodeError.

In order to implement all these changes, the Source trait had to be
adjusted. First, it needs to be able to provide the current position. This
meant that it couldn’t be implemented on Bytes and &[u8] directly anymore.
Therefore, the new trait IntoSource allows to convert a type into its Source
implementation.

Finally, the trait's methods got cleaned up a bit. Specifically,
Source::advance now only allows advancing as far as the length most
recently returned by Source::request which also means it cannot fail
anymore but needs to panic if the length is too large. This consistent to
how Source::bytes already behaves.
Copy link
Contributor

@timbru timbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The error structure, here and in rpki-rs, looks quite elegant and flexible. I tried checking the actual message text used as well. I did not spot any issues - but if there is a need to rephrase things then I believe we can always do so later.

@partim partim removed the request for review from ximon18 July 18, 2022 08:58
@partim partim merged commit 30c3809 into main Jul 18, 2022
@partim partim deleted the error-redesign branch July 18, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants