Merge cockroachdb#114779

114779: sem/tree: add unredacted info to some assertions r=yuzefovich a=yuzefovich This commit audits `errors.AssertionFailedf` calls in `sem/tree` package to include more details that are not redacted. In most cases, the changes involve calling `SQLStringForError` on the `types.T`, in a few other places it marks some things as `redact.Safe`. Fixes: cockroachdb#114093. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
lyang24 · Nov 21, 2023 · 64dacab · 64dacab
2 parents f0bd0a2 + 1146377
commit 64dacab
Show file tree

Hide file tree

Showing 8 changed files with 34 additions and 17 deletions.
diff --git a/pkg/sql/sem/tree/constant.go b/pkg/sql/sem/tree/constant.go
@@ -415,7 +415,7 @@ func (expr *NumVal) ResolveAsType(
 		dInt := MustBeDInt(d)
 		return IntToOid(dInt)
 	default:
-		return nil, errors.AssertionFailedf("could not resolve %T %v into a %T", expr, expr, typ)
+		return nil, errors.AssertionFailedf("could not resolve %T %v into a %s", expr, expr, typ.SQLStringForError())
 	}
 }
 
@@ -624,7 +624,7 @@ func (expr *StrVal) ResolveAsType(
 			expr.resString = DString(expr.s)
 			return &expr.resString, nil
 		}
-		return nil, errors.AssertionFailedf("attempt to type byte array literal to %T", typ)
+		return nil, errors.AssertionFailedf("attempt to type byte array literal to %s", typ.SQLStringForError())
 	}
 
 	// Typing a string literal constant into some value type.

diff --git a/pkg/sql/sem/tree/datum.go b/pkg/sql/sem/tree/datum.go
@@ -421,7 +421,7 @@ func GetBool(d Datum) (DBool, error) {
 	if d == DNull {
 		return DBool(false), nil
 	}
-	return false, errors.AssertionFailedf("cannot convert %s to type %s", d.ResolvedType(), types.Bool)
+	return false, errors.AssertionFailedf("cannot convert %s to type %s", d.ResolvedType().SQLStringForError(), types.Bool)
 }
 
 // ResolvedType implements the TypedExpr interface.
@@ -2790,7 +2790,7 @@ func TimeFromDatumForComparison(ctx CompareContext, d Datum) (time.Time, error)
 	case *DTimeTZ:
 		return t.ToTime(), nil
 	default:
-		return time.Time{}, errors.AssertionFailedf("unexpected type: %v", t.ResolvedType())
+		return time.Time{}, errors.AssertionFailedf("unexpected type: %s", t.ResolvedType().SQLStringForError())
 	}
 }
 
