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

proto gen: support optionals for proto syntax 3 #105

Merged
merged 2 commits into from
Mar 23, 2022
Merged
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ _testmain.go
!/integration/additional_bindings/strings/strings.to_upper.go
!/integration/binding_with_body_and_response/strings/strings.go
!/integration/binding_with_body_and_response/strings/strings.to_upper.go
!/integration/binding_with_optional_field/strings/strings.to_upper.go
!/integration/binding_with_repeated_field/strings/strings.to_upper.go
!/integration/client_cancel_request/app/strings/strings.to_upper.go
!/integration/impl_file_name_tmpl/strings/strings.to_upper.go
Expand Down
21 changes: 17 additions & 4 deletions cmd/protoc-gen-goclay/genhandler/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,17 +203,30 @@ func addValueTyped(f *descriptor.Field) string {
if valueFormatter != "" {
return fmt.Sprintf(valueFormatter, getter)
}

if f.GetProto3Optional() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to have both value formatter and optional for the same field?
I guess not, but could you please check 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.

We can have both -- currently, the valueformatter is used for encoding bytes. Bytes evaluate to the byte[] scalar type on go which is nullable. So the current implementation works.

The question is do we want to create pointers for bytes[] on code generation instead? And if yes, why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. I don't think we need a pointer to a slice

getter = fmt.Sprintf("*%s", getter)
}

return fmt.Sprintf(`fmt.Sprintf("%s", %s)`, valueVerb, getter)
}

