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

Update clients/endpoints to use thriftrw Arg structs #92

Merged
merged 14 commits into from
May 10, 2017
Merged
48 changes: 10 additions & 38 deletions codegen/method.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import (
"strconv"
"strings"

"fmt"

"github.com/pkg/errors"
"go.uber.org/thriftrw/compile"
)
Expand Down Expand Up @@ -64,8 +62,7 @@ type MethodSpec struct {
ExceptionsIndex map[string]ExceptionSpec
ValidStatusCodes []int
// Additional struct generated from the bundle of request args.
RequestBoxed bool
RequestStruct []StructSpec
RequestBoxed bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we don't support RequestBoxed=false case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we kind of support it. We want to get rid of it, but that's a different PR.

// Thrift service name the method belongs to.
ThriftService string
// The thriftrw-generated go package name
Expand Down Expand Up @@ -196,10 +193,17 @@ func (ms *MethodSpec) setRequestType(curThriftFile string, funcSpec *compile.Fun
var err error
if isRequestBoxed(funcSpec) {
ms.RequestBoxed = true
ms.RequestType, err = packageHelper.TypeFullName(curThriftFile, funcSpec.ArgsSpec[0].Type)
ms.RequestType, err = packageHelper.TypeFullName(
curThriftFile, funcSpec.ArgsSpec[0].Type,
)
} else {
ms.RequestBoxed = false
ms.RequestType, err = ms.newRequestType(curThriftFile, funcSpec, packageHelper)

goPackageName, err := packageHelper.TypePackageName(curThriftFile)
if err == nil {
ms.RequestType = goPackageName + "." +
ms.ThriftService + "_" + strings.Title(ms.Name) + "_Args"
}
}
if err != nil {
return errors.Wrap(err, "failed to set request type")
Expand All @@ -213,38 +217,6 @@ func isStructType(spec compile.TypeSpec) bool {
return isStruct
}

func (ms *MethodSpec) newRequestType(curThriftFile string, f *compile.FunctionSpec, h *PackageHelper) (string, error) {
var requestType string
// TODO: (lu) the assumption here is 'no annotation == tchannel', good enough until new protocol is introduced
if ms.WantAnnot {
requestType = strings.Title(f.Name) + "HTTPRequest"
} else {
// This is specifically generating the "Args" type that thriftrw generates.
requestType = fmt.Sprintf(
"%s.%s_%s_Args",
ms.GenCodePkgName, strings.Title(ms.ThriftService), strings.Title(f.Name),
)

}

ms.RequestStruct = make([]StructSpec, len(f.ArgsSpec))
for i, arg := range f.ArgsSpec {
typeName, err := h.TypeFullName(curThriftFile, arg.Type)
if err != nil {
return "", errors.Wrap(err, "failed to generate new request type")
}
if isStructType(arg.Type) {
typeName = "*" + typeName
}
ms.RequestStruct[i] = StructSpec{
Type: typeName,
Name: arg.Name,
Annotations: arg.Annotations,
}
}
return requestType, nil
}

func (ms *MethodSpec) setResponseType(curThriftFile string, respSpec *compile.ResultSpec, packageHelper *PackageHelper) error {
if respSpec == nil {
ms.ResponseType = ""
Expand Down
5 changes: 3 additions & 2 deletions codegen/runner/pre-steps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ echo "Compiled thriftrw : +$runtime"
echo "Generating Go code from Thrift files"
mkdir -p "$BUILD_DIR/gen-code"
for tfile in $(find "$CONFIG_DIR/idl" -name '*.thrift'); do
"$THRIFTRW_BINARY" --out="$BUILD_DIR/gen-code" \
"$THRIFTRW_BINARY" --out="$BUILD_DIR/gen-code" \
--no-embed-idl \
--thrift-root="$CONFIG_DIR/idl" "$tfile"
--thrift-root="$CONFIG_DIR/idl" "$tfile"
done
gofmt -w "$BUILD_DIR/gen-code/"

end=`date +%s`
runtime=$((end-start))
Expand Down
22 changes: 3 additions & 19 deletions codegen/template_bundle/template_files.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion codegen/templates/endpoint.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ func (w {{$workflow}}) Handle(
}

{{if and (ne .RequestType "") (ne $clientReqType "") -}}
func convertTo{{title .Name}}ClientRequest(body *{{title .RequestType}}) *{{$clientReqType}} {
func convertTo{{title .Name}}ClientRequest(body *{{.RequestType}}) *{{$clientReqType}} {
clientRequest := &{{$clientReqType}}{}

{{ range $key, $value := .RequestFieldMap -}}
Expand Down
16 changes: 0 additions & 16 deletions codegen/templates/structs.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,3 @@ func getDirName() string {
_, file, _, _ := runtime.Caller(0)
return zanzibar.GetDirnameFromRuntimeCaller(file)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this template file? seems we can delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this function getDirName() will double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok we only use this for endpoint tests, ill get rid of it for clients :)


{{range .Services}}

{{/* ========================= Method ========================= */ -}}

{{range .Methods}}
{{if len .RequestStruct | ne 0}} {{- /* generate struct to wrap request args*/ -}}
// {{.RequestType}} is the http body type for endpoint {{.Name}}.
type {{.RequestType}} struct {
{{range .RequestStruct -}}
{{title .Name}} {{.Type}} `json:"{{.Name}}"`
{{end -}}
}{{end -}}

{{end}} {{- /* <range .Methods> */ -}}
{{end}} {{- /* <range .Services> */ -}}
34 changes: 3 additions & 31 deletions codegen/test_data/bar.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
],
"ReqHeaders": null,
"ResHeaders": null,
"RequestType": "ArgNotStructHTTPRequest",
"RequestType": "endpointsBarBar.Bar_ArgNotStruct_Args",
"ResponseType": "",
"OKStatusCode": {
"Code": 200,
Expand Down Expand Up @@ -76,13 +76,6 @@
403
],
"RequestBoxed": false,
"RequestStruct": [
{
"Type": "string",
"Name": "request",
"Annotations": null
}
],
"ThriftService": "Bar",
"GenCodePkgName": "endpointsBarBar",
"WantAnnot": true,
Expand Down Expand Up @@ -149,7 +142,6 @@
403
],
"RequestBoxed": false,
"RequestStruct": null,
"ThriftService": "Bar",
"GenCodePkgName": "endpointsBarBar",
"WantAnnot": true,
Expand Down Expand Up @@ -216,7 +208,6 @@
403
],
"RequestBoxed": false,
"RequestStruct": null,
"ThriftService": "Bar",
"GenCodePkgName": "endpointsBarBar",
"WantAnnot": true,
Expand Down Expand Up @@ -246,7 +237,7 @@
],
"ReqHeaders": null,
"ResHeaders": null,
"RequestType": "NormalHTTPRequest",
"RequestType": "endpointsBarBar.Bar_Normal_Args",
"ResponseType": "endpointsBarBar.BarResponse",
"OKStatusCode": {
"Code": 200,
Expand Down Expand Up @@ -283,13 +274,6 @@
403
],
"RequestBoxed": false,
"RequestStruct": [
{
"Type": "*endpointsBarBar.BarRequest",
"Name": "request",
"Annotations": null
}
],
"ThriftService": "Bar",
"GenCodePkgName": "endpointsBarBar",
"WantAnnot": true,
Expand Down Expand Up @@ -325,7 +309,7 @@
"x-uuid",
"x-token"
],
"RequestType": "TooManyArgsHTTPRequest",
"RequestType": "endpointsBarBar.Bar_TooManyArgs_Args",
"ResponseType": "endpointsBarBar.BarResponse",
"OKStatusCode": {
"Code": 200,
Expand Down Expand Up @@ -362,18 +346,6 @@
403
],
"RequestBoxed": false,
"RequestStruct": [
{
"Type": "*endpointsBarBar.BarRequest",
"Name": "request",
"Annotations": null
},
{
"Type": "*endpointsFooFoo.FooStruct",
"Name": "foo",
"Annotations": null
}
],
"ThriftService": "Bar",
"GenCodePkgName": "endpointsBarBar",
"WantAnnot": true,
Expand Down
6 changes: 3 additions & 3 deletions codegen/test_data/clients/bar.gogen
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func NewClient(
func (c *BarClient) ArgNotStruct(
ctx context.Context,
headers map[string]string,
r *ArgNotStructHTTPRequest,
r *clientsBarBar.Bar_ArgNotStruct_Args,
) (map[string]string, error) {

req := zanzibar.NewClientHTTPRequest(
Expand Down Expand Up @@ -237,7 +237,7 @@ func (c *BarClient) NoRequest(
func (c *BarClient) Normal(
ctx context.Context,
headers map[string]string,
r *NormalHTTPRequest,
r *clientsBarBar.Bar_Normal_Args,
) (*clientsBarBar.BarResponse, map[string]string, error) {

req := zanzibar.NewClientHTTPRequest(
Expand Down Expand Up @@ -298,7 +298,7 @@ func (c *BarClient) Normal(
func (c *BarClient) TooManyArgs(
ctx context.Context,
headers map[string]string,
r *TooManyArgsHTTPRequest,
r *clientsBarBar.Bar_TooManyArgs_Args,
) (*clientsBarBar.BarResponse, map[string]string, error) {

req := zanzibar.NewClientHTTPRequest(
Expand Down
18 changes: 0 additions & 18 deletions codegen/test_data/clients/bar_structs.gogen
Original file line number Diff line number Diff line change
Expand Up @@ -26,28 +26,10 @@ package barClient
import (
"runtime"

clientsBarBar "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/clients/bar/bar"
clientsFooFoo "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/clients/foo/foo"
"github.com/uber/zanzibar/runtime"
)

func getDirName() string {
_, file, _, _ := runtime.Caller(0)
return zanzibar.GetDirnameFromRuntimeCaller(file)
}

// ArgNotStructHTTPRequest is the http body type for endpoint argNotStruct.
type ArgNotStructHTTPRequest struct {
Request string `json:"request"`
}

// NormalHTTPRequest is the http body type for endpoint normal.
type NormalHTTPRequest struct {
Request *clientsBarBar.BarRequest `json:"request"`
}

// TooManyArgsHTTPRequest is the http body type for endpoint tooManyArgs.
type TooManyArgsHTTPRequest struct {
Request *clientsBarBar.BarRequest `json:"request"`
Foo *clientsFooFoo.FooStruct `json:"foo"`
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestArgNotStructSuccessfulRequestOKResponse(t *testing.T) {
"POST",
"/bar/arg-not-struct-path",
headers,
bytes.NewReader([]byte(`{}`)),
bytes.NewReader([]byte(`{"request":"foo"}`)),
)
if !assert.NoError(t, err, "got http error") {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestNormalSuccessfulRequestOKResponse(t *testing.T) {
"POST",
"/bar/bar-path",
headers,
bytes.NewReader([]byte(`{}`)),
bytes.NewReader([]byte(`{"request":{"boolField":true,"stringField":"foo"}}`)),
)
if !assert.NoError(t, err, "got http error") {
return
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestTooManyArgsSuccessfulRequestOKResponse(t *testing.T) {
"POST",
"/bar/too-many-args-path",
headers,
bytes.NewReader([]byte(`{}`)),
bytes.NewReader([]byte(`{"request":{"boolField":true,"stringField":"foo"}}`)),
)
if !assert.NoError(t, err, "got http error") {
return
Expand Down
9 changes: 4 additions & 5 deletions codegen/test_data/endpoints/bar_bar_method_argnotstruct.gogen
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
zanzibar "github.com/uber/zanzibar/runtime"
"go.uber.org/zap"

"github.com/uber/zanzibar/.tmp_gen/clients/bar"
clientsBarBar "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/clients/bar/bar"
endpointsBarBar "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/endpoints/bar/bar"
)
Expand All @@ -55,7 +54,7 @@ func (handler *ArgNotStructHandler) HandleRequest(
req *zanzibar.ServerHTTPRequest,
res *zanzibar.ServerHTTPResponse,
) {
var requestBody ArgNotStructHTTPRequest
var requestBody endpointsBarBar.Bar_ArgNotStruct_Args
if ok := req.ReadAndUnmarshalBody(&requestBody); !ok {
return
}
Expand Down Expand Up @@ -99,7 +98,7 @@ type ArgNotStructEndpoint struct {
func (w ArgNotStructEndpoint) Handle(
ctx context.Context,
reqHeaders zanzibar.Header,
r *ArgNotStructHTTPRequest,
r *endpointsBarBar.Bar_ArgNotStruct_Args,
) (zanzibar.Header, error) {
clientRequest := convertToArgNotStructClientRequest(r)

Expand Down Expand Up @@ -139,8 +138,8 @@ func (w ArgNotStructEndpoint) Handle(
return resHeaders, nil
}

func convertToArgNotStructClientRequest(body *ArgNotStructHTTPRequest) *barClient.ArgNotStructHTTPRequest {
clientRequest := &barClient.ArgNotStructHTTPRequest{}
func convertToArgNotStructClientRequest(body *endpointsBarBar.Bar_ArgNotStruct_Args) *clientsBarBar.Bar_ArgNotStruct_Args {
clientRequest := &clientsBarBar.Bar_ArgNotStruct_Args{}

clientRequest.Request = string(body.Request)

Expand Down
Loading