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.

Sign up for GitHub

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!: use resource field in http body for Updates #1198

Merged
merged 3 commits into from
Sep 1, 2022
Merged

fix!: use resource field in http body for Updates #1198

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21b2894
fix!: use resource field in http body for Updates
noahdietz Aug 31, 2022
51f5e66
Merge branch 'main' into regapic-wkt
noahdietz Sep 1, 2022
40b1151
address feedback
noahdietz Sep 1, 2022
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 schema/google/showcase/v1beta1/identity.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ service Identity {
rpc UpdateUser(UpdateUserRequest) returns (User) {
option (google.api.http) = {
patch: "/v1beta1/{user.name=users/*}"
body: "*"
body: "user"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have * so that the user can specify which fields to PATCH? It seems like this is one of the prime uses cases for field masks.

Copy link
Collaborator Author

@noahdietz noahdietz Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what you mean. The update_mask will end up in the query params. We don't lose any functionality here, just fixing a non-compliant binding (https://google.aip.dev/134) so that we can test a real use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. "There must be a body key in the google.api.http annotation, and it must map to the resource field in the request message. All remaining fields should map to URI query parameters."

OK, my bad. LGTM!

};
}

Expand Down
6 changes: 3 additions & 3 deletions schema/google/showcase/v1beta1/messaging.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ service Messaging {
rpc UpdateRoom(UpdateRoomRequest) returns (Room) {
option (google.api.http) = {
patch: "/v1beta1/{room.name=rooms/*}"
body: "*"
body: "room"
};
}

Expand Down Expand Up @@ -109,10 +109,10 @@ service Messaging {
rpc UpdateBlurb(UpdateBlurbRequest) returns (Blurb) {
option (google.api.http) = {
patch: "/v1beta1/{blurb.name=rooms/*/blurbs/*}"
body: "*"
body: "blurb"
additional_bindings: {
patch: "/v1beta1/{blurb.name=users/*/profile/blurbs/*}"
body: "*"
body: "blurb"
}
};
}
Expand Down
2 changes: 1 addition & 1 deletion util/genrest/gomodelcreator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func NewGoModel(protoModel *protomodel.Model) (*gomodel.Model, error) {
if bodyFieldDesc == nil {
goModel.AccumulateError(fmt.Errorf("could not find body field %q in %q", binding.BodyField, inProtoType.GetName()))
}
bodyFieldTypeDesc, ok := protoInfo.Type[*bodyFieldDesc.TypeName]
bodyFieldTypeDesc, ok := protoInfo.Type[bodyFieldDesc.GetTypeName()]
if !ok {
goModel.AccumulateError(fmt.Errorf("could not read protoInfo[%q]", inProtoType))
}
Expand Down
50 changes: 48 additions & 2 deletions util/genrest/resttools/populatefield.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,31 @@ import (
"strconv"
"strings"

"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
)

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,
"google.protobuf.Int64Value": true,
"google.protobuf.UInt64Value": true,
"google.protobuf.Int32Value": true,
"google.protobuf.UInt32Value": true,
"google.protobuf.BoolValue": true,
"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,
}

// PopulateSingularFields sets the fields within protoMessage to the values provided in
// fieldValues. The fields and values are provided as a map of field paths to the string
// representation of their values. The field paths can refer to fields nested arbitrarily deep
Expand Down Expand Up @@ -113,8 +134,14 @@ func PopulateOneField(protoMessage proto.Message, fieldPath string, fieldValues
kind := fieldDescriptor.Kind()
switch kind {
case protoreflect.MessageKind:
parseError = fmt.Errorf("terminal field %q of field path %q in message %q is a message type",
fieldName, fieldPath, messageFullName)
if pval, err := parseWellKnownType(message, fieldDescriptor, value); err != nil {
parseError = err
} else if pval != nil {
protoValue = *pval
} else {
parseError = fmt.Errorf("terminal field %q of field path %q in message %q is a message type",
fieldName, fieldPath, messageFullName)
}

// reference for proto scalar types:
// https://developers.google.com/protocol-buffers/docs/proto3#scalar
Expand Down Expand Up @@ -214,3 +241,22 @@ func findProtoFieldByJSONName(fieldName string, fields protoreflect.FieldDescrip
}
return result, nil
}

func parseWellKnownType(message protoreflect.Message, fieldDescriptor protoreflect.FieldDescriptor, value string) (*protoreflect.Value, error) {
messageFieldTypes := message.Type().(protoreflect.MessageFieldTypes)
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
fieldMsg := messageFieldTypes.Message(fieldDescriptor.Index())
fullName := string(fieldMsg.Descriptor().FullName())
if !wellKnownTypes[fullName] {
return nil, nil
}

msgValue := fieldMsg.New()
err := protojson.Unmarshal([]byte(value), msgValue.Interface())
if err != nil {
return nil, fmt.Errorf("unable to unmarshal field %q in message %q: %w",
string(fieldDescriptor.Name()), string(message.Descriptor().FullName()), err)
}

v := protoreflect.ValueOfMessage(msgValue)
return &v, nil
}
52 changes: 52 additions & 0 deletions util/genrest/resttools/populatefield_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,63 @@ package resttools
import (
"reflect"
"testing"
"time"

"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"
"google.golang.org/protobuf/types/known/durationpb"
"google.golang.org/protobuf/types/known/fieldmaskpb"
"google.golang.org/protobuf/types/known/timestamppb"
)

func TestParseWellKnownType(t *testing.T) {
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.Fatal(err)
}
if gotp == nil {
t.Fatal("expected non-nil value from parsing")
}
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 TestPopulateOneFieldError(t *testing.T) {
for idx, testCase := range []struct {
field string
Expand Down