From dcc58b3316becb27d33272abf1be65e1e2f259c1 Mon Sep 17 00:00:00 2001 From: Ibrahim Barghouthi Date: Wed, 29 Mar 2023 20:40:18 +0300 Subject: [PATCH 1/3] Adding Add/Values fn for transport and metadata layers, fixed tests and included tests to new functionaltieis, updated metadata middleware to append instead of hard set of key-values --- metadata/metadata.go | 39 ++++++-- metadata/metadata_test.go | 135 +++++++++++++++++++++------ middleware/auth/jwt/jwt_test.go | 7 ++ middleware/metadata/metadata.go | 22 +++-- middleware/metadata/metadata_test.go | 11 ++- middleware/selector/selector_test.go | 26 ++++-- middleware/tracing/tracing_test.go | 10 ++ transport/grpc/transport.go | 10 ++ transport/http/transport.go | 10 ++ transport/transport.go | 2 + 10 files changed, 218 insertions(+), 54 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 3dabdbaeff0..92032049685 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -3,28 +3,44 @@ package metadata import ( "context" "fmt" + grpcmd "google.golang.org/grpc/metadata" "strings" ) // Metadata is our way of representing request headers internally. // They're used at the RPC level and translate back and forth // from Transport headers. -type Metadata map[string]string +type Metadata map[string][]string // New creates an MD from a given key-values map. -func New(mds ...map[string]string) Metadata { +func New(mds ...map[string][]string) Metadata { md := Metadata{} for _, m := range mds { - for k, v := range m { - md.Set(k, v) + for k, vList := range m { + for _, v := range vList { + md.Add(k, v) + } } } return md } +// Add adds the key, value pair to the header. +func (m Metadata) Add(key, value string) { + if len(key) == 0 { + return + } + + m[strings.ToLower(key)] = append(m[strings.ToLower(key)], value) +} + // Get returns the value associated with the passed key. func (m Metadata) Get(key string) string { - return m[strings.ToLower(key)] + v := m[strings.ToLower(key)] + if len(v) == 0 { + return "" + } + return v[0] } // Set stores the key-value pair. @@ -32,11 +48,11 @@ func (m Metadata) Set(key string, value string) { if key == "" || value == "" { return } - m[strings.ToLower(key)] = value + m[strings.ToLower(key)] = []string{value} } // Range iterate over element in metadata. -func (m Metadata) Range(f func(k, v string) bool) { +func (m Metadata) Range(f func(k string, v []string) bool) { for k, v := range m { if !f(k, v) { break @@ -44,6 +60,11 @@ func (m Metadata) Range(f func(k, v string) bool) { } } +// Values returns a slice of values associated with the passed key. +func (m Metadata) Values(key string) []string { + return m[strings.ToLower(key)] +} + // Clone returns a deep copy of Metadata func (m Metadata) Clone() Metadata { md := make(Metadata, len(m)) @@ -53,6 +74,10 @@ func (m Metadata) Clone() Metadata { return md } +func (m Metadata) ToGrpcMD() grpcmd.MD { + return grpcmd.MD(m) +} + type serverMetadataKey struct{} // NewServerContext creates a new context with client md attached. diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 8653bbe5589..eea8dcec693 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -8,7 +8,7 @@ import ( func TestNew(t *testing.T) { type args struct { - mds []map[string]string + mds []map[string][]string } tests := []struct { name string @@ -17,13 +17,13 @@ func TestNew(t *testing.T) { }{ { name: "hello", - args: args{[]map[string]string{{"hello": "kratos"}, {"hello2": "go-kratos"}}}, - want: Metadata{"hello": "kratos", "hello2": "go-kratos"}, + args: args{[]map[string][]string{{"hello": {"kratos"}}, {"hello2": {"go-kratos"}}}}, + want: Metadata{"hello": {"kratos"}, "hello2": {"go-kratos"}}, }, { name: "hi", - args: args{[]map[string]string{{"hi": "kratos"}, {"hi2": "go-kratos"}}}, - want: Metadata{"hi": "kratos", "hi2": "go-kratos"}, + args: args{[]map[string][]string{{"hi": {"kratos"}}, {"hi2": {"go-kratos"}}}}, + want: Metadata{"hi": {"kratos"}, "hi2": {"go-kratos"}}, }, } for _, tt := range tests { @@ -47,13 +47,13 @@ func TestMetadata_Get(t *testing.T) { }{ { name: "kratos", - m: Metadata{"kratos": "value", "env": "dev"}, + m: Metadata{"kratos": {"value"}, "env": {"dev"}}, args: args{key: "kratos"}, want: "value", }, { name: "env", - m: Metadata{"kratos": "value", "env": "dev"}, + m: Metadata{"kratos": {"value"}, "env": {"dev"}}, args: args{key: "env"}, want: "dev", }, @@ -67,6 +67,39 @@ func TestMetadata_Get(t *testing.T) { } } +func TestMetadata_Values(t *testing.T) { + type args struct { + key string + } + tests := []struct { + name string + m Metadata + args args + want []string + }{ + { + name: "kratos", + m: Metadata{"kratos": {"value", "value2"}, "env": {"dev"}}, + args: args{key: "kratos"}, + want: []string{"value", "value2"}, + }, + { + name: "env", + m: Metadata{"kratos": {"value", "value2"}, "env": {"dev"}}, + args: args{key: "env"}, + want: []string{"dev"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + if got := tt.m.Values(tt.args.key); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Get() = %v, want %v", got, tt.want) + } + }) + } +} + func TestMetadata_Set(t *testing.T) { type args struct { key string @@ -82,13 +115,13 @@ func TestMetadata_Set(t *testing.T) { name: "kratos", m: Metadata{}, args: args{key: "hello", value: "kratos"}, - want: Metadata{"hello": "kratos"}, + want: Metadata{"hello": {"kratos"}}, }, { name: "env", - m: Metadata{"hello": "kratos"}, + m: Metadata{"hello": {"kratos"}}, args: args{key: "env", value: "pro"}, - want: Metadata{"hello": "kratos", "env": "pro"}, + want: Metadata{"hello": {"kratos"}, "env": {"pro"}}, }, { name: "empty", @@ -107,6 +140,46 @@ func TestMetadata_Set(t *testing.T) { } } +func TestMetadata_Add(t *testing.T) { + type args struct { + key string + value string + } + tests := []struct { + name string + m Metadata + args args + want Metadata + }{ + { + name: "kratos", + m: Metadata{}, + args: args{key: "hello", value: "kratos"}, + want: Metadata{"hello": {"kratos"}}, + }, + { + name: "env", + m: Metadata{"hello": {"kratos"}}, + args: args{key: "hello", value: "again"}, + want: Metadata{"hello": {"kratos", "again"}}, + }, + { + name: "empty", + m: Metadata{}, + args: args{key: "", value: ""}, + want: Metadata{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.m.Add(tt.args.key, tt.args.value) + if !reflect.DeepEqual(tt.m, tt.want) { + t.Errorf("Set() = %v, want %v", tt.m, tt.want) + } + }) + } +} + func TestClientContext(t *testing.T) { type args struct { ctx context.Context @@ -118,11 +191,11 @@ func TestClientContext(t *testing.T) { }{ { name: "kratos", - args: args{context.Background(), Metadata{"hello": "kratos", "kratos": "https://go-kratos.dev"}}, + args: args{context.Background(), Metadata{"hello": {"kratos"}, "kratos": {"https://go-kratos.dev"}}}, }, { name: "hello", - args: args{context.Background(), Metadata{"hello": "kratos", "hello2": "https://go-kratos.dev"}}, + args: args{context.Background(), Metadata{"hello": {"kratos"}, "hello2": {"https://go-kratos.dev"}}}, }, } for _, tt := range tests { @@ -151,11 +224,11 @@ func TestServerContext(t *testing.T) { }{ { name: "kratos", - args: args{context.Background(), Metadata{"hello": "kratos", "kratos": "https://go-kratos.dev"}}, + args: args{context.Background(), Metadata{"hello": {"kratos"}, "kratos": {"https://go-kratos.dev"}}}, }, { name: "hello", - args: args{context.Background(), Metadata{"hello": "kratos", "hello2": "https://go-kratos.dev"}}, + args: args{context.Background(), Metadata{"hello": {"kratos"}, "hello2": {"https://go-kratos.dev"}}}, }, } for _, tt := range tests { @@ -186,12 +259,12 @@ func TestAppendToClientContext(t *testing.T) { { name: "kratos", args: args{Metadata{}, []string{"hello", "kratos", "env", "dev"}}, - want: Metadata{"hello": "kratos", "env": "dev"}, + want: Metadata{"hello": {"kratos"}, "env": {"dev"}}, }, { name: "hello", - args: args{Metadata{"hi": "https://go-kratos.dev/"}, []string{"hello", "kratos", "env", "dev"}}, - want: Metadata{"hello": "kratos", "env": "dev", "hi": "https://go-kratos.dev/"}, + args: args{Metadata{"hi": {"https://go-kratos.dev/"}}, []string{"hello", "kratos", "env", "dev"}}, + want: Metadata{"hello": {"kratos"}, "env": {"dev"}, "hi": {"https://go-kratos.dev/"}}, }, } for _, tt := range tests { @@ -240,13 +313,13 @@ func TestMergeToClientContext(t *testing.T) { }{ { name: "kratos", - args: args{Metadata{}, Metadata{"hello": "kratos", "env": "dev"}}, - want: Metadata{"hello": "kratos", "env": "dev"}, + args: args{Metadata{}, Metadata{"hello": {"kratos"}, "env": {"dev"}}}, + want: Metadata{"hello": {"kratos"}, "env": {"dev"}}, }, { name: "hello", - args: args{Metadata{"hi": "https://go-kratos.dev/"}, Metadata{"hello": "kratos", "env": "dev"}}, - want: Metadata{"hello": "kratos", "env": "dev", "hi": "https://go-kratos.dev/"}, + args: args{Metadata{"hi": {"https://go-kratos.dev/"}}, Metadata{"hello": {"kratos"}, "env": {"dev"}}}, + want: Metadata{"hello": {"kratos"}, "env": {"dev"}, "hi": {"https://go-kratos.dev/"}}, }, } for _, tt := range tests { @@ -265,19 +338,19 @@ func TestMergeToClientContext(t *testing.T) { } func TestMetadata_Range(t *testing.T) { - md := Metadata{"kratos": "kratos", "https://go-kratos.dev/": "https://go-kratos.dev/", "go-kratos": "go-kratos"} + md := Metadata{"kratos": {"kratos"}, "https://go-kratos.dev/": {"https://go-kratos.dev/"}, "go-kratos": {"go-kratos"}} tmp := Metadata{} - md.Range(func(k, v string) bool { + md.Range(func(k string, v []string) bool { if k == "https://go-kratos.dev/" || k == "kratos" { tmp[k] = v } return true }) - if !reflect.DeepEqual(tmp, Metadata{"https://go-kratos.dev/": "https://go-kratos.dev/", "kratos": "kratos"}) { - t.Errorf("metadata = %v, want %v", tmp, Metadata{"https://go-kratos.dev/": "https://go-kratos.dev/", "kratos": "kratos"}) + if !reflect.DeepEqual(tmp, Metadata{"https://go-kratos.dev/": {"https://go-kratos.dev/"}, "kratos": {"kratos"}}) { + t.Errorf("metadata = %v, want %v", tmp, Metadata{"https://go-kratos.dev/": {"https://go-kratos.dev/"}, "kratos": {"kratos"}}) } tmp = Metadata{} - md.Range(func(k, v string) bool { + md.Range(func(k string, v []string) bool { return false }) if !reflect.DeepEqual(tmp, Metadata{}) { @@ -293,13 +366,13 @@ func TestMetadata_Clone(t *testing.T) { }{ { name: "kratos", - m: Metadata{"kratos": "kratos", "https://go-kratos.dev/": "https://go-kratos.dev/", "go-kratos": "go-kratos"}, - want: Metadata{"kratos": "kratos", "https://go-kratos.dev/": "https://go-kratos.dev/", "go-kratos": "go-kratos"}, + m: Metadata{"kratos": {"kratos"}, "https://go-kratos.dev/": {"https://go-kratos.dev/"}, "go-kratos": {"go-kratos"}}, + want: Metadata{"kratos": {"kratos"}, "https://go-kratos.dev/": {"https://go-kratos.dev/"}, "go-kratos": {"go-kratos"}}, }, { name: "go", - m: Metadata{"language": "golang"}, - want: Metadata{"language": "golang"}, + m: Metadata{"language": {"golang"}}, + want: Metadata{"language": {"golang"}}, }, } for _, tt := range tests { @@ -308,7 +381,7 @@ func TestMetadata_Clone(t *testing.T) { if !reflect.DeepEqual(got, tt.want) { t.Errorf("Clone() = %v, want %v", got, tt.want) } - got["kratos"] = "go" + got["kratos"] = []string{"go"} if reflect.DeepEqual(got, tt.want) { t.Errorf("want got != want got %v want %v", got, tt.want) } diff --git a/middleware/auth/jwt/jwt_test.go b/middleware/auth/jwt/jwt_test.go index 711757bcfad..dc2cc9e02db 100644 --- a/middleware/auth/jwt/jwt_test.go +++ b/middleware/auth/jwt/jwt_test.go @@ -24,6 +24,8 @@ func (hc headerCarrier) Get(key string) string { return http.Header(hc).Get(key) func (hc headerCarrier) Set(key string, value string) { http.Header(hc).Set(key, value) } +func (hc headerCarrier) Add(key string, value string) { http.Header(hc).Add(key, value) } + // Keys lists the keys stored in this carrier. func (hc headerCarrier) Keys() []string { keys := make([]string, 0, len(hc)) @@ -33,6 +35,11 @@ func (hc headerCarrier) Keys() []string { return keys } +// Values returns a slice value associated with the passed key. +func (hc headerCarrier) Values(key string) []string { + return http.Header(hc).Values(key) +} + func newTokenHeader(headerKey string, token string) *headerCarrier { header := &headerCarrier{} header.Set(headerKey, token) diff --git a/middleware/metadata/metadata.go b/middleware/metadata/metadata.go index da9e43a962d..30cb8df903f 100644 --- a/middleware/metadata/metadata.go +++ b/middleware/metadata/metadata.go @@ -60,7 +60,9 @@ func Server(opts ...Option) middleware.Middleware { header := tr.RequestHeader() for _, k := range header.Keys() { if options.hasPrefix(k) { - md.Set(k, header.Get(k)) + for _, v := range header.Values(k) { + md.Add(k, v) + } } } ctx = metadata.NewServerContext(ctx, md) @@ -86,19 +88,25 @@ func Client(opts ...Option) middleware.Middleware { header := tr.RequestHeader() // x-md-local- - for k, v := range options.md { - header.Set(k, v) + for k, vList := range options.md { + for _, v := range vList { + header.Add(k, v) + } } if md, ok := metadata.FromClientContext(ctx); ok { - for k, v := range md { - header.Set(k, v) + for k, vList := range md { + for _, v := range vList { + header.Add(k, v) + } } } // x-md-global- if md, ok := metadata.FromServerContext(ctx); ok { - for k, v := range md { + for k, vList := range md { if options.hasPrefix(k) { - header.Set(k, v) + for _, v := range vList { + header.Add(k, v) + } } } } diff --git a/middleware/metadata/metadata_test.go b/middleware/metadata/metadata_test.go index b083f9c10cc..9e9436d75b2 100644 --- a/middleware/metadata/metadata_test.go +++ b/middleware/metadata/metadata_test.go @@ -17,6 +17,8 @@ func (hc headerCarrier) Get(key string) string { return http.Header(hc).Get(key) func (hc headerCarrier) Set(key string, value string) { http.Header(hc).Set(key, value) } +func (hc headerCarrier) Add(key string, value string) { http.Header(hc).Add(key, value) } + // Keys lists the keys stored in this carrier. func (hc headerCarrier) Keys() []string { keys := make([]string, 0, len(hc)) @@ -26,6 +28,11 @@ func (hc headerCarrier) Keys() []string { return keys } +// Values returns a slice value associated with the passed key. +func (hc headerCarrier) Values(key string) []string { + return http.Header(hc).Values(key) +} + type testTransport struct{ header headerCarrier } func (tr *testTransport) Kind() transport.Kind { return transport.KindHTTP } @@ -123,11 +130,11 @@ func TestClient(t *testing.T) { func TestWithConstants(t *testing.T) { md := metadata.Metadata{ - constKey: constValue, + constKey: {constValue}, } options := &options{ md: metadata.Metadata{ - "override": "override", + "override": {"override"}, }, } diff --git a/middleware/selector/selector_test.go b/middleware/selector/selector_test.go index f12943e43e4..0835ef84414 100644 --- a/middleware/selector/selector_test.go +++ b/middleware/selector/selector_test.go @@ -40,15 +40,23 @@ func (tr *Transport) ReplyHeader() transport.Header { } type mockHeader struct { - m map[string]string + m map[string][]string } func (m *mockHeader) Get(key string) string { - return m.m[key] + vals := m.m[key] + if len(vals) > 0 { + return vals[0] + } + return "" } func (m *mockHeader) Set(key, value string) { - m.m[key] = value + m.m[key] = []string{value} +} + +func (m *mockHeader) Add(key, value string) { + m.m[key] = append(m.m[key], value) } func (m *mockHeader) Keys() []string { @@ -59,6 +67,10 @@ func (m *mockHeader) Keys() []string { return keys } +func (m *mockHeader) Values(key string) []string { + return m.m[key] +} + func TestMatch(t *testing.T) { tests := []struct { name string @@ -185,28 +197,28 @@ func TestHeaderFunc(t *testing.T) { name: "/hello.Update/world", ctx: transport.NewServerContext(context.Background(), &Transport{ operation: "/hello.Update/world", - headers: &mockHeader{map[string]string{"X-Test": "test"}}, + headers: &mockHeader{map[string][]string{"X-Test": {"test"}}}, }), }, { name: "/hi.Create/world", ctx: transport.NewServerContext(context.Background(), &Transport{ operation: "/hi.Create/world", - headers: &mockHeader{map[string]string{"X-Test": "test2", "go-kratos": "kratos"}}, + headers: &mockHeader{map[string][]string{"X-Test": {"test2"}, "go-kratos": {"kratos"}}}, }), }, { name: "/test.Name/1234", ctx: transport.NewServerContext(context.Background(), &Transport{ operation: "/test.Name/1234", - headers: &mockHeader{map[string]string{"X-Test": "test3"}}, + headers: &mockHeader{map[string][]string{"X-Test": {"test3"}}}, }), }, { name: "/go-kratos.dev/kratos", ctx: transport.NewServerContext(context.Background(), &Transport{ operation: "/go-kratos.dev/kratos", - headers: &mockHeader{map[string]string{"X-Test": "test"}}, + headers: &mockHeader{map[string][]string{"X-Test": {"test"}}}, }), }, } diff --git a/middleware/tracing/tracing_test.go b/middleware/tracing/tracing_test.go index b95771ab9cc..72740bafa24 100644 --- a/middleware/tracing/tracing_test.go +++ b/middleware/tracing/tracing_test.go @@ -29,6 +29,11 @@ func (hc headerCarrier) Set(key string, value string) { http.Header(hc).Set(key, value) } +// Add value to the key-value pair. +func (hc headerCarrier) Add(key string, value string) { + http.Header(hc).Add(key, value) +} + // Keys lists the keys stored in this carrier. func (hc headerCarrier) Keys() []string { keys := make([]string, 0, len(hc)) @@ -38,6 +43,11 @@ func (hc headerCarrier) Keys() []string { return keys } +// Values returns a slice value associated with the passed key. +func (hc headerCarrier) Values(key string) []string { + return http.Header(hc).Values(key) +} + type mockTransport struct { kind transport.Kind endpoint string diff --git a/transport/grpc/transport.go b/transport/grpc/transport.go index 50ca5c91da2..56e21a86365 100644 --- a/transport/grpc/transport.go +++ b/transport/grpc/transport.go @@ -64,6 +64,11 @@ func (mc headerCarrier) Set(key string, value string) { metadata.MD(mc).Set(key, value) } +// Add append value to key-values pair. +func (mc headerCarrier) Add(key string, value string) { + metadata.MD(mc).Append(key, value) +} + // Keys lists the keys stored in this carrier. func (mc headerCarrier) Keys() []string { keys := make([]string, 0, len(mc)) @@ -72,3 +77,8 @@ func (mc headerCarrier) Keys() []string { } return keys } + +// Values returns a slice of values associated with the passed key. +func (mc headerCarrier) Values(key string) []string { + return metadata.MD(mc).Get(key) +} diff --git a/transport/http/transport.go b/transport/http/transport.go index 32f054edd63..686baed7606 100644 --- a/transport/http/transport.go +++ b/transport/http/transport.go @@ -92,6 +92,11 @@ func (hc headerCarrier) Set(key string, value string) { http.Header(hc).Set(key, value) } +// Add append value to key-values pair. +func (hc headerCarrier) Add(key string, value string) { + http.Header(hc).Add(key, value) +} + // Keys lists the keys stored in this carrier. func (hc headerCarrier) Keys() []string { keys := make([]string, 0, len(hc)) @@ -100,3 +105,8 @@ func (hc headerCarrier) Keys() []string { } return keys } + +// Values returns a slice of values associated with the passed key. +func (hc headerCarrier) Values(key string) []string { + return http.Header(hc).Values(key) +} diff --git a/transport/transport.go b/transport/transport.go index 42510df3063..c1a5396f16f 100644 --- a/transport/transport.go +++ b/transport/transport.go @@ -27,7 +27,9 @@ type Endpointer interface { type Header interface { Get(key string) string Set(key string, value string) + Add(key string, value string) Keys() []string + Values(key string) []string } // Transporter is transport context value interface. From edc7bebada3b40d869ce11ddddfe8e66833ca8f3 Mon Sep 17 00:00:00 2001 From: Ibrahim Barghouthi Date: Sun, 2 Apr 2023 11:23:49 +0300 Subject: [PATCH 2/3] Remove useless function --- metadata/metadata.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 92032049685..8b072146eed 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -3,7 +3,6 @@ package metadata import ( "context" "fmt" - grpcmd "google.golang.org/grpc/metadata" "strings" ) @@ -74,10 +73,6 @@ func (m Metadata) Clone() Metadata { return md } -func (m Metadata) ToGrpcMD() grpcmd.MD { - return grpcmd.MD(m) -} - type serverMetadataKey struct{} // NewServerContext creates a new context with client md attached. From 4f808feff7a4c9e7a4e2c4614648ce603a758ff5 Mon Sep 17 00:00:00 2001 From: Ibrahim Barghouthi Date: Sat, 8 Apr 2023 18:29:28 +0300 Subject: [PATCH 3/3] Linit fix --- metadata/metadata_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index eea8dcec693..9aa1c620bb6 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -92,7 +92,6 @@ func TestMetadata_Values(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := tt.m.Values(tt.args.key); !reflect.DeepEqual(got, tt.want) { t.Errorf("Get() = %v, want %v", got, tt.want) }