cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

Add ability to wrap an error with an Unimplemented error #86

Open rafiss opened 2 years ago

rafiss commented 2 years ago

I have a bit of code that does something like this:

errors.UnimplementedErrorf(
  link,
  "unsupported expression %q: %v",
  exprString,
  err,
)

This flattens the err argument. What I'd really like is something like

errors.UnimplementedWrapf(
  err,
  link,
  "unsupported expression %q",
  exprString,
)

(analogous to errors.Errorf versus errors.Wrapf)

The exact piece of code I'm referring to is: https://github.com/cockroachdb/cockroach/blob/7bf398fc9c715e67ae4ab342569b26f50809fb59/pkg/ccl/importccl/read_import_mysql.go#L764

cc @knz

knz commented 2 years ago

Hm so changing the underlying type of the error returned by errors.UnimplementedXXX from a leaf to a wrapper would introduce cross-version compatibility issues: previous versions wouldn't be able to recognize the new type.

I suppose that if very well motivated, we could do that.

But what about this, changing:

return nil, unimplemented.Newf("import.mysql.default", "unsupported default expression %q for column %q: %v", exprString, name, err)

to:

return nil, errors.Wrapf(
    unimplemented.Newf("import.mysql.default", "unsupported default expression"),
    "error parsing expression %q for column %q (%v)",` exprString, name, err /* XX lint exclusion */)

The difference with the original code is that the err argument to errors.Wrapf will not just be included in the error string, it will also present itself as a secondary error when displaying the error's details (and its underlying structure will be preserved)

rafiss commented 2 years ago
return nil, errors.Wrapf(
   unimplemented.Newf("import.mysql.default", "unsupported default expression"),
   "error parsing expression %q for column %q (%v)",` exprString, name, err /* XX lint exclusion */)

In both the original and this new example, the unimplemented error participates in cause analysis. But I don't think either example lets the err variable show up as a secondary error with the structure preserved. I could use errors.SecondaryError(unimplementedErr, err) to achieve that though, but even with this, err will not show up when formatting with %v or calling Error() and the newly created error.

knz commented 2 years ago

But I don't think either example lets the err variable show up as a secondary error with the structure preserved

It's actually an undocumented feature of errors.Wrapf (and errors.Newf): if there's an object of type error in the argument list, it's automatically captured with WithSecondaryError(). You can print with %+v to check.

knz commented 2 years ago

This code is responsible (errutil/utilities.go):

λ NewWithDepthf(depth int, format 𝓢, args ...any) error {
  var errRefs []error
  for _, a ∈ args {
    if e, ✓ ≔ a.(error); ✓ {
      errRefs = append(errRefs, e)
    }
  }
   // ...
  for _, e ∈ errRefs {
    err = secondary.WithSecondaryError(err, e)
  }