Merge cockroachdb#138011

138011: schemachanger: mark notImplementedError with errors.SafeFormatter r=rafiss a=rafiss This allows the error to be printed without redacting the entire text. Epic: None Release note: None Co-authored-by: Rafi Shamim <[email protected]>
rytaft · Dec 30, 2024 · 6f8bbe1 · 6f8bbe1
2 parents 8735d91 + 77dabe0
commit 6f8bbe1
Show file tree

Hide file tree

Showing 7 changed files with 60 additions and 17 deletions.
diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go
@@ -43,6 +43,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 	"github.com/lib/pq/oid"
 )
 
@@ -110,8 +111,13 @@ func (b *builderState) Ensure(e scpb.Element, target scpb.TargetStatus, meta scp
 		dst.target == scpb.ToAbsent &&
 		(target == scpb.ToPublic || target == scpb.Transient) &&
 		dst.metadata.IsLinkedToSchemaChange() {
-		panic(scerrors.NotImplementedErrorf(nil, "attempt to revive a ghost element:"+
-			" [elem=%v],[current=ABSENT],[target=ToAbsent],[newTarget=%v]", dst.element.String(), target.Status()))
+		panic(scerrors.NotImplementedErrorf(
+			nil,
+			redact.Sprintf(
+				"attempt to revive a ghost element: [elem=%v],[current=ABSENT],[target=ToAbsent],[newTarget=%v]",
+				dst.element.String(), target.Status(),
+			),
+		))
 	}
 
 	// Henceforth all possibilities lead to the target and metadata being
@@ -1472,8 +1478,10 @@ func (b *builderState) resolveBackReferences(c *cachedDesc) {
 	case catalog.DatabaseDescriptor:
 		if !d.HasPublicSchemaWithDescriptor() {
 			panic(scerrors.NotImplementedErrorf(nil, /* n */
-				"database %q (%d) with a descriptorless public schema",
-				d.GetName(), d.GetID()))
+				redact.Sprintf(
+					"database %q (%d) with a descriptorless public schema",
+					d.GetName(), d.GetID()),
+			))
 		}
 		// Handle special case of database children, which may include temporary
 		// schemas, which aren't explicitly referenced in the database's schemas
@@ -1606,7 +1614,7 @@ func (b *builderState) readDescriptor(id catid.DescID) catalog.Descriptor {
 		panic(errors.AssertionFailedf("invalid descriptor ID %d", id))
 	}
 	if id == keys.SystemPublicSchemaID || id == keys.PublicSchemaIDForBackup || id == keys.PublicSchemaID {
-		panic(scerrors.NotImplementedErrorf(nil /* n */, "descriptorless public schema %d", id))
+		panic(scerrors.NotImplementedErrorf(nil /* n */, redact.Sprintf("descriptorless public schema %d", id)))
 	}
 	if tempSchema := b.tempSchemas[id]; tempSchema != nil {
 		return tempSchema

diff --git a/pkg/sql/schemachanger/scdecomp/BUILD.bazel b/pkg/sql/schemachanger/scdecomp/BUILD.bazel
@@ -29,6 +29,7 @@ go_library(
         "//pkg/util/iterutil",
         "//pkg/util/protoutil",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
         "@com_github_lib_pq//oid",
     ],
 )

diff --git a/pkg/sql/schemachanger/scdecomp/helpers.go b/pkg/sql/schemachanger/scdecomp/helpers.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 	"github.com/lib/pq/oid"
 )
 
@@ -71,7 +72,8 @@ func (w *walkCtx) newExpression(expr string) (*scpb.Expression, error) {
 		for _, si := range seqIdents {
 			if !si.IsByID() {
 				panic(scerrors.NotImplementedErrorf(nil, /* n */
-					"sequence %q referenced by name", si.SeqName))
+					redact.Sprintf("sequence %q referenced by name", si.SeqName),
+				))
 			}
 			seqIDs.Add(descpb.ID(si.SeqID))
 		}

