From 5b7b06e3cdb5f8902e4df7ae7e5c7bbf38d109b8 Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Sat, 10 Dec 2016 16:59:01 -0800 Subject: [PATCH 1/6] added plugin parameter 'allow_delete_body' to allow REST mappings for legacy services that inapproriately use HTTP DELETE with body --- protoc-gen-grpc-gateway/descriptor/registry.go | 7 +++++++ protoc-gen-grpc-gateway/descriptor/services.go | 2 +- protoc-gen-swagger/main.go | 13 +++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/protoc-gen-grpc-gateway/descriptor/registry.go b/protoc-gen-grpc-gateway/descriptor/registry.go index f293f555824..6078d5a9feb 100644 --- a/protoc-gen-grpc-gateway/descriptor/registry.go +++ b/protoc-gen-grpc-gateway/descriptor/registry.go @@ -30,6 +30,9 @@ type Registry struct { // pkgAliases is a mapping from package aliases to package paths in go which are already taken. pkgAliases map[string]string + + // allowDeleteBody permits http delete methods to have a body + allowDeleteBody bool } // NewRegistry returns a new Registry. @@ -260,6 +263,10 @@ func (r *Registry) GetAllFQENs() []string { return keys } +func (r *Registry) SetAllowDeleteBody(allow bool) { + r.allowDeleteBody = allow +} + // defaultGoPackageName returns the default go package name to be used for go files generated from "f". // You might need to use an unique alias for the package when you import it. Use ReserveGoPackageAlias to get a unique alias. func defaultGoPackageName(f *descriptor.FileDescriptorProto) string { diff --git a/protoc-gen-grpc-gateway/descriptor/services.go b/protoc-gen-grpc-gateway/descriptor/services.go index edf73caf77b..7bd928678f0 100644 --- a/protoc-gen-grpc-gateway/descriptor/services.go +++ b/protoc-gen-grpc-gateway/descriptor/services.go @@ -89,7 +89,7 @@ func (r *Registry) newMethod(svc *Service, md *descriptor.MethodDescriptorProto, case opts.GetDelete() != "": httpMethod = "DELETE" pathTemplate = opts.GetDelete() - if opts.Body != "" { + if opts.Body != "" && !r.allowDeleteBody { return nil, fmt.Errorf("needs request body even though http method is DELETE: %s", md.GetName()) } diff --git a/protoc-gen-swagger/main.go b/protoc-gen-swagger/main.go index fc3a0030a11..4cf1facd237 100644 --- a/protoc-gen-swagger/main.go +++ b/protoc-gen-swagger/main.go @@ -15,8 +15,9 @@ import ( ) var ( - importPrefix = flag.String("import_prefix", "", "prefix to be added to go package paths for imported proto files") - file = flag.String("file", "stdin", "where to load data from") + importPrefix = flag.String("import_prefix", "", "prefix to be added to go package paths for imported proto files") + file = flag.String("file", "stdin", "where to load data from") + allowDeleteBody = flag.Bool("allow_delete_body", false, "unless set, HTTP DELETE methods may not have a body") ) func parseReq(r io.Reader) (*plugin.CodeGeneratorRequest, error) { @@ -54,6 +55,12 @@ func main() { for _, p := range strings.Split(req.GetParameter(), ",") { spec := strings.SplitN(p, "=", 2) if len(spec) == 1 { + if spec[0] == "allow_delete_body" { + if err := flag.CommandLine.Set(spec[0], "true"); err != nil { + glog.Fatalf("Cannot set flag %s", p) + } + continue + } if err := flag.CommandLine.Set(spec[0], ""); err != nil { glog.Fatalf("Cannot set flag %s", p) } @@ -73,6 +80,8 @@ func main() { g := genswagger.New(reg) reg.SetPrefix(*importPrefix) + reg.SetAllowDeleteBody(*allowDeleteBody) + if err := reg.Load(req); err != nil { emitError(err) return From 419ac1d908be967338504d3d230316109f70c813 Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Sat, 10 Dec 2016 23:26:01 -0800 Subject: [PATCH 2/6] added comment to pubic registy method so lint doesn't complain --- protoc-gen-grpc-gateway/descriptor/registry.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/protoc-gen-grpc-gateway/descriptor/registry.go b/protoc-gen-grpc-gateway/descriptor/registry.go index 6078d5a9feb..123280990d3 100644 --- a/protoc-gen-grpc-gateway/descriptor/registry.go +++ b/protoc-gen-grpc-gateway/descriptor/registry.go @@ -263,6 +263,8 @@ func (r *Registry) GetAllFQENs() []string { return keys } +// SetAllowDeleteBody controls whether http delete methods may have a +// body or fail loading if encountered. func (r *Registry) SetAllowDeleteBody(allow bool) { r.allowDeleteBody = allow } From b0e4939a36e1dcc70ce7eed29db1977d7d846648 Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Sun, 8 Jan 2017 22:36:15 -0800 Subject: [PATCH 3/6] added tests for allow_delete_body both on command line/code-gen-req-param parsing and in registry service generation. Required mild refactoring of protoc-gen-swagger cmd line parsing to make testable --- .../descriptor/services_test.go | 101 ++++++++++++++++++ protoc-gen-swagger/main.go | 73 ++++++++----- protoc-gen-swagger/main_test.go | 97 +++++++++++++++++ utilities/kvvar.go | 56 ++++++++++ utilities/kvvar_test.go | 86 +++++++++++++++ 5 files changed, 389 insertions(+), 24 deletions(-) create mode 100644 protoc-gen-swagger/main_test.go create mode 100644 utilities/kvvar.go create mode 100644 utilities/kvvar_test.go diff --git a/protoc-gen-grpc-gateway/descriptor/services_test.go b/protoc-gen-grpc-gateway/descriptor/services_test.go index 7b4af4e6898..eda34d4141e 100644 --- a/protoc-gen-grpc-gateway/descriptor/services_test.go +++ b/protoc-gen-grpc-gateway/descriptor/services_test.go @@ -1107,3 +1107,104 @@ func TestResolveFieldPath(t *testing.T) { } } } + +func TestExtractServicesWithDeleteBody(t *testing.T) { + for _, spec := range []struct { + allowDeleteBody bool + expectErr bool + target string + srcs []string + }{ + // body for DELETE, but registry configured to allow it + { + allowDeleteBody: true, + expectErr: false, + target: "path/to/example.proto", + srcs: []string{ + ` + name: "path/to/example.proto", + package: "example" + message_type < + name: "StringMessage" + field < + name: "string" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_STRING + > + > + service < + name: "ExampleService" + method < + name: "RemoveResource" + input_type: "StringMessage" + output_type: "StringMessage" + options < + [google.api.http] < + delete: "/v1/example/resource" + body: "string" + > + > + > + > + `, + }, + }, + // body for DELETE, registry configured not to allow it + { + allowDeleteBody: false, + expectErr: true, + target: "path/to/example.proto", + srcs: []string{ + ` + name: "path/to/example.proto", + package: "example" + message_type < + name: "StringMessage" + field < + name: "string" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_STRING + > + > + service < + name: "ExampleService" + method < + name: "RemoveResource" + input_type: "StringMessage" + output_type: "StringMessage" + options < + [google.api.http] < + delete: "/v1/example/resource" + body: "string" + > + > + > + > + `, + }, + }, + } { + reg := NewRegistry() + reg.SetAllowDeleteBody(spec.allowDeleteBody) + + var fds []*descriptor.FileDescriptorProto + for _, src := range spec.srcs { + var fd descriptor.FileDescriptorProto + if err := proto.UnmarshalText(src, &fd); err != nil { + t.Fatalf("proto.UnmarshalText(%s, &fd) failed with %v; want success", src, err) + } + reg.loadFile(&fd) + fds = append(fds, &fd) + } + err := reg.loadServices(reg.files[spec.target]) + if spec.expectErr && err == nil { + t.Errorf("loadServices(%q) succeeded; want an error; allowDeleteBody=%v, files=%v", spec.target, spec.allowDeleteBody, spec.srcs) + } + if !spec.expectErr && err != nil { + t.Errorf("loadServices(%q) failed; do not want an error; allowDeleteBody=%v, files=%v", spec.target, spec.allowDeleteBody, spec.srcs) + } + t.Log(err) + } +} diff --git a/protoc-gen-swagger/main.go b/protoc-gen-swagger/main.go index 4cf1facd237..34338f41dd9 100644 --- a/protoc-gen-swagger/main.go +++ b/protoc-gen-swagger/main.go @@ -2,6 +2,7 @@ package main import ( "flag" + "fmt" "io" "io/ioutil" "os" @@ -12,14 +13,20 @@ import ( plugin "github.com/golang/protobuf/protoc-gen-go/plugin" "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor" "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger" + "github.com/grpc-ecosystem/grpc-gateway/utilities" ) var ( importPrefix = flag.String("import_prefix", "", "prefix to be added to go package paths for imported proto files") file = flag.String("file", "stdin", "where to load data from") allowDeleteBody = flag.Bool("allow_delete_body", false, "unless set, HTTP DELETE methods may not have a body") + pkgMap = utilities.NewKVVar() ) +func init() { + flag.Var(pkgMap, "pkg_map", "mapping from imported proto file path to Go pkg with its generated files") +} + func parseReq(r io.Reader) (*plugin.CodeGeneratorRequest, error) { glog.V(1).Info("Parsing code generator request") input, err := ioutil.ReadAll(r) @@ -52,35 +59,18 @@ func main() { glog.Fatal(err) } if req.Parameter != nil { - for _, p := range strings.Split(req.GetParameter(), ",") { - spec := strings.SplitN(p, "=", 2) - if len(spec) == 1 { - if spec[0] == "allow_delete_body" { - if err := flag.CommandLine.Set(spec[0], "true"); err != nil { - glog.Fatalf("Cannot set flag %s", p) - } - continue - } - if err := flag.CommandLine.Set(spec[0], ""); err != nil { - glog.Fatalf("Cannot set flag %s", p) - } - continue - } - name, value := spec[0], spec[1] - if strings.HasPrefix(name, "M") { - reg.AddPkgMap(name[1:], value) - continue - } - if err := flag.CommandLine.Set(name, value); err != nil { - glog.Fatalf("Cannot set flag %s", p) - } + err := parseReqParam(req.GetParameter(), flag.CommandLine) + if err != nil { + glog.Fatalf("Error parsing flags: %v", err) } } - g := genswagger.New(reg) - reg.SetPrefix(*importPrefix) reg.SetAllowDeleteBody(*allowDeleteBody) + for k, v := range pkgMap { + reg.AddPkgMap(k, v) + } + g := genswagger.New(reg) if err := reg.Load(req); err != nil { emitError(err) @@ -122,3 +112,38 @@ func emitResp(resp *plugin.CodeGeneratorResponse) { glog.Fatal(err) } } + +// parseReqParam parses a CodeGeneratorRequest parameter and adds the +// extracted values to the given FlagSet. Returns a non-nil error if +// setting a flag failed. +func parseReqParam(param string, f *flag.FlagSet) error { + if param == "" { + return nil + } + for _, p := range strings.Split(param, ",") { + spec := strings.SplitN(p, "=", 2) + if len(spec) == 1 { + if spec[0] == "allow_delete_body" { + err := flag.CommandLine.Set(spec[0], "true") + if err != nil { + return fmt.Errorf("Cannot set flag %s: %v", p, err) + } + continue + } + err := flag.CommandLine.Set(spec[0], "") + if err != nil { + return fmt.Errorf("Cannot set flag %s: %v", p, err) + } + continue + } + name, value := spec[0], spec[1] + if strings.HasPrefix(name, "M") { + f.Set("pkg_map", name[1:]+"="+value) + continue + } + if err := flag.CommandLine.Set(name, value); err != nil { + return fmt.Errorf("Cannot set flag %s: %v", p, err) + } + } + return nil +} diff --git a/protoc-gen-swagger/main_test.go b/protoc-gen-swagger/main_test.go new file mode 100644 index 00000000000..8c097cd6f2a --- /dev/null +++ b/protoc-gen-swagger/main_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "flag" + "reflect" + "testing" +) + +func TestParseReqParam(t *testing.T) { + + f := flag.CommandLine + + // this one must be first - with no leading clearFlags call it + // verifies our expectation of default values as we reset by + // clearFlags + err := parseReqParam("", f) + if err != nil { + t.Errorf("Test 0: unexpected parse error '%v'", err) + } + checkFlags(false, "stdin", "", map[string]string{}, t, 0) + + clearFlags() + err = parseReqParam("allow_delete_body,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f) + if err != nil { + t.Errorf("Test 1: unexpected parse error '%v'", err) + } + checkFlags(true, "./foo.pb", "/bar/baz", + map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"}, t, 1) + + clearFlags() + err = parseReqParam("allow_delete_body=true,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f) + if err != nil { + t.Errorf("Test 2: unexpected parse error '%v'", err) + } + checkFlags(true, "./foo.pb", "/bar/baz", + map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"}, t, 2) + + clearFlags() + err = parseReqParam("allow_delete_body=false,Ma/b/c.proto=github.com/x/y/z,Mf/g/h.proto=github.com/1/2/3/", f) + if err != nil { + t.Errorf("Test 3: unexpected parse error '%v'", err) + } + checkFlags(false, "stdin", "", map[string]string{"a/b/c.proto": "github.com/x/y/z", "f/g/h.proto": "github.com/1/2/3/"}, t, 3) + + clearFlags() + err = parseReqParam("", f) + if err != nil { + t.Errorf("Test 4: unexpected parse error '%v'", err) + } + checkFlags(false, "stdin", "", map[string]string{}, t, 4) + + clearFlags() + err = parseReqParam("unknown_param=17", f) + if err == nil { + t.Error("Test 5: expected parse error not returned") + } + checkFlags(false, "stdin", "", map[string]string{}, t, 5) + + clearFlags() + err = parseReqParam("Mfoo", f) + if err == nil { + t.Error("Test 6: expected parse error not returned") + } + checkFlags(false, "stdin", "", map[string]string{}, t, 6) + + clearFlags() + err = parseReqParam("allow_delete_body,file,import_prefix", f) + if err != nil { + t.Errorf("Test 7: unexpected parse error '%v'", err) + } + checkFlags(true, "", "", map[string]string{}, t, 7) + +} + +func checkFlags(allowDeleteV bool, fileV, importPathV string, pkgMapV map[string]string, t *testing.T, tid int) { + if *importPrefix != importPathV { + t.Errorf("Test %v: import_prefix misparsed, expected '%v', got '%v'", tid, importPathV, *importPrefix) + } + if *file != fileV { + t.Errorf("Test %v: file misparsed, expected '%v', got '%v'", tid, fileV, *file) + } + if *allowDeleteBody != allowDeleteV { + t.Errorf("Test %v: allow_delete_body misparsed, expected '%v', got '%v'", tid, allowDeleteV, *allowDeleteBody) + } + if !reflect.DeepEqual(map[string]string(pkgMap), pkgMapV) { + t.Errorf("Test %v: pkg_map misparsed, expected '%v', got '%v'", tid, pkgMapV, (map[string]string)(pkgMap)) + } +} + +func clearFlags() { + *importPrefix = "" + *file = "stdin" + *allowDeleteBody = false + for k := range pkgMap { + delete(pkgMap, k) + } +} diff --git a/utilities/kvvar.go b/utilities/kvvar.go new file mode 100644 index 00000000000..348bd3d2df0 --- /dev/null +++ b/utilities/kvvar.go @@ -0,0 +1,56 @@ +package utilities + +import "strings" + +// KVVar is a command line flag.Value implementation accepting 0 or +// more instances on the command line with a value like +// "param1=some-str-val1,param2=some-str-val2". Multiple occurrences +// are concatenated and the last instance of a duplicate key wins. +type KVVar map[string]string + +// NewKVVar return an empty KVar ready for Set() & String() calls +func NewKVVar() KVVar { + return make(map[string]string) +} + +// String returns the serialized value, e.g. "k=v,k2=v2" with keys in +// unspecified order across calls. +func (k KVVar) String() string { + var b []byte + i := 1 + for key, val := range k { + b = append(b, key...) + b = append(b, byte('=')) + b = append(b, val...) + if i < len(k) { + b = append(b, byte(',')) + } + i++ + } + return string(b) +} + +// Set adds the given key value pairs to the KVVar in the order +// given. Syntax of given string is "k=v,k2=v2,k=v3,k3" where last +// duplicated key wins (k=v3) and keys without a value are set to "" +// (k3=""). Set may be called more than once, e.g. when a KVVar +// command line flag is given more than once, in which case they are +// added as described above. Leading and trailing whitespace around keys +// and values is removed. +func (k KVVar) Set(s string) error { + kvs := strings.Split(s, ",") + for _, kv := range kvs { + p := strings.SplitN(kv, "=", 2) + if len(p) == 1 { + k[strings.TrimSpace(p[0])] = "" + continue + } + k[strings.TrimSpace(p[0])] = strings.TrimSpace(p[1]) + } + return nil +} + +// Get makes this flag.Getter compatible +func (k KVVar) Get() interface{} { + return k +} diff --git a/utilities/kvvar_test.go b/utilities/kvvar_test.go new file mode 100644 index 00000000000..c4c33177279 --- /dev/null +++ b/utilities/kvvar_test.go @@ -0,0 +1,86 @@ +package utilities + +import ( + "reflect" + "testing" +) + +func TestKVVar(t *testing.T) { + // simple + kvv := NewKVVar() + kvv.Set("x=y") + kvcheck("x", "y", kvv, t) + kvv2 := NewKVVar() + kvv2.Set(kvv.String()) // check equiv string serialization + kveq(kvv, kvv2, t) + + // more than one kv + kvv = NewKVVar() + kvv.Set("a=2,b=1") + kvcheck("a", "2", kvv, t) + kvcheck("b", "1", kvv, t) + kvv2 = NewKVVar() + kvv2.Set(kvv.String()) + kveq(kvv, kvv2, t) + + // multiple Set() calls with mutiple kv's per calls + kvv = NewKVVar() + kvv.Set(" c = 1 , d = 2 ") + kvv.Set("mountain = top of the world ,time=") + kvcheck("c", "1", kvv, t) + kvcheck("d", "2", kvv, t) + kvcheck("mountain", "top of the world", kvv, t) + kvcheck("time", "", kvv, t) + kvv2 = NewKVVar() + kvv2.Set(kvv.String()) + kveq(kvv, kvv2, t) + + // last dup key wins + kvv = NewKVVar() + kvv.Set("f=1,f=2") + kvcheck("f", "2", kvv, t) + kvv2 = NewKVVar() + kvv2.Set(kvv.String()) + kveq(kvv, kvv2, t) + + kvv = NewKVVar() + kvv.Set("g=1,h=2") + kvv.Set("g=3") + kvcheck("g", "3", kvv, t) + kvcheck("h", "2", kvv, t) + kvv2 = NewKVVar() + kvv2.Set(kvv.String()) + kveq(kvv, kvv2, t) + + kvv = NewKVVar() + kvv.Set("i=1") + kvv.Set("i=3") + kvv.Set("i=4,j=3") + kvv.Set("k=4,foo=3") + kvcheck("i", "4", kvv, t) + kvcheck("j", "3", kvv, t) + kvcheck("k", "4", kvv, t) + kvcheck("foo", "3", kvv, t) + kvv2 = NewKVVar() + kvv2.Set(kvv.String()) + kveq(kvv, kvv2, t) + + if k2, ok := (kvv.Get()).(KVVar); !ok || !reflect.DeepEqual(k2, kvv) { + t.Errorf("KVVar.Get is broken") + } +} + +func kvcheck(key, value string, kvv KVVar, t *testing.T) { + if _, ok := kvv[key]; value != "" && !ok { + t.Errorf("KVVar key '%v' does not exist", key) + } + if kvv[key] != value { + t.Errorf("got '%v' for KVVar key '%v', expected '%v'", kvv[key], key, value) + } +} + +func kveq(kvv1, kvv2 KVVar, t *testing.T) { + if !reflect.DeepEqual(kvv1, kvv2) { + t.Errorf("KVars not equal: '%v' vs '%v'", kvv1, kvv2) + } +} From 76902bbc4b92b61e7d239b677ec05163be8aa84b Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Sun, 8 Jan 2017 22:43:18 -0800 Subject: [PATCH 4/6] fixed copy paste oops on using param vs global flag.Commandline global --- protoc-gen-swagger/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/protoc-gen-swagger/main.go b/protoc-gen-swagger/main.go index 34338f41dd9..2d9bc43a671 100644 --- a/protoc-gen-swagger/main.go +++ b/protoc-gen-swagger/main.go @@ -124,13 +124,13 @@ func parseReqParam(param string, f *flag.FlagSet) error { spec := strings.SplitN(p, "=", 2) if len(spec) == 1 { if spec[0] == "allow_delete_body" { - err := flag.CommandLine.Set(spec[0], "true") + err := f.Set(spec[0], "true") if err != nil { return fmt.Errorf("Cannot set flag %s: %v", p, err) } continue } - err := flag.CommandLine.Set(spec[0], "") + err := f.Set(spec[0], "") if err != nil { return fmt.Errorf("Cannot set flag %s: %v", p, err) } @@ -141,7 +141,7 @@ func parseReqParam(param string, f *flag.FlagSet) error { f.Set("pkg_map", name[1:]+"="+value) continue } - if err := flag.CommandLine.Set(name, value); err != nil { + if err := f.Set(name, value); err != nil { return fmt.Errorf("Cannot set flag %s: %v", p, err) } } From 11d190e87d5c80bd16bbe32302eb2214891a9205 Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Sun, 8 Jan 2017 23:49:13 -0800 Subject: [PATCH 5/6] kvvar ignores whitespace & empty string Set() input now --- utilities/kvvar.go | 3 +++ utilities/kvvar_test.go | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/utilities/kvvar.go b/utilities/kvvar.go index 348bd3d2df0..29c5e90c5bc 100644 --- a/utilities/kvvar.go +++ b/utilities/kvvar.go @@ -38,6 +38,9 @@ func (k KVVar) String() string { // added as described above. Leading and trailing whitespace around keys // and values is removed. func (k KVVar) Set(s string) error { + if strings.TrimSpace(s) == "" { + return nil + } kvs := strings.Split(s, ",") for _, kv := range kvs { p := strings.SplitN(kv, "=", 2) diff --git a/utilities/kvvar_test.go b/utilities/kvvar_test.go index c4c33177279..7257257be23 100644 --- a/utilities/kvvar_test.go +++ b/utilities/kvvar_test.go @@ -23,6 +23,14 @@ func TestKVVar(t *testing.T) { kvv2.Set(kvv.String()) kveq(kvv, kvv2, t) + // empty input + kvv = NewKVVar() + kvv.Set("") + kvv.Set(" ") + if len(kvv) != 0 { + t.Errorf("KVVar should remain empty after empty input string: %v", kvv) + } + // multiple Set() calls with mutiple kv's per calls kvv = NewKVVar() kvv.Set(" c = 1 , d = 2 ") From c5fb87ee27e9830e5760f8bbb8c89b586574d130 Mon Sep 17 00:00:00 2001 From: Mike Sample Date: Mon, 9 Jan 2017 14:35:10 -0800 Subject: [PATCH 6/6] removed protoc-gen-swagger pkg_map cmd line flag and KVVar flag.Value impl from utilities pkg --- protoc-gen-swagger/main.go | 17 +++--- protoc-gen-swagger/main_test.go | 82 +++++++++++++++++++--------- utilities/kvvar.go | 59 --------------------- utilities/kvvar_test.go | 94 --------------------------------- 4 files changed, 63 insertions(+), 189 deletions(-) delete mode 100644 utilities/kvvar.go delete mode 100644 utilities/kvvar_test.go diff --git a/protoc-gen-swagger/main.go b/protoc-gen-swagger/main.go index 2d9bc43a671..db747704e23 100644 --- a/protoc-gen-swagger/main.go +++ b/protoc-gen-swagger/main.go @@ -13,20 +13,14 @@ import ( plugin "github.com/golang/protobuf/protoc-gen-go/plugin" "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/descriptor" "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger" - "github.com/grpc-ecosystem/grpc-gateway/utilities" ) var ( importPrefix = flag.String("import_prefix", "", "prefix to be added to go package paths for imported proto files") file = flag.String("file", "stdin", "where to load data from") allowDeleteBody = flag.Bool("allow_delete_body", false, "unless set, HTTP DELETE methods may not have a body") - pkgMap = utilities.NewKVVar() ) -func init() { - flag.Var(pkgMap, "pkg_map", "mapping from imported proto file path to Go pkg with its generated files") -} - func parseReq(r io.Reader) (*plugin.CodeGeneratorRequest, error) { glog.V(1).Info("Parsing code generator request") input, err := ioutil.ReadAll(r) @@ -58,8 +52,9 @@ func main() { if err != nil { glog.Fatal(err) } + pkgMap := make(map[string]string) if req.Parameter != nil { - err := parseReqParam(req.GetParameter(), flag.CommandLine) + err := parseReqParam(req.GetParameter(), flag.CommandLine, pkgMap) if err != nil { glog.Fatalf("Error parsing flags: %v", err) } @@ -114,9 +109,9 @@ func emitResp(resp *plugin.CodeGeneratorResponse) { } // parseReqParam parses a CodeGeneratorRequest parameter and adds the -// extracted values to the given FlagSet. Returns a non-nil error if -// setting a flag failed. -func parseReqParam(param string, f *flag.FlagSet) error { +// extracted values to the given FlagSet and pkgMap. Returns a non-nil +// error if setting a flag failed. +func parseReqParam(param string, f *flag.FlagSet, pkgMap map[string]string) error { if param == "" { return nil } @@ -138,7 +133,7 @@ func parseReqParam(param string, f *flag.FlagSet) error { } name, value := spec[0], spec[1] if strings.HasPrefix(name, "M") { - f.Set("pkg_map", name[1:]+"="+value) + pkgMap[name[1:]] = value continue } if err := f.Set(name, value); err != nil { diff --git a/protoc-gen-swagger/main_test.go b/protoc-gen-swagger/main_test.go index 8c097cd6f2a..c4d12dd20bd 100644 --- a/protoc-gen-swagger/main_test.go +++ b/protoc-gen-swagger/main_test.go @@ -13,66 +13,104 @@ func TestParseReqParam(t *testing.T) { // this one must be first - with no leading clearFlags call it // verifies our expectation of default values as we reset by // clearFlags - err := parseReqParam("", f) + pkgMap := make(map[string]string) + expected := map[string]string{} + err := parseReqParam("", f, pkgMap) if err != nil { t.Errorf("Test 0: unexpected parse error '%v'", err) } - checkFlags(false, "stdin", "", map[string]string{}, t, 0) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 0: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(false, "stdin", "", t, 0) clearFlags() - err = parseReqParam("allow_delete_body,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f) + pkgMap = make(map[string]string) + expected = map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"} + err = parseReqParam("allow_delete_body,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f, pkgMap) if err != nil { t.Errorf("Test 1: unexpected parse error '%v'", err) } - checkFlags(true, "./foo.pb", "/bar/baz", - map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"}, t, 1) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 1: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(true, "./foo.pb", "/bar/baz", t, 1) clearFlags() - err = parseReqParam("allow_delete_body=true,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f) + pkgMap = make(map[string]string) + expected = map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"} + err = parseReqParam("allow_delete_body=true,file=./foo.pb,import_prefix=/bar/baz,Mgoogle/api/annotations.proto=github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api", f, pkgMap) if err != nil { t.Errorf("Test 2: unexpected parse error '%v'", err) } - checkFlags(true, "./foo.pb", "/bar/baz", - map[string]string{"google/api/annotations.proto": "github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api"}, t, 2) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 2: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(true, "./foo.pb", "/bar/baz", t, 2) clearFlags() - err = parseReqParam("allow_delete_body=false,Ma/b/c.proto=github.com/x/y/z,Mf/g/h.proto=github.com/1/2/3/", f) + pkgMap = make(map[string]string) + expected = map[string]string{"a/b/c.proto": "github.com/x/y/z", "f/g/h.proto": "github.com/1/2/3/"} + err = parseReqParam("allow_delete_body=false,Ma/b/c.proto=github.com/x/y/z,Mf/g/h.proto=github.com/1/2/3/", f, pkgMap) if err != nil { t.Errorf("Test 3: unexpected parse error '%v'", err) } - checkFlags(false, "stdin", "", map[string]string{"a/b/c.proto": "github.com/x/y/z", "f/g/h.proto": "github.com/1/2/3/"}, t, 3) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 3: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(false, "stdin", "", t, 3) clearFlags() - err = parseReqParam("", f) + pkgMap = make(map[string]string) + expected = map[string]string{} + err = parseReqParam("", f, pkgMap) if err != nil { t.Errorf("Test 4: unexpected parse error '%v'", err) } - checkFlags(false, "stdin", "", map[string]string{}, t, 4) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 4: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(false, "stdin", "", t, 4) clearFlags() - err = parseReqParam("unknown_param=17", f) + pkgMap = make(map[string]string) + expected = map[string]string{} + err = parseReqParam("unknown_param=17", f, pkgMap) if err == nil { t.Error("Test 5: expected parse error not returned") } - checkFlags(false, "stdin", "", map[string]string{}, t, 5) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 5: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(false, "stdin", "", t, 5) clearFlags() - err = parseReqParam("Mfoo", f) + pkgMap = make(map[string]string) + expected = map[string]string{} + err = parseReqParam("Mfoo", f, pkgMap) if err == nil { t.Error("Test 6: expected parse error not returned") } - checkFlags(false, "stdin", "", map[string]string{}, t, 6) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 6: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(false, "stdin", "", t, 6) clearFlags() - err = parseReqParam("allow_delete_body,file,import_prefix", f) + pkgMap = make(map[string]string) + expected = map[string]string{} + err = parseReqParam("allow_delete_body,file,import_prefix", f, pkgMap) if err != nil { t.Errorf("Test 7: unexpected parse error '%v'", err) } - checkFlags(true, "", "", map[string]string{}, t, 7) + if !reflect.DeepEqual(pkgMap, expected) { + t.Errorf("Test 7: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap) + } + checkFlags(true, "", "", t, 7) } -func checkFlags(allowDeleteV bool, fileV, importPathV string, pkgMapV map[string]string, t *testing.T, tid int) { +func checkFlags(allowDeleteV bool, fileV, importPathV string, t *testing.T, tid int) { if *importPrefix != importPathV { t.Errorf("Test %v: import_prefix misparsed, expected '%v', got '%v'", tid, importPathV, *importPrefix) } @@ -82,16 +120,10 @@ func checkFlags(allowDeleteV bool, fileV, importPathV string, pkgMapV map[string if *allowDeleteBody != allowDeleteV { t.Errorf("Test %v: allow_delete_body misparsed, expected '%v', got '%v'", tid, allowDeleteV, *allowDeleteBody) } - if !reflect.DeepEqual(map[string]string(pkgMap), pkgMapV) { - t.Errorf("Test %v: pkg_map misparsed, expected '%v', got '%v'", tid, pkgMapV, (map[string]string)(pkgMap)) - } } func clearFlags() { *importPrefix = "" *file = "stdin" *allowDeleteBody = false - for k := range pkgMap { - delete(pkgMap, k) - } } diff --git a/utilities/kvvar.go b/utilities/kvvar.go deleted file mode 100644 index 29c5e90c5bc..00000000000 --- a/utilities/kvvar.go +++ /dev/null @@ -1,59 +0,0 @@ -package utilities - -import "strings" - -// KVVar is a command line flag.Value implementation accepting 0 or -// more instances on the command line with a value like -// "param1=some-str-val1,param2=some-str-val2". Multiple occurrences -// are concatenated and the last instance of a duplicate key wins. -type KVVar map[string]string - -// NewKVVar return an empty KVar ready for Set() & String() calls -func NewKVVar() KVVar { - return make(map[string]string) -} - -// String returns the serialized value, e.g. "k=v,k2=v2" with keys in -// unspecified order across calls. -func (k KVVar) String() string { - var b []byte - i := 1 - for key, val := range k { - b = append(b, key...) - b = append(b, byte('=')) - b = append(b, val...) - if i < len(k) { - b = append(b, byte(',')) - } - i++ - } - return string(b) -} - -// Set adds the given key value pairs to the KVVar in the order -// given. Syntax of given string is "k=v,k2=v2,k=v3,k3" where last -// duplicated key wins (k=v3) and keys without a value are set to "" -// (k3=""). Set may be called more than once, e.g. when a KVVar -// command line flag is given more than once, in which case they are -// added as described above. Leading and trailing whitespace around keys -// and values is removed. -func (k KVVar) Set(s string) error { - if strings.TrimSpace(s) == "" { - return nil - } - kvs := strings.Split(s, ",") - for _, kv := range kvs { - p := strings.SplitN(kv, "=", 2) - if len(p) == 1 { - k[strings.TrimSpace(p[0])] = "" - continue - } - k[strings.TrimSpace(p[0])] = strings.TrimSpace(p[1]) - } - return nil -} - -// Get makes this flag.Getter compatible -func (k KVVar) Get() interface{} { - return k -} diff --git a/utilities/kvvar_test.go b/utilities/kvvar_test.go deleted file mode 100644 index 7257257be23..00000000000 --- a/utilities/kvvar_test.go +++ /dev/null @@ -1,94 +0,0 @@ -package utilities - -import ( - "reflect" - "testing" -) - -func TestKVVar(t *testing.T) { - // simple - kvv := NewKVVar() - kvv.Set("x=y") - kvcheck("x", "y", kvv, t) - kvv2 := NewKVVar() - kvv2.Set(kvv.String()) // check equiv string serialization - kveq(kvv, kvv2, t) - - // more than one kv - kvv = NewKVVar() - kvv.Set("a=2,b=1") - kvcheck("a", "2", kvv, t) - kvcheck("b", "1", kvv, t) - kvv2 = NewKVVar() - kvv2.Set(kvv.String()) - kveq(kvv, kvv2, t) - - // empty input - kvv = NewKVVar() - kvv.Set("") - kvv.Set(" ") - if len(kvv) != 0 { - t.Errorf("KVVar should remain empty after empty input string: %v", kvv) - } - - // multiple Set() calls with mutiple kv's per calls - kvv = NewKVVar() - kvv.Set(" c = 1 , d = 2 ") - kvv.Set("mountain = top of the world ,time=") - kvcheck("c", "1", kvv, t) - kvcheck("d", "2", kvv, t) - kvcheck("mountain", "top of the world", kvv, t) - kvcheck("time", "", kvv, t) - kvv2 = NewKVVar() - kvv2.Set(kvv.String()) - kveq(kvv, kvv2, t) - - // last dup key wins - kvv = NewKVVar() - kvv.Set("f=1,f=2") - kvcheck("f", "2", kvv, t) - kvv2 = NewKVVar() - kvv2.Set(kvv.String()) - kveq(kvv, kvv2, t) - - kvv = NewKVVar() - kvv.Set("g=1,h=2") - kvv.Set("g=3") - kvcheck("g", "3", kvv, t) - kvcheck("h", "2", kvv, t) - kvv2 = NewKVVar() - kvv2.Set(kvv.String()) - kveq(kvv, kvv2, t) - - kvv = NewKVVar() - kvv.Set("i=1") - kvv.Set("i=3") - kvv.Set("i=4,j=3") - kvv.Set("k=4,foo=3") - kvcheck("i", "4", kvv, t) - kvcheck("j", "3", kvv, t) - kvcheck("k", "4", kvv, t) - kvcheck("foo", "3", kvv, t) - kvv2 = NewKVVar() - kvv2.Set(kvv.String()) - kveq(kvv, kvv2, t) - - if k2, ok := (kvv.Get()).(KVVar); !ok || !reflect.DeepEqual(k2, kvv) { - t.Errorf("KVVar.Get is broken") - } -} - -func kvcheck(key, value string, kvv KVVar, t *testing.T) { - if _, ok := kvv[key]; value != "" && !ok { - t.Errorf("KVVar key '%v' does not exist", key) - } - if kvv[key] != value { - t.Errorf("got '%v' for KVVar key '%v', expected '%v'", kvv[key], key, value) - } -} - -func kveq(kvv1, kvv2 KVVar, t *testing.T) { - if !reflect.DeepEqual(kvv1, kvv2) { - t.Errorf("KVars not equal: '%v' vs '%v'", kvv1, kvv2) - } -}