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

README.md

@@ -238,6 +238,10 @@ tarantool> function get_cluster_nodes() return { 'host1:3301', 'host2:3302', 'ho
 You need to pay attention to a function contract we are currently supporting:
 * The client never passes any arguments to a discovery function.
 * A discovery function _must_ return an array of strings (i.e `return {'host1:3301', 'host2:3301'}`).
+* Each string _should_ satisfy the following pattern `host[:port]`
+  (or more strictly `/^[^:]+(:\d)?$/` - a mandatory host containing any string
+  and an optional colon followed by digits of the port). Also, the port must be
+  in a range between 1 and 65535 if one is presented.
 * A discovery function _may_ return multi-results but the client takes
   into account only first of them (i.e. `return {'host:3301'}, discovery_delay`, where 
   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));
 * A discovery task always uses an active client connection to get the nodes list.
   It's in your responsibility to provide a function availability as well as a consistent
   nodes list on all instances you initially set or obtain from the task.
+* Every address which is unmatched with `host[:port]` pattern will be filtered out from
+  the target addresses list.
 * If some error occurs while a discovery task is running then this task
   will be aborted without any after-effects for next task executions. These cases, for instance, are 
   a wrong function result (see discovery function contract) or a broken connection. 

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

@@ -3,6 +3,7 @@
 import org.tarantool.TarantoolClient;
 import org.tarantool.TarantoolClientOps;
 import org.tarantool.TarantoolClusterClientConfig;
+import org.tarantool.util.StringUtils;
 
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -36,28 +37,57 @@ public Set<String> getInstances() {
         // validation against the data returned;
         // this strict-mode allows us to extend the contract in a non-breaking
         // way for old clients just reserve an extra return value in
-        // terms of LUA multi-result support.
-        checkResult(list);
-
-        List<Object> funcResult = (List<Object>) list.get(0);
-        return funcResult.stream()
-                .map(Object::toString)
-                .collect(Collectors.toCollection(LinkedHashSet::new));
+        // terms of Lua multi-result support.;
+        return checkAndFilterAddresses(list);
     }
 
     /**
      * Check whether the result follows the contract or not.
-     * The contract is a mandatory <b>single array of strings</b>
+     * The contract is a mandatory <b>single array of strings</b>.
+     *
+     * The simplified format for each string is host[:port].
      *
      * @param result result to be validated
      */
-    private void checkResult(List<?> result) {
+    private Set<String> checkAndFilterAddresses(List<?> result) {
         if (result == null || result.isEmpty()) {
             throw new IllegalDiscoveryFunctionResult("Discovery function returned no data");
         }
         if (!(result.get(0) instanceof List)) {
             throw new IllegalDiscoveryFunctionResult("The first value must be an array of strings");
         }
+
+        return ((List<Object>) result.get(0)).stream()
+            .filter(item -> item instanceof String)
+            .map(Object::toString)
+            .filter(s -> !StringUtils.isBlank(s))
+            .filter(this::isAddress)
+            .collect(Collectors.toCollection(LinkedHashSet::new));
+    }
+
+    /**
+     * Checks that address matches host[:port] format.
+     *
+     * @param address to be checked
+     * @return true if address follows the format
+     */
+    private boolean isAddress(String address) {
+        if (address.endsWith(":")) {
+            return false;
+        }
+        String[] addressParts = address.split(":");
+        if (addressParts.length > 2) {
+            return false;
+        }
+        if (addressParts.length == 2) {
+            try {
+                int port = Integer.parseInt(addressParts[1]);
+                return (port > 0 && port < 65536);
+            } catch (NumberFormatException e) {
+                return false;
+            }
+        }
+        return true;
     }
 
 }

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

@@ -180,8 +180,7 @@ void testCall(AsyncOpsProvider provider) throws ExecutionException, InterruptedE
         testHelper.executeLua("function echo(...) return ... end");
 
         Future<List<?>> fut = provider.getAsyncOps().call("echo", "hello");
-        assertEquals(Collections.singletonList(Collections.singletonList("hello")),
-            fut.get(TIMEOUT, TimeUnit.MILLISECONDS));
+        assertEquals(Collections.singletonList("hello"), fut.get(TIMEOUT, TimeUnit.MILLISECONDS));
 
         provider.close();
     }

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

@@ -489,7 +489,7 @@ public void testEval(SyncOpsProvider provider) {
     @MethodSource("getClientOps")
     public void testCall(SyncOpsProvider provider) {
         assertEquals(
-            Collections.singletonList(Collections.singletonList("true")),
+            Collections.singletonList("true"),
             provider.getClientOps().call("echo", "true")
         );
 

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

@@ -1,6 +1,7 @@
 package org.tarantool.cluster;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -23,6 +24,7 @@
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -72,7 +74,7 @@ public void testSuccessfulAddressParsing() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -91,7 +93,7 @@ public void testSuccessfulUniqueAddressParsing() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -110,7 +112,7 @@ public void testFunctionReturnedEmptyList() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -124,7 +126,7 @@ public void testWrongFunctionName() {
         clusterConfig.clusterDiscoveryEntryFunction = "wrongFunction";
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(TarantoolException.class, discoverer::getInstances);
     }
@@ -136,7 +138,7 @@ public void testWrongInstanceAddress() {
 
         client.close();
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(CommunicationException.class, discoverer::getInstances);
     }
@@ -148,7 +150,7 @@ public void testWrongTypeResultData() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances);
     }
@@ -157,7 +159,7 @@ public void testWrongTypeResultData() {
     @DisplayName("fetched with an exception when a single string returned")
     public void testSingleStringResultData() {
         String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host1:3301'");
-        control.openConsole(INSTANCE_NAME).exec(functionCode);
+        testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
             new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
@@ -184,7 +186,7 @@ public void testWrongMultiResultData() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -200,9 +202,68 @@ public void testFunctionWithError() {
         testHelper.executeLua(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(TarantoolException.class, discoverer::getInstances);
     }
 
+    @Test
+    @DisplayName("fetched a subset of valid addresses")
+    public void testFilterBadAddressesData() {
+        final List<String> allHosts = Arrays.asList(
+            "host1:3313",
+            "host:abc",
+            "192.168.33.90",
+            "myHost",
+            "10.30.10.4:7814",
+            "host:311:sub-host",
+            "instance-2:",
+            "host:0",
+            "host:321981"
+        );
+
+        final Set<String> expectedFiltered = new HashSet<>(
+            Arrays.asList(
+                "host1:3313",
+                "192.168.33.90",
+                "myHost",
+                "10.30.10.4:7814"
+            )
+        );
+
+        String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts);
+        testHelper.executeLua(functionCode);
+
+        TarantoolClusterStoredFunctionDiscoverer discoverer =
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+
+        Set<String> instances = discoverer.getInstances();
+
+        assertNotNull(instances);
+        assertFalse(instances.isEmpty());
+        assertEquals(expectedFiltered, instances);
+    }
+
+    @Test
+    @DisplayName("fetched an empty set after filtration")
+    public void testFullBrokenAddressesList() {
+        List<String> allHosts = Arrays.asList(
+            "abc:edf",
+            "192.168.33.90:",
+            "host:-123",
+            "host:0"
+        );
+
+        String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts);
+        testHelper.executeLua(functionCode);
+
+        TarantoolClusterStoredFunctionDiscoverer discoverer =
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+
+        Set<String> instances = discoverer.getInstances();
+
+        assertNotNull(instances);
+        assertTrue(instances.isEmpty());
+    }
+
 }