diff --git a/pkg/sql/schemachanger/scerrors/errors.go b/pkg/sql/schemachanger/scerrors/errors.go
@@ -9,7 +9,6 @@ import (
 	"context"
 	"fmt"
 	"runtime"
-	"strings"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -88,12 +87,11 @@ func (el EventLogger) HandlePanicAndLogError(ctx context.Context, err *error) {
 
 type notImplementedError struct {
 	n      tree.NodeFormatter
-	detail string
+	detail redact.RedactableString
 }
 
-// TODO(ajwerner): Deal with redaction.
-
 var _ error = (*notImplementedError)(nil)
+var _ errors.SafeFormatter = (*notImplementedError)(nil)
 
 // HasNotImplemented returns true if the error indicates that the builder does
 // not support the provided statement.
@@ -102,12 +100,21 @@ func HasNotImplemented(err error) bool {
 }
 
 func (e *notImplementedError) Error() string {
-	var buf strings.Builder
-	fmt.Fprintf(&buf, "%T not implemented in the new schema changer", e.n)
+	return redact.Sprint(e).StripMarkers()
+}
+
+// SafeFormatError implements the errors.SafeFormatter interface.
+func (e *notImplementedError) SafeFormatError(p errors.Printer) (next error) {
+	p.Printf("%T not implemented in the new schema changer", e.n)
 	if e.detail != "" {
-		fmt.Fprintf(&buf, ": %s", e.detail)
+		p.Printf(": %s", e.detail)
 	}
-	return buf.String()
+	return nil
+}
+
+// Format implements fmt.Formatter.
+func (e *notImplementedError) Format(s fmt.State, verb rune) {
+	errors.FormatError(e, s, verb)
 }
 
 // NotImplementedError returns an error for which HasNotImplemented would
@@ -118,8 +125,11 @@ func NotImplementedError(n tree.NodeFormatter) error {
 
 // NotImplementedErrorf returns an error for which HasNotImplemented would
 // return true.
-func NotImplementedErrorf(n tree.NodeFormatter, fmtstr string, args ...interface{}) error {
-	return &notImplementedError{n: n, detail: fmt.Sprintf(fmtstr, args...)}
+func NotImplementedErrorf(n tree.NodeFormatter, detail redact.RedactableString) error {
+	return &notImplementedError{
+		n:      n,
+		detail: detail,
+	}
 }
 
 // concurrentSchemaChangeError indicates that building the schema change plan

diff --git a/pkg/sql/schemachanger/scpb/BUILD.bazel b/pkg/sql/schemachanger/scpb/BUILD.bazel
@@ -26,6 +26,7 @@ go_library(
         "//pkg/util/debugutil",
         "//pkg/util/protoutil",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
     ],
 )
 

diff --git a/pkg/sql/schemachanger/scpb/constants.go b/pkg/sql/schemachanger/scpb/constants.go
@@ -5,7 +5,10 @@
 
 package scpb
 
-import "github.com/cockroachdb/errors"
+import (
+	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
+)
 
 const (
 	// PlaceHolderRoleName placeholder string for non-fetched role names.
@@ -16,6 +19,15 @@ const (
 // statuses.
 type TargetStatus Status
 
+var _ redact.SafeValue = Status(0)
+var _ redact.SafeValue = TargetStatus(0)
+
+// SafeValue implements the redact.SafeValue interface.
+func (t TargetStatus) SafeValue() {}
+
+// SafeValue implements the redact.SafeValue interface.
+func (s Status) SafeValue() {}
+
 const (
 	// InvalidTarget indicates that the element doesn't have a target status
 	// for the current schema change.
@@ -38,6 +50,11 @@ const (
 	Transient TargetStatus = TargetStatus(Status_TRANSIENT_ABSENT)
 )
 
+// Status returns the TargetStatus as a Status.
+func (t TargetStatus) String() string {
+	return Status(t).String()
+}
+
 // Status returns the TargetStatus as a Status.
 func (t TargetStatus) Status() Status {
 	return Status(t)

diff --git a/pkg/testutils/lint/passes/redactcheck/redactcheck.go b/pkg/testutils/lint/passes/redactcheck/redactcheck.go
@@ -211,6 +211,10 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) {
 						"Phase": {},
 						"Type":  {},
 					},
+					"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb": {
+						"Status":       {},
+						"TargetStatus": {},
+					},
 					"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scplan/internal/scgraph": {
 						"RuleName": {},
 					},