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

Shared error module #164

Merged
merged 21 commits into from
Oct 28, 2023
Merged

Shared error module #164

merged 21 commits into from
Oct 28, 2023

Conversation

Nicceboy
Copy link
Contributor

Hi,

I think I need a bit help on deciding how we should do this.
Currently errors have been combined and they follow the similar way than existing errors in PER.

I was not sure how we should support snafu.
Currently errors try to create stable API by allowing calling the errors either by calling function or calling snafu enum with .into(). Constructing the whole enum with regular style is still possible, but I tried make this simpler by adding functions similarly than in PER.

snafu is abstracted mostly away in this way, which lets as to change the internals of the error, if needed in the future.

This makes it harder to use .context() method of snafu, but now every error has backtrace included, so maybe it is not needed.

Currently the error module is constructed from the following structs/enums

EncodeError
EncoreErrorKind (snafu)

and

DecodeError
DecoreErrorKind (snafu)

Also, what should we do with the codec.rs now, since the all error types are same format. Do we need separate Error type for every codec? Existing way of adding codec context for every error should still work, but I am not sure how useful it is.

@Nicceboy Nicceboy force-pushed the error-refactor branch 6 times, most recently from 65c8f39 to 4cf512f Compare October 26, 2023 12:04
@XAMPPRocky
Copy link
Collaborator

How much more work do you think is needed for this? I'm wondering if there's merit in reviewing and merging what we have and making future improvements in later PRs.

@Nicceboy
Copy link
Contributor Author

Nicceboy commented Oct 26, 2023

I was planning to say today that it is better to review now before it gets too bloated.
There are already quite many changes..

I will add one decode sample and maybe two tests and I think it can be then reviewed. Doing them right now.

The main issue is on categorising the errors, since most of the codec-specific ones will be empty. I guess we should hide the visibility of those.

I know that many if not all of the UPER errors will be re-used by OER and even BER could re-use many of them one day if it supports constraints and also if the general error handling will be improved, so most are generic errors. But I can adjust them after you have reviewed.

So, I think we should decide, whether

  • Codec specific error is error which appears on current implementation and only on that codec, regardless if other codecs can use it some day when improved.
  • Codec specific error is error which can appear only on that codec on perfect implementation

@Nicceboy
Copy link
Contributor Author

Also I noticed that decoding strings on release build do not throw any errors, and on debug mode they are only debug asserts. There will be performance impact if we check for every character whether the string type is valid, and on the other hand, should we be very strict on that? Seems like original idea is, that on decoding side it is okay to get invalid strings.

We could also check validity after the whole string is decoded, but string can be also very long and you know then only in the end that it is not valid. Maybe this is better to leave some other PR.

@XAMPPRocky
Copy link
Collaborator

Codec specific error is error which can appear only on that codec on perfect implementation

Could you clarify this part?

Also I noticed that decoding strings on release build do not throw any errors, and on debug mode they are only debug asserts.

I think we should reverse course on this, and make it error in release mode, we could add an unchecked method if it's a significant performance impact.

@Nicceboy
Copy link
Contributor Author

Could you clarify this part?

If the codecs will be perfected some day, being fully standard compliant, and equally checking general error conditions, and similarly implemented, then only codec-specific errors would be those which are different situations based on the standards.

E.g. DER does not allow constructed encoding, but BER and CER do. Other codecs use mostly just primitive encoding. So for example ConstructedEncodingNotAllowed error makes more sense for DER and not elsewhere.

But currently codecs are implemented differently and in different quality, they might not be equally good on handling error situations yet, so there are no that many shared errors, which could be shared. So I am just wondering should I think the end situation or current situation when trying to categorise this. I also don't know codecs that well in general, so maybe this is too much thinking to avoid few lines of duplicated code.

@XAMPPRocky
Copy link
Collaborator

If the codecs will be perfected some day, being fully standard compliant, and equally checking general error conditions, and similarly implemented, then only codec-specific errors would be those which are different situations based on the standards.

Yes this is what we should be designing for, anything not compliant is a bug.

@Nicceboy
Copy link
Contributor Author

I think it is better to review now if I need to do some major changes. Everything but correct and complete error categorisation should be there.

@Nicceboy Nicceboy changed the title Draft: Shared error module Shared error module Oct 26, 2023
@XAMPPRocky
Copy link
Collaborator

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 0bad2e3 into librasn:main Oct 28, 2023
65 checks passed
@github-actions github-actions bot mentioned this pull request Oct 28, 2023
@Nicceboy
Copy link
Contributor Author

I guess everything was good 😄 It is easy to move errors later on to better categories. I can finish OER now and it might bring some clarity as well.

@XAMPPRocky
Copy link
Collaborator

Yeah nothing stood out to me as needing to be changed. I saw some docs on some errors that need to be improved but they were there before your PR.

@github-actions github-actions bot mentioned this pull request Oct 30, 2023
This was referenced Nov 7, 2023
This was referenced Nov 23, 2023
This was referenced Dec 4, 2023
@github-actions github-actions bot mentioned this pull request Mar 9, 2024
@github-actions github-actions bot mentioned this pull request Mar 21, 2024
@github-actions github-actions bot mentioned this pull request Apr 3, 2024
@github-actions github-actions bot mentioned this pull request May 8, 2024
This was referenced May 21, 2024
This was referenced Jun 11, 2024
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.

None yet

2 participants