Skip to content

Commit 9ec767d

Browse files
rafissrafiss
committed
tree, types: don't redact type names and cmp operators
This will make it easier to debug logs that contain type checking errors. Release note: None
1 parent 481e179 commit 9ec767d

File tree

5 files changed

+54
-20
lines changed

5 files changed

+54
-20
lines changed

‎pkg/sql/sem/tree/treecmp/BUILD.bazel‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,8 @@ go_library(
88
],
99
importpath = "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp",
1010
visibility = ["//visibility:public"],
11-
deps = ["@com_github_cockroachdb_errors//:errors"],
11+
deps = [
12+
"@com_github_cockroachdb_errors//:errors",
13+
"@com_github_cockroachdb_redact//:redact",
14+
],
1215
)

‎pkg/sql/sem/tree/treecmp/comparison_operator.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"fmt"
1010

1111
"github.com/cockroachdb/errors"
12+
"github.com/cockroachdb/redact"
1213
)
1314

1415
// ComparisonOperator represents a binary operator which returns a bool.
@@ -30,6 +31,10 @@ func (o ComparisonOperator) String() string {
3031
return o.Symbol.String()
3132
}
3233

34+
// SafeValue implements redact.SafeValue.
35+
// Comparison operators are SQL language constructs and safe to log.
36+
func (ComparisonOperator) SafeValue() {}
37+
3338
// Operator implements tree.Operator.
3439
func (ComparisonOperator) Operator() {}
3540

@@ -129,6 +134,10 @@ func (i ComparisonOperatorSymbol) String() string {
129134
return comparisonOpName[i]
130135
}
131136

