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

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

Closed
wants to merge 2 commits into from
Closed

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ac28a5
Update grpc conn.go, support google protobuf wrappers
zibul444 Jul 28, 2023
9aaf218
Update conn_test.go
zibul444 Aug 28, 2023
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
5 changes: 4 additions & 1 deletion lib/netext/grpcext/conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,11 @@ func (c *Conn) Invoke(
// {"x":6,"y":4,"z":0}
raw, _ := marshaler.Marshal(resp)
msg := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I've looked into this and believe that this line is an issue, switching to this should help

Suggested change
msg := make(map[string]interface{})
var msg interface{}

_ = json.Unmarshal(raw, &msg)
er := json.Unmarshal(raw, &msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we could somehow identify that raw is a wrapped 🤔

Or, more precisely, I'm concerned that in case of Unmarshaling error, we safely assume that we can use raw

Copy link
Author

Choose a reason for hiding this comment

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

unfortunately, I can't tell you exactly how this can be done, but I will accept any help. I will assume that it is possible, but if you complicate the checks, it can greatly affect performance - this is also bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this fix ... strange at best and mostly hacky at this point.

I can't imagine that this is how it is supposed to work and also this will practically return you broken responses in case of real errors.

Additionally the lack of unit test for this means that I can't even easily check how this change at this point.

Can we please at least get a unit test for this.

@olegbespalov I guess you are trying to figure out how this is supposed to work actually ?

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you should be able to assign to err without := and without getting a complain about shadowing

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what you're talking about . Changing the variable name solves the shading problem. The err variable already exists. What exactly did I do wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should just assign this to err instead of addng a new variable.

I don't se ay reason not to.

response.Message = msg
if er != nil {
response.Message = string(raw)
}
}
return &response, nil
}
Expand Down
77 changes: 62 additions & 15 deletions lib/netext/grpcext/conn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,70 @@ import (
)

func TestInvoke(t *testing.T) {
t.Parallel()
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the https://github.com/grafana/k6/blob/master/js/modules/k6/grpc/client_test.go is the better test for testing this, and we might need to introduce one more test service, which could contain a different method to test the wrappers.

tests := []struct {
name string
ctx context.Context
md metadata.MD
url string
req Request
resp Response
invokemock func(in, out *dynamicpb.Message, _ ...grpc.CallOption) error
}{
{
name: "hello",
ctx: ctx,
md: metadata.New(nil),
url: "/hello.HelloService/SayHello",
req: Request{
Message: []byte(`{"greeting":"text request"}`),
MethodDescriptor: methodFromProto("SayHello"),
},
invokemock: func(in, out *dynamicpb.Message, _ ...grpc.CallOption) error {
err := protojson.Unmarshal([]byte(`{"reply":"text reply"}`), out)
require.NoError(t, err)

helloReply := func(in, out *dynamicpb.Message, _ ...grpc.CallOption) error {
err := protojson.Unmarshal([]byte(`{"reply":"text reply"}`), out)
require.NoError(t, err)
return nil
},
resp: Response{
Message: map[string]interface{}{"reply": "text reply"},
},
},
{
name: "primitive",
ctx: ctx,
md: metadata.New(nil),
url: "/hello.HelloService/SayHello",
req: Request{
Message: []byte(`text request`),
MethodDescriptor: methodFromProto("SayPrimitive"),
},
invokemock: func(in, out *dynamicpb.Message, _ ...grpc.CallOption) error {
err := protojson.Unmarshal([]byte(`false`), out)
require.NoError(t, err)

return nil
return nil
},
resp: Response{
Message: "text reply",
},
},
}

c := Conn{raw: invokemock(helloReply)}
r := Request{
MethodDescriptor: methodFromProto("SayHello"),
Message: []byte(`{"greeting":"text request"}`),
}
res, err := c.Invoke(context.Background(), "/hello.HelloService/SayHello", metadata.New(nil), r)
require.NoError(t, err)
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

c := Conn{raw: invokemock(tt.invokemock)}
res, err := c.Invoke(ctx, tt.url, tt.md, tt.req)
require.NoError(t, err)

assert.Equal(t, codes.OK, res.Status)
assert.Equal(t, map[string]interface{}{"reply": "text reply"}, res.Message)
assert.Empty(t, res.Error)
assert.Equal(t, codes.OK, res.Status)
assert.Equal(t, tt.resp.Message, res.Message)
assert.Empty(t, res.Error)
})
}
}

func TestInvokeWithCallOptions(t *testing.T) {
Expand Down Expand Up @@ -275,8 +319,11 @@ syntax = "proto3";
package hello;
import "google/protobuf/wrappers.proto";
service HelloService {
rpc SayHello(HelloRequest) returns (HelloResponse);
rpc SayPrimitive(google.protobuf.StringValue) returns (google.protobuf.BoolValue);
rpc NoOp(Empty) returns (Empty);
rpc LotsOfReplies(HelloRequest) returns (stream HelloResponse);
rpc LotsOfGreetings(stream HelloRequest) returns (HelloResponse);
Expand Down