Skip to content

Commit

Permalink
roachpb: remove deprecated fields from Error
Browse files Browse the repository at this point in the history 
CRDB 21.2 already populates `Errors.EncodedError`, so we can remove the
deprecated fields.

I was motivated by this when working on a follow-up to #73602 and
ended up trying to marshal an `Error` to JSON, which panics since
it ends up trying to use `reflect` to get an unexported value (due
to the `customname` tags on the deprecated fields). This behavior
persists even with the `json: "-"` tag, by the way, indicating
that we should never use `(gogoproto.customname)` to make a field
private. It's also just one more papercut that comes with using
`gogoproto` but that's a much bigger fish to fry.

Release note: None
  • Loading branch information
@tbg
tbg committed Dec 10, 2021
1 parent 3bed3a6 commit 561e655
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 448 deletions.
127 changes: 28 additions & 99 deletions pkg/roachpb/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,30 +149,12 @@ func NewError(err error) *Error {
if err == nil {
return nil
}
e := &Error{
EncodedError: errors.EncodeError(context.Background(), err),
if intErr, ok := err.(*internalError); ok {
return (*Error)(intErr)
}

// This block is deprecated behavior retained for compat with
// 20.2 nodes. It makes sure that if applicable, deprecatedMessage,
// ErrorDetail, and deprecatedTransactionRestart are set.
{
if intErr, ok := err.(*internalError); ok {
*e = *(*Error)(intErr)
} else if detail := ErrorDetailInterface(nil); errors.As(err, &detail) {
e.deprecatedMessage = detail.message(e)
if r, ok := detail.(transactionRestartError); ok {
e.deprecatedTransactionRestart = r.canRestartTransaction()
} else {
e.deprecatedTransactionRestart = TransactionRestart_NONE
}
e.deprecatedDetail.MustSetInner(detail)
e.checkTxnStatusValid()
} else {
e.deprecatedMessage = err.Error()
}
return &Error{
EncodedError: errors.EncodeError(context.Background(), err),
}
return e
}

// NewErrorWithTxn creates an Error from the given error and a transaction.
Expand Down Expand Up @@ -201,42 +183,31 @@ func (e *Error) SafeFormat(s redact.SafePrinter, _ rune) {
return
}

if e.EncodedError != (errors.EncodedError{}) {
err := errors.DecodeError(context.Background(), e.EncodedError)
var iface ErrorDetailInterface
if errors.As(err, &iface) {
// Deprecated code: if there is a detail and the message produced by the detail
// (which gets to see the surrounding Error) is different, then use that message.
// What is likely the cause of that is that someone passed an updated Transaction
// to the Error via SetTxn, and the error detail prints that txn.
//
// TODO(tbg): change SetTxn so that instead of stashing the transaction on the
// Error struct, it wraps the EncodedError with an error containing the updated
// txn. Make GetTxn retrieve the first one it sees (overridden by UnexposedTxn
// while it's still around). Remove the `message(Error)` methods from ErrorDetailInterface.
// We also have to remove GetDetail() in the process since it doesn't understand the
// wrapping; instead we need a method that looks for a specific kind of detail, i.e.
// we basically want to use `errors.As` instead.
deprecatedMsg := iface.message(e)
if deprecatedMsg != err.Error() {
s.Print(deprecatedMsg)
return
}
}
s.Print(err)
} else {
// TODO(tbg): remove this block in the 21.2 cycle and rely on EncodedError
// always being populated.
switch t := e.GetDetail().(type) {
case nil:
s.Print(e.deprecatedMessage)
default:
// We have a detail and ignore e.deprecatedMessage. We do assume that if a detail is
// present, e.deprecatedMessage does correspond to that detail's message. This
// assumption is not enforced but appears sane.
s.Print(t)
err := errors.DecodeError(context.Background(), e.EncodedError)
if err == nil {
err = errors.AssertionFailedf("Error with nil EncodedError")
}
var iface ErrorDetailInterface
if errors.As(err, &iface) {
// Deprecated code: if there is a detail and the message produced by the detail
// (which gets to see the surrounding Error) is different, then use that message.
// What is likely the cause of that is that someone passed an updated Transaction
// to the Error via SetTxn, and the error detail prints that txn.
//
// TODO(tbg): change SetTxn so that instead of stashing the transaction on the
// Error struct, it wraps the EncodedError with an error containing the updated
// txn. Make GetTxn retrieve the first one it sees (overridden by UnexposedTxn
// while it's still around). Remove the `message(Error)` methods from ErrorDetailInterface.
// We also have to remove GetDetail() in the process since it doesn't understand the
// wrapping; instead we need a method that looks for a specific kind of detail, i.e.
// we basically want to use `errors.As` instead.
deprecatedMsg := iface.message(e)
if deprecatedMsg != err.Error() {
s.Print(deprecatedMsg)
return
}
}
s.Print(err)

if txn := e.GetTxn(); txn != nil {
s.SafeString(": ")
Expand All @@ -251,12 +222,6 @@ func (e *Error) String() string {

// TransactionRestart returns the TransactionRestart for this Error.
func (e *Error) TransactionRestart() TransactionRestart {
if e.EncodedError == (errorspb.EncodedError{}) {
// Legacy code.
//
// TODO(tbg): delete in 21.2.
return e.deprecatedTransactionRestart
}
var iface transactionRestartError
if errors.As(errors.DecodeError(context.Background(), e.EncodedError), &iface) {
return iface.canRestartTransaction()
Expand Down Expand Up @@ -375,14 +340,7 @@ func (e *Error) GetDetail() ErrorDetailInterface {
return nil
}
var detail ErrorDetailInterface
if e.EncodedError != (errorspb.EncodedError{}) {
errors.As(errors.DecodeError(context.Background(), e.EncodedError), &detail)
} else {
// Legacy behavior.
//
// TODO(tbg): delete in v21.2.
detail, _ = e.deprecatedDetail.GetInner().(ErrorDetailInterface)
}
errors.As(errors.DecodeError(context.Background(), e.EncodedError), &detail)
return detail
}

Expand All @@ -404,35 +362,6 @@ func (e *Error) UpdateTxn(o *Transaction) {
} else {
e.UnexposedTxn.Update(o)
}
if sErr, ok := e.deprecatedDetail.GetInner().(ErrorDetailInterface); ok {
// Refresh the message as the txn is updated.
//
// TODO(tbg): deprecated, remove in 21.2.
e.deprecatedMessage = sErr.message(e)
}
e.checkTxnStatusValid()
}

// checkTxnStatusValid verifies that the transaction status is in-sync with the
// error detail.
func (e *Error) checkTxnStatusValid() {
// TODO(tbg): this will need to be updated when we
// remove all of these deprecated fields in 21.2.

txn := e.UnexposedTxn
err := e.deprecatedDetail.GetInner()
if txn == nil {
return
}
if e.deprecatedTransactionRestart == TransactionRestart_NONE {
return
}
if errors.HasType(err, (*TransactionAbortedError)(nil)) {
return
}
if txn.Status.IsFinalized() {
log.Fatalf(context.TODO(), "transaction unexpectedly finalized in (%T): %v", err, e)
}
}

// GetTxn returns the txn.
Expand Down
Loading

0 comments on commit 561e655

Please sign in to comment.