Skip to content

Commit

Permalink
add plugin param 'allow_delete_body' (grpc-ecosystem#280)
Browse files Browse the repository at this point in the history
* added plugin parameter 'allow_delete_body' to allow REST mappings for legacy services that inapproriately use HTTP DELETE with body

* added comment to pubic registy method so lint doesn't complain

* 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

* fixed copy paste oops on using param vs global flag.Commandline global

* kvvar ignores whitespace & empty string Set() input now

* removed protoc-gen-swagger pkg_map cmd line flag and KVVar flag.Value impl from utilities pkg
  • Loading branch information
msample authored and tmc committed Jan 9, 2017
1 parent 241a93d commit 6b73ae5
Show file tree
Hide file tree
Showing 5 changed files with 288 additions and 20 deletions.
9 changes: 9 additions & 0 deletions protoc-gen-grpc-gateway/descriptor/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -260,6 +263,12 @@ 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
}

// 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 {
Expand Down
2 changes: 1 addition & 1 deletion protoc-gen-grpc-gateway/descriptor/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down
101 changes: 101 additions & 0 deletions protoc-gen-grpc-gateway/descriptor/services_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
67 changes: 48 additions & 19 deletions protoc-gen-swagger/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"flag"
"fmt"
"io"
"io/ioutil"
"os"
Expand All @@ -15,8 +16,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) {
Expand Down Expand Up @@ -50,29 +52,21 @@ func main() {
if err != nil {
glog.Fatal(err)
}
pkgMap := make(map[string]string)
if req.Parameter != nil {
for _, p := range strings.Split(req.GetParameter(), ",") {
spec := strings.SplitN(p, "=", 2)
if len(spec) == 1 {
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, pkgMap)
if err != nil {
glog.Fatalf("Error parsing flags: %v", err)
}
}

reg.SetPrefix(*importPrefix)
reg.SetAllowDeleteBody(*allowDeleteBody)
for k, v := range pkgMap {
reg.AddPkgMap(k, v)
}
g := genswagger.New(reg)

reg.SetPrefix(*importPrefix)
if err := reg.Load(req); err != nil {
emitError(err)
return
Expand Down Expand Up @@ -113,3 +107,38 @@ func emitResp(resp *plugin.CodeGeneratorResponse) {
glog.Fatal(err)
}
}

// parseReqParam parses a CodeGeneratorRequest parameter and adds the
// 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
}
for _, p := range strings.Split(param, ",") {
spec := strings.SplitN(p, "=", 2)
if len(spec) == 1 {
if spec[0] == "allow_delete_body" {
err := f.Set(spec[0], "true")
if err != nil {
return fmt.Errorf("Cannot set flag %s: %v", p, err)
}
continue
}
err := f.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") {
pkgMap[name[1:]] = value
continue
}
if err := f.Set(name, value); err != nil {
return fmt.Errorf("Cannot set flag %s: %v", p, err)
}
}
return nil
}
129 changes: 129 additions & 0 deletions protoc-gen-swagger/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
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
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)
}
if !reflect.DeepEqual(pkgMap, expected) {
t.Errorf("Test 0: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap)
}
checkFlags(false, "stdin", "", t, 0)

clearFlags()
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)
}
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()
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)
}
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()
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)
}
if !reflect.DeepEqual(pkgMap, expected) {
t.Errorf("Test 3: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap)
}
checkFlags(false, "stdin", "", t, 3)

clearFlags()
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)
}
if !reflect.DeepEqual(pkgMap, expected) {
t.Errorf("Test 4: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap)
}
checkFlags(false, "stdin", "", t, 4)

clearFlags()
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")
}
if !reflect.DeepEqual(pkgMap, expected) {
t.Errorf("Test 5: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap)
}
checkFlags(false, "stdin", "", t, 5)

clearFlags()
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")
}
if !reflect.DeepEqual(pkgMap, expected) {
t.Errorf("Test 6: pkgMap parse error, expected '%v', got '%v'", expected, pkgMap)
}
checkFlags(false, "stdin", "", t, 6)

clearFlags()
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)
}
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, 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)
}
}

func clearFlags() {
*importPrefix = ""
*file = "stdin"
*allowDeleteBody = false
}

0 comments on commit 6b73ae5

Please sign in to comment.