From 9dae2c7f1a074b811a7f4087b49288a75fef9ff9 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 4 Jan 2026 10:55:14 +0100 Subject: [PATCH 1/3] doc: fixed a few hiccups in README Signed-off-by: Frederic BIDON --- README.md | 2 +- docs/doc-site/_index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6d95eb653..adf1d08ac 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,7 @@ Becomes: ```go import ( "testing" - "github.com/go-openapi/testify/v2" + "github.com/go-openapi/testify/v2/require" ) ... diff --git a/docs/doc-site/_index.md b/docs/doc-site/_index.md index 172401bdc..5e2257725 100644 --- a/docs/doc-site/_index.md +++ b/docs/doc-site/_index.md @@ -86,7 +86,7 @@ To use this package in your projects: ```go import ( "testing" - "github.com/go-openapi/testify/v2" + "github.com/go-openapi/testify/v2/require" ) ... From 70fe1c5c1764bf56e98d8d8069db461fd8559368 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 4 Jan 2026 10:55:23 +0100 Subject: [PATCH 2/3] fix: rendering time values This is inspired by https://github.com/stretchr/testify/pull/1829, but we proceed differently, not checking for a string type but for type convertibility to a time instead. Added more tests to check how embedded types, pointers etc actually render and don't cause panic. From the original issues: * fixes stretchr/testify#1078 * fixes stretchr/testify#1079 Signed-off-by: Frederic BIDON --- internal/spew/common.go | 70 +++++++++++++++++++++++---- internal/spew/config.go | 11 ++++- internal/spew/dump.go | 2 +- internal/spew/dump_test.go | 94 ++++++++++++++++++++++++++++++++++++- internal/spew/format.go | 2 +- internal/spew/spew_test.go | 69 ++++++++++++++++++++++++--- internal/spew/time.go | 47 +++++++++++++++++++ internal/spew/time_test.go | 96 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 372 insertions(+), 19 deletions(-) create mode 100644 internal/spew/time.go create mode 100644 internal/spew/time_test.go diff --git a/internal/spew/common.go b/internal/spew/common.go index 2151e3395..84c7390e0 100644 --- a/internal/spew/common.go +++ b/internal/spew/common.go @@ -98,47 +98,99 @@ func handleMethods(cs *ConfigState, w io.Writer, v reflect.Value) (handled bool) v = unsafeReflectValue(v) } + defer catchPanic(w, v) + handled, continued := handleErrorOrStringer(cs, w, v) + if handled { + // short-circuit: if we can handle directly without trying to convert to a pointer receiver, we're done. + // This allows avoiding to call unsafeReflectValue() or retrieving the value's address when not necessary. + return true + } + if continued { + // printed, and wants to return now to continue digging + return false + } + // Choose whether or not to do error and Stringer interface lookups against // the base type or a pointer to the base type depending on settings. + // // Technically calling one of these methods with a pointer receiver can - // mutate the value, however, types which choose to satisify an error or + // mutate the value, however, types which choose to satisfy an error or // Stringer interface with a pointer receiver should not be mutating their // state inside these interface methods. if !cs.DisablePointerMethods && !UnsafeDisabled && !v.CanAddr() { + // since this is unsafe, there are a few edge cases where it doesn't work well v = unsafeReflectValue(v) } + if v.CanAddr() { v = v.Addr() } + handled, _ = handleErrorOrStringer(cs, w, v) + + return handled +} + +// handleErrorOrString is only called when v.CanInterface(). +// +// NOTE: we should prove that unsafReflectValue doesn't alter this property. +func handleErrorOrStringer(cs *ConfigState, w io.Writer, v reflect.Value) (handled, continued bool) { // Is it an error or Stringer? switch iface := v.Interface().(type) { case error: - defer catchPanic(w, v) if cs.ContinueOnMethod { w.Write(openParenBytes) w.Write([]byte(iface.Error())) w.Write(closeParenBytes) w.Write(spaceBytes) - return false + return false, true } w.Write([]byte(iface.Error())) - return true + return true, false case fmt.Stringer: - defer catchPanic(w, v) if cs.ContinueOnMethod { w.Write(openParenBytes) w.Write([]byte(iface.String())) w.Write(closeParenBytes) w.Write(spaceBytes) - return false + return false, true } + w.Write([]byte(iface.String())) - return true + return true, false + + default: + // is it convertible to time.Time (or *time.Time)? + converted, ok := isConvertibleToTime(v) + if !ok { + // can't handle this value + return false, false + } + + if !converted.CanInterface() { + return false, false // safeguard + } + + timeIface := converted.Interface() + stringer, ok := timeIface.(fmt.Stringer) + if !ok { + return false, false // safeguard + } + + if cs.ContinueOnMethod { + _, _ = w.Write(openParenBytes) + _, _ = w.Write([]byte(stringer.String())) + _, _ = w.Write(closeParenBytes) + _, _ = w.Write(spaceBytes) + return false, true + } + + _, _ = w.Write([]byte(stringer.String())) + + return true, false } - return false } // printBool outputs a boolean value as true or false to Writer w. @@ -242,12 +294,14 @@ func newValuesSorter(values []reflect.Value, cs *ConfigState) sort.Interface { vs.strings[i] = b.String() } } + if vs.strings == nil && cs.SpewKeys { vs.strings = make([]string, len(values)) for i := range vs.values { vs.strings[i] = Sprintf("%#v", vs.values[i].Interface()) } } + return vs } diff --git a/internal/spew/config.go b/internal/spew/config.go index c76a91035..aa57909b6 100644 --- a/internal/spew/config.go +++ b/internal/spew/config.go @@ -53,6 +53,12 @@ type ConfigState struct { // invoked for types that implement them. DisableMethods bool + // EnableTimeStringer specifies whether to invoke the Stringer interface on + // time.Time values even when DisableMethods is true. This is useful to get + // human-readable output for time.Time values while keeping method calls + // disabled for other types. + EnableTimeStringer bool + // DisablePointerMethods specifies whether or not to check for and invoke // error and Stringer interfaces on types which only accept a pointer // receiver when the current type is not a pointer. @@ -102,7 +108,10 @@ type ConfigState struct { // Config is the active configuration of the top-level functions. // The configuration can be changed by modifying the contents of spew.Config. -var Config = ConfigState{Indent: " "} +var Config = ConfigState{ //nolint:gochecknoglobals // this is global configuration and we leave it for backward-compatibility + Indent: " ", + EnableTimeStringer: true, +} // Errorf is a wrapper for fmt.Errorf that treats each argument as if it were // passed with a Formatter interface returned by c.NewFormatter. It returns diff --git a/internal/spew/dump.go b/internal/spew/dump.go index adc14cfbc..d2402216a 100644 --- a/internal/spew/dump.go +++ b/internal/spew/dump.go @@ -303,7 +303,7 @@ func (d *dumpState) dump(v reflect.Value) { // Call Stringer/error interfaces if they exist and the handle methods flag // is enabled - if !d.cs.DisableMethods { + if !d.cs.DisableMethods || (d.cs.EnableTimeStringer && isTime(v)) { if (kind != reflect.Invalid) && (kind != reflect.Interface) { if handled := handleMethods(d.cs, d.w, v); handled { return diff --git a/internal/spew/dump_test.go b/internal/spew/dump_test.go index 3f0c6439d..a21012b40 100644 --- a/internal/spew/dump_test.go +++ b/internal/spew/dump_test.go @@ -66,6 +66,7 @@ import ( "fmt" "strconv" "testing" + "time" "unsafe" "github.com/go-openapi/testify/v2/internal/spew" @@ -957,6 +958,91 @@ func addErrorDumpTests() { addDumpTest(nv, "(*"+vt+")()\n") } +type ( + embeddedTime struct { + time.Time + } + embeddedTimePtr struct { // this type is an example where the unsafeReflectValue does not work well + *time.Time + } + redeclaredTime time.Time + redeclaredTimePtr *time.Time + aliasedTime = time.Time +) + +func addTimeDumpTests() { + ts := time.Date(2006, time.January, 2, 15, 4, 5, 999999999, time.UTC) + alias := aliasedTime(ts) //nolint:unconvert // we want to prove here that aliased types don't matter + tsAddr := fmt.Sprintf("%p", &ts) + es := embeddedTime{ + Time: ts, + } + ps := embeddedTimePtr{ + Time: &ts, + } + var tptr *time.Time + panick := embeddedTimePtr{ + Time: nil, + } + rtptr := redeclaredTimePtr(&ts) + ppts := ptr(ptr(ts)) + ppAddr := fmt.Sprintf("%p", ppts) + ppIAddr := fmt.Sprintf("%p", *ppts) + + addDumpTest( + // simple time.Time + ts, + "(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n", + ) + addDumpTest( + // aliases are ignored at runtime + alias, + "(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n", + ) + addDumpTest( + // pointer to time.Time + &ts, + "(*time.Time)("+tsAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n", + ) + addDumpTest( + // struct with embedded time.Time + es, + "(spew_test.embeddedTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n", + ) + addDumpTest( + // struct with embedded pointer to time.Time + ps, + "(spew_test.embeddedTimePtr) 2006-01-02 15:04:05.999999999 +0000 UTC\n", + ) + addDumpTest( + // nil time.Time + tptr, + "(*time.Time)()\n", + ) + addDumpTest( + // **time.Time + ppts, + "(**time.Time)("+ppAddr+"->"+ppIAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n", + ) + addDumpTest( + panick, // this is a stringer, but the inner member that implements String() string is nil + "(spew_test.embeddedTimePtr) (PANIC=runtime error: invalid memory address or nil pointer dereference){\n Time: (*time.Time)()\n}\n", + ) + addDumpTest( + // redeclared type convertible to time.Time + redeclaredTime(ts), + "(spew_test.redeclaredTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n", + ) + addDumpTest( + // redeclared type convertible to *time.Time + // + // NOTE: the information about the original (redeclared) type is lost. This is due to + // how displaying pointer type information is displayed (i.e. using v.Elem().Type()). + rtptr, + "(*time.Time)("+tsAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n", + ) +} + // TestDump executes all of the tests described by dumpTests. func TestDump(t *testing.T) { // Setup tests. @@ -979,6 +1065,7 @@ func TestDump(t *testing.T) { addPanicDumpTests() addErrorDumpTests() addCgoDumpTests() + addTimeDumpTests() t.Logf("Running %d tests", len(dumpTests)) for i, test := range dumpTests { @@ -986,7 +1073,7 @@ func TestDump(t *testing.T) { spew.Fdump(buf, test.in) s := buf.String() if testFailed(s, test.wants) { - t.Errorf("Dump #%d\n got: %s %s", i, s, stringizeWants(test.wants)) + t.Errorf("Dump #%d\n got: %s %s", i, s, stringizeWants(test.wants)) continue } } @@ -1041,3 +1128,8 @@ func TestDumpSortedKeys(t *testing.T) { } } + +func ptr[T any](value T) *T { + v := value + return &v +} diff --git a/internal/spew/format.go b/internal/spew/format.go index c92a38ea9..fd22d1d91 100644 --- a/internal/spew/format.go +++ b/internal/spew/format.go @@ -222,7 +222,7 @@ func (f *formatState) format(v reflect.Value) { // Call Stringer/error interfaces if they exist and the handle methods // flag is enabled. - if !f.cs.DisableMethods { + if !f.cs.DisableMethods || (f.cs.EnableTimeStringer && isTime(v)) { // we consider the case when we want times printed out if (kind != reflect.Invalid) && (kind != reflect.Interface) { if handled := handleMethods(f.cs, f.fs, v); handled { return diff --git a/internal/spew/spew_test.go b/internal/spew/spew_test.go index 2e12c5236..afba821e3 100644 --- a/internal/spew/spew_test.go +++ b/internal/spew/spew_test.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "testing" + "time" "github.com/go-openapi/testify/v2/internal/spew" ) @@ -126,6 +127,7 @@ func initSpewTests() { // Config states with various settings. scsDefault := spew.NewDefaultConfig() scsNoMethods := &spew.ConfigState{Indent: " ", DisableMethods: true} + scsNoMethodsButTimeStringer := &spew.ConfigState{Indent: " ", DisableMethods: true, EnableTimeStringer: true} scsNoPmethods := &spew.ConfigState{Indent: " ", DisablePointerMethods: true} scsMaxDepth := &spew.ConfigState{Indent: " ", MaxDepth: 1} scsContinue := &spew.ConfigState{Indent: " ", ContinueOnMethod: true} @@ -150,13 +152,22 @@ func initSpewTests() { slice []string m map[string]int } - dt := depthTester{indirCir1{nil}, [1]string{"arr"}, []string{"slice"}, - map[string]int{"one": 1}} + dt := depthTester{ + indirCir1{nil}, + [1]string{"arr"}, + []string{"slice"}, + map[string]int{"one": 1}, + } // Variable for tests on types which implement error interface. te := customError(10) + tm := time.Date(2006, time.January, 2, 15, 4, 5, 999999999, time.UTC) + tmAddr := fmt.Sprintf("%p", &tm) + em := embeddedTime{Time: tm} + emptr := embeddedTimePtr{Time: &tm} spewTests = []spewTest{ + // default config {scsDefault, fCSFdump, "", int8(127), "(int8) 127\n"}, {scsDefault, fCSFprint, "", int16(32767), "32767"}, {scsDefault, fCSFprintf, "%v", int32(2147483647), "2147483647"}, @@ -178,6 +189,7 @@ func initSpewTests() { {scsDefault, fSprint, "", complex(-1, -2), "(-1-2i)"}, {scsDefault, fSprintf, "%v", complex(float32(-3), -4), "(-3-4i)"}, {scsDefault, fSprintln, "", complex(float64(-5), -6), "(-5-6i)\n"}, + // config with no methods {scsNoMethods, fCSFprint, "", ts, "test"}, {scsNoMethods, fCSFprint, "", &ts, "<*>test"}, {scsNoMethods, fCSFprint, "", tps, "test"}, @@ -186,22 +198,65 @@ func initSpewTests() { {scsNoPmethods, fCSFprint, "", &ts, "<*>stringer test"}, {scsNoPmethods, fCSFprint, "", tps, "test"}, {scsNoPmethods, fCSFprint, "", &tps, "<*>stringer test"}, + // config with maxdepth {scsMaxDepth, fCSFprint, "", dt, "{{} [] [] map[]}"}, {scsMaxDepth, fCSFdump, "", dt, "(spew_test.depthTester) {\n" + " ic: (spew_test.indirCir1) {\n \n },\n" + " arr: ([1]string) (len=1 cap=1) {\n \n },\n" + " slice: ([]string) (len=1 cap=1) {\n \n },\n" + " m: (map[string]int) (len=1) {\n \n }\n}\n"}, + // config with continue on method {scsContinue, fCSFprint, "", ts, "(stringer test) test"}, - {scsContinue, fCSFdump, "", ts, "(spew_test.stringer) " + - "(len=4) (stringer test) \"test\"\n"}, + {scsContinue, fCSFdump, "", ts, "(spew_test.stringer) (len=4) (stringer test) \"test\"\n"}, {scsContinue, fCSFprint, "", te, "(error: 10) 10"}, - {scsContinue, fCSFdump, "", te, "(spew_test.customError) " + - "(error: 10) 10\n"}, + {scsContinue, fCSFdump, "", te, "(spew_test.customError) (error: 10) 10\n"}, {scsNoPtrAddr, fCSFprint, "", tptr, "<*>{<*>{}}"}, {scsNoPtrAddr, fCSSdump, "", tptr, "(*spew_test.ptrTester)({\ns: (*struct {})({\n})\n})\n"}, {scsNoCap, fCSSdump, "", make([]string, 0, 10), "([]string) {\n}\n"}, {scsNoCap, fCSSdump, "", make([]string, 1, 10), "([]string) (len=1) {\n(string) \"\"\n}\n"}, + // + // time.Time formatting with all configs + {scsDefault, fCSFprint, "", tm, "2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsDefault, fCSFdump, "", tm, "(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n"}, + {scsDefault, fCSFprint, "", &tm, "<*>2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsDefault, fCSFdump, "", &tm, "(*time.Time)(" + tmAddr + ")(2006-01-02 15:04:05.999999999 +0000 UTC)\n"}, + {scsDefault, fCSFprint, "", em, "2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsDefault, fCSFdump, "", em, "(spew_test.embeddedTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n"}, + {scsDefault, fCSFprint, "", emptr, "2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsDefault, fCSFdump, "", emptr, "(spew_test.embeddedTimePtr) 2006-01-02 15:04:05.999999999 +0000 UTC\n"}, + {scsNoMethods, fCSFprint, "", tm, "{999999999 63271811045 }"}, + {scsNoMethods, fCSFprint, "", &tm, "<*>{999999999 63271811045 }"}, + {scsNoMethods, fCSFprint, "", em, "{{999999999 63271811045 }}"}, + {scsNoMethods, fCSFprint, "", emptr, "{<*>{999999999 63271811045 }}"}, // NOTE: the type of the embedded pointer is not rendered, on purpose + { + scsContinue, fCSFdump, "", tm, // this prints time and continues digging the struct + "(time.Time) (2006-01-02 15:04:05.999999999 +0000 UTC)" + + " {\n wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n}\n", + }, + { + scsContinue, fCSFdump, "", &tm, // this prints time and continues digging the struct + "(*time.Time)(" + tmAddr + ")((2006-01-02 15:04:05.999999999 +0000 UTC)" + + " {\n wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n})\n", + }, + { + scsContinue, fCSFdump, "", em, // this prints time and continues digging the struct + "(spew_test.embeddedTime) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + + " Time: (time.Time) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + + " wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n }\n}\n", + }, + { + scsContinue, fCSFdump, "", emptr, // this prints time and continues digging the struct + "(spew_test.embeddedTimePtr) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + + " Time: (*time.Time)(" + tmAddr + ")((2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + + " wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n })\n}\n", + }, + {scsNoMethodsButTimeStringer, fCSFprint, "", tm, "2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsNoMethodsButTimeStringer, fCSFdump, "", tm, "(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n"}, + {scsNoMethodsButTimeStringer, fCSFprint, "", &tm, "<*>2006-01-02 15:04:05.999999999 +0000 UTC"}, + {scsNoMethodsButTimeStringer, fCSFprint, "", em, "{2006-01-02 15:04:05.999999999 +0000 UTC}"}, + {scsNoMethodsButTimeStringer, fCSFdump, "", em, "(spew_test.embeddedTime) {\n Time: (time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n}\n"}, + {scsNoMethodsButTimeStringer, fCSFprint, "", emptr, "{<*>2006-01-02 15:04:05.999999999 +0000 UTC}"}, + {scsNoMethodsButTimeStringer, fCSFdump, "", emptr, "(spew_test.embeddedTimePtr) {\n Time: (*time.Time)(" + tmAddr + ")(2006-01-02 15:04:05.999999999 +0000 UTC)\n}\n"}, } } @@ -312,7 +367,7 @@ func TestSpew(t *testing.T) { } s := buf.String() if test.want != s { - t.Errorf("ConfigState #%d\n got: %s want: %s", i, s, test.want) + t.Errorf("ConfigState #%d\n got: %s want: %s", i, s, test.want) continue } } diff --git a/internal/spew/time.go b/internal/spew/time.go new file mode 100644 index 000000000..b52b38785 --- /dev/null +++ b/internal/spew/time.go @@ -0,0 +1,47 @@ +package spew + +import ( + "reflect" + "time" +) + +// isTime detects if a value may be assimilated to time.Time. +// +// It may be time.Time or a *time.Time, +// but also a redeclared type convertible to time.Time or *time.Time, +// +// Conversely, a struct that embeds a time.Time or *time.Time is not considered a time.Time +// and we'll have to dig the individual fields. +func isTime(v reflect.Value) bool { + k := v.Kind() + t := v.Type() + + // for pointers, we reason about the pointer type, because the value may be nil + if k == reflect.Pointer && t.Elem().Kind() == reflect.Pointer { + return isTime(v.Elem()) + } + + if k == reflect.Struct || (k == reflect.Pointer && t.Elem().Kind() == reflect.Struct) { + return v.CanConvert(reflect.TypeFor[time.Time]()) || + v.CanConvert(reflect.TypeFor[*time.Time]()) + } + + return false +} + +// isConvertibleToTime returns a converted reflect.Value and true when v is convertible to time.Time or *time.Time. +func isConvertibleToTime(v reflect.Value) (reflect.Value, bool) { + k := v.Kind() + + timeTyp := reflect.TypeFor[time.Time]() + if k == reflect.Struct && v.CanConvert(timeTyp) { + return v.Convert(timeTyp), true + } + + timePtrTyp := reflect.TypeFor[*time.Time]() + if k == reflect.Pointer && v.Elem().Kind() == reflect.Struct && v.CanConvert(timePtrTyp) { + return v.Convert(timePtrTyp), true + } + + return reflect.Value{}, false // the returned value is Invalid in this case +} diff --git a/internal/spew/time_test.go b/internal/spew/time_test.go new file mode 100644 index 000000000..e99625991 --- /dev/null +++ b/internal/spew/time_test.go @@ -0,0 +1,96 @@ +package spew + +import ( + "iter" + "reflect" + "slices" + "testing" + "time" +) + +func TestIsTime(t *testing.T) { + t.Parallel() + + for tt := range isTimeCases() { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + val := reflect.ValueOf(tt.value) + if result := isTime(val); result != tt.expectedTime { + t.Errorf("expected %v to be considered a time.Time", tt.value) + } + }) + } +} + +type isTimeCase struct { + name string + value any + expectedTime bool +} + +func isTimeCases() iter.Seq[isTimeCase] { + type tr time.Time + type trptr *time.Time + type te struct { + time.Time + } + + return slices.Values([]isTimeCase{ + { + name: "time.Time", + value: time.Now(), + expectedTime: true, + }, + { + name: "*time.Time", + value: ptr(time.Now()), + expectedTime: true, + }, + { + name: "nil *time.Time", + value: (*time.Time)(nil), + expectedTime: true, + }, + { + name: "redefined time.Time", + value: tr{}, + expectedTime: true, + }, + { + name: "pointer to redefined time.Time", + value: &tr{}, + expectedTime: true, + }, + { + name: "pointer to redefined *time.Time", + value: trptr(ptr(time.Now())), + expectedTime: true, + }, + { + name: "pointer to nil redefined *time.Time", + value: trptr(nil), + expectedTime: true, + }, + { + name: "embedded time.Time", + value: te{}, + expectedTime: false, + }, + { + name: "pointer to embedded time.Time", + value: &te{}, + expectedTime: false, + }, + { + name: "not a time", + value: time.Duration(0), + expectedTime: false, + }, + }) +} + +func ptr[T any](value T) *T { + v := value + return &v +} From 5319d1e12042ad27d04302b1cb0ea730754c167d Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 4 Jan 2026 15:02:21 +0100 Subject: [PATCH 3/3] added sortability for time.Time Signed-off-by: Frederic BIDON --- internal/spew/common.go | 89 ++++++++++++++++++++++++++---- internal/spew/common_test.go | 103 ++++++++++++++++++++++++++++++++++- internal/spew/dump_test.go | 18 +++++- internal/spew/spew_test.go | 17 +++++- internal/spew/time.go | 12 ++++ internal/spew/time_test.go | 18 +++++- 6 files changed, 237 insertions(+), 20 deletions(-) diff --git a/internal/spew/common.go b/internal/spew/common.go index 84c7390e0..656bbe864 100644 --- a/internal/spew/common.go +++ b/internal/spew/common.go @@ -23,6 +23,7 @@ import ( "reflect" "sort" "strconv" + "time" ) // Some constants in the form of bytes to avoid string overhead. This mirrors @@ -169,14 +170,15 @@ func handleErrorOrStringer(cs *ConfigState, w io.Writer, v reflect.Value) (handl return false, false } + // rendering time.Time if !converted.CanInterface() { - return false, false // safeguard + return false, false // safeguard: should never be the case } timeIface := converted.Interface() stringer, ok := timeIface.(fmt.Stringer) if !ok { - return false, false // safeguard + return false, false // safeguard: should never be the case } if cs.ContinueOnMethod { @@ -278,12 +280,21 @@ type valuesSorter struct { // newValuesSorter initializes a valuesSorter instance, which holds a set of // surrogate keys on which the data should be sorted. It uses flags in // ConfigState to decide if and how to populate those surrogate keys. +// +// Values that are convertible to a time.Time are compared using [time.Time.Compare()]. +// This is safer than sorting their string representation, because the [time.Location] +// may differ. +// +// NOTE: if all values are not of the same underlying type, +// comparison will resort to the default reflect.Value.String() representation. func newValuesSorter(values []reflect.Value, cs *ConfigState) sort.Interface { vs := &valuesSorter{values: values, cs: cs} - if canSortSimply(vs.values[0].Kind()) { + v0 := vs.values[0] + if canSortSimply(v0.Kind()) || isTime(v0) { return vs } - if !cs.DisableMethods { + + if !vs.cs.DisableMethods { vs.strings = make([]string, len(values)) for i := range vs.values { b := bytes.Buffer{} @@ -345,6 +356,15 @@ func (s *valuesSorter) Swap(i, j int) { } } +// Less returns whether the value at index i should sort before the +// value at index j. It is part of the sort.Interface implementation. +func (s *valuesSorter) Less(i, j int) bool { + if s.strings == nil { + return valueSortLess(s.values[i], s.values[j]) + } + return s.strings[i] < s.strings[j] +} + // valueSortLess returns whether the first value should sort before the second // value. It is used by valueSorter.Less as part of the sort.Interface // implementation. @@ -381,17 +401,64 @@ func valueSortLess(a, b reflect.Value) bool { } fallthrough default: - return a.String() < b.String() + isTimeA := isTime(a) + isTimeB := isTime(b) + if !isTimeA || !isTimeB { + return a.String() < b.String() + } + + return timeLess(a, b) } } -// Less returns whether the value at index i should sort before the -// value at index j. It is part of the sort.Interface implementation. -func (s *valuesSorter) Less(i, j int) bool { - if s.strings == nil { - return valueSortLess(s.values[i], s.values[j]) +// timeLess compare two values that convert to a time.Time. +// +// This handles nil values gracefully. +func timeLess(a, b reflect.Value) bool { + convertedA, okForA := isConvertibleToTime(a) + convertedB, okForB := isConvertibleToTime(b) + if !okForA || !okForB { + // types can convert, but not values: this means we have at least one nil + if !okForA { // a =< b because nil =< (any value) + return true + } + + return false // b > a } - return s.strings[i] < s.strings[j] + + // time comparison: we need to retrieve the time.Time values from a chain of pointers + for convertedA.Kind() == reflect.Pointer && convertedA.IsValid() { + convertedA = reflect.Indirect(convertedA) + } + for convertedB.Kind() == reflect.Pointer && convertedB.IsValid() { + convertedB = reflect.Indirect(convertedB) + } + + // handle the case when some values are nil, after resolving pointers + switch { + case !convertedA.IsValid(): // nil <= (any value) + return true + case !convertedB.IsValid(): // (valid) > nil + return false + } + + okIfaceA := convertedA.CanInterface() + okIfaceB := convertedB.CanInterface() + + if !okIfaceA || !okIfaceB { + // defensive safeguard (should never get there, since we have successfully converted) + return a.String() < b.String() + } + + tA, okTimeA := convertedA.Interface().(time.Time) + tB, okTimeB := convertedB.Interface().(time.Time) + + if !okTimeA || !okTimeB { + // defensive safeguard (should never get there, since we have successfully indirected and converted) + return a.String() < b.String() + } + + return tA.Compare(tB) <= 0 } // sortValues is a sort function that handles both native types and any type that diff --git a/internal/spew/common_test.go b/internal/spew/common_test.go index 718d00250..03b3b8f7b 100644 --- a/internal/spew/common_test.go +++ b/internal/spew/common_test.go @@ -22,6 +22,7 @@ import ( "slices" "strings" "testing" + "time" "github.com/go-openapi/testify/v2/internal/spew" ) @@ -92,7 +93,7 @@ func (e customError) Error() string { return fmt.Sprintf("error: %d", int(e)) } -// stringizeWants converts a slice of wanted test output into a format suitable +// stringizeWants verts a slice of wanted test output into a format suitable // for a test error message. func stringizeWants(wants []string) string { s := "" @@ -151,7 +152,7 @@ func helpTestSortValues(t *testing.T, tests []sortTestCase, cs *spew.ConfigState input := getInterfaces(test.input) expected := getInterfaces(test.expected) if !reflect.DeepEqual(input, expected) { - t.Errorf("Sort mismatch:\n %v != %v", input, expected) + t.Errorf("Sort mismatch:\n %v != %v\n\n%#v != %#v", input, expected, input, expected) } } } @@ -167,6 +168,16 @@ func TestSortValues(t *testing.T) { embedA := v(embed{"a"}) embedB := v(embed{"b"}) embedC := v(embed{"c"}) + + // test values for times + t0, t1, t2 := testTimings() + lt0, lt1, lt2 := testTimingsWithLocation() + pt0, pt1, pt2 := ptr(t0), ptr(t1), ptr(t2) + ppt2 := ptr(pt2) + nilTimePtr := (*time.Time)(nil) + nilTimePtrPtr := &nilTimePtr + invalidTime := (**time.Time)(nil) + tests := []sortTestCase{ // No values. { @@ -220,6 +231,41 @@ func TestSortValues(t *testing.T) { []reflect.Value{v(unsortableStruct{2}), v(unsortableStruct{1}), v(unsortableStruct{3})}, []reflect.Value{v(unsortableStruct{2}), v(unsortableStruct{1}), v(unsortableStruct{3})}, }, + // time.Time + { + []reflect.Value{v(t0), v(t2), v(t1)}, + []reflect.Value{v(t0), v(t1), v(t2)}, + }, + // comparison with location + { + []reflect.Value{v(lt2), v(lt1), v(lt0)}, + []reflect.Value{v(lt0), v(lt1), v(lt2)}, + }, + // *time.Time and types that vert to it + { + []reflect.Value{v(pt0), v(pt2), v(pt1)}, + []reflect.Value{v(pt0), v(pt1), v(pt2)}, + }, + // hybrid pointers: *time.Time + { + []reflect.Value{v(pt0), v(t2), v(pt1)}, + []reflect.Value{v(pt0), v(pt1), v(t2)}, + }, + // nil pointers: *time.Time (vention: nil < any value) + { + []reflect.Value{v(pt0), v(t2), v(pt1), v(nilTimePtr)}, + []reflect.Value{v(nilTimePtr), v(pt0), v(pt1), v(t2)}, + }, + // indirection pointers: **time.Time + { + []reflect.Value{v(pt0), v(ppt2), v((nilTimePtrPtr)), v(t1)}, + []reflect.Value{v(nilTimePtrPtr), v(pt0), v(t1), v(ppt2)}, + }, + // invalid **time.Time (nil) + { + []reflect.Value{v(pt0), v(invalidTime)}, + []reflect.Value{v(invalidTime), v(pt0)}, + }, // Invalid. { []reflect.Value{embedB, embedA, embedC}, @@ -238,6 +284,9 @@ func TestSortValuesWithMethods(t *testing.T) { a := v("a") b := v("b") c := v("c") + t0, t1, t2 := testTimings() + lt0, lt1, lt2 := testTimingsWithLocation() + tests := []sortTestCase{ // Ints. { @@ -254,6 +303,16 @@ func TestSortValuesWithMethods(t *testing.T) { []reflect.Value{v(sortableStruct{2}), v(sortableStruct{1}), v(sortableStruct{3})}, []reflect.Value{v(sortableStruct{1}), v(sortableStruct{2}), v(sortableStruct{3})}, }, + // time.Time and types that convvert to it + { + []reflect.Value{v(t0), v(t2), v(t1)}, + []reflect.Value{v(t0), v(t1), v(t2)}, + }, + // comparison with location + { + []reflect.Value{v(lt2), v(lt1), v(lt0)}, + []reflect.Value{v(lt0), v(lt1), v(lt2)}, + }, // UnsortableStructs. { // Note: not sorted - SpewKeys is false. @@ -273,6 +332,8 @@ func TestSortValuesWithSpew(t *testing.T) { a := v("a") b := v("b") c := v("c") + t0, t1, t2 := testTimings() + tests := []sortTestCase{ // Ints. { @@ -289,6 +350,11 @@ func TestSortValuesWithSpew(t *testing.T) { []reflect.Value{v(sortableStruct{2}), v(sortableStruct{1}), v(sortableStruct{3})}, []reflect.Value{v(sortableStruct{1}), v(sortableStruct{2}), v(sortableStruct{3})}, }, + // time.Time and types that vert to it + { + []reflect.Value{v(t0), v(t2), v(t1)}, + []reflect.Value{v(t0), v(t1), v(t2)}, + }, // UnsortableStructs. { []reflect.Value{v(unsortableStruct{2}), v(unsortableStruct{1}), v(unsortableStruct{3})}, @@ -298,3 +364,36 @@ func TestSortValuesWithSpew(t *testing.T) { cs := spew.ConfigState{DisableMethods: true, SpewKeys: true} helpTestSortValues(t, tests, &cs) } + +// TestSortValuesWithString ensures the sort functionality for relect.Value +// based string sorting for times works as intended. +func TestSortTimeValuesWithString(t *testing.T) { + v := reflect.ValueOf + t0, t1, t2 := testTimings() + + tests := []sortTestCase{ + // time.Time and types that vert to it + { + []reflect.Value{v(t0), v(t2), v(t1)}, + []reflect.Value{v(t0), v(t1), v(t2)}, + }, + } + cs := spew.ConfigState{DisableMethods: true, EnableTimeStringer: true} + helpTestSortValues(t, tests, &cs) +} + +func testTimings() (t0, t1, t2 time.Time) { + t0 = time.Now() + t1 = t0.Add(time.Hour) + t2 = t1.Add(time.Hour) + return t0, t1, t2 +} + +func testTimingsWithLocation() (lt0, lt1, lt2 time.Time) { + t0, t1, t2 := testTimings() + lt0 = t0.In(time.FixedZone("UTC+5", 5)) + lt1 = t1.In(time.FixedZone("UTC-4", -4)) + lt2 = t2.In(time.FixedZone("UTC-6", -6)) + + return lt0, lt1, lt2 +} diff --git a/internal/spew/dump_test.go b/internal/spew/dump_test.go index a21012b40..53c9e49b3 100644 --- a/internal/spew/dump_test.go +++ b/internal/spew/dump_test.go @@ -965,9 +965,12 @@ type ( embeddedTimePtr struct { // this type is an example where the unsafeReflectValue does not work well *time.Time } - redeclaredTime time.Time - redeclaredTimePtr *time.Time - aliasedTime = time.Time + redeclaredTime time.Time + redeclaredTimePtr *time.Time + aliasedTime = time.Time + embeddedRedeclaredTime struct { + redeclaredTime + } ) func addTimeDumpTests() { @@ -988,6 +991,9 @@ func addTimeDumpTests() { ppts := ptr(ptr(ts)) ppAddr := fmt.Sprintf("%p", ppts) ppIAddr := fmt.Sprintf("%p", *ppts) + er := embeddedRedeclaredTime{ + redeclaredTime: redeclaredTime(ts), + } addDumpTest( // simple time.Time @@ -1041,6 +1047,12 @@ func addTimeDumpTests() { rtptr, "(*time.Time)("+tsAddr+")(2006-01-02 15:04:05.999999999 +0000 UTC)\n", ) + addDumpTest( + // embedded redeclared type convertible to time.Time + er, + "(spew_test.embeddedRedeclaredTime) {\n"+ + " redeclaredTime: (spew_test.redeclaredTime) 2006-01-02 15:04:05.999999999 +0000 UTC\n}\n", + ) } // TestDump executes all of the tests described by dumpTests. diff --git a/internal/spew/spew_test.go b/internal/spew/spew_test.go index afba821e3..1c3d5b108 100644 --- a/internal/spew/spew_test.go +++ b/internal/spew/spew_test.go @@ -161,10 +161,15 @@ func initSpewTests() { // Variable for tests on types which implement error interface. te := customError(10) + + // Variables for testing time.Time behavior tm := time.Date(2006, time.January, 2, 15, 4, 5, 999999999, time.UTC) tmAddr := fmt.Sprintf("%p", &tm) em := embeddedTime{Time: tm} emptr := embeddedTimePtr{Time: &tm} + er := embeddedRedeclaredTime{ + redeclaredTime: redeclaredTime(tm), + } spewTests = []spewTest{ // default config @@ -234,22 +239,28 @@ func initSpewTests() { " {\n wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n}\n", }, { - scsContinue, fCSFdump, "", &tm, // this prints time and continues digging the struct + scsContinue, fCSFdump, "", &tm, "(*time.Time)(" + tmAddr + ")((2006-01-02 15:04:05.999999999 +0000 UTC)" + " {\n wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n})\n", }, { - scsContinue, fCSFdump, "", em, // this prints time and continues digging the struct + scsContinue, fCSFdump, "", em, "(spew_test.embeddedTime) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + " Time: (time.Time) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + " wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n }\n}\n", }, { - scsContinue, fCSFdump, "", emptr, // this prints time and continues digging the struct + scsContinue, fCSFdump, "", emptr, "(spew_test.embeddedTimePtr) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + " Time: (*time.Time)(" + tmAddr + ")((2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + " wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n })\n}\n", }, + { + scsContinue, fCSFdump, "", er, + "(spew_test.embeddedRedeclaredTime) {\n" + + " redeclaredTime: (spew_test.redeclaredTime) (2006-01-02 15:04:05.999999999 +0000 UTC) {\n" + + " wall: (uint64) 999999999,\n ext: (int64) 63271811045,\n loc: (*time.Location)()\n }\n}\n", + }, {scsNoMethodsButTimeStringer, fCSFprint, "", tm, "2006-01-02 15:04:05.999999999 +0000 UTC"}, {scsNoMethodsButTimeStringer, fCSFdump, "", tm, "(time.Time) 2006-01-02 15:04:05.999999999 +0000 UTC\n"}, {scsNoMethodsButTimeStringer, fCSFprint, "", &tm, "<*>2006-01-02 15:04:05.999999999 +0000 UTC"}, diff --git a/internal/spew/time.go b/internal/spew/time.go index b52b38785..1ab10641d 100644 --- a/internal/spew/time.go +++ b/internal/spew/time.go @@ -13,6 +13,10 @@ import ( // Conversely, a struct that embeds a time.Time or *time.Time is not considered a time.Time // and we'll have to dig the individual fields. func isTime(v reflect.Value) bool { + if !v.IsValid() { + return false + } + k := v.Kind() t := v.Type() @@ -31,6 +35,10 @@ func isTime(v reflect.Value) bool { // isConvertibleToTime returns a converted reflect.Value and true when v is convertible to time.Time or *time.Time. func isConvertibleToTime(v reflect.Value) (reflect.Value, bool) { + if !v.IsValid() { + return reflect.Value{}, false + } + k := v.Kind() timeTyp := reflect.TypeFor[time.Time]() @@ -43,5 +51,9 @@ func isConvertibleToTime(v reflect.Value) (reflect.Value, bool) { return v.Convert(timePtrTyp), true } + if k == reflect.Pointer && v.Elem().Kind() == reflect.Pointer { + return isConvertibleToTime(v.Elem()) + } + return reflect.Value{}, false // the returned value is Invalid in this case } diff --git a/internal/spew/time_test.go b/internal/spew/time_test.go index e99625991..0075c3113 100644 --- a/internal/spew/time_test.go +++ b/internal/spew/time_test.go @@ -35,6 +35,7 @@ func isTimeCases() iter.Seq[isTimeCase] { type te struct { time.Time } + nilTimePtr := (*time.Time)(nil) return slices.Values([]isTimeCase{ { @@ -49,7 +50,7 @@ func isTimeCases() iter.Seq[isTimeCase] { }, { name: "nil *time.Time", - value: (*time.Time)(nil), + value: nilTimePtr, expectedTime: true, }, { @@ -72,6 +73,16 @@ func isTimeCases() iter.Seq[isTimeCase] { value: trptr(nil), expectedTime: true, }, + { + name: "pointer indirection **time.Time", + value: ptr(ptr(time.Now())), + expectedTime: true, + }, + { + name: "pointer indirection to nil **time.Time", + value: ptr(nilTimePtr), + expectedTime: true, + }, { name: "embedded time.Time", value: te{}, @@ -82,6 +93,11 @@ func isTimeCases() iter.Seq[isTimeCase] { value: &te{}, expectedTime: false, }, + { + name: "invalid **time.Time", + value: (**time.Time)(nil), + expectedTime: false, + }, { name: "not a time", value: time.Duration(0),