From 1146377183a80e70b44967403d38f0933ac860f6 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 20 Nov 2023 17:09:31 -0800 Subject: [PATCH] sem/tree: add unredacted info to some assertions 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`. Release note: None --- pkg/sql/sem/tree/constant.go | 4 ++-- pkg/sql/sem/tree/datum.go | 17 ++++++++++------- pkg/sql/sem/tree/eval.go | 6 +++++- pkg/sql/sem/tree/expr.go | 13 +++++++++---- pkg/sql/sem/tree/parse_string.go | 2 +- pkg/sql/sem/tree/parse_tuple.go | 2 +- pkg/sql/sem/tree/type_check.go | 4 +++- pkg/sql/types/types.go | 3 +++ 8 files changed, 34 insertions(+), 17 deletions(-) diff --git a/pkg/sql/sem/tree/constant.go b/pkg/sql/sem/tree/constant.go index 82d6734bf650..267b58bcccb0 100644 --- 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 index 74247a6105f3..2f49d48bb6a9 100644 --- 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 index 5146bf2d4d5e..c85329d70394 100644 --- 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 index 33b63889b882..cd66df32b4db 100644 --- 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 index 60cd856fbf0e..3b383b8c043e 100644 --- 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 index daff6e2c81e8..75df3899b266 100644 --- 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 index e3d69de85c0e..79d38681f4d3 100644 --- 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 index 99314c9b442e..404d80137371 100644 --- 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 "" + } 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).