adding more tests

Signed-off-by: Amit Galitzky <[email protected]>

src/main/java/org/opensearch/timeseries/constant/CommonMessages.java

@@ -76,7 +76,7 @@ public static String getTooManyCategoricalFieldErr(int limit) {
     public static final String INDEX_NOT_FOUND = "index does not exist";
     public static final String FAIL_TO_GET_MAPPING_MSG = "Fail to get the index mapping of %s";
     public static final String FAIL_TO_GET_MAPPING = "Fail to get the index mapping";
-    public static final String TIMESTAMP_VALIDATION_FAILED = "Validation failed for timefield of %s ";
+    public static final String TIMESTAMP_VALIDATION_FAILED = "Validation failed for timefield of %s, ";
 
     public static final String FAIL_TO_GET_CONFIG_MSG = "Fail to get config";
 

src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java

@@ -377,15 +377,11 @@ protected void validateTimeField(boolean indexingDryRun, ActionListener<T> liste
                 }
 
                 multiGetMappingResponseListener
-                    .onResponse(
-                        new MergeableList<Optional<double[]>>(
-                            new ArrayList<Optional<double[]>>(Collections.singletonList(Optional.empty()))
-                        )
-                    );
+                    .onResponse(new MergeableList<>(new ArrayList<>(Collections.singletonList(Optional.empty()))));
             }, e -> {
                 String errorMessage = String.format(Locale.ROOT, "Fail to get the index mapping of %s", clusterIndicesEntry.getValue());
                 logger.error(errorMessage, e);
-                multiGetMappingResponseListener.onFailure(new OpenSearchStatusException(errorMessage, RestStatus.BAD_REQUEST, e));
+                multiGetMappingResponseListener.onFailure(new IllegalArgumentException(errorMessage, e));
             });
             clientUtil
                 .executeWithInjectedSecurity(

src/main/java/org/opensearch/timeseries/util/CrossClusterConfigUtils.java

@@ -9,6 +9,7 @@
 import java.util.HashMap;
 import java.util.List;
 
+import org.apache.commons.lang3.tuple.Pair;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.client.Client;
@@ -39,9 +40,6 @@ public static Client getClientForCluster(String clusterName, Client client, Stri
      * @return The local {@link NodeClient} for the local cluster, or a remote client for a remote cluster.
      */
     public static Client getClientForCluster(String clusterName, Client client, ClusterService clusterService) {
-        logger.info("clusterName1: " + clusterName);
-        logger.info("clusterService.getClusterName().value(): " + clusterService.getClusterName().value());
-
         return getClientForCluster(clusterName, client, clusterService.getClusterName().value());
     }
 
@@ -65,8 +63,12 @@ public static HashMap<String, List<String>> separateClusterIndexes(List<String>
     public static HashMap<String, List<String>> separateClusterIndexes(List<String> indexes, String localClusterName) {
         HashMap<String, List<String>> output = new HashMap<>();
         for (String index : indexes) {
-            String clusterName = parseClusterName(index);
-            String indexName = parseIndexName(index);
+            // Use the refactored method to get both cluster and index names in one call
+            Pair<String, String> clusterAndIndex = parseClusterAndIndexName(index);
+            String clusterName = clusterAndIndex.getKey();
+            String indexName = clusterAndIndex.getValue();
+            logger.info("clusterName: " + clusterName);
+            logger.info("indexName: " + indexName);
 
             // If the index entry does not have a cluster_name, it indicates the index is on the local cluster.
             if (clusterName.isEmpty()) {
@@ -78,25 +80,19 @@ public static HashMap<String, List<String>> separateClusterIndexes(List<String>
     }
 
     /**
+     * Parses the cluster and index names from the given input string.
+     * The input can be in either "cluster_name:index_name" format or just "index_name".
      * @param index The name of the index to evaluate.
-     *      Can be in either cluster_name:index_name or index_name format.
-     * @return The index name.
+     * @return A Pair where the left is the cluster name (or empty if not present), and the right is the index name.
      */
-    public static String parseIndexName(String index) {
+    public static Pair<String, String> parseClusterAndIndexName(String index) {
         if (index.contains(":")) {
-            String[] parts = index.split(":");
-            return parts.length > 1 ? parts[1] : index;
+            String[] parts = index.split(":", 2);
+            String clusterName = parts[0];
+            String indexName = parts.length > 1 ? parts[1] : "";
+            return Pair.of(clusterName, indexName);
         } else {
-            return index;
+            return Pair.of("", index);
         }
     }
-
-    /**
-     * @param index The name of the index to evaluate.
-     *      Can be in either cluster_name:index_name or index_name format.
-     * @return The index name.
-     */
-    public static String parseClusterName(String index) {
-        return index.contains(":") ? index.substring(0, index.indexOf(':')) : "";
-    }
 }

src/main/java/org/opensearch/timeseries/util/MultiResponsesDelegateActionListener.java

@@ -11,6 +11,8 @@
 
 package org.opensearch.timeseries.util;
 
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.concurrent.atomic.AtomicInteger;

src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateForecasterActionHandlerTests.java

@@ -22,6 +22,7 @@
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.core.action.ActionResponse;
 import org.opensearch.forecast.rest.handler.ValidateForecasterActionHandler;
+import org.opensearch.timeseries.common.exception.ValidationException;
 import org.opensearch.timeseries.model.ValidationAspect;
 
 public class ValidateForecasterActionHandlerTests extends AbstractForecasterActionHandlerTestCase {
@@ -100,7 +101,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
             assertTrue("should not reach here", false);
             inProgressLatch.countDown();
         }, e -> {
-            assertTrue(e instanceof IllegalArgumentException);
+            assertTrue(e instanceof ValidationException);
             inProgressLatch.countDown();
         }));
         assertTrue(inProgressLatch.await(100, TimeUnit.SECONDS));

src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java

@@ -1377,6 +1377,73 @@ public void testValidateAnomalyDetectorWithNoTimeField() throws Exception {
         assertEquals("time field missing", CommonMessages.NULL_TIME_FIELD, messageMap.get("time_field").get("message"));
     }
 
+    public void testValidateAnomalyDetectorWithMultipleIndicesOneNotFound() throws Exception {
+        TestHelpers.createIndex(client(), "test-index", TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}"));
+        Response resp = TestHelpers
+            .makeRequest(
+                client(),
+                "POST",
+                TestHelpers.AD_BASE_DETECTORS_URI + "/_validate",
+                ImmutableMap.of(),
+                TestHelpers
+                    .toHttpEntity(
+                        "{\"name\":\""
+                            + "test-detector"
+                            + "\",\"description\":\"Test detector\",\"time_field\":\"timestamp\","
+                            + "\"indices\":[\"test-index\", \"test-index-2\"],\"feature_attributes\":[{\"feature_name\":\"cpu-sum\",\""
+                            + "feature_enabled\":true,\"aggregation_query\":{\"total_cpu\":{\"sum\":{\"field\":\"cpu\"}}}},"
+                            + "{\"feature_name\":\"error-sum\",\"feature_enabled\":true,\"aggregation_query\":"
+                            + "{\"total_error\":"
+                            + "{\"sum\":{\"field\":\"error\"}}}}],\"filter_query\":{\"bool\":{\"filter\":[{\"exists\":"
+                            + "{\"field\":"
+                            + "\"cpu\",\"boost\":1}}],\"adjust_pure_negative\":true,\"boost\":1}},\"detection_interval\":"
+                            + "{\"period\":{\"interval\":1,\"unit\":\"Minutes\"}},"
+                            + "\"window_delay\":{\"period\":{\"interval\":2,\"unit\":\"Minutes\"}},"
+                            + "\"shingle_size\": 8}"
+                    ),
+                null
+            );
+        Map<String, Object> responseMap = entityAsMap(resp);
+
+        @SuppressWarnings("unchecked")
+        Map<String, Map<String, String>> messageMap = (Map<String, Map<String, String>>) XContentMapValues
+            .extractValue("detector", responseMap);
+        String errorMessage = "index does not exist";
+        assertEquals("index does not exist", errorMessage, messageMap.get("indices").get("message"));
+    }
+
+    public void testValidateAnomalyDetectorWithMultipleIndices() throws Exception {
+        TestHelpers.createIndexWithTimeField(client(), "test-index", TIME_FIELD);
+        TestHelpers.createIndexWithTimeField(client(), "test-index-2", TIME_FIELD);
+
+        Response resp = TestHelpers
+            .makeRequest(
+                client(),
+                "POST",
+                TestHelpers.AD_BASE_DETECTORS_URI + "/_validate",
+                ImmutableMap.of(),
+                TestHelpers
+                    .toHttpEntity(
+                        "{\"name\":\""
+                            + "test-detector"
+                            + "\",\"description\":\"Test detector\",\"time_field\":\"timestamp\","
+                            + "\"indices\":[\"test-index\", \"test-index-2\"],\"feature_attributes\":[{\"feature_name\":\"cpu-sum\",\""
+                            + "feature_enabled\":true,\"aggregation_query\":{\"total_cpu\":{\"sum\":{\"field\":\"cpu\"}}}},"
+                            + "{\"feature_name\":\"error-sum\",\"feature_enabled\":true,\"aggregation_query\":"
+                            + "{\"total_error\":"
+                            + "{\"sum\":{\"field\":\"error\"}}}}],\"filter_query\":{\"bool\":{\"filter\":[{\"exists\":"
+                            + "{\"field\":"
+                            + "\"cpu\",\"boost\":1}}],\"adjust_pure_negative\":true,\"boost\":1}},\"detection_interval\":"
+                            + "{\"period\":{\"interval\":1,\"unit\":\"Minutes\"}},"
+                            + "\"window_delay\":{\"period\":{\"interval\":2,\"unit\":\"Minutes\"}},"
+                            + "\"shingle_size\": 8}"
+                    ),
+                null
+            );
+        Map<String, Object> responseMap = entityAsMap(resp);
+        assertEquals("no issue, empty response body", new HashMap<String, Object>(), responseMap);
+    }
+
     public void testValidateAnomalyDetectorWithIncorrectShingleSize() throws Exception {
         TestHelpers.createIndex(client(), "test-index", TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}"));
         Response resp = TestHelpers

src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java

@@ -491,10 +491,7 @@ public void testValidateAnomalyDetectorWithNonExistentTimefield() throws IOExcep
         assertEquals(ValidationIssueType.TIMEFIELD_FIELD, response.getIssue().getType());
         assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect());
         assertTrue(
-            response
-                .getIssue()
-                .getMessage()
-                .contains(String.format(Locale.ROOT, CommonMessages.INVALID_TIMESTAMP, anomalyDetector.getTimeField()))
+            response.getIssue().getMessage().contains("Timestamp field: (" + anomalyDetector.getTimeField() + ") is not found in the")
         );
     }
 

src/test/java/org/opensearch/timeseries/util/CrossClusterConfigUtilsTests.java

@@ -1,14 +1,23 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
 package org.opensearch.timeseries.util;
 
-import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 
-import org.mockito.Mock;
+import org.apache.commons.lang3.tuple.Pair;
+import org.junit.Before;
 import org.opensearch.client.Client;
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.cluster.ClusterName;
@@ -17,50 +26,50 @@
 
 public class CrossClusterConfigUtilsTests extends OpenSearchTestCase {
 
-    @Mock
-    private Client clientMock;
-
-    public void testGetClientForClusterLocalCluster() {
-        String clusterName = "localCluster";
-        Client mockClient = mock(NodeClient.class);
-        String localClusterName = "localCluster";
+    private Client mockClient;
+    private String remoteClusterName;
+    private String localClusterName;
 
-        Client result = CrossClusterConfigUtils.getClientForCluster(clusterName, mockClient, localClusterName);
+    @Before
+    public void setup() {
+        // Initialize the mock clients
+        mockClient = mock(NodeClient.class);
+        localClusterName = "localCluster";
+        remoteClusterName = "remoteCluster";
+    }
 
+    public void testGetClientForClusterLocalCluster() {
+        Client result = CrossClusterConfigUtils.getClientForCluster(localClusterName, mockClient, localClusterName);
         assertEquals(mockClient, result);
+        verify(mockClient, never()).getRemoteClusterClient(anyString());
     }
 
     public void testGetClientForClusterRemoteCluster() {
-        String clusterName = "remoteCluster";
         Client mockClient = mock(NodeClient.class);
-        // Client mockRemoteClient = mock(Client.class);
+        CrossClusterConfigUtils.getClientForCluster(remoteClusterName, mockClient, localClusterName);
 
-        when(mockClient.getRemoteClusterClient(clusterName)).thenReturn(mockClient);
-
-        Client result = CrossClusterConfigUtils.getClientForCluster(clusterName, mockClient, "localCluster");
-
-        assertEquals(mockClient, result);
+        // Verify that getRemoteClusterClient was called once with the correct cluster name
+        verify(mockClient, times(1)).getRemoteClusterClient("remoteCluster");
+        when(mockClient.getRemoteClusterClient(remoteClusterName)).thenReturn(mockClient);
     }
 
     public void testSeparateClusterIndexesRemoteCluster() {
-        List<String> indexes = Arrays.asList("remoteCluster:index1", "index2");
+        List<String> indexes = Arrays.asList("remoteCluster:index1", "index2", "remoteCluster2:index2");
         ClusterService mockClusterService = mock(ClusterService.class);
         when(mockClusterService.getClusterName()).thenReturn(new ClusterName("localCluster"));
 
         HashMap<String, List<String>> result = CrossClusterConfigUtils.separateClusterIndexes(indexes, mockClusterService);
 
-        assertEquals(2, result.size());
+        assertEquals(3, result.size());
         assertEquals(Arrays.asList("index1"), result.get("remoteCluster"));
         assertEquals(Arrays.asList("index2"), result.get("localCluster"));
+        assertEquals(Arrays.asList("index2"), result.get("remoteCluster2"));
     }
 
-    public void testParseIndexName() {
-        assertEquals("index1", CrossClusterConfigUtils.parseIndexName("remoteCluster:index1"));
-        assertEquals("index2", CrossClusterConfigUtils.parseIndexName("index2"));
-    }
-
-    public void testParseClusterName() {
-        assertEquals("remoteCluster", CrossClusterConfigUtils.parseClusterName("remoteCluster:index1"));
-        assertEquals("", CrossClusterConfigUtils.parseClusterName("index2"));
+    public void testParseClusterAndIndexName_WithClusterAndIndex() {
+        String input = "clusterA:index1";
+        Pair<String, String> result = CrossClusterConfigUtils.parseClusterAndIndexName(input);
+        assertEquals("clusterA", result.getKey());
+        assertEquals("index1", result.getValue());
     }
 }