From d62365ea7431d426e3abc8ee5fb6fd863dcc0f91 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 24 Oct 2021 14:42:42 +0200 Subject: [PATCH 1/5] feat: make it possible to define optional durations --- autonat.go | 2 +- types.go | 40 ++++++++++++++++++++++----- types_test.go | 75 ++++++++++++++++++++++++++++++++------------------- 3 files changed, 83 insertions(+), 34 deletions(-) diff --git a/autonat.go b/autonat.go index a1a3f69..2fbd3fc 100644 --- a/autonat.go +++ b/autonat.go @@ -77,5 +77,5 @@ type AutoNATThrottleConfig struct { // global/peer dialback limits. // // When unset, this defaults to 1 minute. - Interval Duration `json:",omitempty"` + Interval Duration } diff --git a/types.go b/types.go index 14b9f7c..864187a 100644 --- a/types.go +++ b/types.go @@ -214,20 +214,48 @@ var _ json.Marshaler = (*Priority)(nil) // Duration wraps time.Duration to provide json serialization and deserialization. // // NOTE: the zero value encodes to an empty string. -type Duration time.Duration +type Duration struct { + value *time.Duration +} func (d *Duration) UnmarshalText(text []byte) error { - dur, err := time.ParseDuration(string(text)) - *d = Duration(dur) - return err + switch string(text) { + case "null", "undefined": + *d = Duration{} + return nil + default: + value, err := time.ParseDuration(string(text)) + if err != nil { + return err + } + *d = Duration{value: &value} + return nil + } +} + +func (d *Duration) IsDefault() bool { + return d.value == nil +} + +func (d *Duration) WithDefault(defaultValue time.Duration) time.Duration { + if d.value == nil { + return defaultValue + } + return *d.value } func (d Duration) MarshalText() ([]byte, error) { - return []byte(time.Duration(d).String()), nil + if d.value != nil { + return []byte(d.value.String()), nil + } + return json.Marshal(nil) } func (d Duration) String() string { - return time.Duration(d).String() + if d.value == nil { + return "defaults" + } + return d.value.String() } var _ encoding.TextUnmarshaler = (*Duration)(nil) diff --git a/types_test.go b/types_test.go index 94a7a63..0161b20 100644 --- a/types_test.go +++ b/types_test.go @@ -7,34 +7,55 @@ import ( ) func TestDuration(t *testing.T) { - out, err := json.Marshal(Duration(time.Second)) - if err != nil { - t.Fatal(err) + makeDurationPointer := func(d time.Duration) *time.Duration { return &d } - } - expected := "\"1s\"" - if string(out) != expected { - t.Fatalf("expected %s, got %s", expected, string(out)) - } - var d Duration - err = json.Unmarshal(out, &d) - if err != nil { - t.Fatal(err) - } - if time.Duration(d) != time.Second { - t.Fatal("expected a second") - } - type Foo struct { - D Duration `json:",omitempty"` - } - out, err = json.Marshal(new(Foo)) - if err != nil { - t.Fatal(err) - } - expected = "{}" - if string(out) != expected { - t.Fatal("expected omitempty to omit the duration") - } + t.Run("marshalling and unmarshalling", func(t *testing.T) { + out, err := json.Marshal(Duration{value: makeDurationPointer(time.Second)}) + if err != nil { + t.Fatal(err) + } + expected := "\"1s\"" + if string(out) != expected { + t.Fatalf("expected %s, got %s", expected, string(out)) + } + var d Duration + + if err := json.Unmarshal(out, &d); err != nil { + t.Fatal(err) + } + if *d.value != time.Second { + t.Fatal("expected a second") + } + }) + + t.Run("default value", func(t *testing.T) { + var d Duration + if !d.IsDefault() { + t.Fatal("expected value to be the default initially") + } + if err := json.Unmarshal([]byte("null"), &d); err != nil { + t.Fatal(err) + } + if dur := d.WithDefault(time.Hour); dur != time.Hour { + t.Fatalf("expected default value to be used, got %s", dur) + } + if !d.IsDefault() { + t.Fatal("expected value to be the default") + } + }) + + t.Run("omitempyt", func(t *testing.T) { + type Foo struct { + D *Duration `json:",omitempty"` + } + out, err := json.Marshal(new(Foo)) + if err != nil { + t.Fatal(err) + } + if string(out) != "{}" { + t.Fatalf("expected omitempty to omit the duration, got %s", out) + } + }) } func TestOneStrings(t *testing.T) { From c9b7984f180c4aa76d69846996888626663b6b8c Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Tue, 26 Oct 2021 17:21:43 +0200 Subject: [PATCH 2/5] test: empty/default optional durations does not crash if user restores default value and sets it to empty string "" --- types.go | 4 ++-- types_test.go | 64 ++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/types.go b/types.go index 864187a..e375af9 100644 --- a/types.go +++ b/types.go @@ -220,7 +220,7 @@ type Duration struct { func (d *Duration) UnmarshalText(text []byte) error { switch string(text) { - case "null", "undefined": + case "null", "undefined", "": *d = Duration{} return nil default: @@ -253,7 +253,7 @@ func (d Duration) MarshalText() ([]byte, error) { func (d Duration) String() string { if d.value == nil { - return "defaults" + return "default" } return d.value.String() } diff --git a/types_test.go b/types_test.go index 0161b20..a948ff9 100644 --- a/types_test.go +++ b/types_test.go @@ -29,22 +29,24 @@ func TestDuration(t *testing.T) { }) t.Run("default value", func(t *testing.T) { - var d Duration - if !d.IsDefault() { - t.Fatal("expected value to be the default initially") - } - if err := json.Unmarshal([]byte("null"), &d); err != nil { - t.Fatal(err) - } - if dur := d.WithDefault(time.Hour); dur != time.Hour { - t.Fatalf("expected default value to be used, got %s", dur) - } - if !d.IsDefault() { - t.Fatal("expected value to be the default") + for _, jsonStr := range []string{"null", "\"\"", "\"null\""} { + var d Duration + if !d.IsDefault() { + t.Fatal("expected value to be the default initially") + } + if err := json.Unmarshal([]byte(jsonStr), &d); err != nil { + t.Fatal(err) + } + if dur := d.WithDefault(time.Hour); dur != time.Hour { + t.Fatalf("expected default value to be used, got %s", dur) + } + if !d.IsDefault() { + t.Fatal("expected value to be the default") + } } }) - t.Run("omitempyt", func(t *testing.T) { + t.Run("omitempty", func(t *testing.T) { type Foo struct { D *Duration `json:",omitempty"` } @@ -56,6 +58,42 @@ func TestDuration(t *testing.T) { t.Fatalf("expected omitempty to omit the duration, got %s", out) } }) + + for jsonStr, goValue := range map[string]Duration{ + "\"\"": {}, + "null": {}, + "\"null\"": {}, + "\"1s\"": {value: makeDurationPointer(time.Second)}, + "\"42h1m3s\"": {value: makeDurationPointer(42*time.Hour + 1*time.Minute + 3*time.Second)}, + } { + var d Duration + err := json.Unmarshal([]byte(jsonStr), &d) + if err != nil { + t.Fatal(err) + } + + if goValue.value == nil && d.value == nil { + } else if goValue.value == nil && d.value != nil { + t.Errorf("expected nil for %s, got %s", jsonStr, d) + } else if *d.value != *goValue.value { + t.Fatalf("expected %s for %s, got %s", goValue, jsonStr, d) + } + + // Test Reverse + out, err := json.Marshal(goValue) + if err != nil { + t.Fatal(err) + } + if goValue.value == nil { + if string(out) != "\"null\"" { + t.Fatalf("expected null string for %s, got %s", jsonStr, string(out)) + } + continue + } + if string(out) != jsonStr { + t.Fatalf("expected %s, got %s", jsonStr, string(out)) + } + } } func TestOneStrings(t *testing.T) { From 16af6f82b91002364c8b8ee493db28fb8ebcb1b8 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Tue, 26 Oct 2021 20:55:25 +0200 Subject: [PATCH 3/5] refactor: use null in JSON --- types.go | 25 +++++++++--------- types_test.go | 71 +++++++++++++++++++++++++++------------------------ 2 files changed, 51 insertions(+), 45 deletions(-) diff --git a/types.go b/types.go index e375af9..c5a6315 100644 --- a/types.go +++ b/types.go @@ -1,9 +1,9 @@ package config import ( - "encoding" "encoding/json" "fmt" + "strings" "time" ) @@ -213,18 +213,19 @@ var _ json.Marshaler = (*Priority)(nil) // Duration wraps time.Duration to provide json serialization and deserialization. // -// NOTE: the zero value encodes to an empty string. +// NOTE: the zero value encodes to "default" string. type Duration struct { value *time.Duration } -func (d *Duration) UnmarshalText(text []byte) error { - switch string(text) { - case "null", "undefined", "": +func (d *Duration) UnmarshalJSON(input []byte) error { + switch string(input) { + case "null", "undefined", "\"null\"", "", "default", "\"\"", "\"default\"": *d = Duration{} return nil default: - value, err := time.ParseDuration(string(text)) + text := strings.Trim(string(input), "\"") + value, err := time.ParseDuration(text) if err != nil { return err } @@ -244,11 +245,11 @@ func (d *Duration) WithDefault(defaultValue time.Duration) time.Duration { return *d.value } -func (d Duration) MarshalText() ([]byte, error) { - if d.value != nil { - return []byte(d.value.String()), nil +func (d Duration) MarshalJSON() ([]byte, error) { + if d.value == nil { + return json.Marshal("default") } - return json.Marshal(nil) + return json.Marshal(d.value.String()) } func (d Duration) String() string { @@ -258,8 +259,8 @@ func (d Duration) String() string { return d.value.String() } -var _ encoding.TextUnmarshaler = (*Duration)(nil) -var _ encoding.TextMarshaler = (*Duration)(nil) +var _ json.Unmarshaler = (*Duration)(nil) +var _ json.Marshaler = (*Duration)(nil) // OptionalInteger represents an integer that has a default value // diff --git a/types_test.go b/types_test.go index a948ff9..47647a9 100644 --- a/types_test.go +++ b/types_test.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "encoding/json" "testing" "time" @@ -29,13 +30,13 @@ func TestDuration(t *testing.T) { }) t.Run("default value", func(t *testing.T) { - for _, jsonStr := range []string{"null", "\"\"", "\"null\""} { + for _, jsonStr := range []string{"null", "\"\"", "\"default\""} { var d Duration if !d.IsDefault() { t.Fatal("expected value to be the default initially") } if err := json.Unmarshal([]byte(jsonStr), &d); err != nil { - t.Fatal(err) + t.Fatalf("%s failed to unmarshall with %s", jsonStr, err) } if dur := d.WithDefault(time.Hour); dur != time.Hour { t.Fatalf("expected default value to be used, got %s", dur) @@ -59,41 +60,45 @@ func TestDuration(t *testing.T) { } }) - for jsonStr, goValue := range map[string]Duration{ - "\"\"": {}, - "null": {}, - "\"null\"": {}, - "\"1s\"": {value: makeDurationPointer(time.Second)}, - "\"42h1m3s\"": {value: makeDurationPointer(42*time.Hour + 1*time.Minute + 3*time.Second)}, - } { - var d Duration - err := json.Unmarshal([]byte(jsonStr), &d) - if err != nil { - t.Fatal(err) - } + t.Run("roundtrip including the default values", func(t *testing.T) { + for jsonStr, goValue := range map[string]Duration{ + // there are various footguns user can hit, normalize them to the canonical default + "null": {}, // JSON null → default value + "\"null\"": {}, // JSON string "null" sent/set by "ipfs config" cli → default value + "\"default\"": {}, // explicit "default" as string + "\"\"": {}, // user removed custom value, empty string should also parse as default + "\"1s\"": {value: makeDurationPointer(time.Second)}, + "\"42h1m3s\"": {value: makeDurationPointer(42*time.Hour + 1*time.Minute + 3*time.Second)}, + } { + var d Duration + err := json.Unmarshal([]byte(jsonStr), &d) + if err != nil { + t.Fatal(err) + } - if goValue.value == nil && d.value == nil { - } else if goValue.value == nil && d.value != nil { - t.Errorf("expected nil for %s, got %s", jsonStr, d) - } else if *d.value != *goValue.value { - t.Fatalf("expected %s for %s, got %s", goValue, jsonStr, d) - } + if goValue.value == nil && d.value == nil { + } else if goValue.value == nil && d.value != nil { + t.Errorf("expected nil for %s, got %s", jsonStr, d) + } else if *d.value != *goValue.value { + t.Fatalf("expected %s for %s, got %s", goValue, jsonStr, d) + } - // Test Reverse - out, err := json.Marshal(goValue) - if err != nil { - t.Fatal(err) - } - if goValue.value == nil { - if string(out) != "\"null\"" { - t.Fatalf("expected null string for %s, got %s", jsonStr, string(out)) + // Test Reverse + out, err := json.Marshal(goValue) + if err != nil { + t.Fatal(err) + } + if goValue.value == nil { + if !bytes.Equal(out, []byte("\"default\"")) { + t.Fatalf("expected default string for %s, got %s", jsonStr, string(out)) + } + continue + } + if string(out) != jsonStr { + t.Fatalf("expected %s, got %s", jsonStr, string(out)) } - continue - } - if string(out) != jsonStr { - t.Fatalf("expected %s, got %s", jsonStr, string(out)) } - } + }) } func TestOneStrings(t *testing.T) { From 753de75911ddc1fa1514fd93c8965c0467241b42 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 27 Oct 2021 00:57:17 +0200 Subject: [PATCH 4/5] refactor(duration): use JSON null as the default Rationale: https://github.com/ipfs/go-ipfs-config/pull/148#discussion_r736975879 --- types.go | 2 +- types_test.go | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/types.go b/types.go index c5a6315..e9b3de1 100644 --- a/types.go +++ b/types.go @@ -247,7 +247,7 @@ func (d *Duration) WithDefault(defaultValue time.Duration) time.Duration { func (d Duration) MarshalJSON() ([]byte, error) { if d.value == nil { - return json.Marshal("default") + return json.Marshal(nil) } return json.Marshal(d.value.String()) } diff --git a/types_test.go b/types_test.go index 47647a9..6a7ce90 100644 --- a/types_test.go +++ b/types_test.go @@ -30,7 +30,7 @@ func TestDuration(t *testing.T) { }) t.Run("default value", func(t *testing.T) { - for _, jsonStr := range []string{"null", "\"\"", "\"default\""} { + for _, jsonStr := range []string{"null", "\"null\"", "\"\"", "\"default\""} { var d Duration if !d.IsDefault() { t.Fatal("expected value to be the default initially") @@ -89,8 +89,8 @@ func TestDuration(t *testing.T) { t.Fatal(err) } if goValue.value == nil { - if !bytes.Equal(out, []byte("\"default\"")) { - t.Fatalf("expected default string for %s, got %s", jsonStr, string(out)) + if !bytes.Equal(out, []byte("null")) { + t.Fatalf("expected JSON null for %s, got %s", jsonStr, string(out)) } continue } @@ -99,6 +99,18 @@ func TestDuration(t *testing.T) { } } }) + + t.Run("invalid duration values", func(t *testing.T) { + for _, invalid := range []string{ + "\"s\"", "\"1ę\"", "\"-1\"", "\"1H\"", "\"day\"", + } { + var d Duration + err := json.Unmarshal([]byte(invalid), &d) + if err == nil { + t.Errorf("expected to fail to decode %s as a Duration, got %s instead", invalid, d) + } + } + }) } func TestOneStrings(t *testing.T) { From 848e479895f2d9eb4bc467a812e63e5046feac17 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 27 Oct 2021 16:31:30 +0200 Subject: [PATCH 5/5] =?UTF-8?q?refactor:=20Duration=20=E2=86=92=20Optional?= =?UTF-8?q?Duration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it possible to use OptionalDuration with `json:",omitempty"` so the null is not serialized to JSON, and get working WithDefault as well. --- autonat.go | 2 +- types.go | 28 ++++++++++++++-------------- types_test.go | 32 ++++++++++++++++++++++---------- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/autonat.go b/autonat.go index 2fbd3fc..64856fa 100644 --- a/autonat.go +++ b/autonat.go @@ -77,5 +77,5 @@ type AutoNATThrottleConfig struct { // global/peer dialback limits. // // When unset, this defaults to 1 minute. - Interval Duration + Interval OptionalDuration `json:",omitempty"` } diff --git a/types.go b/types.go index e9b3de1..ac90fa9 100644 --- a/types.go +++ b/types.go @@ -211,17 +211,17 @@ func (p Priority) String() string { var _ json.Unmarshaler = (*Priority)(nil) var _ json.Marshaler = (*Priority)(nil) -// Duration wraps time.Duration to provide json serialization and deserialization. +// OptionalDuration wraps time.Duration to provide json serialization and deserialization. // -// NOTE: the zero value encodes to "default" string. -type Duration struct { +// NOTE: the zero value encodes to JSON nill +type OptionalDuration struct { value *time.Duration } -func (d *Duration) UnmarshalJSON(input []byte) error { +func (d *OptionalDuration) UnmarshalJSON(input []byte) error { switch string(input) { case "null", "undefined", "\"null\"", "", "default", "\"\"", "\"default\"": - *d = Duration{} + *d = OptionalDuration{} return nil default: text := strings.Trim(string(input), "\"") @@ -229,38 +229,38 @@ func (d *Duration) UnmarshalJSON(input []byte) error { if err != nil { return err } - *d = Duration{value: &value} + *d = OptionalDuration{value: &value} return nil } } -func (d *Duration) IsDefault() bool { - return d.value == nil +func (d *OptionalDuration) IsDefault() bool { + return d == nil || d.value == nil } -func (d *Duration) WithDefault(defaultValue time.Duration) time.Duration { - if d.value == nil { +func (d *OptionalDuration) WithDefault(defaultValue time.Duration) time.Duration { + if d == nil || d.value == nil { return defaultValue } return *d.value } -func (d Duration) MarshalJSON() ([]byte, error) { +func (d OptionalDuration) MarshalJSON() ([]byte, error) { if d.value == nil { return json.Marshal(nil) } return json.Marshal(d.value.String()) } -func (d Duration) String() string { +func (d OptionalDuration) String() string { if d.value == nil { return "default" } return d.value.String() } -var _ json.Unmarshaler = (*Duration)(nil) -var _ json.Marshaler = (*Duration)(nil) +var _ json.Unmarshaler = (*OptionalDuration)(nil) +var _ json.Marshaler = (*OptionalDuration)(nil) // OptionalInteger represents an integer that has a default value // diff --git a/types_test.go b/types_test.go index 6a7ce90..06ea73a 100644 --- a/types_test.go +++ b/types_test.go @@ -7,11 +7,11 @@ import ( "time" ) -func TestDuration(t *testing.T) { +func TestOptionalDuration(t *testing.T) { makeDurationPointer := func(d time.Duration) *time.Duration { return &d } t.Run("marshalling and unmarshalling", func(t *testing.T) { - out, err := json.Marshal(Duration{value: makeDurationPointer(time.Second)}) + out, err := json.Marshal(OptionalDuration{value: makeDurationPointer(time.Second)}) if err != nil { t.Fatal(err) } @@ -19,7 +19,7 @@ func TestDuration(t *testing.T) { if string(out) != expected { t.Fatalf("expected %s, got %s", expected, string(out)) } - var d Duration + var d OptionalDuration if err := json.Unmarshal(out, &d); err != nil { t.Fatal(err) @@ -31,7 +31,7 @@ func TestDuration(t *testing.T) { t.Run("default value", func(t *testing.T) { for _, jsonStr := range []string{"null", "\"null\"", "\"\"", "\"default\""} { - var d Duration + var d OptionalDuration if !d.IsDefault() { t.Fatal("expected value to be the default initially") } @@ -47,10 +47,11 @@ func TestDuration(t *testing.T) { } }) - t.Run("omitempty", func(t *testing.T) { + t.Run("omitempty with default value", func(t *testing.T) { type Foo struct { - D *Duration `json:",omitempty"` + D *OptionalDuration `json:",omitempty"` } + // marshall to JSON without empty field out, err := json.Marshal(new(Foo)) if err != nil { t.Fatal(err) @@ -58,10 +59,21 @@ func TestDuration(t *testing.T) { if string(out) != "{}" { t.Fatalf("expected omitempty to omit the duration, got %s", out) } + // unmarshall missing value and get the default + var foo2 Foo + if err := json.Unmarshal(out, &foo2); err != nil { + t.Fatalf("%s failed to unmarshall with %s", string(out), err) + } + if dur := foo2.D.WithDefault(time.Hour); dur != time.Hour { + t.Fatalf("expected default value to be used, got %s", dur) + } + if !foo2.D.IsDefault() { + t.Fatal("expected value to be the default") + } }) t.Run("roundtrip including the default values", func(t *testing.T) { - for jsonStr, goValue := range map[string]Duration{ + for jsonStr, goValue := range map[string]OptionalDuration{ // there are various footguns user can hit, normalize them to the canonical default "null": {}, // JSON null → default value "\"null\"": {}, // JSON string "null" sent/set by "ipfs config" cli → default value @@ -70,7 +82,7 @@ func TestDuration(t *testing.T) { "\"1s\"": {value: makeDurationPointer(time.Second)}, "\"42h1m3s\"": {value: makeDurationPointer(42*time.Hour + 1*time.Minute + 3*time.Second)}, } { - var d Duration + var d OptionalDuration err := json.Unmarshal([]byte(jsonStr), &d) if err != nil { t.Fatal(err) @@ -104,10 +116,10 @@ func TestDuration(t *testing.T) { for _, invalid := range []string{ "\"s\"", "\"1ę\"", "\"-1\"", "\"1H\"", "\"day\"", } { - var d Duration + var d OptionalDuration err := json.Unmarshal([]byte(invalid), &d) if err == nil { - t.Errorf("expected to fail to decode %s as a Duration, got %s instead", invalid, d) + t.Errorf("expected to fail to decode %s as an OptionalDuration, got %s instead", invalid, d) } } })