Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correctly map request body field by name #327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions gengokit/httptransport/httptransport.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ func NewBinding(i int, meth *svcdef.ServiceMethod) *Binding {
newField.IsEnum = field.Type.Enum != nil
newField.ConvertFunc, newField.ConvertFuncNeedsErrorCheck = createDecodeConvertFunc(newField)
newField.TypeConversion = createDecodeTypeConversion(newField)
if newField.Location == "body_root" {
newField.Location = "body"
nBinding.RequestRootField = &newField
}

nBinding.Fields = append(nBinding.Fields, &newField)

Expand Down
91 changes: 91 additions & 0 deletions gengokit/httptransport/httptransport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,97 @@ func init() {
gopath = filepath.SplitList(os.Getenv("GOPATH"))
}

func TestNewMethodWithBody(t *testing.T) {
defStr := `
syntax = "proto3";

// General package
package general;

import "github.com/metaverse/truss/deftree/googlethirdparty/annotations.proto";

message Inner {
string a = 1;
}

message SumRequest {
int64 a = 1;
Inner in = 2;
}

message SumReply {
int64 v = 1;
string err = 2;
}

service SumSvc {
rpc Sum(SumRequest) returns (SumReply) {
option (google.api.http) = {
put: "/sum/{a}"
body: "in"
};
}
}
`
sd, err := svcdef.NewFromString(defStr, gopath)
if err != nil {
t.Fatal(err, "Failed to create a service from the definition string")
}
innerField := Field{
Name: "In",
QueryParamName: "in",
CamelName: "In",
LowCamelName: "in",
LocalName: "InSum",
Location: "body",
GoType: "pb.Inner",
ConvertFunc: "\nvar InSum *pb.Inner\nInSum = &pb.Inner{}\nerr = json.Unmarshal([]byte(InSumStr), InSum)\nif err != nil {\n\treturn nil, errors.Wrapf(err, \"couldn't decode InSum from %v\", InSumStr)\n}",
ConvertFuncNeedsErrorCheck: false,
TypeConversion: "InSum",
IsBaseType: false,
}
binding := &Binding{
Label: "SumZero",
PathTemplate: "/sum/{a}",
BasePath: "/sum/",
Verb: "put",
RequestRootField: &innerField,
Fields: []*Field{
&Field{
Name: "A",
QueryParamName: "a",
CamelName: "A",
LowCamelName: "a",
LocalName: "ASum",
Location: "path",
GoType: "int64",
ConvertFunc: "ASum, err := strconv.ParseInt(ASumStr, 10, 64)",
ConvertFuncNeedsErrorCheck: true,
TypeConversion: "ASum",
IsBaseType: true,
},
&innerField,
},
}

meth := &Method{
Name: "Sum",
RequestType: "SumRequest",
ResponseType: "SumReply",
Bindings: []*Binding{
binding,
},
}
binding.Parent = meth

newMeth := NewMethod(sd.Service.Methods[0])
t.Logf("%v\n", spew.Sdump(sd.Service.Methods[0]))
if got, want := newMeth, meth; !reflect.DeepEqual(got, want) {
diff := gentesthelper.DiffStrings(spew.Sdump(got), spew.Sdump(want))
t.Errorf("got != want; methods differ: %v\n", diff)
}
}

