-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
metadata: fix validation issues #6001
Changes from all commits
e614562
f514755
8bc5ce1
e21027b
fa750b7
282edb3
f697294
083a379
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,29 +36,55 @@ import ( | |
) | ||
|
||
func (s) TestInvalidMetadata(t *testing.T) { | ||
grpctest.TLogger.ExpectErrorN("stream: failed to validate md when setting trailer", 2) | ||
grpctest.TLogger.ExpectErrorN("stream: failed to validate md when setting trailer", 5) | ||
|
||
tests := []struct { | ||
md metadata.MD | ||
want error | ||
recv error | ||
name string | ||
md metadata.MD | ||
appendMD []string | ||
want error | ||
recv error | ||
}{ | ||
{ | ||
name: "invalid key", | ||
md: map[string][]string{string(rune(0x19)): {"testVal"}}, | ||
want: status.Error(codes.Internal, "header key \"\\x19\" contains illegal characters not in [0-9a-z-_.]"), | ||
recv: status.Error(codes.Internal, "invalid header field"), | ||
}, | ||
{ | ||
name: "invalid value", | ||
md: map[string][]string{"test": {string(rune(0x19))}}, | ||
want: status.Error(codes.Internal, "header key \"test\" contains value with non-printable ASCII characters"), | ||
recv: status.Error(codes.Internal, "invalid header field"), | ||
}, | ||
{ | ||
name: "invalid appended value", | ||
md: map[string][]string{"test": {"test"}}, | ||
appendMD: []string{"/", "value"}, | ||
want: status.Error(codes.Internal, "header key \"/\" contains illegal characters not in [0-9a-z-_.]"), | ||
recv: status.Error(codes.Internal, "invalid header field"), | ||
}, | ||
{ | ||
name: "empty appended key", | ||
md: map[string][]string{"test": {"test"}}, | ||
appendMD: []string{"", "value"}, | ||
want: status.Error(codes.Internal, "there is an empty key in the header"), | ||
recv: status.Error(codes.Internal, "invalid header field"), | ||
}, | ||
{ | ||
name: "empty key", | ||
md: map[string][]string{"": {"test"}}, | ||
want: status.Error(codes.Internal, "there is an empty key in the header"), | ||
recv: status.Error(codes.Internal, "invalid header field"), | ||
}, | ||
{ | ||
name: "-bin key with arbitrary value", | ||
md: map[string][]string{"test-bin": {string(rune(0x19))}}, | ||
want: nil, | ||
recv: io.EOF, | ||
}, | ||
{ | ||
name: "valid key and value", | ||
md: map[string][]string{"test": {"value"}}, | ||
want: nil, | ||
recv: io.EOF, | ||
|
@@ -77,13 +103,16 @@ func (s) TestInvalidMetadata(t *testing.T) { | |
} | ||
test := tests[testNum] | ||
testNum++ | ||
if err := stream.SetHeader(test.md); !reflect.DeepEqual(test.want, err) { | ||
return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) | ||
// merge original md and added md. | ||
md := metadata.Join(test.md, metadata.Pairs(test.appendMD...)) | ||
|
||
if err := stream.SetHeader(md); !reflect.DeepEqual(test.want, err) { | ||
return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", md, err, test.want) | ||
} | ||
if err := stream.SendHeader(test.md); !reflect.DeepEqual(test.want, err) { | ||
return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) | ||
if err := stream.SendHeader(md); !reflect.DeepEqual(test.want, err) { | ||
return fmt.Errorf("call stream.SendHeader(md) validate metadata which is %v got err :%v, want err :%v", md, err, test.want) | ||
} | ||
stream.SetTrailer(test.md) | ||
stream.SetTrailer(md) | ||
return nil | ||
}, | ||
} | ||
|
@@ -93,29 +122,33 @@ func (s) TestInvalidMetadata(t *testing.T) { | |
defer ss.Stop() | ||
|
||
for _, test := range tests { | ||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
defer cancel() | ||
|
||
ctx = metadata.NewOutgoingContext(ctx, test.md) | ||
if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); !reflect.DeepEqual(test.want, err) { | ||
t.Errorf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) | ||
} | ||
t.Run("unary "+test.name, func(t *testing.T) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
defer cancel() | ||
ctx = metadata.NewOutgoingContext(ctx, test.md) | ||
ctx = metadata.AppendToOutgoingContext(ctx, test.appendMD...) | ||
if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); !reflect.DeepEqual(test.want, err) { | ||
t.Errorf("call ss.Client.EmptyCall() validate metadata which is %v got err :%v, want err :%v", test.md, err, test.want) | ||
} | ||
}) | ||
} | ||
|
||
// call the stream server's api to drive the server-side unit testing | ||
for _, test := range tests { | ||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
stream, err := ss.Client.FullDuplexCall(ctx) | ||
defer cancel() | ||
if err != nil { | ||
t.Errorf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) | ||
continue | ||
} | ||
if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { | ||
t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) | ||
} | ||
if _, err := stream.Recv(); status.Code(err) != status.Code(test.recv) || !strings.Contains(err.Error(), test.recv.Error()) { | ||
t.Errorf("stream.Recv() = _, get err :%v, want err :%v", err, test.recv) | ||
} | ||
t.Run("streaming "+test.name, func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional nit (not important for linter or correctness): I prefer spaces in between operators i.e. "streaming " + test.name). Here and elsewhere |
||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
defer cancel() | ||
stream, err := ss.Client.FullDuplexCall(ctx) | ||
if err != nil { | ||
t.Errorf("call ss.Client.FullDuplexCall(context.Background()) will success but got err :%v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you didn't write this, but this grammar is strange, and is incorrect (it's not being called with context.Background(), it's being called with context with a timeout). Please reword (see other examples in codebase). Perhaps something like "ss.Client.FullDuplexCall(ctx) want err: %v, got err : %v", nil, err). Here and elsewhere. |
||
return | ||
} | ||
if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil { | ||
t.Errorf("call ss.Client stream Send(nil) will success but got err :%v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here (see comment above). This isn't sending nil, but an allocated testpb.StreamingOutputCallRequest. |
||
} | ||
if _, err := stream.Recv(); status.Code(err) != status.Code(test.recv) || !strings.Contains(err.Error(), test.recv.Error()) { | ||
t.Errorf("stream.Recv() = _, get err :%v, want err :%v", err, test.recv) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. *got err |
||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As below, please reword.