Changed forbiddenAPIsTest files and made relevant forbidden fixes (#450)

Signed-off-by: Amit Galitzky <[email protected]>
opensearch-project · Mar 30, 2022 · 407e061 · 407e061
1 parent 9dcf010
commit 407e061
Show file tree

Hide file tree

Showing 28 changed files with 171 additions and 94 deletions.
diff --git a/build.gradle b/build.gradle
@@ -131,12 +131,10 @@ tasks.named('forbiddenApisMain').configure {
     signaturesFiles += files('src/forbidden/ad-signatures.txt')
 }
 
-tasks.named('forbiddenApisTest').configure {
-    // Disable check because AD code has too many violations.
-    // For example, we have to allow @Test to be used in test classes not inherited from LuceneTestCase.
-    // see https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/resources/forbidden/es-test-signatures.txt
-    ignoreFailures = true
-}
+//Adds custom file that only has some of the checks for testApis checks since too many violations
+//For example, we have to allow @Test to be used in test classes not inherited from LuceneTestCase.
+//example: warning for every file: `Forbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar]`
+forbiddenApisTest.setSignaturesFiles(files('src/forbidden/ad-test-signatures.txt'))
 
 // Allow test cases to be named Tests without having to be inherited from LuceneTestCase.
 // see https://github.com/elastic/elasticsearch/blob/323f312bbc829a63056a79ebe45adced5099f6e6/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java

diff --git a/src/forbidden/ad-test-signatures.txt b/src/forbidden/ad-test-signatures.txt
@@ -0,0 +1,16 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+@defaultMessage use format with Locale
+java.lang.String#format(java.lang.String,java.lang.Object[])
+java.util.concurrent.ThreadLocalRandom
+
+java.security.MessageDigest#clone() @ use org.opensearch.common.hash.MessageDigests
+
+@defaultMessage Don't use MethodHandles in slow or lenient ways, use invokeExact instead.
+java.lang.invoke.MethodHandle#invoke(java.lang.Object[])
+java.lang.invoke.MethodHandle#invokeWithArguments(java.lang.Object[])
+java.lang.invoke.MethodHandle#invokeWithArguments(java.util.List)
+java.lang.Math#random() @ Use one of the various randomization methods from LuceneTestCase or OpenSearchTestCase for reproducibility
diff --git a/...g/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java b/...g/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java
@@ -264,7 +264,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
                         listener.onResponse((Response) response);
                     }
                 } catch (IOException e) {
-                    e.printStackTrace();
+                    logger.error("Create field mapping threw an exception", e);
                 }
             }
         };
@@ -345,7 +345,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
                         listener.onResponse((Response) response);
                     }
                 } catch (IOException e) {
-                    e.printStackTrace();
+                    logger.error("Create field mapping threw an exception", e);
                 }
             }
         };
@@ -437,7 +437,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
                         listener.onResponse((Response) response);
                     }
                 } catch (IOException e) {
-                    e.printStackTrace();
+                    logger.error("Create field mapping threw an exception", e);
                 }
             }
         };
@@ -527,7 +527,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
                         listener.onResponse((Response) response);
                     }
                 } catch (IOException e) {
-                    e.printStackTrace();
+                    logger.error("Create field mapping threw an exception", e);
                 }
             }
         };
@@ -599,7 +599,10 @@ public void testTenMultiEntityDetectorsUpdateSingleEntityAdToMulti() throws IOEx
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 2
+            );
 
             assertTrue(args[0] instanceof SearchRequest);
             assertTrue(args[1] instanceof ActionListener);
@@ -613,7 +616,10 @@ public void testTenMultiEntityDetectorsUpdateSingleEntityAdToMulti() throws IOEx
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 2
+            );
 
             assertTrue(args[0] instanceof GetRequest);
             assertTrue(args[1] instanceof ActionListener);
@@ -675,7 +681,10 @@ public void testTenMultiEntityDetectorsUpdateExistingMultiEntityAd() throws IOEx
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 2
+            );
 
             assertTrue(args[0] instanceof SearchRequest);
             assertTrue(args[1] instanceof ActionListener);
@@ -689,7 +698,10 @@ public void testTenMultiEntityDetectorsUpdateExistingMultiEntityAd() throws IOEx
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 2
+            );
 
             assertTrue(args[0] instanceof GetRequest);
             assertTrue(args[1] instanceof ActionListener);

diff --git a/...pensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java b/...pensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java
@@ -137,38 +137,9 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc
 
         // extend NodeClient since its execute method is final and mockito does not allow to mock final methods
         // we can also use spy to overstep the final methods
-
         NodeClient client = IndexAnomalyDetectorActionHandlerTests
             .getCustomNodeClient(detectorResponse, userIndexResponse, singleEntityDetector, threadPool);
 
