Fix the bug that masterOperation(with task param) is bypassed (#4103) (…

…#4114)
opensearch-project · Aug 3, 2022 · 1a15a79 · 1a15a79
1 parent 98dfdf5
commit 1a15a79
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 3 deletions.
diff --git a/.../java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java b/.../java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java
@@ -125,22 +125,27 @@ protected void masterOperation(Request request, ClusterState state, ActionListen
         throw new UnsupportedOperationException("Must be overridden");
     }
 
+    // TODO: Add abstract keyword after removing the deprecated masterOperation()
     protected void clusterManagerOperation(Request request, ClusterState state, ActionListener<Response> listener) throws Exception {
         masterOperation(request, state, listener);
     }
 
-    /** @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #clusterManagerOperation(Task, ClusterManagerNodeRequest, ClusterState, ActionListener)} */
+    /**
+     * Override this operation if access to the task parameter is needed
+     * @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #clusterManagerOperation(Task, ClusterManagerNodeRequest, ClusterState, ActionListener)}
+     */
     @Deprecated
     protected void masterOperation(Task task, Request request, ClusterState state, ActionListener<Response> listener) throws Exception {
-        clusterManagerOperation(task, request, state, listener);
+        clusterManagerOperation(request, state, listener);
     }
 
     /**
      * Override this operation if access to the task parameter is needed
      */
+    // TODO: Change the implementation to call 'clusterManagerOperation(request...)' after removing the deprecated masterOperation()
     protected void clusterManagerOperation(Task task, Request request, ClusterState state, ActionListener<Response> listener)
         throws Exception {
-        clusterManagerOperation(request, state, listener);
+        masterOperation(task, request, state, listener);
     }
 
     protected boolean localExecute(Request request) {

diff --git a/...in/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java b/...in/java/org/opensearch/action/support/clustermanager/info/TransportClusterInfoAction.java
@@ -88,6 +88,7 @@ protected final void clusterManagerOperation(final Request request, final Cluste
         doClusterManagerOperation(request, concreteIndices, state, listener);
     }
 
+    // TODO: Add abstract keyword after removing the deprecated doMasterOperation()
     protected void doClusterManagerOperation(
         Request request,
         String[] concreteIndices,

diff --git a/.../org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java b/.../org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java
@@ -274,6 +274,43 @@ protected void clusterManagerOperation(Task task, Request request, ClusterState
         }
     }
 
+    /* The test is copied from testLocalOperationWithoutBlocks()
+    to validate the backwards compatibility for the deprecated method masterOperation(with task parameter). */
+    public void testDeprecatedMasterOperationWithTaskParameterCanBeCalled() throws ExecutionException, InterruptedException {
+        final boolean clusterManagerOperationFailure = randomBoolean();
+
+        Request request = new Request();
+        PlainActionFuture<Response> listener = new PlainActionFuture<>();
+
+        final Exception exception = new Exception();
+        final Response response = new Response();
+
+        setState(clusterService, ClusterStateCreationUtils.state(localNode, localNode, allNodes));
+
+        new Action("internal:testAction", transportService, clusterService, threadPool) {
+            @Override
+            protected void masterOperation(Task task, Request request, ClusterState state, ActionListener<Response> listener) {
+                if (clusterManagerOperationFailure) {
+                    listener.onFailure(exception);
+                } else {
+                    listener.onResponse(response);
+                }
+            }
+        }.execute(request, listener);
+        assertTrue(listener.isDone());
+
+        if (clusterManagerOperationFailure) {
+            try {
+                listener.get();
+                fail("Expected exception but returned proper result");
+            } catch (ExecutionException ex) {
+                assertThat(ex.getCause(), equalTo(exception));
+            }
+        } else {
+            assertThat(listener.get(), equalTo(response));
+        }
+    }
+
     public void testLocalOperationWithBlocks() throws ExecutionException, InterruptedException {
         final boolean retryableBlock = randomBoolean();
         final boolean unblockBeforeTimeout = randomBoolean();

diff --git a/test/framework/src/main/java/org/opensearch/test/TestCluster.java b/test/framework/src/main/java/org/opensearch/test/TestCluster.java
@@ -127,6 +127,7 @@ public void assertAfterTest() throws Exception {
     /**
      * Returns the number of data and cluster-manager eligible nodes in the cluster.
      */
+    // TODO: Add abstract keyword after removing the deprecated numDataAndMasterNodes()
     public int numDataAndClusterManagerNodes() {
         return numDataAndMasterNodes();
     }