From f1c0bee525a9a5fb1d7acc12bc66a81a14a7858d Mon Sep 17 00:00:00 2001 From: Victor Chudnovsky Date: Fri, 24 Mar 2023 12:59:45 -0700 Subject: [PATCH] fix: Unconditionally quote string-encoded well-known types PopulateOneField is called only for path params and query params, which are both unquoted. --- util/genrest/goviewcreator.go | 2 +- util/genrest/resttools/populatefield.go | 70 +++++++++----------- util/genrest/resttools/populatefield_test.go | 49 +------------- 3 files changed, 32 insertions(+), 89 deletions(-) diff --git a/util/genrest/goviewcreator.go b/util/genrest/goviewcreator.go index 1347a4993..b782d6a30 100644 --- a/util/genrest/goviewcreator.go +++ b/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) diff --git a/util/genrest/resttools/populatefield.go b/util/genrest/resttools/populatefield.go index 603409d5e..23b90f5f4 100644 --- a/util/genrest/resttools/populatefield.go +++ b/util/genrest/resttools/populatefield.go @@ -24,43 +24,31 @@ import ( "google.golang.org/protobuf/reflect/protoreflect" ) -type valueTransform func(string) (string, error) - -func ensureQuotedValue(pre string) (string, error) { - hasStartQuote := strings.HasPrefix(pre, `"`) - hasEndQuote := strings.HasSuffix(pre, `"`) - if hasStartQuote != hasEndQuote { - return "", fmt.Errorf("unbalanced quotes in value %q", pre) - } - if hasStartQuote { - return pre, nil - } - return fmt.Sprintf(`"%s"`, pre), nil -} - -var wellKnownTypes = map[string]valueTransform{ - // testing various types in https://go.dev/play/p/rNhsXY364qQ - +// 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": ensureQuotedValue, - "google.protobuf.Timestamp": ensureQuotedValue, - "google.protobuf.Duration": ensureQuotedValue, + "google.protobuf.FieldMask": true, + "google.protobuf.Timestamp": true, + "google.protobuf.Duration": true, // == End utilized types == - // TODO: When the following start being used int he Showcase API, add tests for their use as - // (unquoted) query params. These types are defined in + // 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": nil, - "google.protobuf.FloatValue": nil, - "google.protobuf.Int64Value": ensureQuotedValue, - "google.protobuf.UInt64Value": ensureQuotedValue, - "google.protobuf.Int32Value": nil, - "google.protobuf.UInt32Value": nil, - "google.protobuf.BoolValue": nil, - "google.protobuf.StringValue": ensureQuotedValue, - "google.protobuf.BytesValue": ensureQuotedValue, + "google.protobuf.DoubleValue": false, + "google.protobuf.FloatValue": false, + "google.protobuf.Int64Value": true, + "google.protobuf.UInt64Value": 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": nil, - "google.protobuf.ListValue": nil, + "google.protobuf.Value": false, + "google.protobuf.ListValue": false, } // PopulateSingularFields sets the fields within protoMessage to the values provided in @@ -153,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 { @@ -268,17 +262,13 @@ func parseWellKnownType(message protoreflect.Message, fieldDescriptor protorefle } fieldMsg := messageFieldTypes.Message(fieldDescriptor.Index()) fullName := string(fieldMsg.Descriptor().FullName()) - transform, isWellKnown := wellKnownTypes[fullName] + stringEncoded, isWellKnown := wellKnownTypes[fullName] if !isWellKnown { return nil, nil } - if transform != nil { - var err error - if value, err = transform(value); err != nil { - return nil, err - } - + if stringEncoded { + value = fmt.Sprintf(`"%s"`, value) } msgValue := fieldMsg.New() diff --git a/util/genrest/resttools/populatefield_test.go b/util/genrest/resttools/populatefield_test.go index a4abb1760..f7cbcd9eb 100644 --- a/util/genrest/resttools/populatefield_test.go +++ b/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,52 +30,6 @@ 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 - want proto.Message - }{ - { - "google.protobuf.FieldMask", - (&genprotopb.UpdateUserRequest{}).ProtoReflect(), - "update_mask", - &fieldmaskpb.FieldMask{Paths: []string{"foo", "bar", "baz"}}, - }, - { - "google.protobuf.Timestamp", - (&genprotopb.User{}).ProtoReflect(), - "create_time", - timestamppb.Now(), - }, - { - "google.protobuf.Duration", - (&genprotopb.Sequence_Response{}).ProtoReflect(), - "delay", - 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) - if err != nil { - t.Fatalf("parsing %q led to error %s", value, err) - } - if gotp == nil { - t.Fatalf("expected non-nil value from parsing: %s", value) - } - 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) - } - } -} - -func TestParseWellKnownTypeUnquoted(t *testing.T) { // TODO: Test the other well-known types as the Showcase API // starts incorporating them. for _, tst := range []struct { @@ -90,7 +43,7 @@ func TestParseWellKnownTypeUnquoted(t *testing.T) { "google.protobuf.FieldMask", (&genprotopb.UpdateUserRequest{}).ProtoReflect(), "update_mask", - `foo,bar,baz`, + "foo,bar,baz", &fieldmaskpb.FieldMask{Paths: []string{"foo", "bar", "baz"}}, },