Skip to content

Commit 4a038d9

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

File tree

12 files changed

+506
-640
lines changed

12 files changed

+506
-640
lines changed

‎xds/internal/clients/config.go

Lines changed: 4 additions & 3 deletions
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

Lines changed: 316 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,316 @@
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+
si1 := ServerIdentifier{
159+
ServerURI: "foo",
160+
Extensions: &testServerIdentifierExtension{1},
161+
}
162+
si2 := ServerIdentifier{
163+
ServerURI: "foo",
164+
Extensions: &testServerIdentifierExtension{2},
165+
}
166+
167+
tests := []struct {
168+
name string
169+
existingKey ServerIdentifier
170+
existingValue any
171+
newKey ServerIdentifier
172+
newValue any
173+
wantExistingKeyValue any
174+
wantNewKeyValue any
175+
}{
176+
{
177+
name: "both_empty",
178+
existingKey: ServerIdentifier{},
179+
existingValue: "old",
180+
newKey: ServerIdentifier{},
181+
newValue: "new",
182+
wantExistingKeyValue: "new",
183+
wantNewKeyValue: "new",
184+
},
185+
{
186+
name: "one_empty",
187+
existingKey: ServerIdentifier{ServerURI: "foo"},
188+
existingValue: "old",
189+
newKey: ServerIdentifier{},
190+
newValue: "new",
191+
wantExistingKeyValue: "old",
192+
wantNewKeyValue: "new",
193+
},
194+
{
195+
name: "other_empty",
196+
existingKey: ServerIdentifier{},
197+
existingValue: "old",
198+
newKey: ServerIdentifier{ServerURI: "foo"},
199+
newValue: "new",
200+
wantExistingKeyValue: "old",
201+
wantNewKeyValue: "new",
202+
},
203+
{
204+
name: "same_ServerURI",
205+
existingKey: ServerIdentifier{ServerURI: "foo"},
206+
existingValue: "old",
207+
newKey: ServerIdentifier{ServerURI: "foo"},
208+
newValue: "new",
209+
wantExistingKeyValue: "new",
210+
wantNewKeyValue: "new",
211+
},
212+
{
213+
name: "different_ServerURI",
214+
existingKey: ServerIdentifier{ServerURI: "foo"},
215+
existingValue: "old",
216+
newKey: ServerIdentifier{ServerURI: "bar"},
217+
newValue: "new",
218+
wantExistingKeyValue: "old",
219+
wantNewKeyValue: "new",
220+
},
221+
{
222+
name: "same_Extensions",
223+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
224+
existingValue: "old",
225+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
226+
newValue: "new",
227+
wantExistingKeyValue: "new",
228+
wantNewKeyValue: "new",
229+
},
230+
{
231+
name: "different_Extensions",
232+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
233+
existingValue: "old",
234+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
235+
newValue: "new",
236+
wantExistingKeyValue: "old",
237+
wantNewKeyValue: "new",
238+
},
239+
{
240+
name: "first_config_Extensions_is_nil",
241+
existingKey: ServerIdentifier{Extensions: testServerIdentifierExtension{1}},
242+
existingValue: "old",
243+
newKey: ServerIdentifier{Extensions: nil},
244+
newValue: "new",
245+
wantExistingKeyValue: "old",
246+
wantNewKeyValue: "new",
247+
},
248+
{
249+
name: "other_config_Extensions_is_nil",
250+
existingKey: ServerIdentifier{Extensions: nil},
251+
existingValue: "old",
252+
newKey: ServerIdentifier{Extensions: testServerIdentifierExtension{2}},
253+
newValue: "new",
254+
wantExistingKeyValue: "old",
255+
wantNewKeyValue: "new",
256+
},
257+
{
258+
name: "all_fields_same",
259+
existingKey: ServerIdentifier{
260+
ServerURI: "foo",
261+
Extensions: testServerIdentifierExtension{1},
262+
},
263+
existingValue: "old",
264+
newKey: ServerIdentifier{
265+
ServerURI: "foo",
266+
Extensions: testServerIdentifierExtension{1},
267+
},
268+
newValue: "new",
269+
wantExistingKeyValue: "new",
270+
wantNewKeyValue: "new",
271+
},
272+
{
273+
name: "pointer_to_same_struct_same_values",
274+
existingKey: ServerIdentifier{
275+
ServerURI: "foo",
276+
Extensions: si1,
277+
},
278+
existingValue: 1,
279+
newKey: ServerIdentifier{
280+
ServerURI: "foo",
281+
Extensions: si1,
282+
},
283+
newValue: "new",
284+
wantExistingKeyValue: "new",
285+
wantNewKeyValue: "new",
286+
},
287+
{
288+
name: "pointer_to_same_struct_different_values",
289+
existingKey: ServerIdentifier{
290+
ServerURI: "foo",
291+
Extensions: si1,
292+
},
293+
existingValue: "old",
294+
newKey: ServerIdentifier{
295+
ServerURI: "foo",
296+
Extensions: si2,
297+
},
298+
newValue: "new",
299+
wantExistingKeyValue: "old",
300+
wantNewKeyValue: "new",
301+
}}
302+
303+
for _, test := range tests {
304+
t.Run(test.name, func(t *testing.T) {
305+
m := map[ServerIdentifier]any{}
306+
m[test.existingKey] = test.existingValue
307+
m[test.newKey] = test.newValue
308+
if got, ok := m[test.existingKey]; !ok || got != test.wantExistingKeyValue {
309+
t.Errorf("got m[test.existingKey] = %v, want %v", got, test.wantExistingKeyValue)
310+
}
311+
if got, ok := m[test.newKey]; !ok || got != test.wantNewKeyValue {
312+
t.Errorf("got m[test.newKey] = %v, want %v", got, test.wantNewKeyValue)
313+
}
314+
})
315+
}
316+
}

0 commit comments

Comments
 (0)