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(rest): properly handle string-encoded well-known types in URLs #1282

Merged
merged 4 commits into from
Mar 27, 2023
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
2 changes: 1 addition & 1 deletion util/genrest/goviewcreator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion util/genrest/resttools/headers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 26 additions & 8 deletions util/genrest/resttools/populatefield.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
24 changes: 16 additions & 8 deletions util/genrest/resttools/populatefield_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
}
}
Expand Down