if !isRepeated {
if isRepeated {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not possible to have both optional and repeated for the same field, right?
It does not make much sense to me, but is it possible from the protobuf perspective?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To the best of my knowledge, repeated fields are inherently optional. If we try to put both optional and repeated on the same field, the parser will raise an error (did a quick experiment btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, an answer on Stack Overflow -- https://stackoverflow.com/a/25637833

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking it

format := `for _, v := range in.%s {
values.Add(%q, %s)
}`
return fmt.Sprintf(format, goName, f.GetName(), valueTemplater("v"))
}

if !f.GetProto3Optional() {
return fmt.Sprintf(`values.Add(%q, %s)`, f.GetName(), valueTemplater("in."+goName))
}

format := `for _, v := range in.%s {
values.Add(%q, %s)
format := `if in.%s != nil {
values.Add(%q, %s)
}`
return fmt.Sprintf(format, goName, f.GetName(), valueTemplater("v"))
return fmt.Sprintf(format, goName, f.GetName(), valueTemplater("in."+goName))

}

var (
Expand Down
5 changes: 4 additions & 1 deletion cmd/protoc-gen-goclay/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"flag"
"fmt"
"github.com/utrack/clay/v3/cmd/protoc-gen-goclay/third-party/grpc-gateway/internals/codegenerator"
"io"
"io/ioutil"
"os"
Expand Down Expand Up @@ -192,7 +193,9 @@ func parseReqParam(param string, f *flag.FlagSet, pkgMap map[string]string) erro
}

func emitFiles(w io.Writer, out []*plugin.CodeGeneratorResponse_File) {
emitResp(w, &plugin.CodeGeneratorResponse{File: out})
responseFile := &plugin.CodeGeneratorResponse{File: out}
codegenerator.SetSupportedFeaturesOnCodeGeneratorResponse(responseFile)
emitResp(w, responseFile)
}

func emitError(err error) {
Expand Down
23 changes: 23 additions & 0 deletions integration/binding_with_optional_field/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
include ../env.mk

$(shell go get github.com/googleapis/googleapis)
GOOGLEAPIS_PKG:=$(shell go list -m all | grep github.com/googleapis/googleapis | awk '{print ($$4 != "" ? $$4 : $$1)}')
GOOGLEAPIS_VERSION:=$(shell go list -m all | grep github.com/googleapis/googleapis | awk '{print ($$5 != "" ? $$5 : $$2)}')
GOOGLEAPIS_PATH:=${FIRST_GOPATH}/pkg/mod/${GOOGLEAPIS_PKG}@${GOOGLEAPIS_VERSION}

pwd:
@pwd

clean:
rm -f ./pb/strings.pb.go
rm -f ./pb/strings_grpc.pb.go
rm -f ./pb/strings.pb.goclay.go
rm -f ./strings/strings.go
rm -f main

protoc: .protoc_pb

build: .build

test: pwd clean protoc build
go test -v ./...
21 changes: 21 additions & 0 deletions integration/binding_with_optional_field/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package main

import (
"net/http"

"github.com/go-chi/chi"
"github.com/utrack/clay/integration/binding_with_optional_field/strings"
)

func main() {
r := chi.NewMux()
desc := strings.NewStrings().GetDescription()
desc.RegisterHTTP(r)

r.Handle("/swagger.json", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Content-Type", "application/javascript")
w.Write(desc.SwaggerDef())
}))

http.ListenAndServe(":8080", r)
}
69 changes: 69 additions & 0 deletions integration/binding_with_optional_field/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package main

import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"

strings_pb "github.com/utrack/clay/integration/binding_with_optional_field/pb"
strings_srv "github.com/utrack/clay/integration/binding_with_optional_field/strings"
)

func TestToUpper(t *testing.T) {
ts := testServer()
defer ts.Close()

t.Run("GET nullable string in request and an object in response", func(t *testing.T) {
httpClient := ts.Client()
client := strings_pb.NewStringsHTTPClient(httpClient, ts.URL)

strVal := "foo"
req := &strings_pb.String{
Str: &strVal,
}

resp, err := client.ToUpper(context.Background(), req)
if err != nil {
t.Fatalf("expected err <nil>, got: %q", err)
}

got := resp.GetStr()
expected := strings.ToUpper(req.GetStr())
if got != expected {
t.Fatalf("expected %q, got: %q", expected, got)
}
})

t.Run("GET nil in request and nil in response", func(t *testing.T) {
httpClient := ts.Client()
client := strings_pb.NewStringsHTTPClient(httpClient, ts.URL)

req := &strings_pb.String{
Str: nil,
}
resp, err := client.ToUpper(context.Background(), req)
if err != nil {
t.Fatalf("expected err <nil>, got: %q", err)
}

got := resp.Str
if got != nil {
t.Fatalf("expected nil, got: %s", *got)
}

})
}

func testServer() *httptest.Server {
mux := http.NewServeMux()
desc := strings_srv.NewStrings().GetDescription()
desc.RegisterHTTP(mux)
mux.Handle("/swagger.json", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
w.Header().Set("Content-Type", "application/javascript")
w.Write(desc.SwaggerDef())
}))
ts := httptest.NewServer(mux)
return ts
}
1 change: 1 addition & 0 deletions integration/binding_with_optional_field/pb/keep.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package strings
19 changes: 19 additions & 0 deletions integration/binding_with_optional_field/pb/strings.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
syntax = "proto3";

option go_package = "github.com/utrack/clay/integration/binding_with_optional_field/pb;strings";
// or just
//option go_package = "./pb;strings";

import "google/api/annotations.proto";

service Strings {
rpc ToUpper (String) returns (String) {
option (google.api.http) = {
get: "/strings/to_upper/v2"
};
}
}

message String {
optional string str = 1;
}

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

2 changes: 1 addition & 1 deletion integration/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ require (
github.com/go-openapi/spec v0.0.0-20180415031709-bcff419492ee
github.com/gogo/protobuf v1.3.2
github.com/google/go-cmp v0.5.6
github.com/googleapis/googleapis v0.0.0-20210901013455-1c01db6a49cc // indirect
github.com/googleapis/googleapis v0.0.0-20220316214218-db9d2a3c5e2f // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.5.0
github.com/jmoiron/jsonq v0.0.0-20150511023944-e874b168d07e
github.com/pkg/errors v0.8.1
Expand Down
2 changes: 2 additions & 0 deletions integration/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ github.com/googleapis/googleapis v0.0.0-20210731092108-41521e288c8c h1:V3racn+UX
github.com/googleapis/googleapis v0.0.0-20210731092108-41521e288c8c/go.mod h1:XrPm4xpez/lHHyE+8/G+NqQRcB4lg42HF9zQVTvxtXw=
github.com/googleapis/googleapis v0.0.0-20210901013455-1c01db6a49cc h1:mFVKt7Ur5wJKxLvYc/jZJNaLiOpVkSGuUl9os9Rzp+Q=
github.com/googleapis/googleapis v0.0.0-20210901013455-1c01db6a49cc/go.mod h1:XrPm4xpez/lHHyE+8/G+NqQRcB4lg42HF9zQVTvxtXw=
github.com/googleapis/googleapis v0.0.0-20220316214218-db9d2a3c5e2f h1:hUbHv6a/LHanMIeCfaDYQ/jBhSkdkjfFxiW8QotjQEY=
github.com/googleapis/googleapis v0.0.0-20220316214218-db9d2a3c5e2f/go.mod h1:XrPm4xpez/lHHyE+8/G+NqQRcB4lg42HF9zQVTvxtXw=
github.com/grpc-ecosystem/go-grpc-middleware v1.1.0 h1:THDBEeQ9xZ8JEaCLyLQqXMMdRqNr0QAUJTIkQAUtFjg=
github.com/grpc-ecosystem/go-grpc-middleware v1.1.0/go.mod h1:f5nM7jw/oeRSadq3xCzHAvxcr8HZnzsqU6ILg/0NiiE=
github.com/grpc-ecosystem/grpc-gateway/v2 v2.5.0 h1:ajue7SzQMywqRjg2fK7dcpc0QhFGpTR2plWfV4EZWR4=
Expand Down