From 3958f9d5c06ec7a9d2b1a4c85d95688825b76ad4 Mon Sep 17 00:00:00 2001 From: ibrahim albarghouthi Date: Mon, 10 Apr 2023 09:35:38 +0300 Subject: [PATCH] refactor: metadata supports key corresponding to multiple values (#2772) * 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 * Remove useless function * Linit fix --------- Co-authored-by: Ibrahim Barghouthi --- metadata/metadata.go | 34 +++++-- metadata/metadata_test.go | 134 ++++++++++++++++++++------- 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, 212 insertions(+), 54 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 3dabdbaef..8b072146e 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -9,22 +9,37 @@ import ( // 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 +47,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 +59,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)) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 8653bbe55..9aa1c620b 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,38 @@ 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 +114,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 +139,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 +190,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 +223,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 +258,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 +312,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 +337,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 +365,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 +380,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 711757bcf..dc2cc9e02 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 da9e43a96..30cb8df90 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 b083f9c10..9e9436d75 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 f12943e43..0835ef844 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 b95771ab9..72740bafa 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 50ca5c91d..56e21a863 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 32f054edd..686baed76 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 42510df30..c1a5396f1 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.