@@ -4915,7 +4915,7 @@ func MustBeDArray(e Expr) *DArray {
 // not an array type.
 func (d *DArray) MaybeSetCustomOid(t *types.T) error {
 	if t.Family() != types.ArrayFamily {
-		return errors.AssertionFailedf("expected array type, got %s", t.SQLString())
+		return errors.AssertionFailedf("expected array type, got %s", t.SQLStringForError())
 	}
 	switch t.Oid() {
 	case oid.T_int2vector:
@@ -5107,7 +5107,10 @@ func (d *DArray) Append(v Datum) error {
 	// v.ResolvedType() must be the left-hand side because EquivalentOrNull
 	// only allows null tuple elements on the left-hand side.
 	if !v.ResolvedType().EquivalentOrNull(d.ParamTyp, true /* allowNullTupleEquivalence */) {
-		return errors.AssertionFailedf("cannot append %s to array containing %s", v.ResolvedType(), d.ParamTyp)
+		return errors.AssertionFailedf(
+			"cannot append %s to array containing %s",
+			v.ResolvedType().SQLStringForError(), d.ParamTyp.SQLStringForError(),
+		)
 	}
 	if d.Len() >= maxArrayLength {
 		return errors.WithStack(errArrayTooLongError)
@@ -5943,7 +5946,7 @@ func NewDRefCursor(d string) Datum {
 func NewDIntVectorFromDArray(d *DArray) Datum {
 	// Sanity: Validate the type of the array, since it should be int2.
 	if d.ParamTyp != types.Int2 {
-		panic(errors.AssertionFailedf("int2vector can only be made from int2 not %v", d.ParamTyp))
+		panic(errors.AssertionFailedf("int2vector can only be made from int2 not %s", d.ParamTyp.SQLStringForError()))
 	}
 	ret := new(DArray)
 	*ret = *d
@@ -6042,7 +6045,7 @@ func NewDefaultDatum(collationEnv *CollationEnvironment, t *types.T) (d Datum, e
 		}
 		return NewDEnum(e), nil
 	default:
-		return nil, errors.AssertionFailedf("unhandled type %v", t.SQLString())
+		return nil, errors.AssertionFailedf("unhandled type %s", t.SQLStringForError())
 	}
 }
 
@@ -6105,7 +6108,7 @@ func DatumTypeSize(t *types.T) (size uintptr, isVarlen bool) {
 		return bSzInfo.sz, bSzInfo.variable
 	}
 
-	panic(errors.AssertionFailedf("unknown type: %T", t))
+	panic(errors.AssertionFailedf("unknown type: %s", t.SQLStringForError()))
 }
 
 const (

diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go
@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 	"github.com/lib/pq/oid"
 )
 
@@ -1410,7 +1411,10 @@ func cmpOpFixups(
 				return o.Volatility
 			}
 		}
-		panic(errors.AssertionFailedf("could not find cmp op %s(%s,%s)", op, t, t))
+		panic(errors.AssertionFailedf(
+			"could not find cmp op %s(%s,%s)",
+			redact.Safe(op.String()), t.SQLStringForError(), t.SQLStringForError(),
+		))
 	}
 
 	// Array equality comparisons.

diff --git a/pkg/sql/sem/tree/expr.go b/pkg/sql/sem/tree/expr.go
@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 )
 
 // Expr represents an expression.
@@ -488,8 +489,10 @@ func MemoizeComparisonExprOp(node *ComparisonExpr) {
 
 	fn, ok := CmpOps[fOp.Symbol].LookupImpl(leftRet, rightRet)
 	if !ok {
-		panic(errors.AssertionFailedf("lookup for ComparisonExpr %s's CmpOp failed",
-			AsStringWithFlags(node, FmtShowTypes)))
+		panic(errors.AssertionFailedf("lookup for ComparisonExpr %s's CmpOp failed (%s(%s,%s))",
+			AsStringWithFlags(node, FmtShowTypes), redact.Safe(fOp.String()),
+			leftRet.SQLStringForError(), rightRet.SQLStringForError(),
+		))
 	}
 	node.Op = fn
 }
@@ -1111,8 +1114,10 @@ func (node *BinaryExpr) memoizeOp() {
 	leftRet, rightRet := node.Left.(TypedExpr).ResolvedType(), node.Right.(TypedExpr).ResolvedType()
 	fn, ok := BinOps[node.Operator.Symbol].LookupImpl(leftRet, rightRet)
 	if !ok {
-		panic(errors.AssertionFailedf("lookup for BinaryExpr %s's BinOp failed",
-			AsStringWithFlags(node, FmtShowTypes)))
+		panic(errors.AssertionFailedf("lookup for BinaryExpr %s's BinOp failed (%s(%s,%s))",
+			AsStringWithFlags(node, FmtShowTypes), redact.Safe(node.Operator.String()),
+			leftRet.SQLStringForError(), rightRet.SQLStringForError(),
+		))
 	}
 	node.Op = fn
 }

diff --git a/pkg/sql/sem/tree/parse_string.go b/pkg/sql/sem/tree/parse_string.go
@@ -115,7 +115,7 @@ func ParseAndRequireString(
 	case types.VoidFamily:
 		d = DVoidDatum
 	default:
-		return nil, false, errors.AssertionFailedf("unknown type %s (%T)", t, t)
+		return nil, false, errors.AssertionFailedf("unknown type %s", t.SQLStringForError())
 	}
 	if err != nil {
 		return d, dependsOnContext, err

diff --git a/pkg/sql/sem/tree/parse_tuple.go b/pkg/sql/sem/tree/parse_tuple.go
@@ -194,7 +194,7 @@ func doParseDTupleFromString(
 	ctx ParseContext, s string, t *types.T,
 ) (_ *DTuple, dependsOnContext bool, _ error) {
 	if t.TupleContents() == nil {
-		return nil, false, errors.AssertionFailedf("not a tuple type %s (%T)", t, t)
+		return nil, false, errors.AssertionFailedf("not a tuple type %s", t.SQLStringForError())
 	}
 	if t == types.AnyTuple {
 		return nil, false, unsupportedRecordError

diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go
@@ -593,7 +593,9 @@ func onCastTypeCheckHook(from, to types.Family) {
 		return
 	}
 	panic(errors.AssertionFailedf(
-		"no cast counter found for cast from %s to %s", from.Name(), to.Name(),
+		"no cast counter found for cast from %s to %s",
+		// Family names are always safe for redacting.
+		redact.Safe(from.Name()), redact.Safe(to.Name()),
 	))
 }
 

diff --git a/pkg/sql/types/types.go b/pkg/sql/types/types.go
@@ -1975,6 +1975,9 @@ func (t *T) SQLString() string {
 // SQLStringForError returns a version of SQLString that will preserve safe
 // information during redaction. It is suitable for usage in error messages.
 func (t *T) SQLStringForError() redact.RedactableString {
+	if t == nil {
+		return "<nil>"
+	}
 	if t.UserDefined() {
 		// Show the redacted SQLString output with an un-redacted prefix to indicate
 		// that the type is user defined (and possibly enum or record).