Deprecate HLRC EmptyResponse used by security (#37540)

The EmptyResponse is essentially the same as returning a boolean, which
is done in other places. This commit deprecates all the existing
EmptyResponse methods and creates new boolean methods that have method
params reordered so they can exist with the deprecated methods. A
followup PR in master will remove the existing deprecated methods, fix
the parameter ordering and deprecate the incorrectly ordered parameter
methods.

Relates #36938

Author: hub-cap

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -1704,7 +1704,7 @@ protected final <Resp> Resp parseEntity(final HttpEntity entity,
         }
     }
 
-    static boolean convertExistsResponse(Response response) {
+    protected static boolean convertExistsResponse(Response response) {
         return response.getStatusLine().getStatusCode() == 200;
     }
 

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java

@@ -237,12 +237,29 @@ public void getRoleMappingsAsync(final GetRoleMappingsRequest request, final Req
      * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @return the response from the enable user call
      * @throws IOException in case there is a problem sending the request or parsing back the response
+     * @deprecated use {@link #enableUser(RequestOptions, EnableUserRequest)} instead
      */
+    @Deprecated
     public EmptyResponse enableUser(EnableUserRequest request, RequestOptions options) throws IOException {
         return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::enableUser, options,
             EmptyResponse::fromXContent, emptySet());
     }
 
+    /**
+     * Enable a native realm or built-in user synchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-enable-user.html">
+     * the docs</a> for more.
+     *
+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request the request with the user to enable
+     * @return {@code true} if the request succeeded (the user is enabled)
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public boolean enableUser(RequestOptions options, EnableUserRequest request) throws IOException {
+        return restHighLevelClient.performRequest(request, SecurityRequestConverters::enableUser, options,
+            RestHighLevelClient::convertExistsResponse, emptySet());
+    }
+
     /**
      * Enable a native realm or built-in user asynchronously.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-enable-user.html">
@@ -251,13 +268,30 @@ public EmptyResponse enableUser(EnableUserRequest request, RequestOptions option
      * @param request  the request with the user to enable
      * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @param listener the listener to be notified upon request completion
+     * @deprecated use {@link #enableUserAsync(RequestOptions, EnableUserRequest, ActionListener)} instead
      */
+    @Deprecated
     public void enableUserAsync(EnableUserRequest request, RequestOptions options,
                                 ActionListener<EmptyResponse> listener) {
         restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::enableUser, options,
             EmptyResponse::fromXContent, listener, emptySet());
     }
 
+    /**
+     * Enable a native realm or built-in user asynchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-enable-user.html">
+     * the docs</a> for more.
+     *
+     * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request  the request with the user to enable
+     * @param listener the listener to be notified upon request completion
+     */
+    public void enableUserAsync(RequestOptions options, EnableUserRequest request,
+                                ActionListener<Boolean> listener) {
+        restHighLevelClient.performRequestAsync(request, SecurityRequestConverters::enableUser, options,
+            RestHighLevelClient::convertExistsResponse, listener, emptySet());
+    }
+
     /**
      * Disable a native realm or built-in user synchronously.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-disable-user.html">
@@ -267,12 +301,29 @@ public void enableUserAsync(EnableUserRequest request, RequestOptions options,
      * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @return the response from the enable user call
      * @throws IOException in case there is a problem sending the request or parsing back the response
+     * @deprecated use {@link #disableUser(RequestOptions, DisableUserRequest)} instead
      */
+    @Deprecated
     public EmptyResponse disableUser(DisableUserRequest request, RequestOptions options) throws IOException {
         return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::disableUser, options,
             EmptyResponse::fromXContent, emptySet());
     }
 
+    /**
+     * Disable a native realm or built-in user synchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-disable-user.html">
+     * the docs</a> for more.
+     *
+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request the request with the user to disable
+     * @return {@code true} if the request succeeded (the user is disabled)
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public boolean disableUser(RequestOptions options, DisableUserRequest request) throws IOException {
+        return restHighLevelClient.performRequest(request, SecurityRequestConverters::disableUser, options,
+            RestHighLevelClient::convertExistsResponse, emptySet());
+    }
+
     /**
      * Disable a native realm or built-in user asynchronously.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-disable-user.html">
@@ -281,13 +332,30 @@ public EmptyResponse disableUser(DisableUserRequest request, RequestOptions opti
      * @param request  the request with the user to disable
      * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @param listener the listener to be notified upon request completion
+     * @deprecated use {@link #disableUserAsync(RequestOptions, DisableUserRequest, ActionListener)} instead
      */
