gcerrors: Support unwrapping error types

[Megakuul]

Is your feature request related to a problem? Please describe.

Certain APIs like in my situation the docstore Action() return non-unwrappable error types like ActionListError.
This results in a lot of boilerplate code to achieve something for which go offers an idiomatic approach (errors.As/Is):

if err = coll.Actions().Create(a).Create(b).Create(c).Do(ctx); err != nil {
	if aErrs, ok := errors.AsType[docstore.ActionListError](err); ok {
		for _, aErr := range aErrs {
			if gcerrors.Code(aErr.Err) == gcerrors.AlreadyExists {
				return "", nil
			}
		}
	}
	return "", err
}

Describe the solution you'd like

Is there a specific reason for the usage of message codes here? Would it be viable to use error structs instead so that the go errors utilities can be used for comparison / unwrapping instead of the gcerrors package?

Describe alternatives you've considered

Writing helper utilities for the boilerplate code shown above.

Additional context

[vangent]

Pretty sure we did this before Go's errors existed (https://go.dev/blog/go1.13-errors). It's probably not super easy to change now.

Feel free to write helpers for the boilerplate.

[vangent]

Sorry, I meant helpers in your codebase... If you want to make a contribution, I'd suggest doing a high-level overview of what you're going to do and running it by me first.

[Megakuul]

Sorry for the late response; but let me quickly push back on the errors situation. Would it possibly be a viable solution to not replace gcerrors with custom error structs but rather support both.

Again just speaking for the docstore package I used so far, but returning a custom error struct that wraps the gcerr.Error should keep the current pattern mostly intact (if someone uses gcerrors.Code(...) nothing changes, it would only break if someone directly type asserted gcerrs.Error).

While writing helpers is a practical workaround for this, I believe slowly moving to error structs is beneficial for this module.

I would be willing to create a pr with the suggested changes if you consider it a viable option for the module / the other affected packages.

[vangent]

I'd prefer to keep the library consistent, so if it's going to support error structs, it should do so everywhere.

Can you rewrite the code snippet in the original comment with what you think it should look like after the proposed changes?
Most of it it seems to be dealing with the "list" nature of the error, and it's not obvious to me how errors helps with that; I don't think errors.Is for an ActionListError makes much sense. Isn't it just the gcerrors.Code(aErr.Err) == gcerrors.AlreadyExists part that would change, to something like a.Err.Is(gcerrors.AlreadyExistsError)?

And maybe a small PR with a sample of what the corresponding code changes in the library would look like?

[Megakuul]

I agree that it should be consistent and added everywhere if gets supported.

The issue with the "list" is that the ActionListError does NOT implement Unwrap() []error while Unwrap() error returns nil if there are multiple errors (documented here).
The mentioned comment effectively also explains the issue right here, even if there were multiple errors: gcerrors.Code() does not work anymore because I want to know if error X is in the error tree, while gcerrors.Code() can only return me one error.

Assuming the ActionListError would implement Unwrap() []error and the errors return are docstore.AlreadyExistsError it would look like this:

if err = coll.Actions().Create(a).Create(b).Create(c).Do(ctx); err != nil {
	if aErr, ok := errors.AsType[docstore.AlreadyExistsError](err); ok {
             return "", nil
	}
	return "", err
}

[vangent]

Hmm. I can see changing

go-cloud/docstore/docstore.go

Line 302 in 595f8b6

func (e ActionListError) Unwrap() error {

to return the first error in the slice if they are all the same; errors.Is makes sense in that case. But if there are three different error codes in the slice, having errors.Is(docstore.AlreadyExistsError) return true seems misleading to me.

[Megakuul]

Well besides that one could argue that this is expected behavior for the errors.Is() function in context of a joined/list errors as it explicitly traverses the full list, in my proposal that code would actually not work at all.

My idea was to use error structs like type AlreadyExistsError struct {...} and return them; the go errors.Is() function does NOT work for errors.Is(err, docstore.AlreadyExistsError) because it operates on value / pointer comparisons not on the type.

Unwrapping would be done with errors.As(err, &aErr) or errors.AsType[docstore.AlreadyExistsError](err) as shown above, to me this largely avoids the mentioned ambiguity.

PR with example following soon

[vangent]

PTAL #3708 and see what you think. I think this solves the problem of being able to use errors.Is, taking advantage of the fact that Go CDK errors are already wrapped.

Whether ActionListError should return all of its errors for Unwrap is a separate question I think. The semantics don't quite match up, I think Unwrap assumes that all of the things it returns are reasonable representations of a single error, whereas ActionListError actually represents N unique errors.

[Megakuul]

I like the approach of #3708 since it keeps the API intact. The only thing I would change is to make error sentinels unique (otherwise this can cause confusion because errors.Is() compares by value). I added a proposal on top of your changes at #3710.

Regarding the ActionListError: I believe it is a common practice to use Unwrap []error for N unique errors, I mean the errors.Join() function works exactly like this and is designed for such scenarios.

[vangent]

OK, sounds good. I'll submit that, you can update your first PR to change ActionListError.Unwrap.

[vangent]

#3705 also submitted.

[vangent]

Thanks for the PRs and discussion!