From 49f30a23d690ff22f6df902f3969f7b6047a496f Mon Sep 17 00:00:00 2001 From: Yizhe Liu Date: Thu, 21 May 2020 22:16:36 -0700 Subject: [PATCH 1/3] Prevent creating detector with duplicate name. Issue:#118 --- .../IndexAnomalyDetectorActionHandler.java | 41 ++++++++++ .../ad/rest/AnomalyDetectorRestApiIT.java | 76 +++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java index 5a3b98af..200222bf 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java @@ -44,6 +44,7 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestResponseListener; +import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.builder.SearchSourceBuilder; import java.io.IOException; @@ -221,6 +222,46 @@ private void onSearchAdInputIndicesResponse(SearchResponse response, String dete + Arrays.toString(anomalyDetector.getIndices().toArray(new String[0])); logger.error(errorMsg); onFailure(new IllegalArgumentException(errorMsg)); + } else { + checkADNameExists(detectorId); + } + } + + private void checkADNameExists(String detectorId) throws IOException { + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + // src/main/resources/mappings/anomaly-detectors.json#L14 + .query(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName())) + .timeout(requestTimeout); + SearchRequest searchRequest = new SearchRequest(ANOMALY_DETECTORS_INDEX).source(searchSourceBuilder); + + client + .search( + searchRequest, + ActionListener + .wrap( + searchResponse -> onSearchADNameResponse(searchResponse, detectorId, anomalyDetector.getName()), + exception -> onFailure(exception) + ) + ); + } + + private void onSearchADNameResponse(SearchResponse response, String detectorId, String name) throws IOException { + boolean hasDuplicateName = false; + String existingDetectorId = null; + if (response.getHits().getTotalHits().value > 0) { + for (SearchHit hit : response.getHits()) { + if (!hit.getId().equals(detectorId)) { + hasDuplicateName = true; + existingDetectorId = hit.getId(); + break; + } + } + } + + if (hasDuplicateName) { + String errorMsg = String.format("Cannot create anomaly detector with name[%s] used by detectorId %s", name, existingDetectorId); + logger.error(errorMsg); + onFailure(new IllegalArgumentException(errorMsg)); } else { indexAnomalyDetector(detectorId); } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java index d52125e0..7e2cbda7 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java @@ -42,6 +42,10 @@ import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.AD_BASE_PREVIEW_URI; import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.randomAnomalyDetectorWithEmptyFeature; +import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.randomFeature; +import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.randomIntervalTimeConfiguration; +import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.randomQuery; +import static com.amazon.opendistroforelasticsearch.ad.TestHelpers.randomUiMetadata; import static org.hamcrest.Matchers.containsString; public class AnomalyDetectorRestApiIT extends AnomalyDetectorRestTestCase { @@ -78,6 +82,41 @@ public void testCreateAnomalyDetectorWithEmptyIndices() throws Exception { ); } + public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { + AnomalyDetector detector = createRandomAnomalyDetector(true, true); + + AnomalyDetector detectorDuplicateName = new AnomalyDetector( + AnomalyDetector.NO_ID, + randomLong(), + detector.getName(), + randomAlphaOfLength(5), + randomAlphaOfLength(5), + detector.getIndices(), + ImmutableList.of(randomFeature()), + randomQuery(), + randomIntervalTimeConfiguration(), + randomIntervalTimeConfiguration(), + randomUiMetadata(), + randomInt(), + null + ); + + TestHelpers + .assertFailWith( + ResponseException.class, + "Cannot create anomaly detector with name", + () -> TestHelpers + .makeRequest( + client(), + "POST", + TestHelpers.AD_BASE_DETECTORS_URI, + ImmutableMap.of(), + toHttpEntity(detectorDuplicateName), + null + ) + ); + } + public void testCreateAnomalyDetector() throws Exception { AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); String indexName = detector.getIndices().get(0); @@ -181,6 +220,43 @@ public void testUpdateAnomalyDetectorA() throws Exception { assertEquals("Anomaly detector description not updated", newDescription, updatedDetector.getDescription()); } + public void testUpdateAnomalyDetectorNameToExisting() throws Exception { + AnomalyDetector detector1 = createRandomAnomalyDetector(true, true); + + AnomalyDetector detector2 = createRandomAnomalyDetector(true, true); + + AnomalyDetector newDetector1WithDetector2Name = new AnomalyDetector( + detector1.getDetectorId(), + detector1.getVersion(), + detector2.getName(), + detector1.getDescription(), + detector1.getTimeField(), + detector1.getIndices(), + detector1.getFeatureAttributes(), + detector1.getFilterQuery(), + detector1.getDetectionInterval(), + detector1.getWindowDelay(), + detector1.getUiMetadata(), + detector1.getSchemaVersion(), + detector1.getLastUpdateTime() + ); + + TestHelpers + .assertFailWith( + ResponseException.class, + "Cannot create anomaly detector with name", + () -> TestHelpers + .makeRequest( + client(), + "POST", + TestHelpers.AD_BASE_DETECTORS_URI, + ImmutableMap.of(), + toHttpEntity(newDetector1WithDetector2Name), + null + ) + ); + } + public void testUpdateAnomalyDetectorWithNotExistingIndex() throws Exception { AnomalyDetector detector = createRandomAnomalyDetector(true, true); From 6f1c83c7bbd1f2537c3eaab7a38e09cd69de9e21 Mon Sep 17 00:00:00 2001 From: Yizhe Liu Date: Fri, 22 May 2020 14:22:22 -0700 Subject: [PATCH 2/3] Exclude detector with input detector id when searching detector with duplicate name --- .../IndexAnomalyDetectorActionHandler.java | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java index 200222bf..187ae89d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java @@ -36,6 +36,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.rest.BytesRestResponse; @@ -44,13 +45,13 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestResponseListener; -import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.builder.SearchSourceBuilder; import java.io.IOException; import java.time.Instant; import java.util.Arrays; import java.util.Locale; +import java.util.stream.Collectors; import static com.amazon.opendistroforelasticsearch.ad.model.AnomalyDetector.ANOMALY_DETECTORS_INDEX; import static com.amazon.opendistroforelasticsearch.ad.util.RestHandlerUtils.XCONTENT_WITH_TYPE; @@ -228,10 +229,12 @@ private void onSearchAdInputIndicesResponse(SearchResponse response, String dete } private void checkADNameExists(String detectorId) throws IOException { - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() - // src/main/resources/mappings/anomaly-detectors.json#L14 - .query(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName())) - .timeout(requestTimeout); + BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + // src/main/resources/mappings/anomaly-detectors.json#L14 + boolQueryBuilder.must(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName())); + // _id field does not allow "", but allows " " + boolQueryBuilder.mustNot(QueryBuilders.termQuery(RestHandlerUtils._ID, StringUtils.isBlank(detectorId) ? " " : detectorId)); + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().query(boolQueryBuilder).timeout(requestTimeout); SearchRequest searchRequest = new SearchRequest(ANOMALY_DETECTORS_INDEX).source(searchSourceBuilder); client @@ -246,21 +249,14 @@ private void checkADNameExists(String detectorId) throws IOException { } private void onSearchADNameResponse(SearchResponse response, String detectorId, String name) throws IOException { - boolean hasDuplicateName = false; - String existingDetectorId = null; if (response.getHits().getTotalHits().value > 0) { - for (SearchHit hit : response.getHits()) { - if (!hit.getId().equals(detectorId)) { - hasDuplicateName = true; - existingDetectorId = hit.getId(); - break; - } - } - } - - if (hasDuplicateName) { - String errorMsg = String.format("Cannot create anomaly detector with name[%s] used by detectorId %s", name, existingDetectorId); - logger.error(errorMsg); + String errorMsg = String + .format( + "Cannot create anomaly detector with name[%s] used by detectorId %s", + name, + Arrays.stream(response.getHits().getHits()).map(hit -> hit.getId()).collect(Collectors.toList()) + ); + logger.warn(errorMsg); onFailure(new IllegalArgumentException(errorMsg)); } else { indexAnomalyDetector(detectorId); From 53a264caabec7cb06c2c9b23f4cbbcc0fe8072d7 Mon Sep 17 00:00:00 2001 From: Yizhe Liu Date: Fri, 22 May 2020 15:57:05 -0700 Subject: [PATCH 3/3] Address Yaliang's comments and add 1 more test case --- .../IndexAnomalyDetectorActionHandler.java | 7 ++-- .../ad/rest/AnomalyDetectorRestApiIT.java | 39 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java index 187ae89d..b28db4ee 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java @@ -232,8 +232,9 @@ private void checkADNameExists(String detectorId) throws IOException { BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); // src/main/resources/mappings/anomaly-detectors.json#L14 boolQueryBuilder.must(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName())); - // _id field does not allow "", but allows " " - boolQueryBuilder.mustNot(QueryBuilders.termQuery(RestHandlerUtils._ID, StringUtils.isBlank(detectorId) ? " " : detectorId)); + if (StringUtils.isNotBlank(detectorId)) { + boolQueryBuilder.mustNot(QueryBuilders.termQuery(RestHandlerUtils._ID, detectorId)); + } SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().query(boolQueryBuilder).timeout(requestTimeout); SearchRequest searchRequest = new SearchRequest(ANOMALY_DETECTORS_INDEX).source(searchSourceBuilder); @@ -252,7 +253,7 @@ private void onSearchADNameResponse(SearchResponse response, String detectorId, if (response.getHits().getTotalHits().value > 0) { String errorMsg = String .format( - "Cannot create anomaly detector with name[%s] used by detectorId %s", + "Cannot create anomaly detector with name [%s] as it's already used by detector %s", name, Arrays.stream(response.getHits().getHits()).map(hit -> hit.getId()).collect(Collectors.toList()) ); diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java index 7e2cbda7..d502d717 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java @@ -257,6 +257,45 @@ public void testUpdateAnomalyDetectorNameToExisting() throws Exception { ); } + public void testUpdateAnomalyDetectorNameToNew() throws Exception { + AnomalyDetector detector = createRandomAnomalyDetector(true, true); + + AnomalyDetector detectorWithNewName = new AnomalyDetector( + detector.getDetectorId(), + detector.getVersion(), + randomAlphaOfLength(5), + detector.getDescription(), + detector.getTimeField(), + detector.getIndices(), + detector.getFeatureAttributes(), + detector.getFilterQuery(), + detector.getDetectionInterval(), + detector.getWindowDelay(), + detector.getUiMetadata(), + detector.getSchemaVersion(), + Instant.now() + ); + + TestHelpers + .makeRequest( + client(), + "PUT", + TestHelpers.AD_BASE_DETECTORS_URI + "/" + detector.getDetectorId() + "?refresh=true", + ImmutableMap.of(), + toHttpEntity(detectorWithNewName), + null + ); + + AnomalyDetector resultDetector = getAnomalyDetector(detectorWithNewName.getDetectorId()); + assertEquals("Detector name updating failed", detectorWithNewName.getName(), resultDetector.getName()); + assertEquals("Updated anomaly detector id doesn't match", detectorWithNewName.getDetectorId(), resultDetector.getDetectorId()); + assertNotEquals( + "Anomaly detector last update time not changed", + detectorWithNewName.getLastUpdateTime(), + resultDetector.getLastUpdateTime() + ); + } + public void testUpdateAnomalyDetectorWithNotExistingIndex() throws Exception { AnomalyDetector detector = createRandomAnomalyDetector(true, true);