Filter wrong formatted discovery addresses

nicktorwald · nicktorwald · commit a911a05f737d · 2019-06-03T23:52:37.000+07:00
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 this broken address and carries on processing. Change discovery function requirements description in part of single value support. This is done to be consistent with other Tarantool connectors. Closes: #195, #196
diff --git a/README.md b/README.md
@@ -237,8 +237,11 @@ 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 _should_ return a single result of strings (i.e. single 
-  string `return 'host:3301'` or array of strings `return {'host1:3301', 'host2:3301'}`).
+* 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
@@ -271,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. 
diff --git a/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java b/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,54 @@ 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 (!((List<Object>)result.get(0)).stream().allMatch(item -> item instanceof String)) {
-            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 < 65535);
+            } catch (NumberFormatException e) {
+                return false;
+            }
         }
+        return true;
     }
 
 }
diff --git a/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java b/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;
 
@@ -70,7 +72,7 @@ public void testSuccessfulAddressParsing() {
         control.openConsole(INSTANCE_NAME).exec(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -89,7 +91,7 @@ public void testSuccessfulUniqueAddressParsing() {
         control.openConsole(INSTANCE_NAME).exec(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -108,7 +110,7 @@ public void testFunctionReturnedEmptyList() {
         control.openConsole(INSTANCE_NAME).exec(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -122,7 +124,7 @@ public void testWrongFunctionName() {
         clusterConfig.clusterDiscoveryEntryFunction = "wrongFunction";
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(TarantoolException.class, discoverer::getInstances);
     }
@@ -134,21 +136,24 @@ public void testWrongInstanceAddress() {
 
         client.close();
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         assertThrows(CommunicationException.class, discoverer::getInstances);
     }
 
     @Test
-    @DisplayName("fetched with an exception when wrong data type returned")
+    @DisplayName("fetched an empty list when wrong data type returned")
     public void testWrongTypeResultData() {
         String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, 42);
         control.openConsole(INSTANCE_NAME).exec(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
-        assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances);
+        Set<String> instances = discoverer.getInstances();
+
+        assertNotNull(instances);
+        assertTrue(instances.isEmpty());
     }
 
     @Test
@@ -170,7 +175,7 @@ public void testWrongMultiResultData() {
         control.openConsole(INSTANCE_NAME).exec(functionCode);
 
         TarantoolClusterStoredFunctionDiscoverer discoverer =
-                new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
 
         Set<String> instances = discoverer.getInstances();
 
@@ -186,9 +191,68 @@ public void testFunctionWithError() {
         control.openConsole(INSTANCE_NAME).exec(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);
+        control.openConsole(INSTANCE_NAME).exec(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);
+        control.openConsole(INSTANCE_NAME).exec(functionCode);
+
+        TarantoolClusterStoredFunctionDiscoverer discoverer =
+            new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client);
+
+        Set<String> instances = discoverer.getInstances();
+
+        assertNotNull(instances);
+        assertTrue(instances.isEmpty());
+    }
+
 }
