From 53187a116ba7fb1d9ec8c4d0b41d74f6048dbf72 Mon Sep 17 00:00:00 2001 From: Mathijs de Bruin Date: Sat, 8 Oct 2022 15:27:10 +0100 Subject: [PATCH 1/5] Struct slice yields wrong no of arguments. --- resp/resp3/flatten_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 resp/resp3/flatten_test.go diff --git a/resp/resp3/flatten_test.go b/resp/resp3/flatten_test.go new file mode 100644 index 0000000..ec63474 --- /dev/null +++ b/resp/resp3/flatten_test.go @@ -0,0 +1,33 @@ +package resp3 + +import ( + "testing" + + "github.com/mediocregopher/radix/v4/resp" + "github.com/stretchr/testify/suite" +) + +type FlattenTestSuite struct { + suite.Suite +} + +type NestedStruct struct{} + +type sliceTestStruct struct { + Other string + Nested []NestedStruct +} + +func (s *FlattenTestSuite) TestEmptyNestedSlice() { + testInst := sliceTestStruct{ + Other: "hello", + } + + flat, err := Flatten(testInst, resp.NewOpts()) + s.NoError(err) + s.Equal([]string{"Other", "hello"}, flat) +} + +func TestFlattenTestSuite(t *testing.T) { + suite.Run(t, new(FlattenTestSuite)) +} From a627f916a9ab5afeb7442e66df27fbe03065f942 Mon Sep 17 00:00:00 2001 From: Mathijs de Bruin Date: Sat, 8 Oct 2022 16:02:16 +0100 Subject: [PATCH 2/5] Resolve issue and add omitempty tag option. --- resp/resp3/flatten.go | 32 ++++++++++++++++++++++++++++++-- resp/resp3/flatten_test.go | 25 ++++++++++++++++++++----- resp/resp3/tags.go | 38 ++++++++++++++++++++++++++++++++++++++ resp/resp3/tags_test.go | 28 ++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 resp/resp3/tags.go create mode 100644 resp/resp3/tags_test.go diff --git a/resp/resp3/flatten.go b/resp/resp3/flatten.go index 50263dd..7c693d2 100644 --- a/resp/resp3/flatten.go +++ b/resp/resp3/flatten.go @@ -25,6 +25,25 @@ func cleanFloatStr(str string) string { return str } +// Source: https://cs.opensource.google/go/go/+/refs/tags/go1.19.2:src/encoding/json/encode.go;drc=d0b0b10b5cbb28d53403c2bd6af343581327e946;l=339 +func isEmptyValue(v reflect.Value) bool { + switch v.Kind() { + case reflect.Array, reflect.Map, reflect.Slice, reflect.String: + return v.Len() == 0 + case reflect.Bool: + return !v.Bool() + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return v.Int() == 0 + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + return v.Uint() == 0 + case reflect.Float32, reflect.Float64: + return v.Float() == 0 + case reflect.Interface, reflect.Pointer: + return v.IsNil() + } + return false +} + // Flatten accepts any type accepted by Marshal, except a resp.Marshaler, and // converts it into a flattened array of strings. For example: // @@ -33,7 +52,6 @@ func cleanFloatStr(str string) string { // Flatten([]string{"a","b"}) -> {"a", "b"} // Flatten(map[string]int{"a":5,"b":10}) -> {"a","5","b","10"} // Flatten([]map[int]float64{{1:2, 3:4},{5:6},{}}) -> {"1","2","3","4","5","6"}) -// func Flatten(i interface{}, o *resp.Opts) ([]string, error) { f := flattener{ opts: o, @@ -165,7 +183,7 @@ func (f *flattener) flatten(i interface{}) error { l := vv.NumField() for i := 0; i < l; i++ { ft, fv := tt.Field(i), vv.Field(i) - tag := ft.Tag.Get("redis") + tag, tagOpts := parseTag(ft.Tag.Get("redis")) if ft.Anonymous { if fv = reflect.Indirect(fv); !fv.IsValid() { // fv is nil continue @@ -177,12 +195,22 @@ func (f *flattener) flatten(i interface{}) error { continue // unexported } + isEmpty := isEmptyValue(fv) + if isEmpty && tagOpts.Contains("omitempty") { + continue + } + keyName := ft.Name if tag != "" { keyName = tag } _ = f.emit(keyName) + if isEmpty { + // Return "", setting empty value + return f.emit("") + } + if err := f.flatten(fv.Interface()); err != nil { return err } diff --git a/resp/resp3/flatten_test.go b/resp/resp3/flatten_test.go index ec63474..f924587 100644 --- a/resp/resp3/flatten_test.go +++ b/resp/resp3/flatten_test.go @@ -13,18 +13,33 @@ type FlattenTestSuite struct { type NestedStruct struct{} -type sliceTestStruct struct { - Other string - Nested []NestedStruct +// Test whether by default, an empty slice in a hashmap should be flattened to an empty value. +func (s *FlattenTestSuite) TestEmptyNestedSlice() { + testInst := struct { + Other string + Nested []NestedStruct + }{ + Other: "hello", + } + + flat, err := Flatten(testInst, resp.NewOpts()) + s.NoError(err) + + s.Equal([]string{"Other", "hello", "Nested", ""}, flat) } -func (s *FlattenTestSuite) TestEmptyNestedSlice() { - testInst := sliceTestStruct{ +// Test with omitempty; empty slices should be left off altogether. +func (s *FlattenTestSuite) TestOmitEmptyNestedSlice() { + testInst := struct { + Other string + NestedOmitEmpty []NestedStruct `redis:",omitempty"` + }{ Other: "hello", } flat, err := Flatten(testInst, resp.NewOpts()) s.NoError(err) + s.Equal([]string{"Other", "hello"}, flat) } diff --git a/resp/resp3/tags.go b/resp/resp3/tags.go new file mode 100644 index 0000000..a5da458 --- /dev/null +++ b/resp/resp3/tags.go @@ -0,0 +1,38 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package resp3 + +import ( + "strings" +) + +// tagOptions is the string following a comma in a struct field's "json" +// tag, or the empty string. It does not include the leading comma. +type tagOptions string + +// parseTag splits a struct field's json tag into its name and +// comma-separated options. +func parseTag(tag string) (string, tagOptions) { + tag, opt, _ := strings.Cut(tag, ",") + return tag, tagOptions(opt) +} + +// Contains reports whether a comma-separated list of options +// contains a particular substr flag. substr must be surrounded by a +// string boundary or commas. +func (o tagOptions) Contains(optionName string) bool { + if len(o) == 0 { + return false + } + s := string(o) + for s != "" { + var name string + name, s, _ = strings.Cut(s, ",") + if name == optionName { + return true + } + } + return false +} diff --git a/resp/resp3/tags_test.go b/resp/resp3/tags_test.go new file mode 100644 index 0000000..774b2af --- /dev/null +++ b/resp/resp3/tags_test.go @@ -0,0 +1,28 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package resp3 + +import ( + "testing" +) + +func TestTagParsing(t *testing.T) { + name, opts := parseTag("field,foobar,foo") + if name != "field" { + t.Fatalf("name = %q, want field", name) + } + for _, tt := range []struct { + opt string + want bool + }{ + {"foobar", true}, + {"foo", true}, + {"bar", false}, + } { + if opts.Contains(tt.opt) != tt.want { + t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) + } + } +} From 3035a65a9543f5329b819cc58decfbe1e1ac395c Mon Sep 17 00:00:00 2001 From: Mathijs de Bruin Date: Sat, 8 Oct 2022 16:03:40 +0100 Subject: [PATCH 3/5] Simplify test case. --- resp/resp3/flatten_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/resp/resp3/flatten_test.go b/resp/resp3/flatten_test.go index f924587..a345c93 100644 --- a/resp/resp3/flatten_test.go +++ b/resp/resp3/flatten_test.go @@ -11,13 +11,11 @@ type FlattenTestSuite struct { suite.Suite } -type NestedStruct struct{} - // Test whether by default, an empty slice in a hashmap should be flattened to an empty value. func (s *FlattenTestSuite) TestEmptyNestedSlice() { testInst := struct { Other string - Nested []NestedStruct + Nested []string }{ Other: "hello", } @@ -32,7 +30,7 @@ func (s *FlattenTestSuite) TestEmptyNestedSlice() { func (s *FlattenTestSuite) TestOmitEmptyNestedSlice() { testInst := struct { Other string - NestedOmitEmpty []NestedStruct `redis:",omitempty"` + NestedOmitEmpty []string `redis:",omitempty"` }{ Other: "hello", } From 5a6bc0e5ecd794d977b2e3d771211ad503431b09 Mon Sep 17 00:00:00 2001 From: Mathijs de Bruin Date: Sat, 8 Oct 2022 16:07:53 +0100 Subject: [PATCH 4/5] Simplify tests more. --- resp/resp3/flatten_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/resp/resp3/flatten_test.go b/resp/resp3/flatten_test.go index a345c93..b8f24db 100644 --- a/resp/resp3/flatten_test.go +++ b/resp/resp3/flatten_test.go @@ -14,31 +14,26 @@ type FlattenTestSuite struct { // Test whether by default, an empty slice in a hashmap should be flattened to an empty value. func (s *FlattenTestSuite) TestEmptyNestedSlice() { testInst := struct { - Other string Nested []string - }{ - Other: "hello", - } + }{} flat, err := Flatten(testInst, resp.NewOpts()) s.NoError(err) - s.Equal([]string{"Other", "hello", "Nested", ""}, flat) + s.Equal([]string{"Nested", ""}, flat) } // Test with omitempty; empty slices should be left off altogether. func (s *FlattenTestSuite) TestOmitEmptyNestedSlice() { testInst := struct { - Other string + Other string // We need at least one non-empty value for commands like HMSET. NestedOmitEmpty []string `redis:",omitempty"` - }{ - Other: "hello", - } + }{} flat, err := Flatten(testInst, resp.NewOpts()) s.NoError(err) - s.Equal([]string{"Other", "hello"}, flat) + s.Equal([]string{"Other", ""}, flat) } func TestFlattenTestSuite(t *testing.T) { From 285efc40920cfb6a76f0b58e54f5682b9d335106 Mon Sep 17 00:00:00 2001 From: Mathijs de Bruin Date: Tue, 11 Oct 2022 21:15:41 +0100 Subject: [PATCH 5/5] Also use tag parser when unmarshalling. --- resp/resp3/resp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resp/resp3/resp.go b/resp/resp3/resp.go index 0f96761..d81cc9e 100644 --- a/resp/resp3/resp.go +++ b/resp/resp3/resp.go @@ -2255,7 +2255,7 @@ func getStructFields(t reflect.Type) map[string]structField { } key, fromTag := ft.Name, false - if tag := ft.Tag.Get("redis"); tag != "" && tag != "-" { + if tag, _ := parseTag(ft.Tag.Get("redis")); tag != "" && tag != "-" { key, fromTag = tag, true } if m[key].fromTag {