diff --git a/src/main/java/org/opensearch/timeseries/constant/CommonMessages.java b/src/main/java/org/opensearch/timeseries/constant/CommonMessages.java index 16aa3fda2..8ffed43b7 100644 --- a/src/main/java/org/opensearch/timeseries/constant/CommonMessages.java +++ b/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"; diff --git a/src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java b/src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java index deb7fed0a..44e29f6ae 100644 --- a/src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java +++ b/src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java @@ -377,15 +377,11 @@ protected void validateTimeField(boolean indexingDryRun, ActionListener liste } multiGetMappingResponseListener - .onResponse( - new MergeableList>( - new ArrayList>(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( diff --git a/src/main/java/org/opensearch/timeseries/util/CrossClusterConfigUtils.java b/src/main/java/org/opensearch/timeseries/util/CrossClusterConfigUtils.java index 72cf2f400..61c97a814 100644 --- a/src/main/java/org/opensearch/timeseries/util/CrossClusterConfigUtils.java +++ b/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> separateClusterIndexes(List public static HashMap> separateClusterIndexes(List indexes, String localClusterName) { HashMap> 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 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> separateClusterIndexes(List } /** + * 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 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(':')) : ""; - } } diff --git a/src/main/java/org/opensearch/timeseries/util/MultiResponsesDelegateActionListener.java b/src/main/java/org/opensearch/timeseries/util/MultiResponsesDelegateActionListener.java index c2c1d7e19..7dd830435 100644 --- a/src/main/java/org/opensearch/timeseries/util/MultiResponsesDelegateActionListener.java +++ b/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; diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateForecasterActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateForecasterActionHandlerTests.java index e179a326f..10077f462 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateForecasterActionHandlerTests.java +++ b/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 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)); diff --git a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java index 95cd0917a..d53b68011 100644 --- a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/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 responseMap = entityAsMap(resp); + + @SuppressWarnings("unchecked") + Map> messageMap = (Map>) 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 responseMap = entityAsMap(resp); + assertEquals("no issue, empty response body", new HashMap(), responseMap); + } + public void testValidateAnomalyDetectorWithIncorrectShingleSize() throws Exception { TestHelpers.createIndex(client(), "test-index", TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); Response resp = TestHelpers diff --git a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java b/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java index 041c1c512..53f6f0ab5 100644 --- a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java +++ b/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") ); } diff --git a/src/test/java/org/opensearch/timeseries/util/CrossClusterConfigUtilsTests.java b/src/test/java/org/opensearch/timeseries/util/CrossClusterConfigUtilsTests.java index 19fe6ffd6..dfc46c6d8 100644 --- a/src/test/java/org/opensearch/timeseries/util/CrossClusterConfigUtilsTests.java +++ b/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 indexes = Arrays.asList("remoteCluster:index1", "index2"); + List indexes = Arrays.asList("remoteCluster:index1", "index2", "remoteCluster2:index2"); ClusterService mockClusterService = mock(ClusterService.class); when(mockClusterService.getClusterName()).thenReturn(new ClusterName("localCluster")); HashMap> 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 result = CrossClusterConfigUtils.parseClusterAndIndexName(input); + assertEquals("clusterA", result.getKey()); + assertEquals("index1", result.getValue()); } }