func TestNewMethod(t *testing.T) {
defStr := `
syntax = "proto3";
Expand Down
13 changes: 12 additions & 1 deletion gengokit/httptransport/templates/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ var ServerDecodeTemplate = `
// body. Primarily useful in a server.
func DecodeHTTP{{$binding.Label}}Request(_ context.Context, r *http.Request) (interface{}, error) {
defer r.Body.Close()

var req pb.{{GoName $binding.Parent.RequestType}}
{{$req_field := "req" -}}
{{if $binding.RequestRootField -}}
{{$req_field = print "req" ($binding.RequestRootField.Name) -}}
var {{$req_field}} {{$binding.RequestRootField.GoType}}
{{end -}}

buf, err := ioutil.ReadAll(r.Body)
if err != nil {
return nil, errors.Wrapf(err, "cannot read body of http request")
Expand All @@ -19,7 +26,7 @@ var ServerDecodeTemplate = `
unmarshaller := jsonpb.Unmarshaler{
AllowUnknownFields: true,
}
if err = unmarshaller.Unmarshal(bytes.NewBuffer(buf), &req); err != nil {
if err = unmarshaller.Unmarshal(bytes.NewBuffer(buf), &{{$req_field}}); err != nil {
const size = 8196
if len(buf) > size {
buf = buf[:size]
Expand All @@ -31,6 +38,10 @@ var ServerDecodeTemplate = `
}
}

{{if $binding.RequestRootField}}
req.{{$binding.RequestRootField.Name}} = &{{$req_field}}
{{end}}

pathParams := mux.Vars(r)
_ = pathParams

Expand Down
131 changes: 131 additions & 0 deletions gengokit/httptransport/templates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,15 @@ func TestGenServerDecode(t *testing.T) {
if err != nil {
t.Errorf("Failed to generate server decode code: %v", err)
}

desired := `

// DecodeHTTPSumZeroRequest is a transport/http.DecodeRequestFunc that
// decodes a JSON-encoded sum request from the HTTP request
// body. Primarily useful in a server.
func DecodeHTTPSumZeroRequest(_ context.Context, r *http.Request) (interface{}, error) {
defer r.Body.Close()

var req pb.SumRequest
buf, err := ioutil.ReadAll(r.Body)
if err != nil {
Expand Down Expand Up @@ -211,3 +213,132 @@ func DecodeHTTPSumZeroRequest(_ context.Context, r *http.Request) (interface{},
t.Log(gentesthelper.DiffStrings(got, want))
}
}

func TestGenServerDecodeWithBody(t *testing.T) {
innerField := &Field{
Name: "c",
QueryParamName: "c",
CamelName: "C",
LowCamelName: "c",
LocalName: "CSum",
Location: "body",
GoType: "pb.Inner",
ConvertFunc: "",
ConvertFuncNeedsErrorCheck: true,
TypeConversion: "CSum",
IsBaseType: true,
}
binding := &Binding{
Label: "SumZero",
PathTemplate: "/sum/{a}",
BasePath: "/sum/",
Verb: "get",
RequestRootField: innerField,
Fields: []*Field{
&Field{
Name: "a",
QueryParamName: "a",
CamelName: "A",
LowCamelName: "a",
LocalName: "ASum",
Location: "path",
GoType: "int64",
ConvertFunc: "ASum, err := strconv.ParseInt(ASumStr, 10, 64)",
ConvertFuncNeedsErrorCheck: true,
TypeConversion: "ASum",
IsBaseType: true,
},
&Field{
Name: "b",
QueryParamName: "b",
CamelName: "B",
LowCamelName: "b",
LocalName: "BSum",
Location: "query",
GoType: "int64",
ConvertFunc: "BSum, err := strconv.ParseInt(BSumStr, 10, 64)",
ConvertFuncNeedsErrorCheck: true,
TypeConversion: "BSum",
IsBaseType: true,
},
innerField,
},
}
meth := &Method{
Name: "Sum",
RequestType: "SumRequest",
ResponseType: "SumReply",
Bindings: []*Binding{
binding,
},
}
binding.Parent = meth

str, err := binding.GenServerDecode()
if err != nil {
t.Errorf("Failed to generate server decode code: %v", err)
}
desired := `

// DecodeHTTPSumZeroRequest is a transport/http.DecodeRequestFunc that
// decodes a JSON-encoded sum request from the HTTP request
// body. Primarily useful in a server.
func DecodeHTTPSumZeroRequest(_ context.Context, r *http.Request) (interface{}, error) {
defer r.Body.Close()

var req pb.SumRequest
var reqc pb.Inner
buf, err := ioutil.ReadAll(r.Body)
if err != nil {
return nil, errors.Wrapf(err, "cannot read body of http request")
}
if len(buf) > 0 {
// AllowUnknownFields stops the unmarshaler from failing if the JSON contains unknown fields.
unmarshaller := jsonpb.Unmarshaler{
AllowUnknownFields: true,
}
if err = unmarshaller.Unmarshal(bytes.NewBuffer(buf), &reqc); err != nil {
const size = 8196
if len(buf) > size {
buf = buf[:size]
}
return nil, httpError{errors.Wrapf(err, "request body '%s': cannot parse non-json request body", buf),
http.StatusBadRequest,
nil,
}
}
}

req.c = &reqc

pathParams := mux.Vars(r)
_ = pathParams

queryParams := r.URL.Query()
_ = queryParams

ASumStr := pathParams["a"]
ASum, err := strconv.ParseInt(ASumStr, 10, 64)
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("Error while extracting ASum from path, pathParams: %v", pathParams))
}
req.A = ASum

if BSumStrArr, ok := queryParams["b"]; ok {
BSumStr := BSumStrArr[0]
BSum, err := strconv.ParseInt(BSumStr, 10, 64)
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("Error while extracting BSum from query, queryParams: %v", queryParams))
}
req.B = BSum
}

return &req, err
}

`
if got, want := strings.TrimSpace(str), strings.TrimSpace(desired); got != want {
t.Errorf("Generated code differs from result.\ngot = %s\nwant = %s", got, want)
t.Log(gentesthelper.DiffStrings(got, want))
}
}
1 change: 1 addition & 0 deletions gengokit/httptransport/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Binding struct {
Verb string
Fields []*Field
OneofFields []*OneofField
RequestRootField *Field
// A pointer back to the parent method of this binding. Used within some
// binding methods
Parent *Method
Expand Down
4 changes: 2 additions & 2 deletions svcdef/consolidate_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,11 @@ func paramLocation(field *Field, binding *svcparse.HTTPBinding) string {
if optField.Value == "*" {
return "body"
} else if optField.Value == field.Name {
return "body"
return "body_root"
// Have to CamelCase the fields from the protobuf file, as they may
// be lowercase while the name from the Go file will be CamelCased.
} else if gogen.CamelCase(strings.Split(optField.Value, ".")[0]) == field.Name {
return "body"
return "body_root"
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion svcdef/consolidate_http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ service Map {
Name string
Location string
}{
{"A", "body"},
{"A", "body_root"},
{"AA", "query"},
{"C", "query"},
{"MapField", "query"},
Expand Down