From 2a694899c343313af85dfa1729f10fb8055b62ae Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Thu, 1 Apr 2021 03:51:08 +0500 Subject: [PATCH 01/10] Add support for nested indexed type, fixes issue #9 --- query/encode.go | 110 +++++++++++++----------- query/encode_test.go | 177 ++++++++++++++++++++++++++++----------- query/tag_parser.go | 39 +++++++++ query/tag_parser_test.go | 24 ++++++ 4 files changed, 253 insertions(+), 97 deletions(-) create mode 100644 query/tag_parser.go create mode 100644 query/tag_parser_test.go diff --git a/query/encode.go b/query/encode.go index 91198f8..150a50e 100644 --- a/query/encode.go +++ b/query/encode.go @@ -26,7 +26,6 @@ import ( "net/url" "reflect" "strconv" - "strings" "time" ) @@ -145,11 +144,16 @@ func Values(v interface{}) (url.Values, error) { return values, err } +type embeddedStruct struct { + Value reflect.Value + Scope string +} + // reflectValue populates the values parameter from the struct fields in val. // Embedded structs are followed recursively (using the rules defined in the // Values function documentation) breadth-first. func reflectValue(values url.Values, val reflect.Value, scope string) error { - var embedded []reflect.Value + var embedded []*embeddedStruct typ := val.Type() for i := 0; i < typ.NumField(); i++ { @@ -163,26 +167,27 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if tag == "-" { continue } - name, opts := parseTag(tag) - if name == "" { + var parsedTag urlTag = parseTag(tag) + + if parsedTag.name == "" { if sf.Anonymous { v := reflect.Indirect(sv) if v.IsValid() && v.Kind() == reflect.Struct { // save embedded struct for later processing - embedded = append(embedded, v) + embedded = append(embedded, &embeddedStruct{v, scope}) continue } } - name = sf.Name + parsedTag.name = sf.Name } if scope != "" { - name = scope + "[" + name + "]" + parsedTag.name = scope + "[" + parsedTag.name + "]" } - if opts.Contains("omitempty") && isEmptyValue(sv) { + if parsedTag.options.Contains(tagStringOmitEmpty) && isEmptyValue(sv) { continue } @@ -194,7 +199,7 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { } m := sv.Interface().(Encoder) - if err := m.EncodeValues(name, &values); err != nil { + if err := m.EncodeValues(parsedTag.name, &values); err != nil { return err } continue @@ -210,14 +215,14 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if sv.Kind() == reflect.Slice || sv.Kind() == reflect.Array { var del string - if opts.Contains("comma") { + if parsedTag.options.Contains(tagStringComma) { del = "," - } else if opts.Contains("space") { + } else if parsedTag.options.Contains(tagStringSpace) { del = " " - } else if opts.Contains("semicolon") { + } else if parsedTag.options.Contains(tagStringSemicolon) { del = ";" - } else if opts.Contains("brackets") { - name = name + "[]" + } else if parsedTag.options.Contains(tagStringBrackets) { + parsedTag.name = parsedTag.name + "[]" } else { del = sf.Tag.Get("del") } @@ -231,38 +236,66 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { } else { s.WriteString(del) } - s.WriteString(valueString(sv.Index(i), opts, sf)) + s.WriteString(valueString(sv.Index(i), parsedTag.options, sf)) } - values.Add(name, s.String()) + values.Add(parsedTag.name, s.String()) } else { for i := 0; i < sv.Len(); i++ { - k := name - if opts.Contains("numbered") { - k = fmt.Sprintf("%s%d", name, i) + tagName := parsedTag.name + var indexValue reflect.Value = sv.Index(i) + + if parsedTag.options.Contains(tagStringNumbered) { + tagName = fmt.Sprintf("%s%d", parsedTag.name, i) } - values.Add(k, valueString(sv.Index(i), opts, sf)) + + if parsedTag.options.Contains(tagStringIndexed) { + tagName = fmt.Sprintf("%s[%d]", parsedTag.name, i) + + for indexValue.Kind() == reflect.Ptr { + if indexValue.IsNil() { + break + } + + indexValue = indexValue.Elem() + } + + if indexValue.Kind() == reflect.Struct { + embedded = append(embedded, &embeddedStruct{indexValue, tagName}) + continue + } + } + + values.Add(tagName, valueString(indexValue, parsedTag.options, sf)) } } continue } if sv.Type() == timeType { - values.Add(name, valueString(sv, opts, sf)) + values.Add(parsedTag.name, valueString(sv, parsedTag.options, sf)) continue } if sv.Kind() == reflect.Struct { - if err := reflectValue(values, sv, name); err != nil { + if err := reflectValue(values, sv, parsedTag.name); err != nil { return err } continue } - values.Add(name, valueString(sv, opts, sf)) + values.Add(parsedTag.name, valueString(sv, parsedTag.options, sf)) } for _, f := range embedded { - if err := reflectValue(values, f, scope); err != nil { + var s string + + if f.Scope == "" { + s = scope + } else { + s = f.Scope + } + + if err := reflectValue(values, f.Value, s); err != nil { return err } } @@ -279,7 +312,7 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin v = v.Elem() } - if v.Kind() == reflect.Bool && opts.Contains("int") { + if v.Kind() == reflect.Bool && opts.Contains(tagStringInt) { if v.Bool() { return "1" } @@ -288,13 +321,13 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin if v.Type() == timeType { t := v.Interface().(time.Time) - if opts.Contains("unix") { + if opts.Contains(tagStringUnix) { return strconv.FormatInt(t.Unix(), 10) } - if opts.Contains("unixmilli") { + if opts.Contains(tagStringUnixMilli) { return strconv.FormatInt((t.UnixNano() / 1e6), 10) } - if opts.Contains("unixnano") { + if opts.Contains(tagStringUnixNano) { return strconv.FormatInt(t.UnixNano(), 10) } if layout := sf.Tag.Get("layout"); layout != "" { @@ -334,24 +367,3 @@ func isEmptyValue(v reflect.Value) bool { return false } - -// tagOptions is the string following a comma in a struct field's "url" tag, or -// the empty string. It does not include the leading comma. -type tagOptions []string - -// parseTag splits a struct field's url tag into its name and comma-separated -// options. -func parseTag(tag string) (string, tagOptions) { - s := strings.Split(tag, ",") - return s[0], s[1:] -} - -// Contains checks whether the tagOptions contains the specified option. -func (o tagOptions) Contains(option string) bool { - for _, s := range o { - if s == option { - return true - } - } - return false -} diff --git a/query/encode_test.go b/query/encode_test.go index 8487858..77f5523 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -15,6 +15,17 @@ import ( "github.com/google/go-cmp/cmp" ) +type GenericTest struct { + input interface{} + want url.Values +} + +func runGenericTests(t *testing.T, genericTests *[]GenericTest) { + for _, genericTest := range *genericTests { + testValue(t, genericTest.input, genericTest.want) + } +} + // test that Values(input) matches want. If not, report an error on t. func testValue(t *testing.T, input interface{}, want url.Values) { v, err := Values(input) @@ -27,10 +38,7 @@ func testValue(t *testing.T, input interface{}, want url.Values) { } func TestValues_BasicTypes(t *testing.T) { - tests := []struct { - input interface{} - want url.Values - }{ + tests := []GenericTest{ // zero values {struct{ V string }{}, url.Values{"V": {""}}}, {struct{ V int }{}, url.Values{"V": {"0"}}}, @@ -52,6 +60,7 @@ func TestValues_BasicTypes(t *testing.T) { }{false}, url.Values{"V": {"0"}}, }, + { struct { V bool `url:",int"` @@ -92,19 +101,14 @@ func TestValues_BasicTypes(t *testing.T) { }, } - for _, tt := range tests { - testValue(t, tt.input, tt.want) - } + runGenericTests(t, &tests) } func TestValues_Pointers(t *testing.T) { str := "s" strPtr := &str - tests := []struct { - input interface{} - want url.Values - }{ + tests := []GenericTest{ // nil pointers (zero values) {struct{ V *string }{}, url.Values{"V": {""}}}, {struct{ V *int }{}, url.Values{"V": {""}}}, @@ -128,21 +132,19 @@ func TestValues_Pointers(t *testing.T) { {&struct{ V string }{"v"}, url.Values{"V": {"v"}}}, } - for _, tt := range tests { - testValue(t, tt.input, tt.want) - } + runGenericTests(t, &tests) } +// IMPORTANT func TestValues_Slices(t *testing.T) { - tests := []struct { - input interface{} - want url.Values - }{ + // The base struct could be abstracted + tests := []GenericTest{ // slices of strings { struct{ V []string }{}, url.Values{}, }, + { struct{ V []string }{[]string{"a", "b"}}, url.Values{"V": {"a", "b"}}, @@ -178,6 +180,13 @@ func TestValues_Slices(t *testing.T) { url.Values{"V0": {"a"}, "V1": {"b"}}, }, + { + struct { + V []string `url:",indexed"` + }{[]string{"a", "b"}}, + url.Values{"V[0]": {"a"}, "V[1]": {"b"}}, + }, + // arrays of strings { struct{ V [2]string }{}, @@ -247,9 +256,7 @@ func TestValues_Slices(t *testing.T) { }, } - for _, tt := range tests { - testValue(t, tt.input, tt.want) - } + runGenericTests(t, &tests) } func TestValues_NestedTypes(t *testing.T) { @@ -309,6 +316,97 @@ func TestValues_NestedTypes(t *testing.T) { } } +func TestValues_ArrayIndexNestedTypes(t *testing.T) { + type AnotherSubNested struct { + AnotherValue string `url:"d"` + } + + type SubNested struct { + Value string `url:"value"` + D []AnotherSubNested `url:"anotherSubNested,indexed"` + } + + type Nested struct { + C []SubNested `url:",indexed"` + } + + tests := []GenericTest{ + { + Nested{ + []SubNested{ + {"value0", []AnotherSubNested{}}, + {"value1", []AnotherSubNested{}}, + {"value2", []AnotherSubNested{}}, + {"value3", []AnotherSubNested{{"value0"}}}, + }, + }, + url.Values{ + "C[0][value]": {"value0"}, + "C[1][value]": {"value1"}, + "C[2][value]": {"value2"}, + "C[3][value]": {"value3"}, + "C[3][anotherSubNested][0][d]": {"value0"}, + }, + }, + { + Nested{ + []SubNested{ + {"value0", []AnotherSubNested{}}, + {"value1", []AnotherSubNested{}}, + {"value2", nil}, + {"value3", []AnotherSubNested{{"value0"}}}, + }, + }, + url.Values{ + "C[0][value]": {"value0"}, + "C[1][value]": {"value1"}, + "C[2][value]": {"value2"}, + "C[3][value]": {"value3"}, + "C[3][anotherSubNested][0][d]": {"value0"}, + }, + }, + } + + runGenericTests(t, &tests) +} + +/** + * Example taken from the author of Original Issue https://github.com/google/go-querystring/issues/8 + */ +func TestValues_ArrayIndexNestedTypes_GithubIssue_Number_8(t *testing.T) { + type Nested struct { + A string `url:"theA,omitempty"` + B string `url:"theB,omitempty"` + } + + type NestedArr []Nested + + type Main struct { + A NestedArr `url:"arr,indexed"` + B Nested `url:"nested"` + } + + tests := []GenericTest{ + { + Main{ + NestedArr{{"aa", "bb"}, {"aaa", "bbb"}}, + Nested{"xx", "zz"}, + }, + + url.Values{ + "arr[0][theA]": {"aa"}, + "arr[0][theB]": {"bb"}, + "arr[1][theA]": {"aaa"}, + "arr[1][theB]": {"bbb"}, + "nested[theA]": {"xx"}, + "nested[theB]": {"zz"}, + }, + }, + } + + runGenericTests(t, &tests) +} + func TestValues_OmitEmpty(t *testing.T) { str := "" @@ -353,20 +451,25 @@ func TestValues_EmbeddedStructs(t *testing.T) { type Inner struct { V string } + type Outer struct { Inner } + type OuterPtr struct { *Inner } + type Mixed struct { Inner V string } + type unexported struct { Inner V string } + type Exported struct { unexported } @@ -384,7 +487,9 @@ func TestValues_EmbeddedStructs(t *testing.T) { url.Values{"V": {"a"}}, }, { + // This step would happen before anything else, so we need not worry about it Mixed{Inner: Inner{V: "a"}, V: "b"}, + url.Values{"V": {"b", "a"}}, }, { @@ -427,10 +532,7 @@ func (m customEncodedStrings) EncodeValues(key string, v *url.Values) error { } func TestValues_CustomEncodingSlice(t *testing.T) { - tests := []struct { - input interface{} - want url.Values - }{ + tests := []GenericTest{ { struct { V customEncodedStrings `url:"v"` @@ -459,9 +561,8 @@ func TestValues_CustomEncodingSlice(t *testing.T) { }, } - for _, tt := range tests { - testValue(t, tt.input, tt.want) - } + runGenericTests(t, &tests) + } // One of the few ways reflectValues will return an error is if a custom @@ -705,23 +806,3 @@ func TestIsEmptyValue(t *testing.T) { } } } - -func TestParseTag(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}, - {"field", false}, - } { - if opts.Contains(tt.opt) != tt.want { - t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) - } - } -} diff --git a/query/tag_parser.go b/query/tag_parser.go new file mode 100644 index 0000000..7082821 --- /dev/null +++ b/query/tag_parser.go @@ -0,0 +1,39 @@ +package query + +import "strings" + +type urlTag struct { + name string + options tagOptions +} + +type tagOptions []string + +// Contains checks whether the tagOptions contains the specified option. +func (o tagOptions) Contains(option string) bool { + for _, s := range o { + if s == option { + return true + } + } + return false +} + +const tagStringComma = "comma" +const tagStringSpace = "space" +const tagStringSemicolon = "semicolon" +const tagStringBrackets = "brackets" +const tagStringNumbered = "numbered" +const tagStringIndexed = "indexed" +const tagStringInt = "int" +const tagStringUnix = "unix" +const tagStringUnixMilli = "unixmilli" +const tagStringUnixNano = "unixnano" +const tagStringOmitEmpty = "omitempty" + +// parseTag splits a struct field's url tag into its name and comma-separated +// options. +func parseTag(tag string) urlTag { + s := strings.Split(tag, ",") + return urlTag{s[0], s[1:]} +} diff --git a/query/tag_parser_test.go b/query/tag_parser_test.go new file mode 100644 index 0000000..8f262a2 --- /dev/null +++ b/query/tag_parser_test.go @@ -0,0 +1,24 @@ +package query + +import "testing" + +func TestParseTag(t *testing.T) { + parsedTag := parseTag("field,foobar,foo") + if parsedTag.name != "field" { + + t.Fatalf("name = %q, want field", parsedTag.name) + } + for _, tt := range []struct { + opt string + want bool + }{ + {"foobar", true}, + {"foo", true}, + {"bar", false}, + {"field", false}, + } { + if parsedTag.options.Contains(tt.opt) != tt.want { + t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) + } + } +} From 9295cf447da8ec8eb0a936307290dfc3d3e3e923 Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 20:53:47 +0500 Subject: [PATCH 02/10] remove refactoring changes Explanation in https://github.com/google/go-querystring/pull/48#issuecomment-811553662 --- query/encode.go | 47 +++++++++++++++++----- query/encode_test.go | 87 +++++++++++++++++++++++++++++----------- query/tag_parser.go | 39 ------------------ query/tag_parser_test.go | 24 ----------- 4 files changed, 99 insertions(+), 98 deletions(-) delete mode 100644 query/tag_parser.go delete mode 100644 query/tag_parser_test.go diff --git a/query/encode.go b/query/encode.go index 150a50e..ab7b3ef 100644 --- a/query/encode.go +++ b/query/encode.go @@ -26,6 +26,7 @@ import ( "net/url" "reflect" "strconv" + "strings" "time" ) @@ -187,7 +188,7 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { parsedTag.name = scope + "[" + parsedTag.name + "]" } - if parsedTag.options.Contains(tagStringOmitEmpty) && isEmptyValue(sv) { + if parsedTag.options.Contains("omitempty") && isEmptyValue(sv) { continue } @@ -215,13 +216,13 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if sv.Kind() == reflect.Slice || sv.Kind() == reflect.Array { var del string - if parsedTag.options.Contains(tagStringComma) { + if parsedTag.options.Contains("comma") { del = "," - } else if parsedTag.options.Contains(tagStringSpace) { + } else if parsedTag.options.Contains("space") { del = " " - } else if parsedTag.options.Contains(tagStringSemicolon) { + } else if parsedTag.options.Contains("semicolon") { del = ";" - } else if parsedTag.options.Contains(tagStringBrackets) { + } else if parsedTag.options.Contains("brackets") { parsedTag.name = parsedTag.name + "[]" } else { del = sf.Tag.Get("del") @@ -244,11 +245,11 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { tagName := parsedTag.name var indexValue reflect.Value = sv.Index(i) - if parsedTag.options.Contains(tagStringNumbered) { + if parsedTag.options.Contains("numbered") { tagName = fmt.Sprintf("%s%d", parsedTag.name, i) } - if parsedTag.options.Contains(tagStringIndexed) { + if parsedTag.options.Contains("indexed") { tagName = fmt.Sprintf("%s[%d]", parsedTag.name, i) for indexValue.Kind() == reflect.Ptr { @@ -312,7 +313,7 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin v = v.Elem() } - if v.Kind() == reflect.Bool && opts.Contains(tagStringInt) { + if v.Kind() == reflect.Bool && opts.Contains("int") { if v.Bool() { return "1" } @@ -321,13 +322,13 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin if v.Type() == timeType { t := v.Interface().(time.Time) - if opts.Contains(tagStringUnix) { + if opts.Contains("unix") { return strconv.FormatInt(t.Unix(), 10) } - if opts.Contains(tagStringUnixMilli) { + if opts.Contains("unixmilli") { return strconv.FormatInt((t.UnixNano() / 1e6), 10) } - if opts.Contains(tagStringUnixNano) { + if opts.Contains("unixnano") { return strconv.FormatInt(t.UnixNano(), 10) } if layout := sf.Tag.Get("layout"); layout != "" { @@ -367,3 +368,27 @@ func isEmptyValue(v reflect.Value) bool { return false } + +type urlTag struct { + name string + options tagOptions +} + +type tagOptions []string + +// Contains checks whether the tagOptions contains the specified option. +func (o tagOptions) Contains(option string) bool { + for _, s := range o { + if s == option { + return true + } + } + return false +} + +// parseTag splits a struct field's url tag into its name and comma-separated +// options. +func parseTag(tag string) urlTag { + s := strings.Split(tag, ",") + return urlTag{s[0], s[1:]} +} diff --git a/query/encode_test.go b/query/encode_test.go index 77f5523..e5a4fb8 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -15,17 +15,6 @@ import ( "github.com/google/go-cmp/cmp" ) -type GenericTest struct { - input interface{} - want url.Values -} - -func runGenericTests(t *testing.T, genericTests *[]GenericTest) { - for _, genericTest := range *genericTests { - testValue(t, genericTest.input, genericTest.want) - } -} - // test that Values(input) matches want. If not, report an error on t. func testValue(t *testing.T, input interface{}, want url.Values) { v, err := Values(input) @@ -38,7 +27,10 @@ func testValue(t *testing.T, input interface{}, want url.Values) { } func TestValues_BasicTypes(t *testing.T) { - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ // zero values {struct{ V string }{}, url.Values{"V": {""}}}, {struct{ V int }{}, url.Values{"V": {"0"}}}, @@ -101,14 +93,19 @@ func TestValues_BasicTypes(t *testing.T) { }, } - runGenericTests(t, &tests) + for _, test := range tests { + testValue(t, test.input, test.want) + } } func TestValues_Pointers(t *testing.T) { str := "s" strPtr := &str - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ // nil pointers (zero values) {struct{ V *string }{}, url.Values{"V": {""}}}, {struct{ V *int }{}, url.Values{"V": {""}}}, @@ -132,13 +129,18 @@ func TestValues_Pointers(t *testing.T) { {&struct{ V string }{"v"}, url.Values{"V": {"v"}}}, } - runGenericTests(t, &tests) + for _, test := range tests { + testValue(t, test.input, test.want) + } } // IMPORTANT func TestValues_Slices(t *testing.T) { // The base struct could be abstracted - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ // slices of strings { struct{ V []string }{}, @@ -256,7 +258,9 @@ func TestValues_Slices(t *testing.T) { }, } - runGenericTests(t, &tests) + for _, test := range tests { + testValue(t, test.input, test.want) + } } func TestValues_NestedTypes(t *testing.T) { @@ -330,7 +334,10 @@ func TestValues_ArrayIndexNestedTypes(t *testing.T) { C []SubNested `url:",indexed"` } - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ { Nested{ []SubNested{ @@ -367,7 +374,9 @@ func TestValues_ArrayIndexNestedTypes(t *testing.T) { }, } - runGenericTests(t, &tests) + for _, test := range tests { + testValue(t, test.input, test.want) + } } /** @@ -386,7 +395,10 @@ func TestValues_ArrayIndexNestedTypes_GithubIssue_Number_8(t *testing.T) { B Nested `url:"nested"` } - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ { Main{ NestedArr{{"aa", "bb"}, {"aaa", "bbb"}}, @@ -404,7 +416,9 @@ func TestValues_ArrayIndexNestedTypes_GithubIssue_Number_8(t *testing.T) { }, } - runGenericTests(t, &tests) + for _, test := range tests { + testValue(t, test.input, test.want) + } } func TestValues_OmitEmpty(t *testing.T) { @@ -532,7 +546,10 @@ func (m customEncodedStrings) EncodeValues(key string, v *url.Values) error { } func TestValues_CustomEncodingSlice(t *testing.T) { - tests := []GenericTest{ + tests := []struct { + input interface{} + want url.Values + }{ { struct { V customEncodedStrings `url:"v"` @@ -561,8 +578,9 @@ func TestValues_CustomEncodingSlice(t *testing.T) { }, } - runGenericTests(t, &tests) - + for _, test := range tests { + testValue(t, test.input, test.want) + } } // One of the few ways reflectValues will return an error is if a custom @@ -806,3 +824,24 @@ func TestIsEmptyValue(t *testing.T) { } } } + +func TestParseTag(t *testing.T) { + parsedTag := parseTag("field,foobar,foo") + if parsedTag.name != "field" { + + t.Fatalf("name = %q, want field", parsedTag.name) + } + for _, tt := range []struct { + opt string + want bool + }{ + {"foobar", true}, + {"foo", true}, + {"bar", false}, + {"field", false}, + } { + if parsedTag.options.Contains(tt.opt) != tt.want { + t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) + } + } +} diff --git a/query/tag_parser.go b/query/tag_parser.go deleted file mode 100644 index 7082821..0000000 --- a/query/tag_parser.go +++ /dev/null @@ -1,39 +0,0 @@ -package query - -import "strings" - -type urlTag struct { - name string - options tagOptions -} - -type tagOptions []string - -// Contains checks whether the tagOptions contains the specified option. -func (o tagOptions) Contains(option string) bool { - for _, s := range o { - if s == option { - return true - } - } - return false -} - -const tagStringComma = "comma" -const tagStringSpace = "space" -const tagStringSemicolon = "semicolon" -const tagStringBrackets = "brackets" -const tagStringNumbered = "numbered" -const tagStringIndexed = "indexed" -const tagStringInt = "int" -const tagStringUnix = "unix" -const tagStringUnixMilli = "unixmilli" -const tagStringUnixNano = "unixnano" -const tagStringOmitEmpty = "omitempty" - -// parseTag splits a struct field's url tag into its name and comma-separated -// options. -func parseTag(tag string) urlTag { - s := strings.Split(tag, ",") - return urlTag{s[0], s[1:]} -} diff --git a/query/tag_parser_test.go b/query/tag_parser_test.go deleted file mode 100644 index 8f262a2..0000000 --- a/query/tag_parser_test.go +++ /dev/null @@ -1,24 +0,0 @@ -package query - -import "testing" - -func TestParseTag(t *testing.T) { - parsedTag := parseTag("field,foobar,foo") - if parsedTag.name != "field" { - - t.Fatalf("name = %q, want field", parsedTag.name) - } - for _, tt := range []struct { - opt string - want bool - }{ - {"foobar", true}, - {"foo", true}, - {"bar", false}, - {"field", false}, - } { - if parsedTag.options.Contains(tt.opt) != tt.want { - t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) - } - } -} From 0b9ef5251313ee1898043876f6d4d8e3e4d41fff Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 21:40:24 +0500 Subject: [PATCH 03/10] add test for ptr slices --- query/encode_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/query/encode_test.go b/query/encode_test.go index e5a4fb8..e9ad2f5 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -379,6 +379,73 @@ func TestValues_ArrayIndexNestedTypes(t *testing.T) { } } +/** + * Tests whether Indirect references are properly serialized or not + */ +func TestValues_ArrayIndexNestedTypes_Pointer(t *testing.T) { + type AnotherSubNested struct { + AnotherValue string `url:"d"` + } + + type SubNested struct { + Value string `url:"value"` + D *[]AnotherSubNested `url:"anotherSubNested,indexed"` + } + + type Nested struct { + C []SubNested `url:",indexed"` + } + + tests := []struct { + input interface{} + want url.Values + }{ + { + Nested{ + []SubNested{ + {"value0", &[]AnotherSubNested{}}, + {"value1", &[]AnotherSubNested{}}, + {"value2", &[]AnotherSubNested{}}, + {"value3", &[]AnotherSubNested{{"value0"}}}, + }, + }, + url.Values{ + "C[0][value]": {"value0"}, + "C[1][value]": {"value1"}, + "C[2][value]": {"value2"}, + "C[3][value]": {"value3"}, + "C[3][anotherSubNested][0][d]": {"value0"}, + }, + }, + { + Nested{ + []SubNested{ + {"value0", &[]AnotherSubNested{}}, + {"value1", &[]AnotherSubNested{}}, + {"value2", nil}, + {"value3", &[]AnotherSubNested{{"value0"}}}, + }, + }, + + url.Values{ + "C[0][value]": {"value0"}, + "C[1][value]": {"value1"}, + "C[2][value]": {"value2"}, + "C[3][value]": {"value3"}, + + // nil value for the pointer leads to this stringified value + "C[2][anotherSubNested]": {""}, + + "C[3][anotherSubNested][0][d]": {"value0"}, + }, + }, + } + + for _, test := range tests { + testValue(t, test.input, test.want) + } +} + /** * Example taken from the author of Original Issue https://github.com/google/go-querystring/issues/8 */ From 90c24ddd87711653aacfb63cb058b4515f27050e Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 22:01:38 +0500 Subject: [PATCH 04/10] remove pointer conversion --- query/encode.go | 8 ------ query/encode_test.go | 67 -------------------------------------------- 2 files changed, 75 deletions(-) diff --git a/query/encode.go b/query/encode.go index ab7b3ef..71b1ba1 100644 --- a/query/encode.go +++ b/query/encode.go @@ -252,14 +252,6 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if parsedTag.options.Contains("indexed") { tagName = fmt.Sprintf("%s[%d]", parsedTag.name, i) - for indexValue.Kind() == reflect.Ptr { - if indexValue.IsNil() { - break - } - - indexValue = indexValue.Elem() - } - if indexValue.Kind() == reflect.Struct { embedded = append(embedded, &embeddedStruct{indexValue, tagName}) continue diff --git a/query/encode_test.go b/query/encode_test.go index e9ad2f5..e5a4fb8 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -379,73 +379,6 @@ func TestValues_ArrayIndexNestedTypes(t *testing.T) { } } -/** - * Tests whether Indirect references are properly serialized or not - */ -func TestValues_ArrayIndexNestedTypes_Pointer(t *testing.T) { - type AnotherSubNested struct { - AnotherValue string `url:"d"` - } - - type SubNested struct { - Value string `url:"value"` - D *[]AnotherSubNested `url:"anotherSubNested,indexed"` - } - - type Nested struct { - C []SubNested `url:",indexed"` - } - - tests := []struct { - input interface{} - want url.Values - }{ - { - Nested{ - []SubNested{ - {"value0", &[]AnotherSubNested{}}, - {"value1", &[]AnotherSubNested{}}, - {"value2", &[]AnotherSubNested{}}, - {"value3", &[]AnotherSubNested{{"value0"}}}, - }, - }, - url.Values{ - "C[0][value]": {"value0"}, - "C[1][value]": {"value1"}, - "C[2][value]": {"value2"}, - "C[3][value]": {"value3"}, - "C[3][anotherSubNested][0][d]": {"value0"}, - }, - }, - { - Nested{ - []SubNested{ - {"value0", &[]AnotherSubNested{}}, - {"value1", &[]AnotherSubNested{}}, - {"value2", nil}, - {"value3", &[]AnotherSubNested{{"value0"}}}, - }, - }, - - url.Values{ - "C[0][value]": {"value0"}, - "C[1][value]": {"value1"}, - "C[2][value]": {"value2"}, - "C[3][value]": {"value3"}, - - // nil value for the pointer leads to this stringified value - "C[2][anotherSubNested]": {""}, - - "C[3][anotherSubNested][0][d]": {"value0"}, - }, - }, - } - - for _, test := range tests { - testValue(t, test.input, test.want) - } -} - /** * Example taken from the author of Original Issue https://github.com/google/go-querystring/issues/8 */ From c0e74e55e6f4083436d1671bf4dee3694ee8bc9c Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 22:07:12 +0500 Subject: [PATCH 05/10] remove leading newline --- query/encode.go | 2 +- query/encode_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/query/encode.go b/query/encode.go index 71b1ba1..a3a11ab 100644 --- a/query/encode.go +++ b/query/encode.go @@ -383,4 +383,4 @@ func (o tagOptions) Contains(option string) bool { func parseTag(tag string) urlTag { s := strings.Split(tag, ",") return urlTag{s[0], s[1:]} -} +} \ No newline at end of file diff --git a/query/encode_test.go b/query/encode_test.go index e5a4fb8..0266e2b 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -844,4 +844,4 @@ func TestParseTag(t *testing.T) { t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) } } -} +} \ No newline at end of file From 2ef6ae4b9fbae87c98fc3c2a3ce8c3bf4ca7fbdb Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 22:58:57 +0500 Subject: [PATCH 06/10] run golangci-lint --- query/encode.go | 2 +- query/encode_test.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/query/encode.go b/query/encode.go index a3a11ab..71b1ba1 100644 --- a/query/encode.go +++ b/query/encode.go @@ -383,4 +383,4 @@ func (o tagOptions) Contains(option string) bool { func parseTag(tag string) urlTag { s := strings.Split(tag, ",") return urlTag{s[0], s[1:]} -} \ No newline at end of file +} diff --git a/query/encode_test.go b/query/encode_test.go index 0266e2b..27a3175 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -828,7 +828,6 @@ func TestIsEmptyValue(t *testing.T) { func TestParseTag(t *testing.T) { parsedTag := parseTag("field,foobar,foo") if parsedTag.name != "field" { - t.Fatalf("name = %q, want field", parsedTag.name) } for _, tt := range []struct { @@ -844,4 +843,4 @@ func TestParseTag(t *testing.T) { t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) } } -} \ No newline at end of file +} From 544706e0e74f8f595b2328599e496f27e40ada82 Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 23:12:36 +0500 Subject: [PATCH 07/10] add comment regarding the "indexed" option --- query/encode.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/query/encode.go b/query/encode.go index 71b1ba1..653f658 100644 --- a/query/encode.go +++ b/query/encode.go @@ -107,6 +107,11 @@ type Encoder interface { // // separated by exclamation points "!". // Field []bool `url:",int" del:"!"` // +// Including the "indexed" option for slices and arrays will encode the Slice and Array +// values using Ruby format, and would lead to recursive serialization of all the nested struct +// fields and slice/array within them as well (This was added in PR # 48) +// +// // Anonymous struct fields are usually encoded as if their inner exported // fields were fields in the outer struct, subject to the standard Go // visibility rules. An anonymous struct field with a name given in its URL @@ -118,6 +123,8 @@ type Encoder interface { // scoping. e.g: // // "user[name]=acme&user[addr][postcode]=1234&user[addr][city]=SFO" +// +// // // All other values are encoded using their default string representation. // From 85ef37ba2c105c285a829fca3972edf9351b760d Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Fri, 2 Apr 2021 23:20:43 +0500 Subject: [PATCH 08/10] run golangci-lint --- query/encode.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/query/encode.go b/query/encode.go index 653f658..12c6c07 100644 --- a/query/encode.go +++ b/query/encode.go @@ -110,7 +110,7 @@ type Encoder interface { // Including the "indexed" option for slices and arrays will encode the Slice and Array // values using Ruby format, and would lead to recursive serialization of all the nested struct // fields and slice/array within them as well (This was added in PR # 48) -// +// // // Anonymous struct fields are usually encoded as if their inner exported // fields were fields in the outer struct, subject to the standard Go @@ -123,8 +123,8 @@ type Encoder interface { // scoping. e.g: // // "user[name]=acme&user[addr][postcode]=1234&user[addr][city]=SFO" -// -// +// +// // // All other values are encoded using their default string representation. // From b8d342f9418268e11efbddefc09baaa210b3aa09 Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Tue, 13 Apr 2021 09:15:14 +0500 Subject: [PATCH 09/10] remove the changes not required --- query/encode.go | 95 +++++++++++++++++++++++--------------------- query/encode_test.go | 32 +++++++-------- 2 files changed, 65 insertions(+), 62 deletions(-) diff --git a/query/encode.go b/query/encode.go index 12c6c07..5a98aaf 100644 --- a/query/encode.go +++ b/query/encode.go @@ -109,7 +109,7 @@ type Encoder interface { // // Including the "indexed" option for slices and arrays will encode the Slice and Array // values using Ruby format, and would lead to recursive serialization of all the nested struct -// fields and slice/array within them as well (This was added in PR # 48) +// fields and slice/array within that struct as well (This was added in PR # 48) // // // Anonymous struct fields are usually encoded as if their inner exported @@ -152,18 +152,20 @@ func Values(v interface{}) (url.Values, error) { return values, err } -type embeddedStruct struct { - Value reflect.Value - Scope string -} - // reflectValue populates the values parameter from the struct fields in val. // Embedded structs are followed recursively (using the rules defined in the // Values function documentation) breadth-first. func reflectValue(values url.Values, val reflect.Value, scope string) error { - var embedded []*embeddedStruct + var embedded []*reflect.Value + + /** + * Provide scopes for embedded values, helpful for the indexed option as the scope argument of this function is used there to + * ensure correct property names for properties of the nested structs + */ + var scopes map[*reflect.Value]string typ := val.Type() + for i := 0; i < typ.NumField(); i++ { sf := typ.Field(i) if sf.PkgPath != "" && !sf.Anonymous { // unexported @@ -176,26 +178,26 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { continue } - var parsedTag urlTag = parseTag(tag) + name, opts := parseTag(tag) - if parsedTag.name == "" { + if name == "" { if sf.Anonymous { v := reflect.Indirect(sv) if v.IsValid() && v.Kind() == reflect.Struct { // save embedded struct for later processing - embedded = append(embedded, &embeddedStruct{v, scope}) + embedded = append(embedded, &v) continue } } - parsedTag.name = sf.Name + name = sf.Name } if scope != "" { - parsedTag.name = scope + "[" + parsedTag.name + "]" + name = scope + "[" + name + "]" } - if parsedTag.options.Contains("omitempty") && isEmptyValue(sv) { + if opts.Contains("omitempty") && isEmptyValue(sv) { continue } @@ -207,7 +209,7 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { } m := sv.Interface().(Encoder) - if err := m.EncodeValues(parsedTag.name, &values); err != nil { + if err := m.EncodeValues(name, &values); err != nil { return err } continue @@ -223,14 +225,14 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if sv.Kind() == reflect.Slice || sv.Kind() == reflect.Array { var del string - if parsedTag.options.Contains("comma") { + if opts.Contains("comma") { del = "," - } else if parsedTag.options.Contains("space") { + } else if opts.Contains("space") { del = " " - } else if parsedTag.options.Contains("semicolon") { + } else if opts.Contains("semicolon") { del = ";" - } else if parsedTag.options.Contains("brackets") { - parsedTag.name = parsedTag.name + "[]" + } else if opts.Contains("brackets") { + name = name + "[]" } else { del = sf.Tag.Get("del") } @@ -244,58 +246,62 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { } else { s.WriteString(del) } - s.WriteString(valueString(sv.Index(i), parsedTag.options, sf)) + s.WriteString(valueString(sv.Index(i), opts, sf)) } - values.Add(parsedTag.name, s.String()) + values.Add(name, s.String()) } else { for i := 0; i < sv.Len(); i++ { - tagName := parsedTag.name + tagName := name var indexValue reflect.Value = sv.Index(i) - if parsedTag.options.Contains("numbered") { - tagName = fmt.Sprintf("%s%d", parsedTag.name, i) + if opts.Contains("numbered") { + tagName = fmt.Sprintf("%s%d", name, i) } - if parsedTag.options.Contains("indexed") { - tagName = fmt.Sprintf("%s[%d]", parsedTag.name, i) + if opts.Contains("indexed") { + if scopes == nil { + scopes = make(map[*reflect.Value]string) + } + + tagName = fmt.Sprintf("%s[%d]", name, i) if indexValue.Kind() == reflect.Struct { - embedded = append(embedded, &embeddedStruct{indexValue, tagName}) + embedded = append(embedded, &indexValue) + scopes[&indexValue] = tagName continue } } - values.Add(tagName, valueString(indexValue, parsedTag.options, sf)) + values.Add(tagName, valueString(indexValue, opts, sf)) } } continue } if sv.Type() == timeType { - values.Add(parsedTag.name, valueString(sv, parsedTag.options, sf)) + values.Add(name, valueString(sv, opts, sf)) continue } if sv.Kind() == reflect.Struct { - if err := reflectValue(values, sv, parsedTag.name); err != nil { + if err := reflectValue(values, sv, name); err != nil { return err } continue } - values.Add(parsedTag.name, valueString(sv, parsedTag.options, sf)) + values.Add(name, valueString(sv, opts, sf)) } - for _, f := range embedded { - var s string + for _, val := range embedded { + var s string = scope + valueScope, ok := scopes[val] - if f.Scope == "" { - s = scope - } else { - s = f.Scope + if ok { + s = valueScope } - if err := reflectValue(values, f.Value, s); err != nil { + if err := reflectValue(values, *val, s); err != nil { return err } } @@ -340,7 +346,7 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin } // isEmptyValue checks if a value should be considered empty for the purposes -// of omitting fields with the "omitempty" option. +// of omit ting fields with the "omitempty" option. func isEmptyValue(v reflect.Value) bool { switch v.Kind() { case reflect.Array, reflect.Map, reflect.Slice, reflect.String: @@ -368,11 +374,8 @@ func isEmptyValue(v reflect.Value) bool { return false } -type urlTag struct { - name string - options tagOptions -} - +// tagOptions is the string following a comma in a struct field's "url" tag, or +// the empty string. It does not include the leading comma. type tagOptions []string // Contains checks whether the tagOptions contains the specified option. @@ -387,7 +390,7 @@ func (o tagOptions) Contains(option string) bool { // parseTag splits a struct field's url tag into its name and comma-separated // options. -func parseTag(tag string) urlTag { +func parseTag(tag string) (string, tagOptions) { s := strings.Split(tag, ",") - return urlTag{s[0], s[1:]} + return s[0], s[1:] } diff --git a/query/encode_test.go b/query/encode_test.go index 27a3175..339520f 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -93,8 +93,8 @@ func TestValues_BasicTypes(t *testing.T) { }, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -129,8 +129,8 @@ func TestValues_Pointers(t *testing.T) { {&struct{ V string }{"v"}, url.Values{"V": {"v"}}}, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -258,8 +258,8 @@ func TestValues_Slices(t *testing.T) { }, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -374,8 +374,8 @@ func TestValues_ArrayIndexNestedTypes(t *testing.T) { }, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -416,8 +416,8 @@ func TestValues_ArrayIndexNestedTypes_GithubIssue_Number_8(t *testing.T) { }, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -578,8 +578,8 @@ func TestValues_CustomEncodingSlice(t *testing.T) { }, } - for _, test := range tests { - testValue(t, test.input, test.want) + for _, tt := range tests { + testValue(t, tt.input, tt.want) } } @@ -826,9 +826,9 @@ func TestIsEmptyValue(t *testing.T) { } func TestParseTag(t *testing.T) { - parsedTag := parseTag("field,foobar,foo") - if parsedTag.name != "field" { - t.Fatalf("name = %q, want field", parsedTag.name) + name, opts := parseTag("field,foobar,foo") + if name != "field" { + t.Fatalf("name = %q, want field", name) } for _, tt := range []struct { opt string @@ -839,7 +839,7 @@ func TestParseTag(t *testing.T) { {"bar", false}, {"field", false}, } { - if parsedTag.options.Contains(tt.opt) != tt.want { + if opts.Contains(tt.opt) != tt.want { t.Errorf("Contains(%q) = %v", tt.opt, !tt.want) } } From e998a994aa6f5411fdb51f06d4f7267e38ed3f7f Mon Sep 17 00:00:00 2001 From: Hannan Ali Date: Tue, 13 Apr 2021 09:24:42 +0500 Subject: [PATCH 10/10] remove additional whitespaces --- query/encode.go | 7 +------ query/encode_test.go | 10 ---------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/query/encode.go b/query/encode.go index 5a98aaf..08c5200 100644 --- a/query/encode.go +++ b/query/encode.go @@ -111,7 +111,6 @@ type Encoder interface { // values using Ruby format, and would lead to recursive serialization of all the nested struct // fields and slice/array within that struct as well (This was added in PR # 48) // -// // Anonymous struct fields are usually encoded as if their inner exported // fields were fields in the outer struct, subject to the standard Go // visibility rules. An anonymous struct field with a name given in its URL @@ -124,8 +123,6 @@ type Encoder interface { // // "user[name]=acme&user[addr][postcode]=1234&user[addr][city]=SFO" // -// -// // All other values are encoded using their default string representation. // // Multiple fields that encode to the same URL parameter name will be included @@ -165,7 +162,6 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { var scopes map[*reflect.Value]string typ := val.Type() - for i := 0; i < typ.NumField(); i++ { sf := typ.Field(i) if sf.PkgPath != "" && !sf.Anonymous { // unexported @@ -177,7 +173,6 @@ func reflectValue(values url.Values, val reflect.Value, scope string) error { if tag == "-" { continue } - name, opts := parseTag(tag) if name == "" { @@ -346,7 +341,7 @@ func valueString(v reflect.Value, opts tagOptions, sf reflect.StructField) strin } // isEmptyValue checks if a value should be considered empty for the purposes -// of omit ting fields with the "omitempty" option. +// of omitting fields with the "omitempty" option. func isEmptyValue(v reflect.Value) bool { switch v.Kind() { case reflect.Array, reflect.Map, reflect.Slice, reflect.String: diff --git a/query/encode_test.go b/query/encode_test.go index 339520f..b856352 100644 --- a/query/encode_test.go +++ b/query/encode_test.go @@ -52,7 +52,6 @@ func TestValues_BasicTypes(t *testing.T) { }{false}, url.Values{"V": {"0"}}, }, - { struct { V bool `url:",int"` @@ -134,9 +133,7 @@ func TestValues_Pointers(t *testing.T) { } } -// IMPORTANT func TestValues_Slices(t *testing.T) { - // The base struct could be abstracted tests := []struct { input interface{} want url.Values @@ -146,7 +143,6 @@ func TestValues_Slices(t *testing.T) { struct{ V []string }{}, url.Values{}, }, - { struct{ V []string }{[]string{"a", "b"}}, url.Values{"V": {"a", "b"}}, @@ -465,25 +461,20 @@ func TestValues_EmbeddedStructs(t *testing.T) { type Inner struct { V string } - type Outer struct { Inner } - type OuterPtr struct { *Inner } - type Mixed struct { Inner V string } - type unexported struct { Inner V string } - type Exported struct { unexported } @@ -503,7 +494,6 @@ func TestValues_EmbeddedStructs(t *testing.T) { { // This step would happen before anything else, so we need not worry about it Mixed{Inner: Inner{V: "a"}, V: "b"}, - url.Values{"V": {"b", "a"}}, }, {