+    @Deprecated
     public void disableUserAsync(DisableUserRequest request, RequestOptions options,
                                  ActionListener<EmptyResponse> listener) {
         restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::disableUser, options,
             EmptyResponse::fromXContent, listener, emptySet());
     }
 
+    /**
+     * Disable a native realm or built-in user asynchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-disable-user.html">
+     * the docs</a> for more.
+     *
+     * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request  the request with the user to disable
+     * @param listener the listener to be notified upon request completion
+     */
+    public void disableUserAsync(RequestOptions options, DisableUserRequest request,
+                                 ActionListener<Boolean> listener) {
+        restHighLevelClient.performRequestAsync(request, SecurityRequestConverters::disableUser, options,
+            RestHighLevelClient::convertExistsResponse, listener, emptySet());
+    }
+
     /**
      * Authenticate the current user and return all the information about the authenticated user.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-authenticate.html">
@@ -457,12 +525,29 @@ public void getSslCertificatesAsync(RequestOptions options, ActionListener<GetSs
      * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @return the response from the change user password call
      * @throws IOException in case there is a problem sending the request or parsing back the response
+     * @deprecated use {@link #changePassword(RequestOptions, ChangePasswordRequest)} instead
      */
+    @Deprecated
     public EmptyResponse changePassword(ChangePasswordRequest request, RequestOptions options) throws IOException {
         return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::changePassword, options,
             EmptyResponse::fromXContent, emptySet());
     }
 
+    /**
+     * Change the password of a user of a native realm or built-in user synchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-change-password.html">
+     * the docs</a> for more.
+     *
+     * @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request the request with the user's new password
+     * @return {@code true} if the request succeeded (the new password was set)
+     * @throws IOException in case there is a problem sending the request or parsing back the response
+     */
+    public boolean changePassword(RequestOptions options, ChangePasswordRequest request) throws IOException {
+        return restHighLevelClient.performRequest(request, SecurityRequestConverters::changePassword, options,
+            RestHighLevelClient::convertExistsResponse, emptySet());
+    }
+
     /**
      * Change the password of a user of a native realm or built-in user asynchronously.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-change-password.html">
@@ -471,13 +556,31 @@ public EmptyResponse changePassword(ChangePasswordRequest request, RequestOption
      * @param request  the request with the user's new password
      * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
      * @param listener the listener to be notified upon request completion
+     * @deprecated use {@link #changePasswordAsync(RequestOptions, ChangePasswordRequest, ActionListener)} instead
      */
+    @Deprecated
     public void changePasswordAsync(ChangePasswordRequest request, RequestOptions options,
                                     ActionListener<EmptyResponse> listener) {
         restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::changePassword, options,
             EmptyResponse::fromXContent, listener, emptySet());
     }
 
+    /**
+     * Change the password of a user of a native realm or built-in user asynchronously.
+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-change-password.html">
+     * the docs</a> for more.
+     *
+     * @param options  the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
+     * @param request  the request with the user's new password
+     * @param listener the listener to be notified upon request completion
+     */
+    public void changePasswordAsync(RequestOptions options, ChangePasswordRequest request,
+                                    ActionListener<Boolean> listener) {
+        restHighLevelClient.performRequestAsync(request, SecurityRequestConverters::changePassword, options,
+            RestHighLevelClient::convertExistsResponse, listener, emptySet());
+    }
+
+
     /**
      * Delete a role mapping.
      * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-delete-role-mapping.html">

client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java

@@ -26,7 +26,9 @@
 
 /**
  * Response for a request which simply returns an empty object.
+   @deprecated Use a boolean instead of this class
  */