-        // NodeClient client = new NodeClient(Settings.EMPTY, threadPool) {
-        // @Override
-        // public <Request extends ActionRequest, Response extends ActionResponse> void doExecute(
-        // ActionType<Response> action,
-        // Request request,
-        // ActionListener<Response> listener
-        // ) {
-        // try {
-        // if (action.equals(SearchAction.INSTANCE)) {
-        // assertTrue(request instanceof SearchRequest);
-        // SearchRequest searchRequest = (SearchRequest) request;
-        // if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) {
-        // listener.onResponse((Response) detectorResponse);
-        // } else {
-        // listener.onResponse((Response) userIndexResponse);
-        // }
-        // } else {
-        // GetFieldMappingsResponse response = new GetFieldMappingsResponse(
-        // TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date")
-        // );
-        // listener.onResponse((Response) response);
-        // }
-        // } catch (IOException e) {
-        // e.printStackTrace();
-        // }
-        // }
-        // };
-
         NodeClient clientSpy = spy(client);
 
         handler = new ValidateAnomalyDetectorActionHandler(

diff --git a/src/test/java/org/opensearch/ad/AnomalyDetectorJobRunnerTests.java b/src/test/java/org/opensearch/ad/AnomalyDetectorJobRunnerTests.java
@@ -29,6 +29,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
+import java.util.Locale;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.ThreadFactory;
 
@@ -174,7 +175,10 @@ public void setup() throws Exception {
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length >= 2
+            );
 
             IndexRequest request = null;
             ActionListener<IndexResponse> listener = null;

diff --git a/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java b/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java
@@ -18,6 +18,7 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Locale;
 import java.util.Map;
 
 import org.apache.http.HttpHeaders;
@@ -196,7 +197,7 @@ protected Response previewAnomalyDetector(String detectorId, RestClient client,
             .makeRequest(
                 client,
                 "POST",
-                String.format(TestHelpers.AD_BASE_PREVIEW_URI, input.getDetectorId()),
+                String.format(Locale.ROOT, TestHelpers.AD_BASE_PREVIEW_URI, input.getDetectorId()),
                 ImmutableMap.of(),
                 TestHelpers.toHttpEntity(input),
                 null

diff --git a/src/test/java/org/opensearch/ad/HistoricalAnalysisRestTestCase.java b/src/test/java/org/opensearch/ad/HistoricalAnalysisRestTestCase.java
@@ -253,7 +253,7 @@ protected List<Object> waitUntilTaskReachState(String detectorId, Set<String> ta
             try {
                 adTaskProfile = getADTaskProfile(detectorId);
             } catch (Exception e) {
-                e.printStackTrace();
+                logger.error("failed to get ADTaskProfile", e);
             } finally {
                 Thread.sleep(1000);
             }

diff --git a/src/test/java/org/opensearch/ad/NodeStateManagerTests.java b/src/test/java/org/opensearch/ad/NodeStateManagerTests.java
@@ -28,6 +28,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.Locale;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
@@ -147,7 +148,10 @@ private String setupDetector() throws IOException {
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length >= 2
+            );
 
             GetRequest request = null;
             ActionListener<GetResponse> listener = null;
@@ -175,7 +179,10 @@ private void setupCheckpoint(boolean responseExists) throws IOException {
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length >= 2
+            );
 
             GetRequest request = null;
             ActionListener<GetResponse> listener = null;

diff --git a/src/test/java/org/opensearch/ad/cluster/DailyCronTests.java b/src/test/java/org/opensearch/ad/cluster/DailyCronTests.java
@@ -20,6 +20,7 @@
 import java.time.Clock;
 import java.time.Duration;
 import java.util.Arrays;
+import java.util.Locale;
 
 import org.opensearch.OpenSearchException;
 import org.opensearch.action.ActionListener;
@@ -57,7 +58,10 @@ private void templateDailyCron(DailyCronTestExecutionMode mode) {
 
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 3);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 3
+            );
             assertTrue(args[2] instanceof ActionListener);
 
             ActionListener<BulkByScrollResponse> listener = (ActionListener<BulkByScrollResponse>) args[2];

diff --git a/src/test/java/org/opensearch/ad/cluster/HashRingTests.java b/src/test/java/org/opensearch/ad/cluster/HashRingTests.java
@@ -147,7 +147,7 @@ public void testGetOwningNode() throws UnknownHostException {
             // Circles for realtime AD will change as it's eligible to build for when its empty
             assertEquals("Wrong hash ring size for realtime AD", 2, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
         }, e -> {
-            e.printStackTrace();
+            logger.error("building hash ring failed", e);
             assertFalse("Build hash ring failed", true);
         }));
 
@@ -166,7 +166,8 @@ public void testGetOwningNode() throws UnknownHostException {
             // Circles for realtime AD will not change as it's eligible to rebuild
             assertEquals("Wrong hash ring size for realtime AD", 2, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
         }, e -> {
-            e.printStackTrace();
+            logger.error("building hash ring failed", e);
+
             assertFalse("Build hash ring failed", true);
         }));
 
@@ -185,7 +186,7 @@ public void testGetOwningNode() throws UnknownHostException {
             );
             assertEquals("Wrong hash ring size for realtime AD", 4, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
         }, e -> {
-            e.printStackTrace();
+            logger.error("building hash ring failed", e);
             assertFalse("Failed to build hash ring", true);
         }));
     }

diff --git a/src/test/java/org/opensearch/ad/cluster/HourlyCronTests.java b/src/test/java/org/opensearch/ad/cluster/HourlyCronTests.java
@@ -20,6 +20,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Locale;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -64,7 +65,10 @@ public void templateHourlyCron(HourlyCronTestExecutionMode mode) {
         Client client = mock(Client.class);
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 3);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 3
+            );
             assertTrue(args[2] instanceof ActionListener);
 
             ActionListener<CronResponse> listener = (ActionListener<CronResponse>) args[2];

diff --git a/src/test/java/org/opensearch/ad/cluster/MasterEventListenerTests.java b/src/test/java/org/opensearch/ad/cluster/MasterEventListenerTests.java
@@ -22,6 +22,7 @@
 import java.time.Clock;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.Locale;
 
 import org.junit.Before;
 import org.opensearch.ad.AbstractADTest;
@@ -83,7 +84,10 @@ public void testOnOffMaster() {
     public void testBeforeStop() {
         doAnswer(invocation -> {
             Object[] args = invocation.getArguments();
-            assertTrue(String.format("The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)), args.length == 1);
+            assertTrue(
+                String.format(Locale.ROOT, "The size of args is %d.  Its content is %s", args.length, Arrays.toString(args)),
+                args.length == 1
+            );
 
             LifecycleListener listener = null;
             if (args[0] instanceof LifecycleListener) {