From 39871ea462ce63c347dd299ac960098c3c1d2db1 Mon Sep 17 00:00:00 2001 From: Zaq? Wiedmann Date: Wed, 19 Aug 2020 18:25:13 -0700 Subject: [PATCH 1/2] feat: support repeated query params and simplified "csv" cases This allows for leveraging repeated fields in the query via foo=bar&foo=bar2, this also simplifies the "csv" case for repeated strings. Before this using `repeated string foo` in the query would require quoted values `foo="bar","bar2"`, which is a little weird as it departs from how we normally pass values in query params, so as special case for strings (and a more efficient one), `foo=bar,bar2` is supported --- gengokit/httptransport/httptransport.go | 67 ++++++++++++++++++------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/gengokit/httptransport/httptransport.go b/gengokit/httptransport/httptransport.go index 6aea6bfb..b0d920f4 100644 --- a/gengokit/httptransport/httptransport.go +++ b/gengokit/httptransport/httptransport.go @@ -11,9 +11,9 @@ import ( "text/template" "unicode" - log "github.com/sirupsen/logrus" gogen "github.com/gogo/protobuf/protoc-gen-gogo/generator" "github.com/pkg/errors" + log "github.com/sirupsen/logrus" "github.com/metaverse/truss/gengokit/httptransport/templates" "github.com/metaverse/truss/svcdef" @@ -345,8 +345,12 @@ case {{$option.LocalName}}OK: // convert the string form of the field to it's correct go type. func createDecodeConvertFunc(f Field) (string, bool) { needsErrorCheck := true + // We may leverage the below convert logic for repeated base types. By + // triming the slice prefix we can easily store the template for the + // type if needed. + goType := strings.TrimPrefix(f.GoType, "[]") fType := "" - switch f.GoType { + switch goType { case "uint32": fType = "%s, err := strconv.ParseUint(%s, 10, 32)" case "uint64": @@ -381,6 +385,11 @@ func createDecodeConvertFunc(f Field) (string, bool) { var {{.LocalName}} *{{.GoType}} {{.LocalName}} = &{{.GoType}}{} err = json.Unmarshal([]byte({{.LocalName}}Str), {{.LocalName}})` + + errorCheckingTmpl := ` +if err != nil { + return nil, errors.Wrapf(err, "couldn't decode {{.LocalName}} from %v", {{.LocalName}}Str) +}` // All repeated args of any type are represented as slices, and bare // assignments to a slice accept a slice as the rvalue. As a result, // LocalName will be declared as a slice, and json.Unmarshal handles @@ -389,28 +398,52 @@ err = json.Unmarshal([]byte({{.LocalName}}Str), {{.LocalName}})` // provided, and if that fails, we try to unmarshal the string // surrounded by square brackets. If THAT fails, then the string does // not represent a valid JSON string and an error is returned. + + repeatedFieldType := strings.TrimPrefix(f.GoType, "[]") + convertedVar := "converted" + switch repeatedFieldType { + case "uint32", "int32", "float32": + convertedVar = repeatedFieldType + "(converted)" + } repeatedUnmarshalTmpl := ` var {{.LocalName}} {{.GoType}} -{{- if and (and .IsBaseType .Repeated) (not (Contains .GoType "[]byte"))}} -err = json.Unmarshal([]byte({{.LocalName}}Str), &{{.LocalName}}) -if err != nil { - {{.LocalName}}Str = "[" + {{.LocalName}}Str + "]" -} +{{- if and (.IsBaseType) (not (Contains .GoType "[]byte"))}} +if len({{.LocalName}}StrArr) > 1 { + {{- if (Contains .GoType "[]string")}} + {{.LocalName}} = {{.LocalName}}StrArr + {{- else}} + {{.LocalName}} = make({{.GoType}}, len({{.LocalName}}StrArr)) + for i, v := range {{.LocalName}}StrArr { + ` + fmt.Sprintf(fType, "converted", "v") + errorCheckingTmpl + ` + {{.LocalName}}[i] = ` + convertedVar + ` + } + {{- end}} +} else { {{- end}} -err = json.Unmarshal([]byte({{.LocalName}}Str), &{{.LocalName}})` - - errorCheckingTmpl := ` -if err != nil { - return nil, errors.Wrapf(err, "couldn't decode {{.LocalName}} from %v", {{.LocalName}}Str) -}` + {{- if (Contains .GoType "[]string")}} + {{.LocalName}} = strings.Split({{.LocalName}}Str, ",") + {{- else if and (and .IsBaseType .Repeated) (not (Contains .GoType "[]byte"))}} + err = json.Unmarshal([]byte({{.LocalName}}Str), &{{.LocalName}}) + if err != nil { + {{.LocalName}}Str = "[" + {{.LocalName}}Str + "]" + } + err = json.Unmarshal([]byte({{.LocalName}}Str), &{{.LocalName}}) + {{- else}} + err = json.Unmarshal([]byte({{.LocalName}}Str), &{{.LocalName}}) + {{- end}} +{{- if and (.IsBaseType) (not (Contains .GoType "[]byte"))}} +} +{{- end}}` - var preamble string + var jsonConvTmpl string if !f.Repeated { - preamble = singleCustomTypeUnmarshalTmpl + jsonConvTmpl = singleCustomTypeUnmarshalTmpl + errorCheckingTmpl } else { - preamble = repeatedUnmarshalTmpl + jsonConvTmpl = repeatedUnmarshalTmpl + if repeatedFieldType != "string" { + jsonConvTmpl = repeatedUnmarshalTmpl + errorCheckingTmpl + } } - jsonConvTmpl := preamble + errorCheckingTmpl code, err := ApplyTemplate("UnmarshalNonBaseType", jsonConvTmpl, f, TemplateFuncs) if err != nil { panic(fmt.Sprintf("Couldn't apply template: %v", err)) From 5891a726e33109473b8683a464aa8257bc03a188 Mon Sep 17 00:00:00 2001 From: Zaq? Wiedmann Date: Sat, 5 Sep 2020 18:30:38 -0700 Subject: [PATCH 2/2] update client tmpl to use new repeated query param strategy expose tests added in #309 --- cmd/_integration-tests/transport/http_test.go | 56 +++++++++---------- gengokit/httptransport/templates/client.go | 10 +++- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/cmd/_integration-tests/transport/http_test.go b/cmd/_integration-tests/transport/http_test.go index 93a0150a..0dafaea7 100644 --- a/cmd/_integration-tests/transport/http_test.go +++ b/cmd/_integration-tests/transport/http_test.go @@ -105,17 +105,16 @@ func TestGetWithRepeatedQueryRequest(t *testing.T) { if err != nil { t.Fatal(errors.Wrap(err, "cannot make http request")) } - // csv style err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedquery?%s=%d,%d", "A", A[0], A[1]) if err != nil { t.Fatal(errors.Wrap(err, "cannot make http request")) } // multi / golang style - // err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedquery?%s=%d&%s=%d", "A", A[0], "A", A[1]) - // if err != nil { - // t.Fatal(errors.Wrap(err, "cannot make http request")) - // } + err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedquery?%s=%d&%s=%d", "A", A[0], "A", A[1]) + if err != nil { + t.Fatal(errors.Wrap(err, "cannot make http request")) + } } func TestGetWithRepeatedStringQueryClient(t *testing.T) { @@ -146,37 +145,32 @@ func TestGetWithRepeatedStringQueryRequest(t *testing.T) { expects := pb.GetWithRepeatedStringQueryResponse{ V: A[0] + A[1], } - - var err error - - err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=[\"%s\",\"%s\"]", "A", A[0], A[1]) - if err != nil { - t.Fatal(errors.Wrap(err, "cannot make http request")) + expectsSingle := pb.GetWithRepeatedStringQueryResponse{ + V: A[0], } - // csv style - err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=\"%s\",\"%s\"", "A", A[0], A[1]) - if err != nil { - t.Fatal(errors.Wrap(err, "cannot make http request")) - } + var err error - // default array, no quotes - // err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=[%s,%s]", "A", A[0], A[1]) - // if err != nil { - // t.Fatal(errors.Wrap(err, "cannot make http request")) - // } + t.Run("single Value", func(t *testing.T) { + err = testHTTP(t, &resp, &expectsSingle, nil, "GET", "getwithrepeatedstringquery?%s=%s", "A", A[0]) + if err != nil { + t.Fatal(errors.Wrap(err, "cannot make http request")) + } + }) - // csv style, no quotes - // err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=[%s,%s]", "A", A[0], A[1]) - // if err != nil { - // t.Fatal(errors.Wrap(err, "cannot make http request")) - // } + t.Run("csv style, no quotes", func(t *testing.T) { + err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=%s,%s", "A", A[0], A[1]) + if err != nil { + t.Fatal(errors.Wrap(err, "cannot make http request")) + } + }) - // multi / golang style - // err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=%s&%s=%s", "A", A[0], "A", A[1]) - // if err != nil { - // t.Fatal(errors.Wrap(err, "cannot make http request")) - // } + t.Run("multi / golang style", func(t *testing.T) { + err = testHTTP(t, &resp, &expects, nil, "GET", "getwithrepeatedstringquery?%s=%s&%s=%s", "A", A[0], "A", A[1]) + if err != nil { + t.Fatal(errors.Wrap(err, "cannot make http request")) + } + }) } diff --git a/gengokit/httptransport/templates/client.go b/gengokit/httptransport/templates/client.go index 58f5f41c..3b08b859 100644 --- a/gengokit/httptransport/templates/client.go +++ b/gengokit/httptransport/templates/client.go @@ -35,7 +35,15 @@ var ClientEncodeTemplate = ` _ = tmp {{- range $field := $binding.Fields }} {{- if eq $field.Location "query"}} - {{if or (not $field.IsBaseType) $field.Repeated}} + {{if and $field.Repeated $field.IsBaseType}} + {{- if (Contains $field.GoType "[]string")}} + values["{{$field.QueryParamName}}"] = req.{{$field.CamelName}} + {{- else}} + for _, v := range req.{{$field.CamelName}} { + values.Add("{{$field.QueryParamName}}", fmt.Sprint(v)) + } + {{- end}} + {{else if or (not $field.IsBaseType) $field.Repeated}} tmp, err = json.Marshal(req.{{$field.CamelName}}) if err != nil { return errors.Wrap(err, "failed to marshal req.{{$field.CamelName}}")