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

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

Open
rafiss opened this issue Dec 14, 2021 · 4 comments
Open

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

rafiss opened this issue Dec 14, 2021 · 4 comments

Comments

@rafiss
Copy link

rafiss commented Dec 14, 2021

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
Copy link
Contributor

knz commented Dec 14, 2021

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
Copy link
Author

rafiss commented Dec 14, 2021

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
Copy link
Contributor

knz commented Dec 14, 2021

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
Copy link
Contributor

knz commented Dec 14, 2021

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

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

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

No branches or pull requests

2 participants