Skip to content

Commit

Permalink
funcr: Handle panic in Stringer, Marshaler, error
Browse files Browse the repository at this point in the history
Produce a maybe-useful error rather than panic when we are in the middle
of logging a value (similar to fmt.Printf).
  • Loading branch information
thockin committed Jan 29, 2022
1 parent af7b868 commit 945d619
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 9 deletions.
33 changes: 30 additions & 3 deletions funcr/funcr.go
Original file line number Diff line number Diff line change
Expand Up @@ -351,15 +351,15 @@ func (f Formatter) prettyWithFlags(value interface{}, flags uint32, depth int) s
if v, ok := value.(logr.Marshaler); ok {
// Replace the value with what the type wants to get logged.
// That then gets handled below via reflection.
value = v.MarshalLog()
value = invokeMarshaler(v)
}

// Handle types that want to format themselves.
switch v := value.(type) {
case fmt.Stringer:
value = v.String()
value = invokeStringer(v)
case error:
value = v.Error()
value = invokeError(v)
}

// Handling the most common types without reflect is a small perf win.
Expand Down Expand Up @@ -597,6 +597,33 @@ func isEmpty(v reflect.Value) bool {
return false
}

func invokeMarshaler(m logr.Marshaler) (ret interface{}) {
defer func() {
if r := recover(); r != nil {
ret = fmt.Sprintf("<panic: %s>", r)
}
}()
return m.MarshalLog()
}

func invokeStringer(s fmt.Stringer) (ret string) {
defer func() {
if r := recover(); r != nil {
ret = fmt.Sprintf("<panic: %s>", r)
}
}()
return s.String()
}

func invokeError(e error) (ret string) {
defer func() {
if r := recover(); r != nil {
ret = fmt.Sprintf("<panic: %s>", r)
}
}()
return e.Error()
}

// Caller represents the original call site for a log line, after considering
// logr.Logger.WithCallDepth and logr.Logger.WithCallStackHelper. The File and
// Line fields will always be provided, while the Func field is optional.
Expand Down
76 changes: 70 additions & 6 deletions funcr/funcr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (p pointErr) MarshalText() ([]byte, error) {
}

// Logging this should result in the MarshalLog() value.
type Tmarshaler string
type Tmarshaler struct{ val string }

func (t Tmarshaler) MarshalLog() interface{} {
return struct{ Inner string }{"I am a logr.Marshaler"}
Expand All @@ -66,8 +66,15 @@ func (t Tmarshaler) Error() string {
return "Error(): you should not see this"
}

// Logging this should result in a panic.
type Tmarshalerpanic struct{ val string }

func (t Tmarshalerpanic) MarshalLog() interface{} {
panic("Tmarshalerpanic")
}

// Logging this should result in the String() value.
type Tstringer string
type Tstringer struct{ val string }

func (t Tstringer) String() string {
return "I am a fmt.Stringer"
Expand All @@ -77,6 +84,27 @@ func (t Tstringer) Error() string {
return "Error(): you should not see this"
}

// Logging this should result in a panic.
type Tstringerpanic struct{ val string }

func (t Tstringerpanic) String() string {
panic("Tstringerpanic")
}

// Logging this should result in the Error() value.
type Terror struct{ val string }

func (t Terror) Error() string {
return "I am an error"
}

// Logging this should result in a panic.
type Terrorpanic struct{ val string }

func (t Terrorpanic) Error() string {
panic("Terrorpanic")
}

type TjsontagsString struct {
String1 string `json:"string1"` // renamed
String2 string `json:"-"` // ignored
Expand Down Expand Up @@ -351,16 +379,52 @@ func TestPretty(t *testing.T) {
},
},
{
val: Tmarshaler("foobar"),
val: Tmarshaler{"foobar"},
exp: `{"Inner":"I am a logr.Marshaler"}`,
},
{
val: Tstringer("foobar"),
val: &Tmarshaler{"foobar"},
exp: `{"Inner":"I am a logr.Marshaler"}`,
},
{
val: (*Tmarshaler)(nil),
exp: `"<panic: value method github.com/go-logr/logr/funcr.Tmarshaler.MarshalLog called using nil *Tmarshaler pointer>"`,
},
{
val: Tmarshalerpanic{"foobar"},
exp: `"<panic: Tmarshalerpanic>"`,
},
{
val: Tstringer{"foobar"},
exp: `"I am a fmt.Stringer"`,
},
{
val: &Tstringer{"foobar"},
exp: `"I am a fmt.Stringer"`,
},
{
val: fmt.Errorf("error"),
exp: `"error"`,
val: (*Tstringer)(nil),
exp: `"<panic: value method github.com/go-logr/logr/funcr.Tstringer.String called using nil *Tstringer pointer>"`,
},
{
val: Tstringerpanic{"foobar"},
exp: `"<panic: Tstringerpanic>"`,
},
{
val: Terror{"foobar"},
exp: `"I am an error"`,
},
{
val: &Terror{"foobar"},
exp: `"I am an error"`,
},
{
val: (*Terror)(nil),
exp: `"<panic: value method github.com/go-logr/logr/funcr.Terror.Error called using nil *Terror pointer>"`,
},
{
val: Terrorpanic{"foobar"},
exp: `"<panic: Terrorpanic>"`,
},
{
val: TjsontagsString{
Expand Down

0 comments on commit 945d619

Please sign in to comment.