ehn-dcc-development / hcert-kotlin

Kotlin multiplatform implementation of the HCERT/DCC specification
Apache License 2.0
25 stars 25 forks source link

Improve verification error handling #32

Closed jsiwrk closed 3 years ago

jsiwrk commented 3 years ago

We should differentiate two cases:

In the first case, it does not make sense for the library client to retry the verification (for that particular hcert), since the hcert is most probably invalid.

In the second case, the problem most likely does not have to do with that hcert, and retrying could make sense. Also, in general, it could make sense to try to apply some corrective action, send an alert to an operator, etc.

Therefore there is the need to deal with both cases in different ways. This commit solves this problem by using a (new) VerificationException to propagate the errors belonging to the first use case. For the second use case, the original exception is simply propagated to the application code (who can know better than us what to do with such an exception).

Also a VerificationResult.errorMessage field has been added containing a more detailed message describing the error.

jsiwrk commented 3 years ago

Sorry for such an apparently big changeset, although actually it's a fairly easy refactor. If that creates conflicts with your work in progress (and assuming you agree with the proposed improvement), I can rebase the PR at a later time, no problem. The changes are pretty straightforward to replicate.

I'm just playing around with the library (on my behalf) so I don't have any special urgency for this PR to be approved. I just think it's an interesting (necessary, in fact, from my point of view) improvement. Thanks for considering it.

JesusMcCloud commented 3 years ago

Much appreciated! Although we removed the errorMessage from the VerificationResult, as such information should be logged, not returned.

jsiwrk commented 3 years ago

Although we removed the errorMessage from the VerificationResult, as such information should be logged, not returned.

I would tend to agree with that in the general sense, if we were talking about an application or a service. However, since this is a reusable library that will presumably be used to build the former, wouldn't it be more appropriate to leave that choice to the library user?

For example, let's consider these scenarios:

If these are deemed valid use cases for this library, then it seems that including an errorMessage (or, generally speaking, detailed information about the error) in the VerificationResult would be the right thing to do. And the library client would be responsible for making a proper use of this field. What do you think?