Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Prevent creating detector with duplicate name. Issue:#118 #134

Merged
merged 3 commits into from
May 22, 2020
Merged

Prevent creating detector with duplicate name. Issue:#118 #134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49f30a2
Prevent creating detector with duplicate name. Issue:#118
yizheliu-amazon May 22, 2020
6f1c83c
Exclude detector with input detector id when searching detector with …
yizheliu-amazon May 22, 2020
53a264c
Address Yaliang's comments and add 1 more test case
yizheliu-amazon May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions .../amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor. can this query also include a condition like not this detector id to utilize es search instead of additional postprocessing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point. will change to use must_not to exclude this detector id

.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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor. i would use a warning for this case since it's a harmless user error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. will do

onFailure(new IllegalArgumentException(errorMsg));
} else {
indexAnomalyDetector(detectorId);
}
Expand Down
76 changes: 76 additions & 0 deletions src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/AnomalyDetectorRestApiIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down