Skip to content

Commit 5224914

Browse files
nicktorwaldnicktorwald
committed
Filter wrong formatted discovery addresses
Instead of discarding the full list of addresses when at least one address does not match the format like host[:port] a discoverer just skips the broken address and carries on processing. Closes: #195
1 parent 013dd2f commit 5224914

File tree

5 files changed

+117
-21
lines changed

5 files changed

+117
-21
lines changed

‎README.md

+6
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,10 @@ tarantool> function get_cluster_nodes() return { 'host1:3301', 'host2:3302', 'ho
238238
You need to pay attention to a function contract we are currently supporting:
239239
* The client never passes any arguments to a discovery function.
240240
* A discovery function _must_ return an array of strings (i.e `return {'host1:3301', 'host2:3301'}`).
241+
* Each string _should_ satisfy the following pattern `host[:port]`
242+
(or more strictly `/^[^:]+(:\d+)?$/` - a mandatory host containing any string
243+
and an optional colon followed by digits of the port). Also, the port must be
244+
in a range between 1 and 65535 if one is presented.
241245
* A discovery function _may_ return multi-results but the client takes
242246
into account only first of them (i.e. `return {'host:3301'}, discovery_delay`, where
243247
the second result is unused). Even more, any extra results __are reserved__ by the client
@@ -270,6 +274,8 @@ client.syncOps().insert(45, Arrays.asList(1, 1));
270274
* A discovery task always uses an active client connection to get the nodes list.
271275
It's in your responsibility to provide a function availability as well as a consistent
272276
nodes list on all instances you initially set or obtain from the task.
277+
* Every address which is unmatched with `host[:port]` pattern will be filtered out from
278+
the target addresses list.
273279
* If some error occurs while a discovery task is running then this task
274280
will be aborted without any after-effects for next task executions. These cases, for instance, are
275281
a wrong function result (see discovery function contract) or a broken connection.

‎src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java

+39-9
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import org.tarantool.TarantoolClient;
44
import org.tarantool.TarantoolClientOps;
55
import org.tarantool.TarantoolClusterClientConfig;
6+
import org.tarantool.util.StringUtils;
67

78
import java.util.LinkedHashSet;
89
import java.util.List;
@@ -36,28 +37,57 @@ public Set<String> getInstances() {
3637
// validation against the data returned;
3738
// this strict-mode allows us to extend the contract in a non-breaking
3839
// way for old clients just reserve an extra return value in
39-
// terms of LUA multi-result support.
40-
checkResult(list);
41-
42-
List<Object> funcResult = (List<Object>) list.get(0);
43-
return funcResult.stream()
44-
.map(Object::toString)
45-
.collect(Collectors.toCollection(LinkedHashSet::new));
40+
// terms of Lua multi-result support.;
41+
return checkAndFilterAddresses(list);
4642
}
4743

4844
/**
4945
* Check whether the result follows the contract or not.
50-
* The contract is a mandatory <b>single array of strings</b>
46+
* The contract is a mandatory <b>single array of strings</b>.
47+
*
48+
* The simplified format for each string is host[:port].
5149
*
5250
* @param result result to be validated
5351
*/
54-
private void checkResult(List<?> result) {
52+
private Set<String> checkAndFilterAddresses(List<?> result) {
5553
if (result == null || result.isEmpty()) {
5654
throw new IllegalDiscoveryFunctionResult("Discovery function returned no data");
5755
}
5856
if (!(result.get(0) instanceof List)) {
5957
throw new IllegalDiscoveryFunctionResult("The first value must be an array of strings");
6058
}
59+
60+
return ((List<Object>) result.get(0)).stream()
61+
.filter(item -> item instanceof String)
62+
.map(Object::toString)
63+
.filter(s -> !StringUtils.isBlank(s))
64+
.filter(this::isAddress)
65+
.collect(Collectors.toCollection(LinkedHashSet::new));
66+
}
67+
68+
/**
69+
* Checks that address matches host[:port] format.
70+
*
71+
* @param address to be checked
72+
* @return true if address follows the format
73+
*/
74+
private boolean isAddress(String address) {
75+
if (address.endsWith(":")) {
76+
return false;
77+
}
78+
String[] addressParts = address.split(":");
79+
if (addressParts.length > 2) {
80+
return false;
81+
}
82+
if (addressParts.length == 2) {
83+
try {
84+
int port = Integer.parseInt(addressParts[1]);
85+
return (port > 0 && port < 65536);
86+
} catch (NumberFormatException e) {
87+
return false;
88+
}
89+
}
90+
return true;
6191
}
6292

6393
}

‎src/test/java/org/tarantool/ClientAsyncOperationsIT.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,7 @@ void testCall(AsyncOpsProvider provider) throws ExecutionException, InterruptedE
180180
testHelper.executeLua("function echo(...) return ... end");
181181

182182
Future<List<?>> fut = provider.getAsyncOps().call("echo", "hello");
183-
assertEquals(Collections.singletonList(Collections.singletonList("hello")),
184-
fut.get(TIMEOUT, TimeUnit.MILLISECONDS));
183+
assertEquals(Collections.singletonList("hello"), fut.get(TIMEOUT, TimeUnit.MILLISECONDS));
185184

186185
provider.close();
187186
}