+@Deprecated
 public final class EmptyResponse {
 
     private static final ObjectParser<EmptyResponse, Void> PARSER = new ObjectParser<>("empty_response", false, EmptyResponse::new);

client/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java

@@ -122,7 +122,8 @@ private static RequestOptions optionsForNodeName(String nodeName) {
      */
     @SuppressForbidden(reason = "We're forced to uses Class#getDeclaredMethods() here because this test checks protected methods")
     public void testMethodsVisibility() {
-        final String[] methodNames = new String[]{"parseEntity",
+        final String[] methodNames = new String[]{"convertExistsResponse",
+                                                  "parseEntity",
                                                   "parseResponseException",
                                                   "performRequest",
                                                   "performRequestAndParseEntity",

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

@@ -715,6 +715,10 @@ public void testApiNamingConventions() throws Exception {
             "nodes.reload_secure_settings",
             "search_shards",
         };
+        List<String> booleanReturnMethods = Arrays.asList(
+            "security.enable_user",
+            "security.disable_user",
+            "security.change_password");
         Set<String> deprecatedMethods = new HashSet<>();
         deprecatedMethods.add("indices.force_merge");
         deprecatedMethods.add("multi_get");
@@ -736,6 +740,7 @@ public void testApiNamingConventions() throws Exception {
                 .map(method -> Tuple.tuple(toSnakeCase(method.getName()), method))
                 .flatMap(tuple -> tuple.v2().getReturnType().getName().endsWith("Client")
                         ? getSubClientMethods(tuple.v1(), tuple.v2().getReturnType()) : Stream.of(tuple))
+                .filter(tuple -> tuple.v2().getAnnotation(Deprecated.class) == null)
                 .collect(Collectors.groupingBy(Tuple::v1,
                     Collectors.mapping(Tuple::v2, Collectors.toSet())));
 
@@ -753,7 +758,7 @@ public void testApiNamingConventions() throws Exception {
                 } else if (isSubmitTaskMethod(apiName)) {
                     assertSubmitTaskMethod(methods, method, apiName, restSpec);
                 } else {
-                    assertSyncMethod(method, apiName);
+                    assertSyncMethod(method, apiName, booleanReturnMethods);
                     apiUnsupported.remove(apiName);
                     if (apiSpec.contains(apiName) == false) {
                         if (deprecatedMethods.contains(apiName)) {
@@ -790,9 +795,9 @@ public void testApiNamingConventions() throws Exception {
         assertThat("Some API are not supported but they should be: " + apiUnsupported, apiUnsupported.size(), equalTo(0));
     }
 
-    private static void assertSyncMethod(Method method, String apiName) {
+    private static void assertSyncMethod(Method method, String apiName, List<String> booleanReturnMethods) {
         //A few methods return a boolean rather than a response object
-        if (apiName.equals("ping") || apiName.contains("exist")) {
+        if (apiName.equals("ping") || apiName.contains("exist") || booleanReturnMethods.contains(apiName)) {
             assertThat("the return type for method [" + method + "] is incorrect",
                 method.getReturnType().getSimpleName(), equalTo("boolean"));
         } else {
@@ -811,10 +816,18 @@ private static void assertSyncMethod(Method method, String apiName) {
                 method.getParameterTypes()[0], equalTo(RequestOptions.class));
         } else {
             assertEquals("incorrect number of arguments for method [" + method + "]", 2, method.getParameterTypes().length);
-            assertThat("the first parameter to method [" + method + "] is the wrong type",
-                method.getParameterTypes()[0].getSimpleName(), endsWith("Request"));
-            assertThat("the second parameter to method [" + method + "] is the wrong type",
-                method.getParameterTypes()[1], equalTo(RequestOptions.class));
+            // This is no longer true for all methods. Some methods can contain these 2 args backwards because of deprecation
+            if (method.getParameterTypes()[0].equals(RequestOptions.class)) {
+                assertThat("the first parameter to method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[0], equalTo(RequestOptions.class));
+                assertThat("the second parameter to method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[1].getSimpleName(), endsWith("Request"));
+            } else {
+                assertThat("the first parameter to method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[0].getSimpleName(), endsWith("Request"));
+                assertThat("the second parameter to method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[1], equalTo(RequestOptions.class));
+            }
         }
     }
 
@@ -829,10 +842,18 @@ private static void assertAsyncMethod(Map<String, Set<Method>> methods, Method m
             assertThat(method.getParameterTypes()[1], equalTo(ActionListener.class));
         } else {
             assertEquals("async method [" + method + "] has the wrong number of arguments", 3, method.getParameterTypes().length);
-            assertThat("the first parameter to async method [" + method + "] should be a request type",
-                method.getParameterTypes()[0].getSimpleName(), endsWith("Request"));
-            assertThat("the second parameter to async method [" + method + "] is the wrong type",
-                method.getParameterTypes()[1], equalTo(RequestOptions.class));
+            // This is no longer true for all methods. Some methods can contain these 2 args backwards because of deprecation
+            if (method.getParameterTypes()[0].equals(RequestOptions.class)) {
+                assertThat("the first parameter to async method [" + method + "] should be a request type",
+                    method.getParameterTypes()[0], equalTo(RequestOptions.class));
+                assertThat("the second parameter to async method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[1].getSimpleName(), endsWith("Request"));
+            } else {
+                assertThat("the first parameter to async method [" + method + "] should be a request type",
+                    method.getParameterTypes()[0].getSimpleName(), endsWith("Request"));
+                assertThat("the second parameter to async method [" + method + "] is the wrong type",
+                    method.getParameterTypes()[1], equalTo(RequestOptions.class));
+            }
             assertThat("the third parameter to async method [" + method + "] is the wrong type",
                 method.getParameterTypes()[2], equalTo(ActionListener.class));
         }