gapic: REGAPIC fix ignored empty value required fields in query (#821)

A required, singular, primitive type field will always be added as a query param when configured as one, whether the default empty value of the primitive is used or not. 

Note: Also added some constants for proto field types to improve readability.

Fixes #819

internal/gengapic/BUILD.bazel

@@ -81,6 +81,7 @@ go_test(
         "@org_golang_google_protobuf//encoding/protojson",
         "@org_golang_google_protobuf//proto",
         "@org_golang_google_protobuf//runtime/protoiface",
+        "@org_golang_google_protobuf//types/descriptorpb",
         "@org_golang_google_protobuf//types/known/apipb",
         "@org_golang_google_protobuf//types/known/durationpb",
         "@org_golang_google_protobuf//types/known/wrapperspb",

internal/gengapic/gengapic.go

@@ -39,6 +39,11 @@ const (
 	alpha               = "alpha"
 	beta                = "beta"
 	disableDeadlinesVar = "GOOGLE_API_GO_EXPERIMENTAL_DISABLE_DEFAULT_DEADLINE"
+	fieldTypeBool       = descriptor.FieldDescriptorProto_TYPE_BOOL
+	fieldTypeString     = descriptor.FieldDescriptorProto_TYPE_STRING
+	fieldTypeBytes      = descriptor.FieldDescriptorProto_TYPE_BYTES
+	fieldTypeMessage    = descriptor.FieldDescriptorProto_TYPE_MESSAGE
+	fieldLabelRepeated  = descriptor.FieldDescriptorProto_LABEL_REPEATED
 )
 
 var headerParamRegexp = regexp.MustCompile(`{([_.a-z0-9]+)`)

internal/gengapic/genrest.go

@@ -307,7 +307,7 @@ func (g *generator) getLeafs(msg *descriptor.DescriptorProto, excludedFields ...
 func (g *generator) generateQueryString(m *descriptor.MethodDescriptorProto) {
 	p := g.printf
 	queryParams := g.queryParams(m)
-	if queryParams == nil || len(queryParams) == 0 {
+	if len(queryParams) == 0 {
 		return
 	}
 
@@ -322,8 +322,21 @@ func (g *generator) generateQueryString(m *descriptor.MethodDescriptorProto) {
 	p("params := url.Values{}")
 	for _, path := range fields {
 		field := queryParams[path]
+		required := isRequired(field)
 		accessor := fieldGetter(path)
-		if field.GetLabel() == descriptor.FieldDescriptorProto_LABEL_REPEATED {
+		singularPrimitive := field.GetType() != fieldTypeMessage &&
+			field.GetType() != fieldTypeBytes &&
+			field.GetLabel() != fieldLabelRepeated
+		paramAdd := fmt.Sprintf("params.Add(%q, fmt.Sprintf(%q, req%s))", lowerFirst(snakeToCamel(path)), "%v", accessor)
+
+		// Only required, singular, primitive field types should be added regardless.
+		if required && singularPrimitive {
+			// Use string format specifier here in order to allow %v to be a raw string.
+			p("%s", paramAdd)
+			continue
+		}
+
+		if field.GetLabel() == fieldLabelRepeated {
 			// It's a slice, so check for nil
 			p("if req%s != nil {", accessor)
 		} else if field.GetProto3Optional() {
@@ -337,17 +350,17 @@ func (g *generator) generateQueryString(m *descriptor.MethodDescriptorProto) {
 			// Default values are type specific
 			switch field.GetType() {
 			// Degenerate case, field should never be a message because that implies it's not a leaf.
-			case descriptor.FieldDescriptorProto_TYPE_MESSAGE, descriptor.FieldDescriptorProto_TYPE_BYTES:
+			case fieldTypeMessage, fieldTypeBytes:
 				p("if req%s != nil {", accessor)
-			case descriptor.FieldDescriptorProto_TYPE_STRING:
+			case fieldTypeString:
 				p(`if req%s != "" {`, accessor)
-			case descriptor.FieldDescriptorProto_TYPE_BOOL:
+			case fieldTypeBool:
 				p(`if req%s {`, accessor)
 			default: // Handles all numeric types including enums
 				p(`if req%s != 0 {`, accessor)
 			}
 		}
-		p("    params.Add(\"%s\", fmt.Sprintf(\"%%v\", req%s))", lowerFirst(snakeToCamel(path)), accessor)
+		p("    %s", paramAdd)
 		p("}")
 	}
 	p("")

internal/gengapic/genrest_test.go

@@ -410,8 +410,22 @@ func TestLeafFields(t *testing.T) {
 func TestGenRestMethod(t *testing.T) {
 	pkg := "google.cloud.foo.v1"
 
+	sizeOpts := &descriptor.FieldOptions{}
+	proto.SetExtension(sizeOpts, annotations.E_FieldBehavior, []annotations.FieldBehavior{annotations.FieldBehavior_REQUIRED})
+	sizeField := &descriptor.FieldDescriptorProto{
+		Name:    proto.String("size"),
+		Type:    descriptor.FieldDescriptorProto_TYPE_INT32.Enum(),
+		Options: sizeOpts,
+	}
+	otherField := &descriptor.FieldDescriptorProto{
+		Name:           proto.String("other"),
+		Type:           descriptor.FieldDescriptorProto_TYPE_STRING.Enum(),
+		Proto3Optional: proto.Bool(true),
+	}
+
 	foo := &descriptor.DescriptorProto{
-		Name: proto.String("Foo"),
+		Name:  proto.String("Foo"),
+		Field: []*descriptor.FieldDescriptorProto{sizeField, otherField},
 	}
 	foofqn := fmt.Sprintf(".%s.Foo", pkg)
 
@@ -460,7 +474,9 @@ func TestGenRestMethod(t *testing.T) {
 				s:   f,
 			},
 			ParentElement: map[pbinfo.ProtoType]pbinfo.ProtoType{
-				opRPC: s,
+				opRPC:      s,
+				sizeField:  foo,
+				otherField: foo,
 			},
 			Type: map[string]pbinfo.ProtoType{
 				opfqn:  op,

internal/gengapic/helpers.go

@@ -20,6 +20,8 @@ import (
 	"unicode/utf8"
 
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
+	"google.golang.org/genproto/googleapis/api/annotations"
+	"google.golang.org/protobuf/proto"
 )
 
 func lowerFirst(s string) string {
@@ -130,3 +132,21 @@ func containsTransport(t []transport, tr transport) bool {
 
 	return false
 }
+
+// isRequired returns if a field is annotated as REQUIRED or not.
+func isRequired(field *descriptor.FieldDescriptorProto) bool {
+	if field.GetOptions() == nil {
+		return false
+	}
+
+	eBehav := proto.GetExtension(field.GetOptions(), annotations.E_FieldBehavior)
+
+	behaviors := eBehav.([]annotations.FieldBehavior)
+	for _, b := range behaviors {
+		if b == annotations.FieldBehavior_REQUIRED {
+			return true
+		}
+	}
+
+	return false
+}

internal/gengapic/helpers_test.go

@@ -19,7 +19,9 @@ import (
 
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/google/go-cmp/cmp"
+	"google.golang.org/genproto/googleapis/api/annotations"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/descriptorpb"
 )
 
 func TestCamelToSnake(t *testing.T) {
@@ -162,3 +164,31 @@ func TestHasMethod(t *testing.T) {
 		}
 	}
 }
+
+func TestIsRequired(t *testing.T) {
+	req := &descriptorpb.FieldOptions{}
+	proto.SetExtension(req, annotations.E_FieldBehavior, []annotations.FieldBehavior{annotations.FieldBehavior_REQUIRED})
+
+	notReq := &descriptorpb.FieldOptions{}
+	proto.SetExtension(notReq, annotations.E_FieldBehavior, []annotations.FieldBehavior{annotations.FieldBehavior_INPUT_ONLY})
+
+	for _, tst := range []struct {
+		opts *descriptor.FieldOptions
+		want bool
+	}{
+		{
+			opts: req,
+			want: true,
+		},
+		{
+			opts: notReq,
+		},
+		{
+			opts: nil,
+		},
+	} {
+		if got := isRequired(&descriptor.FieldDescriptorProto{Options: tst.opts}); got != tst.want {
+			t.Errorf("isRequired(%q) = got %v, want %v", tst.opts, got, tst.want)
+		}
+	}
+}

internal/gengapic/testdata/rest_CustomOp.want

@@ -2,6 +2,14 @@ func (c *fooRESTClient) CustomOp(ctx context.Context, req *foopb.Foo, opts ...ga
 	baseUrl, _ := url.Parse(c.endpoint)
 	baseUrl.Path += fmt.Sprintf("/v1/foo")
 
+	params := url.Values{}
+	if req != nil && req.Other != nil {
+		params.Add("other", fmt.Sprintf("%v", req.GetOther()))
+	}
+	params.Add("size", fmt.Sprintf("%v", req.GetSize()))
+
+	baseUrl.RawQuery = params.Encode()
+
 
 	httpReq, err := http.NewRequest("POST", baseUrl.String(), nil)
 	if err != nil {