fix(rest): properly handle string-encoded well-known types in URLs (#…

…1282)

util/genrest/goviewcreator.go

@@ -75,7 +75,7 @@ func NewView(model *gomodel.Model) (*goview.View, error) {
 
 			source.P("")
 			source.P("// %s translates REST requests/responses on the wire to internal proto messages for %s", handlerName, handler.GoMethod)
-			source.P("//    Generated for HTTP binding pattern: %q", handler.URIPattern)
+			source.P("//    Generated for HTTP binding pattern: %s %q", handler.HTTPMethod, handler.URIPattern)
 			source.P("func (backend *RESTBackend) %s(w http.ResponseWriter, r *http.Request) {", handlerName)
 			if handler.StreamingClient {
 				source.P(`  backend.Error(w, http.StatusNotImplemented, "client-streaming methods not implemented yet (request matched '%s': %%q)", r.URL)`, handler.URIPattern)

util/genrest/resttools/headers.go

@@ -56,7 +56,8 @@ func CheckContentType(header http.Header) error {
 func CheckAPIClientHeader(header http.Header) error {
 	content, ok := header[headerNameAPIClient]
 	if !ok || len(content) != 1 {
-		return fmt.Errorf("(HeaderAPIClientError) did not find expected HTTP header %q: %q", headerNameAPIClient, headerValueTransportRESTPrefix)
+		return fmt.Errorf("(HeaderAPIClientError) did not find expected HTTP header %q: %q\n                found: %q",
+			headerNameAPIClient, headerValueTransportRESTPrefix, header)
 	}
 
 	var haveREST, haveGAPIC bool

util/genrest/resttools/populatefield.go

@@ -24,24 +24,31 @@ import (
 	"google.golang.org/protobuf/reflect/protoreflect"
 )
 
+// wellKnownTypes has a key for every well-known type message which JSON-encodes to an atomic symbol
+// (like a number or a string) as opposed to a structured object. The value for each key is true iff
+// the JSON encoding for the type is a string. We need both these data for well-known types so that
+// we can properly decode them in URL paths and query strings.
 var wellKnownTypes = map[string]bool{
 	// == The following are the only three common types used in this API ==
 	"google.protobuf.FieldMask": true,
 	"google.protobuf.Timestamp": true,
 	"google.protobuf.Duration":  true,
 	// == End utilized types ==
-	"google.protobuf.DoubleValue": true,
-	"google.protobuf.FloatValue":  true,
+	// TODO: When the following start being used in the Showcase API, add tests for each of
+	// these. These types are defined in
+	// /~https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/wrappers.proto
+	"google.protobuf.DoubleValue": false,
+	"google.protobuf.FloatValue":  false,
 	"google.protobuf.Int64Value":  true,
 	"google.protobuf.UInt64Value": true,
-	"google.protobuf.Int32Value":  true,
-	"google.protobuf.UInt32Value": true,
-	"google.protobuf.BoolValue":   true,
+	"google.protobuf.Int32Value":  false,
+	"google.protobuf.UInt32Value": false,
+	"google.protobuf.BoolValue":   false,
 	"google.protobuf.StringValue": true,
 	"google.protobuf.BytesValue":  true,
 	// TODO: Determine if the following are even viable as query params.
-	"google.protobuf.Value":     true,
-	"google.protobuf.ListValue": true,
+	"google.protobuf.Value":     false,
+	"google.protobuf.ListValue": false,
 }
 
 // PopulateSingularFields sets the fields within protoMessage to the values provided in
@@ -134,6 +141,12 @@ func PopulateOneField(protoMessage proto.Message, fieldPath string, fieldValues
 		kind := fieldDescriptor.Kind()
 		switch kind {
 		case protoreflect.MessageKind:
+			// The only `MessageKind`s we accept in URL paths and query params are
+			// well-known types where the message as a whole encodes into a format
+			// similar to a regular terminal field. Normal messages conveyed via URL
+			// paths or query params must be non-repeated and are represented by listing
+			// their terminal primitive fields, which will trigger the other cases
+			// below instead of this one.
 			if pval, err := parseWellKnownType(message, fieldDescriptor, value); err != nil {
 				parseError = err
 			} else if pval != nil {
@@ -249,10 +262,15 @@ func parseWellKnownType(message protoreflect.Message, fieldDescriptor protorefle
 	}
 	fieldMsg := messageFieldTypes.Message(fieldDescriptor.Index())
 	fullName := string(fieldMsg.Descriptor().FullName())
-	if !wellKnownTypes[fullName] {
+	stringEncoded, isWellKnown := wellKnownTypes[fullName]
+	if !isWellKnown {
 		return nil, nil
 	}
 
+	if stringEncoded {
+		value = fmt.Sprintf("%q", value)
+	}
+
 	msgValue := fieldMsg.New()
 	err := protojson.Unmarshal([]byte(value), msgValue.Interface())
 	if err != nil {

util/genrest/resttools/populatefield_test.go

@@ -21,7 +21,6 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	genprotopb "github.com/googleapis/gapic-showcase/server/genproto"
-	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/encoding/prototext"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
@@ -31,45 +30,54 @@ import (
 )
 
 func TestParseWellKnownType(t *testing.T) {
+	// TODO: Test the other well-known types as the Showcase API
+	// starts incorporating them.
 	for _, tst := range []struct {
 		name  string
 		msg   protoreflect.Message
 		field protoreflect.Name
+		value string
 		want  proto.Message
 	}{
 		{
 			"google.protobuf.FieldMask",
 			(&genprotopb.UpdateUserRequest{}).ProtoReflect(),
 			"update_mask",
+			"foo,bar,baz",
 			&fieldmaskpb.FieldMask{Paths: []string{"foo", "bar", "baz"}},
 		},
+
 		{
 			"google.protobuf.Timestamp",
 			(&genprotopb.User{}).ProtoReflect(),
 			"create_time",
-			timestamppb.Now(),
+			"2023-03-21T12:01:02.000000003Z",
+			timestamppb.New(time.Date(2023, 03, 21, 12, 1, 2, 3, time.UTC)),
 		},
+
 		{
 			"google.protobuf.Duration",
 			(&genprotopb.Sequence_Response{}).ProtoReflect(),
 			"delay",
+			"5s",
 			durationpb.New(5 * time.Second),
 		},
 	} {
-		data, _ := protojson.Marshal(tst.want)
-		value := string(data)
 		fd := tst.msg.Descriptor().Fields().ByName(tst.field)
 
-		gotp, err := parseWellKnownType(tst.msg, fd, value)
+		gotp, err := parseWellKnownType(tst.msg, fd, tst.value)
 		if err != nil {
-			t.Fatal(err)
+			t.Errorf("parsing %q led to error %s", tst.value, err)
+			continue
 		}
 		if gotp == nil {
-			t.Fatal("expected non-nil value from parsing")
+			t.Errorf("expected non-nil value from parsing: %s", tst.value)
+			continue
 		}
 		got := gotp.Message().Interface()
 		if diff := cmp.Diff(got, tst.want, cmp.Comparer(proto.Equal)); diff != "" {
-			t.Fatalf("%s: got(-),want(+):\n%s", "FieldMask", diff)
+			t.Errorf("%s: got(-),want(+):\n%s", "FieldMask", diff)
+			continue
 		}
 	}
 }