‎src/test/java/org/tarantool/TarantoolClientOpsIT.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ public void testEval(SyncOpsProvider provider) {
489489
@MethodSource("getClientOps")
490490
public void testCall(SyncOpsProvider provider) {
491491
assertEquals(
492-
Collections.singletonList(Collections.singletonList("true")),
492+
Collections.singletonList("true"),
493493
provider.getClientOps().call("echo", "true")
494494
);
495495

‎src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java

+70-9
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package org.tarantool.cluster;
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
45
import static org.junit.jupiter.api.Assertions.assertNotNull;
56
import static org.junit.jupiter.api.Assertions.assertThrows;
67
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -23,6 +24,7 @@
2324

2425
import java.util.Arrays;
2526
import java.util.Collections;
27+
import java.util.HashSet;
2628
import java.util.List;
2729
import java.util.Set;
2830

@@ -72,7 +74,7 @@ public void testSuccessfulAddressParsing() {
7274
testHelper.executeLua(functionCode);
7375

7476
TarantoolClusterStoredFunctionDiscoverer discoverer =
75-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
77+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
7678

7779
Set<String> instances = discoverer.getInstances();
7880

@@ -91,7 +93,7 @@ public void testSuccessfulUniqueAddressParsing() {
9193
testHelper.executeLua(functionCode);
9294

9395
TarantoolClusterStoredFunctionDiscoverer discoverer =
94-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
96+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
9597

9698
Set<String> instances = discoverer.getInstances();
9799

@@ -110,7 +112,7 @@ public void testFunctionReturnedEmptyList() {
110112
testHelper.executeLua(functionCode);
111113

112114
TarantoolClusterStoredFunctionDiscoverer discoverer =
113-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
115+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
114116

115117
Set<String> instances = discoverer.getInstances();
116118

@@ -124,7 +126,7 @@ public void testWrongFunctionName() {
124126
clusterConfig.clusterDiscoveryEntryFunction = "wrongFunction";
125127

126128
TarantoolClusterStoredFunctionDiscoverer discoverer =
127-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
129+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
128130

129131
assertThrows(TarantoolException.class, discoverer::getInstances);
130132
}
@@ -136,7 +138,7 @@ public void testWrongInstanceAddress() {
136138

137139
client.close();
138140
TarantoolClusterStoredFunctionDiscoverer discoverer =
139-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
141+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
140142

141143
assertThrows(CommunicationException.class, discoverer::getInstances);
142144
}
@@ -148,7 +150,7 @@ public void testWrongTypeResultData() {
148150
testHelper.executeLua(functionCode);
149151

150152
TarantoolClusterStoredFunctionDiscoverer discoverer =
151-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
153+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
152154

153155
assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances);
154156
}
@@ -157,7 +159,7 @@ public void testWrongTypeResultData() {
157159
@DisplayName("fetched with an exception when a single string returned")
158160
public void testSingleStringResultData() {
159161
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host1:3301'");
160-
control.openConsole(INSTANCE_NAME).exec(functionCode);
162+
testHelper.executeLua(functionCode);
161163

162164
TarantoolClusterStoredFunctionDiscoverer discoverer =
163165
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
@@ -184,7 +186,7 @@ public void testWrongMultiResultData() {
184186
testHelper.executeLua(functionCode);
185187

186188
TarantoolClusterStoredFunctionDiscoverer discoverer =
187-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
189+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
188190

189191
Set<String> instances = discoverer.getInstances();
190192

@@ -200,9 +202,68 @@ public void testFunctionWithError() {
200202
testHelper.executeLua(functionCode);
201203

202204
TarantoolClusterStoredFunctionDiscoverer discoverer =
203-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
205+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
204206

205207
assertThrows(TarantoolException.class, discoverer::getInstances);
206208
}
207209

210+
@Test
211+
@DisplayName("fetched a subset of valid addresses")
212+
public void testFilterBadAddressesData() {
213+
final List<String> allHosts = Arrays.asList(
214+
"host1:3313",
215+
"host:abc",
216+
"192.168.33.90",
217+
"myHost",
218+
"10.30.10.4:7814",
219+
"host:311:sub-host",
220+
"instance-2:",
221+
"host:0",
222+
"host:321981"
223+
);
224+
225+
final Set<String> expectedFiltered = new HashSet<>(
226+
Arrays.asList(
227+
"host1:3313",
228+
"192.168.33.90",
229+
"myHost",
230+
"10.30.10.4:7814"
231+
)
232+
);
233+
234+
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts);
235+
testHelper.executeLua(functionCode);
236+
237+
TarantoolClusterStoredFunctionDiscoverer discoverer =
238+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
239+
240+
Set<String> instances = discoverer.getInstances();
241+
242+
assertNotNull(instances);
243+
assertFalse(instances.isEmpty());
244+
assertEquals(expectedFiltered, instances);
245+
}
246+
247+
@Test
248+
@DisplayName("fetched an empty set after filtration")
249+
public void testFullBrokenAddressesList() {
250+
List<String> allHosts = Arrays.asList(
251+
"abc:edf",
252+
"192.168.33.90:",
253+
"host:-123",
254+
"host:0"
255+
);
256+
257+
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts);
258+
testHelper.executeLua(functionCode);
259+
260+
TarantoolClusterStoredFunctionDiscoverer discoverer =
261+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
262+
263+
Set<String> instances = discoverer.getInstances();
264+
265+
assertNotNull(instances);
266+
assertTrue(instances.isEmpty());
267+
}
268+
208269
}

0 commit comments

Comments
 (0)