Skip to content

Commit 3bacb44

Browse files
purnesh42Hpurnesh42H
committed
dfawley review 2: consider ServerIdentifier to be a map key
1 parent 5e4a675 commit 3bacb44

15 files changed

+500
-1111
lines changed

‎xds/internal/clients/config.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,10 @@ type ServerIdentifier struct {
6060
// For example, a custom TransportBuilder might use this field to
6161
// configure a specific security credentials.
6262
//
63-
// If present, Extensions must implement the `Equal(other any) bool`
64-
// method. Two ServerIdentifiers with Extensions will be considered unequal
65-
// if either Extensions does not implement this method.
63+
// Extensions must be able to be used as a map key, or the client may
64+
// panic. Any equivalent extensions in all ServerIdentifiers present in a
65+
// single client's configuration should have the same value. Not following
66+
// this restriction may result in excess resource usage.
6667
Extensions any
6768
}
6869

‎xds/internal/clients/config_test.go

+310
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,310 @@
1+
/*
2+
*
3+
* Copyright 2024 gRPC authors.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*
17+
*/
18+
19+
package clients
20+
21+
import (
22+
"testing"
23+
24+
"google.golang.org/grpc/internal/grpctest"
25+
)
26+
27+
type s struct {
28+
grpctest.Tester
29+
}
30+
31+
func Test(t *testing.T) {
32+
grpctest.RunSubTests(t, s{})
33+
}
34+
35+
type testServerIdentifierExtension struct{ x int }
36+
37+
func (s) TestServerIdentifierMap_Exist(t *testing.T) {
38+
si1 := ServerIdentifier{
39+
ServerURI: "foo",
40+
Extensions: &testServerIdentifierExtension{1},
41+
}
42+
si2 := ServerIdentifier{
43+
ServerURI: "foo",
44+
Extensions: &testServerIdentifierExtension{2},
45+
}
46+
47+
tests := []struct {
48+
name string
49+
setKey ServerIdentifier
50+
getKey ServerIdentifier
51+
exist bool
52+
}{
53+
{
54+
name: "both_empty",
55+
setKey: ServerIdentifier{},
56+
getKey: ServerIdentifier{},
57+
exist: true,
58+
},
59+
{
60+
name: "one_empty",
61+
setKey: ServerIdentifier{},
62+
getKey: ServerIdentifier{ServerURI: "bar"},
63+
exist: false,
64+
},
65+
{
66+
name: "other_empty",
67+
setKey: ServerIdentifier{ServerURI: "foo"},
68+
getKey: ServerIdentifier{},
69+
exist: false,
70+
},
71+
{
72+
name: "same_ServerURI",
73+
setKey: ServerIdentifier{ServerURI: "foo"},
74+
getKey: ServerIdentifier{ServerURI: "foo"},
75+
exist: true,
76+
},
77+
{
78+
name: "different_ServerURI",
79+
setKey: ServerIdentifier{ServerURI: "foo"},
80+
getKey: ServerIdentifier{ServerURI: "bar"},
81+
exist: false,
82+
},
83+
{
84+
name: "same_Extensions",
85+
setKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
86+
getKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
87+
exist: true,
88+
},
89+
{
90+
name: "different_Extensions",
91+
setKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
92+
getKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
93+
exist: false,
94+
},
95+
{
96+
name: "first_config_Extensions_is_nil",
97+
setKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
98+
getKey: ServerIdentifier{Extensions: nil},
99+
exist: false,
100+
},
101+
{
102+
name: "other_config_Extensions_is_nil",
103+
setKey: ServerIdentifier{Extensions: nil},
104+
getKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
105+
exist: false,
106+
},
107+
{
108+
name: "all_fields_same",
109+
setKey: ServerIdentifier{
110+
ServerURI: "foo",
111+
Extensions: testServerIdentifierExtension{1},
112+
},
113+
getKey: ServerIdentifier{
114+
ServerURI: "foo",
115+
Extensions: testServerIdentifierExtension{1},
116+
},
117+
exist: true,
118+
},
119+
{
120+
name: "pointer_to_same_struct_same_values",
121+
setKey: ServerIdentifier{
122+
ServerURI: "foo",
123+
Extensions: si1,
124+
},
125+
getKey: ServerIdentifier{
126+
ServerURI: "foo",
127+
Extensions: si1,
128+
},
129+
exist: true,
130+
},
131+
{
132+
name: "pointer_to_same_struct_different_values",
133+
setKey: ServerIdentifier{
134+
ServerURI: "foo",
135+
Extensions: si1,
136+
},
137+
getKey: ServerIdentifier{
138+
ServerURI: "foo",
139+
Extensions: si2,
140+
},
141+
exist: false,
142+
},
143+
}
144+
145+
for _, test := range tests {
146+
t.Run(test.name, func(t *testing.T) {
147+
m := map[ServerIdentifier]any{}
148+
m[test.setKey] = true
149+
_, ok := m[test.getKey]
150+
if test.exist != ok {
151+
t.Errorf("got m[test.getKey] exist = %v, want %v", ok, test.exist)
152+
}
153+
})
154+
}
155+
}
156+
157+
func (s) TestServerIdentifierMap_Add(t *testing.T) {
158+
ext1 := &testServerIdentifierExtension{1}
159+
ext2 := &testServerIdentifierExtension{2}
160+
161+
tests := []struct {
162+
name string
163+
existingKey ServerIdentifier
164+
existingValue any
165+
newKey ServerIdentifier
166+
newValue any
167+
wantExistingKeyValue any
168+
wantNewKeyValue any
169+
}{
170+
{
171+
name: "both_empty",
172+
existingKey: ServerIdentifier{},
173+
existingValue: "old",
174+
newKey: ServerIdentifier{},
175+
newValue: "new",
176+
wantExistingKeyValue: "new",
177+
wantNewKeyValue: "new",
178+
},
179+
{
180+
name: "one_empty",
181+
existingKey: ServerIdentifier{ServerURI: "foo"},
182+
existingValue: "old",
183+
newKey: ServerIdentifier{},
184+
newValue: "new",
185+
wantExistingKeyValue: "old",
186+
wantNewKeyValue: "new",
187+
},
188+
{
189+
name: "other_empty",
190+
existingKey: ServerIdentifier{},
191+
existingValue: "old",
192+
newKey: ServerIdentifier{ServerURI: "foo"},
193+
newValue: "new",
194+
wantExistingKeyValue: "old",
195+
wantNewKeyValue: "new",
196+
},
197+
{
198+
name: "same_ServerURI",
199+
existingKey: ServerIdentifier{ServerURI: "foo"},
200+
existingValue: "old",
201+
newKey: ServerIdentifier{ServerURI: "foo"},
202+
newValue: "new",
203+
wantExistingKeyValue: "new",
204+
wantNewKeyValue: "new",
205+
},
206+
{
207+
name: "different_ServerURI",
208+
existingKey: ServerIdentifier{ServerURI: "foo"},
209+
existingValue: "old",
210+
newKey: ServerIdentifier{ServerURI: "bar"},
211+
newValue: "new",
212+
wantExistingKeyValue: "old",
213+
wantNewKeyValue: "new",
214+
},
215+
{
216+
name: "same_Extensions",
217+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
218+
existingValue: "old",
219+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
220+
newValue: "new",
221+
wantExistingKeyValue: "new",
222+
wantNewKeyValue: "new",
223+
},
224+
{
225+
name: "different_Extensions",
226+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
227+
existingValue: "old",
228+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
229+
newValue: "new",
230+
wantExistingKeyValue: "old",
231+
wantNewKeyValue: "new",
232+
},
233+
{
234+
name: "first_config_Extensions_is_nil",
235+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
236+
existingValue: "old",
237+
newKey: ServerIdentifier{Extensions: nil},
238+
newValue: "new",
239+
wantExistingKeyValue: "old",
240+
wantNewKeyValue: "new",
241+
},
242+
{
243+
name: "other_config_Extensions_is_nil",
244+
existingKey: ServerIdentifier{Extensions: nil},
245+
existingValue: "old",
246+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
247+
newValue: "new",
248+
wantExistingKeyValue: "old",
249+
wantNewKeyValue: "new",
250+
},
251+
{
252+
name: "all_fields_same",
253+
existingKey: ServerIdentifier{
254+
ServerURI: "foo",
255+
Extensions: testServerIdentifierExtension{1},
256+
},
257+
existingValue: "old",
258+
newKey: ServerIdentifier{
259+
ServerURI: "foo",
260+
Extensions: testServerIdentifierExtension{1},
261+
},
262+
newValue: "new",
263+
wantExistingKeyValue: "new",
264+
wantNewKeyValue: "new",
265+
},
266+
{
267+
name: "pointer_to_same_struct_same_values",
268+
existingKey: ServerIdentifier{
269+
ServerURI: "foo",
270+
Extensions: ext1,
271+
},
272+
existingValue: 1,
273+
newKey: ServerIdentifier{
274+
ServerURI: "foo",
275+
Extensions: ext1,
276+
},
277+
newValue: "new",
278+
wantExistingKeyValue: "new",
279+
wantNewKeyValue: "new",
280+
},
281+
{
282+
name: "pointer_to_same_struct_different_values",
283+
existingKey: ServerIdentifier{
284+
ServerURI: "foo",
285+
Extensions: ext1,
286+
},
287+
existingValue: "old",
288+
newKey: ServerIdentifier{
289+
ServerURI: "foo",
290+
Extensions: ext2,
291+
},
292+
newValue: "new",
293+
wantExistingKeyValue: "old",
294+
wantNewKeyValue: "new",
295+
}}
296+
297+
for _, test := range tests {
298+
t.Run(test.name, func(t *testing.T) {
299+
m := map[ServerIdentifier]any{}
300+
m[test.existingKey] = test.existingValue
301+
m[test.newKey] = test.newValue
302+
if got, ok := m[test.existingKey]; !ok || got != test.wantExistingKeyValue {
303+
t.Errorf("got m[test.existingKey] = %v, want %v", got, test.wantExistingKeyValue)
304+
}
305+
if got, ok := m[test.newKey]; !ok || got != test.wantNewKeyValue {
306+
t.Errorf("got m[test.newKey] = %v, want %v", got, test.wantNewKeyValue)
307+
}
308+
})
309+
}
310+
}

0 commit comments

Comments
 (0)