137+
// SafeValue implements redact.SafeValue.
138+
// Comparison operator symbols are SQL language constructs and safe to log.
139+
func (ComparisonOperatorSymbol) SafeValue() {}
140+
132141
// HasSubOperator returns if the ComparisonOperator is used with a sub-operator.
133142
func (i ComparisonOperatorSymbol) HasSubOperator() bool {
134143
switch i {
@@ -148,3 +157,7 @@ func ComparisonOpName(op ComparisonOperatorSymbol) string {
148157
}
149158
return comparisonOpName[op]
150159
}
160+
161+
// Verify that ComparisonOperator and ComparisonOperatorSymbol implement redact.SafeValue.
162+
var _ redact.SafeValue = ComparisonOperator{}
163+
var _ redact.SafeValue = ComparisonOperatorSymbol(0)

‎pkg/sql/sem/tree/type_check.go‎

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -439,11 +439,11 @@ func (expr *BinaryExpr) TypeCheck(
439439
// Throw a typing error if overload resolution found either no compatible candidates
440440
// or if it found an ambiguity.
441441
if len(s.overloadIdxs) != 1 {
442-
var desStr string
442+
var desStr redact.RedactableString
443443
if desired.Family() != types.AnyFamily {
444-
desStr = fmt.Sprintf(" (returning <%s>)", desired)
444+
desStr = redact.Sprintf(" (returning <%s>)", desired)
445445
}
446-
sig := fmt.Sprintf("<%s> %s <%s>%s", leftReturn, expr.Operator, rightReturn, desStr)
446+
sig := redact.Sprintf("<%s> %s <%s>%s", leftReturn, expr.Operator, rightReturn, desStr)
447447
if len(s.overloadIdxs) == 0 {
448448
return nil,
449449
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedBinaryOpErrFmt, sig)
@@ -805,7 +805,7 @@ func (expr *AnnotateTypeExpr) TypeCheck(
805805
semaCtx,
806806
expr.Expr,
807807
annotateType,
808-
fmt.Sprintf(
808+
redact.Sprintf(
809809
"type annotation for %v as %s, found",
810810
expr.Expr,
811811
annotateType,
@@ -2302,13 +2302,17 @@ func typeCheckAndRequireTupleElems(
23022302
}
23032303

23042304
func typeCheckAndRequireBoolean(
2305-
ctx context.Context, semaCtx *SemaContext, expr Expr, op string,
2305+
ctx context.Context, semaCtx *SemaContext, expr Expr, op redact.RedactableString,
23062306
) (TypedExpr, error) {
23072307
return typeCheckAndRequire(ctx, semaCtx, expr, types.Bool, op)
23082308
}
23092309

23102310
func typeCheckAndRequire(
2311-
ctx context.Context, semaCtx *SemaContext, expr Expr, required *types.T, op string,
2311+
ctx context.Context,
2312+
semaCtx *SemaContext,
2313+
expr Expr,
2314+
required *types.T,
2315+
op redact.RedactableString,
23122316
) (TypedExpr, error) {
23132317
typedExpr, err := expr.TypeCheck(ctx, semaCtx, required)
23142318
if err != nil {
@@ -2368,7 +2372,7 @@ func typeCheckComparisonOpWithSubOperator(
23682372

23692373
typedSubExprs, retType, err := typeCheckSameTypedExprs(ctx, semaCtx, types.AnyElement, sameTypeExprs...)
23702374
if err != nil {
2371-
sigWithErr := fmt.Sprintf(compExprsWithSubOpFmt, left, subOp, op, right, err)
2375+
sigWithErr := redact.Sprintf(compExprsWithSubOpFmt, left, subOp, op, right, err)
23722376
return nil, nil, nil, false,
23732377
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
23742378
}
@@ -2446,8 +2450,8 @@ func typeCheckComparisonOpWithSubOperator(
24462450
cmpTypeRight = rightReturn.TupleContents()[0]
24472451
}
24482452
default:
2449-
sigWithErr := fmt.Sprintf(compExprsWithSubOpFmt, left, subOp, op, right,
2450-
fmt.Sprintf("op %s <right> requires array, tuple or subquery on right side", op))
2453+
sigWithErr := redact.Sprintf(compExprsWithSubOpFmt, left, subOp, op, right,
2454+
redact.Sprintf("op %s <right> requires array, tuple or subquery on right side", op))
24512455
return nil, nil, nil, false, pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
24522456
}
24532457
}
@@ -2480,7 +2484,7 @@ func deepCheckValidCmpOp(ops *CmpOpOverloads, leftType, rightType *types.T) bool
24802484
}
24812485

24822486
func subOpCompError(leftType, rightType *types.T, subOp, op treecmp.ComparisonOperator) error {
2483-
sig := fmt.Sprintf(compSignatureWithSubOpFmt, leftType, subOp, op, rightType)
2487+
sig := redact.Sprintf(compSignatureWithSubOpFmt, leftType, subOp, op, rightType)
24842488
return pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sig)
24852489
}
24862490

@@ -2543,13 +2547,13 @@ func typeCheckComparisonOp(
25432547
disallowSwitch = true
25442548
typedLeft, err = foldedLeft.TypeCheck(ctx, semaCtx, types.AnyElement)
25452549
if err != nil {
2546-
sigWithErr := fmt.Sprintf(compExprsFmt, left, op, right, err)
2550+
sigWithErr := redact.Sprintf(compExprsFmt, left, op, right, err)
25472551
return nil, nil, nil, false,
25482552
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
25492553
}
25502554
typedRight, err = foldedRight.TypeCheck(ctx, semaCtx, types.AnyElement)
25512555
if err != nil {
2552-
sigWithErr := fmt.Sprintf(compExprsFmt, left, op, right, err)
2556+
sigWithErr := redact.Sprintf(compExprsFmt, left, op, right, err)
25532557
return nil, nil, nil, false,
25542558
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
25552559
}
@@ -2570,14 +2574,14 @@ func typeCheckComparisonOp(
25702574

25712575
typedSubExprs, retType, err := typeCheckSameTypedExprs(ctx, semaCtx, types.AnyElement, sameTypeExprs...)
25722576
if err != nil {
2573-
sigWithErr := fmt.Sprintf(compExprsFmt, left, op, right, err)
2577+
sigWithErr := redact.Sprintf(compExprsFmt, left, op, right, err)
25742578
return nil, nil, nil, false,
25752579
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
25762580
}
25772581

25782582
fn, ok := ops.LookupImpl(retType, types.AnyTuple)
25792583
if !ok {
2580-
sig := fmt.Sprintf(compSignatureFmt, retType, op, types.AnyTuple)
2584+
sig := redact.Sprintf(compSignatureFmt, retType, op, types.AnyTuple)
25812585
return nil, nil, nil, false,
25822586
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sig)
25832587
}
@@ -2598,15 +2602,15 @@ func typeCheckComparisonOp(
25982602
case foldedOp.Symbol == treecmp.In && rightIsSubquery:
25992603
typedLeft, err = foldedLeft.TypeCheck(ctx, semaCtx, types.AnyElement)
26002604
if err != nil {
2601-
sigWithErr := fmt.Sprintf(compExprsFmt, left, op, right, err)
2605+
sigWithErr := redact.Sprintf(compExprsFmt, left, op, right, err)
26022606
return nil, nil, nil, false,
26032607
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
26042608
}
26052609

26062610
typ := typedLeft.ResolvedType()
26072611
fn, ok := ops.LookupImpl(typ, types.AnyTuple)
26082612
if !ok {
2609-
sig := fmt.Sprintf(compSignatureFmt, typ, op, types.AnyTuple)
2613+
sig := redact.Sprintf(compSignatureFmt, typ, op, types.AnyTuple)
26102614
return nil, nil, nil, false,
26112615
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sig)
26122616
}
@@ -2618,7 +2622,7 @@ func typeCheckComparisonOp(
26182622

26192623
typedRight, err = foldedRight.TypeCheck(ctx, semaCtx, desired)
26202624
if err != nil {
2621-
sigWithErr := fmt.Sprintf(compExprsFmt, left, op, right, err)
2625+
sigWithErr := redact.Sprintf(compExprsFmt, left, op, right, err)
26222626
return nil, nil, nil, false,
26232627
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sigWithErr)
26242628
}
@@ -2633,7 +2637,7 @@ func typeCheckComparisonOp(
26332637
case leftIsTuple && rightIsTuple:
26342638
fn, ok := ops.LookupImpl(types.AnyTuple, types.AnyTuple)
26352639
if !ok {
2636-
sig := fmt.Sprintf(compSignatureFmt, types.AnyTuple, op, types.AnyTuple)
2640+
sig := redact.Sprintf(compSignatureFmt, types.AnyTuple, op, types.AnyTuple)
26372641
return nil, nil, nil, false,
26382642
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedCompErrFmt, sig)
26392643
}
@@ -2784,7 +2788,7 @@ func typeCheckComparisonOp(
27842788
// Throw a typing error if overload resolution found either no compatible candidates
27852789
// or if it found an ambiguity.
27862790
if len(s.overloadIdxs) != 1 || typeMismatch {
2787-
sig := fmt.Sprintf(compSignatureFmt, leftReturn, op, rightReturn)
2791+
sig := redact.Sprintf(compSignatureFmt, leftReturn, op, rightReturn)
27882792
if len(s.overloadIdxs) == 0 || typeMismatch {
27892793
// For some typeMismatch errors, we want to emit a more specific error
27902794
// message than "unknown comparison". In particular, comparison between

‎pkg/sql/types/types.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2975,6 +2975,10 @@ func (t *T) String() string {
29752975
return t.Name()
29762976
}
29772977

2978+
// SafeValue implements redact.SafeValue.
2979+
// SQL type names are language constructs and safe to log.
2980+
func (*T) SafeValue() {}
2981+
29782982
// MarshalText is implemented here so that gogo/protobuf know how to text marshal
29792983
// protobuf struct directly/indirectly depends on types.T without panic.
29802984
func (t *T) MarshalText() (text []byte, err error) {
@@ -3381,3 +3385,6 @@ func (t *T) Delimiter() string {
33813385
return ","
33823386
}
33833387
}
3388+
3389+
// Verify that T implements redact.SafeValue.
3390+
var _ redact.SafeValue = (*T)(nil)

‎pkg/testutils/lint/passes/redactcheck/redactcheck.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,16 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) {
242242
"PolicyType": {},
243243
"TableRLSMode": {},
244244
},
245+
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp": {
246+
"ComparisonOperator": {},
247+
"ComparisonOperatorSymbol": {},
248+
},
245249
"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness": {
246250
"SessionID": {},
247251
},
252+
"github.com/cockroachdb/cockroach/pkg/sql/types": {
253+
"T": {},
254+
},
248255
"github.com/cockroachdb/cockroach/pkg/storage/enginepb": {
249256
"MVCCStats": {},
250257
"MVCCStatsDelta": {},

0 commit comments

Comments
 (0)