Skip to content

Commit ff10903

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. Discard a single string return type for a cluster discovery function. Change discovery function requirements in part of single value support. This is done to be consistent with other Tarantool connectors. Closes: #195, #196
1 parent ae6432a commit ff10903

File tree

3 files changed

+129
-21
lines changed

3 files changed

+129
-21
lines changed

‎README.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,11 @@ tarantool> function get_cluster_nodes() return { 'host1:3301', 'host2:3302', 'ho
237237
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.
240-
* A discovery function _should_ return a single result of strings (i.e. single
241-
string `return 'host:3301'` or array of strings `return {'host1:3301', 'host2:3301'}`).
240+
* 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.
242245
* A discovery function _may_ return multi-results but the client takes
243246
into account only first of them (i.e. `return {'host:3301'}, discovery_delay`, where
244247
the second result is unused). Even more, any extra results __are reserved__ by the client
@@ -271,6 +274,8 @@ client.syncOps().insert(45, Arrays.asList(1, 1));
271274
* A discovery task always uses an active client connection to get the nodes list.
272275
It's in your responsibility to provide a function availability as well as a consistent
273276
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.
274279
* If some error occurs while a discovery task is running then this task
275280
will be aborted without any after-effects for next task executions. These cases, for instance, are
276281
a wrong function result (see discovery function contract) or a broken connection.

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

Lines changed: 40 additions & 10 deletions
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
}
58-
if (!((List<Object>)result.get(0)).stream().allMatch(item -> item instanceof String)) {
56+
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 < 65535);
86+
} catch (NumberFormatException e) {
87+
return false;
88+
}
89+
}
90+
return true;
6191
}
6292

6393
}

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

Lines changed: 82 additions & 9 deletions
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

@@ -70,7 +72,7 @@ public void testSuccessfulAddressParsing() {
7072
control.openConsole(INSTANCE_NAME).exec(functionCode);
7173

7274
TarantoolClusterStoredFunctionDiscoverer discoverer =
73-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
75+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
7476

7577
Set<String> instances = discoverer.getInstances();
7678

@@ -89,7 +91,7 @@ public void testSuccessfulUniqueAddressParsing() {
8991
control.openConsole(INSTANCE_NAME).exec(functionCode);
9092

9193
TarantoolClusterStoredFunctionDiscoverer discoverer =
92-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
94+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
9395

9496
Set<String> instances = discoverer.getInstances();
9597

@@ -108,7 +110,7 @@ public void testFunctionReturnedEmptyList() {
108110
control.openConsole(INSTANCE_NAME).exec(functionCode);
109111

110112
TarantoolClusterStoredFunctionDiscoverer discoverer =
111-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
113+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
112114

113115
Set<String> instances = discoverer.getInstances();
114116

@@ -122,7 +124,7 @@ public void testWrongFunctionName() {
122124
clusterConfig.clusterDiscoveryEntryFunction = "wrongFunction";
123125

124126
TarantoolClusterStoredFunctionDiscoverer discoverer =
125-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
127+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
126128

127129
assertThrows(TarantoolException.class, discoverer::getInstances);
128130
}
@@ -134,7 +136,7 @@ public void testWrongInstanceAddress() {
134136

135137
client.close();
136138
TarantoolClusterStoredFunctionDiscoverer discoverer =
137-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
139+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
138140

139141
assertThrows(CommunicationException.class, discoverer::getInstances);
140142
}
@@ -146,15 +148,27 @@ public void testWrongTypeResultData() {
146148
control.openConsole(INSTANCE_NAME).exec(functionCode);
147149

148150
TarantoolClusterStoredFunctionDiscoverer discoverer =
149-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
151+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
152+
153+
assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances);
154+
}
155+
156+
@Test
157+
@DisplayName("fetched with an exception when a single string returned")
158+
public void testSingleStringResultData() {
159+
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host1:3301'");
160+
control.openConsole(INSTANCE_NAME).exec(functionCode);
161+
162+
TarantoolClusterStoredFunctionDiscoverer discoverer =
163+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
150164

151165
assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances);
152166
}
153167

154168
@Test
155169
@DisplayName("fetched with an exception using no return function")
156170
public void testFunctionWithNoReturn() {
157-
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "");
171+
String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host'");
158172
control.openConsole(INSTANCE_NAME).exec(functionCode);
159173

160174
TarantoolClusterStoredFunctionDiscoverer discoverer =
@@ -170,7 +184,7 @@ public void testWrongMultiResultData() {
170184
control.openConsole(INSTANCE_NAME).exec(functionCode);
171185

172186
TarantoolClusterStoredFunctionDiscoverer discoverer =
173-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
187+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
174188

175189
Set<String> instances = discoverer.getInstances();
176190

@@ -186,9 +200,68 @@ public void testFunctionWithError() {
186200
control.openConsole(INSTANCE_NAME).exec(functionCode);
187201

188202
TarantoolClusterStoredFunctionDiscoverer discoverer =
189-
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
203+
new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
190204

191205
assertThrows(TarantoolException.class, discoverer::getInstances);
192206
}
193207

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

0 commit comments

Comments
 (0)