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 6 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,19 @@ 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...)) | ||
for i := 0; i < len(test.appendMD); i += 2 { | ||
md.Append(test.appendMD[i], test.appendMD[i+1]) | ||
} | ||
ktalg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.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) | ||
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. As below, please reword. |
||
} | ||
stream.SetTrailer(test.md) | ||
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(md) | ||
return nil | ||
}, | ||
} | ||
|
@@ -93,29 +125,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.
Nit: I know these comments are similar to ones on master here and throughout the PR, but can you please reword to have proper grammar (capitalization and periods). Also, some of these comments are not explaining why/reasoning a block of code is happening, simply stating exactly the operations taking place within the code block. Please reword to proper grammar/explaining why/delete those which you find appropriate.