feat(errors): create new error types to carry more info

[dpopp07]

~This is a commit with the prototype code for the new error message formats. This isn't final but it's ready for a more thorough review and using a PR seems like the best way to do it. I'll mark this as a draft and tests are still failing as I clean things up but please begin to leave feedback and comments!~

The PR is now complete and ready for review.

[dpopp07]

We discussed this in person but I'll leave responses to your feedback here so we have it in writing

Perhaps we could consolidate a few of the "discriminator" values. If the discriminator value is only one of multiple values that are used to create a "hash" then it might not be important to have super granular discriminator values. For example, in jwt_utils.go perhaps all three calls to coreSDKErrorf() could use "invalid-token" or "invalid-jwt" for the discriminator.

The purpose of the discriminator is to differentiate between errors that would otherwise have the same hash. For example, in jwt_utils.go, two of the calls to coreSDKErrorf() are in the same function but happen for different reasons. The discriminator is there to ensure we can capture different scenarios with different hashes. Therefore, I think it often helps for them to be fairly granular. We could potentially revisit how granular we want the hashes to be, though.

We might want to scrutinize the various discriminator values that we use (I have my info developer hat on while making this statement 😂). Perhaps "invalid-input" instead of "bad-char", etc.

Something I should have documented is that the discriminator is not ever meant to be seen by the user. It's a private field, so it won't be accessible and it won't appear in the YAML-ified log messages. It's just there as a convenience field for developers to "discriminate" between errors. So, the user-friendliness shouldn't be too relevant. That said, I agree it's worth taking a look to see if any of them detract from the readability of the code itself.

We might also want to include as part of this work changes to our error message constants so that the actual messages are not capitalized, which I think is idiomatic for Go (e.g. "Error while turning the light on" should be "error while turning the light on" 😄)

Good idea - I hadn't been aware of that but I agree, this would be an appropriate time to make such a change.

[bartlomiej-malecki]

As potential consumer of this PR - i would like to extend HTTPError with

type HTTPError struct {
...
    // StatusCode same as Response.StatusCode
    StatusCode int

    // Errors array of `errors` from Response
    // if Response contains `error` instead of `errors` it will be the only element of Errors
    Errors []Error
}

so i could do something like

    err = callClientFunction()
    if err != nil {
        var httpErr HTTPError
        if errors.As(err, &httpErr) {
            if httpErr.StatusCode == 404 {
                return
            }
            fmt.Println(httpErr.Errors)
        }
    }

vast majority of errors we have to handle is coming from: https://github.com/IBM/go-sdk-core/blob/main/core/base_service.go#L457

assuming we do this call in some handler in our microservice and we want e.g. to fail request with proper array of errors from downstream services, that would help a lot (we can do all this using DetailedResponse, but if we had this in error that would be very convenient)

PS. please don not change current value returned by err.Error() i bet there is a lot of existing code that currently do things like: strings.Contains(err.Error(), "Not Found") etc - that would have massive impact

[dpopp07]

As potential consumer of this PR - i would like to extend HTTPError

Thank you for posting the feedback! The more eyes we can get on this change, the better. I'll discuss this proposed change with the team. That is helpful to be aware of.

PS. please don not change current value returned by err.Error() i bet there is a lot of existing code

Great call. It has been my goal to keep the existing error messages intact so that err.Error() would return a consistent string. I'll double check the code to make sure I didn't thoughtlessly change anything.

Thanks again for looking this over and providing feedback. Feel free to share any other thoughts you have.

[dpopp07]

@padamstx @pyrooka @hudlow this is now ready for review. I need to add some tests and finalize a couple of details but the bulk of the logic should remain as it is now. I also might split some of the new logic into separate files because the errors.go file is a bit monolithic. But it should be okay to review as is.

[dpopp07]

we can do all this using DetailedResponse, but if we had this in error that would be very convenient

@bartlomiej-malecki unfortunately, this was a bit too complicated to make it into the initial MVP scope but I still think it's a great idea and I hope to deliver it as a follow-on soon. It should be doable in a compatible fashion.

[dpopp07]

@pyrooka good questions! I'll answer here:

Wouldn't be good (in long term) to create and use the discriminator strings as constants? E.g. having a discriminator.go so we can use them as discriminator.ObjIsNil.

It's a good idea but I still want to avoid using constants for a couple of reasons. The biggest one is, we need to keep in mind how we are going to enumerate the possible errors within this package. Having the string value of each discriminator right in the error constructor makes statically computing the hash value for that created error much easier. I know that's not the best reason in the world to make code decisions but it's a very real problem before us. That, and the fact that many of the discriminators only have one usage and I'm not sure it's worth writing a whole new file for keeping track of them. Some values are used more than once, like in authenticators, but that doesn't actually matter that much. They shouldn't ever change once I commit them but even if one did or if I messed one up, consistency doesn't add any value beyond aesthetic.

Wouldn't it worth to pass the err objects to the "constructors" like SDKErrorf every time, even if they are not used? I am thinking of future scenarios where we might need them... 😄

The errors are only meant to be passed to the constructors if they are coming from a separate IBM Cloud component, like an HTTP service or another SDK. They are used as the "caused by" error to chain them together. Even though we have to accept an error type, the argument should really should be a Problem. I prefer to keep that clear by only passing in the variable when we know we want to use it as a "caused by". If we need the errors in the future (the only thing we could ever really use from them is the message), we can always just pass them in without a compatibility issue (I think).

I know this is the only initial work and the change is already huge, but I am pretty sure we want to have unit tests for the new functions in the future.

100% - not just for the future, but now 🙂 I am working on them for this PR.

[pyrooka]

Thanks for the answer @dpopp07!

Re: the discriminator constants, it makes sense and I think you have already explained to me. I just wanted to make sure I understand your approach.

Re: passing the errors - okay now I see.

100% - not just for the future, but now 🙂 I am working on them for this PR.

Sorry, I might missed that but agree, we should have them before merging. I just didn't want to be pushy and say "LGTM, but can't approve without proper unit tests...". :)

[ibm-devx-sdk]

:tada: This PR is included in version 5.16.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: