From b44e5abff369af03c50a863bce5b55c18928d02c Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 16 Jan 2019 09:56:03 -0600 Subject: [PATCH 1/2] Deprecate HLRC EmptyResponse used by security 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 --- .../client/RestHighLevelClient.java | 2 +- .../elasticsearch/client/SecurityClient.java | 103 ++++++++++++++++++ .../client/security/EmptyResponse.java | 2 + .../SecurityDocumentationIT.java | 32 +++--- .../security/change-password.asciidoc | 5 +- .../high-level/security/disable-user.asciidoc | 5 +- .../high-level/security/enable-user.asciidoc | 5 +- 7 files changed, 127 insertions(+), 27 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java index a9c6901d9820a..e82c2dc620494 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java @@ -1704,7 +1704,7 @@ protected final Resp parseEntity(final HttpEntity entity, } } - static boolean convertExistsResponse(Response response) { + protected static boolean convertExistsResponse(Response response) { return response.getStatusLine().getStatusCode() == 200; } diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java index 48a1cdb778243..3636d19c9730d 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java +++ b/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 + * the docs 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 the response from the enable user call + * @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 @@ -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 listener) { restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::enableUser, options, EmptyResponse::fromXContent, listener, emptySet()); } + /** + * Enable a native realm or built-in user asynchronously. + * See + * the docs 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 listener) { + restHighLevelClient.performRequestAsync(request, SecurityRequestConverters::enableUser, options, + RestHighLevelClient::convertExistsResponse, listener, emptySet()); + } + /** * Disable a native realm or built-in user synchronously. * See @@ -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 + * the docs 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 the response from the enable user call + * @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 @@ -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 listener) { restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::disableUser, options, EmptyResponse::fromXContent, listener, emptySet()); } + /** + * Disable a native realm or built-in user asynchronously. + * See + * the docs 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 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 @@ -457,12 +525,29 @@ public void getSslCertificatesAsync(RequestOptions options, ActionListener + * the docs 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 the response from the change user password call + * @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 @@ -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 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 + * the docs 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 listener) { + restHighLevelClient.performRequestAsync(request, SecurityRequestConverters::changePassword, options, + RestHighLevelClient::convertExistsResponse, listener, emptySet()); + } + + /** * Delete a role mapping. * See diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java index 62fea88e52356..c4f42c719140d 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java +++ b/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 to instead of this class */ +@Deprecated public final class EmptyResponse { private static final ObjectParser PARSER = new ObjectParser<>("empty_response", false, EmptyResponse::new); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index c8220e9cc0c05..0320337b717d2 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -44,7 +44,6 @@ import org.elasticsearch.client.security.DeleteUserRequest; import org.elasticsearch.client.security.DeleteUserResponse; import org.elasticsearch.client.security.DisableUserRequest; -import org.elasticsearch.client.security.EmptyResponse; import org.elasticsearch.client.security.EnableUserRequest; import org.elasticsearch.client.security.ExpressionRoleMapping; import org.elasticsearch.client.security.GetPrivilegesRequest; @@ -85,7 +84,6 @@ import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; - import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -519,18 +517,18 @@ public void testEnableUser() throws Exception { { //tag::enable-user-execute EnableUserRequest request = new EnableUserRequest("enable_user", RefreshPolicy.NONE); - EmptyResponse response = client.security().enableUser(request, RequestOptions.DEFAULT); + boolean response = client.security().enableUser(RequestOptions.DEFAULT, request); //end::enable-user-execute - assertNotNull(response); + assertTrue(response); } { //tag::enable-user-execute-listener EnableUserRequest request = new EnableUserRequest("enable_user", RefreshPolicy.NONE); - ActionListener listener = new ActionListener() { + ActionListener listener = new ActionListener() { @Override - public void onResponse(EmptyResponse setUserEnabledResponse) { + public void onResponse(Boolean response) { // <1> } @@ -546,7 +544,7 @@ public void onFailure(Exception e) { listener = new LatchedActionListener<>(listener, latch); // tag::enable-user-execute-async - client.security().enableUserAsync(request, RequestOptions.DEFAULT, listener); // <1> + client.security().enableUserAsync(RequestOptions.DEFAULT, request, listener); // <1> // end::enable-user-execute-async assertTrue(latch.await(30L, TimeUnit.SECONDS)); @@ -563,18 +561,18 @@ public void testDisableUser() throws Exception { { //tag::disable-user-execute DisableUserRequest request = new DisableUserRequest("disable_user", RefreshPolicy.NONE); - EmptyResponse response = client.security().disableUser(request, RequestOptions.DEFAULT); + boolean response = client.security().disableUser(RequestOptions.DEFAULT, request); //end::disable-user-execute - assertNotNull(response); + assertTrue(response); } { //tag::disable-user-execute-listener DisableUserRequest request = new DisableUserRequest("disable_user", RefreshPolicy.NONE); - ActionListener listener = new ActionListener() { + ActionListener listener = new ActionListener() { @Override - public void onResponse(EmptyResponse setUserEnabledResponse) { + public void onResponse(Boolean response) { // <1> } @@ -590,7 +588,7 @@ public void onFailure(Exception e) { listener = new LatchedActionListener<>(listener, latch); // tag::disable-user-execute-async - client.security().disableUserAsync(request, RequestOptions.DEFAULT, listener); // <1> + client.security().disableUserAsync(RequestOptions.DEFAULT, request, listener); // <1> // end::disable-user-execute-async assertTrue(latch.await(30L, TimeUnit.SECONDS)); @@ -1038,17 +1036,17 @@ public void testChangePassword() throws Exception { { //tag::change-password-execute ChangePasswordRequest request = new ChangePasswordRequest("change_password_user", newPassword, RefreshPolicy.NONE); - EmptyResponse response = client.security().changePassword(request, RequestOptions.DEFAULT); + boolean response = client.security().changePassword(RequestOptions.DEFAULT, request); //end::change-password-execute - assertNotNull(response); + assertTrue(response); } { //tag::change-password-execute-listener ChangePasswordRequest request = new ChangePasswordRequest("change_password_user", password, RefreshPolicy.NONE); - ActionListener listener = new ActionListener() { + ActionListener listener = new ActionListener() { @Override - public void onResponse(EmptyResponse response) { + public void onResponse(Boolean response) { // <1> } @@ -1064,7 +1062,7 @@ public void onFailure(Exception e) { listener = new LatchedActionListener<>(listener, latch); //tag::change-password-execute-async - client.security().changePasswordAsync(request, RequestOptions.DEFAULT, listener); // <1> + client.security().changePasswordAsync(RequestOptions.DEFAULT, request, listener); // <1> //end::change-password-execute-async assertTrue(latch.await(30L, TimeUnit.SECONDS)); diff --git a/docs/java-rest/high-level/security/change-password.asciidoc b/docs/java-rest/high-level/security/change-password.asciidoc index 40490ad6a83b3..36d66b194cfea 100644 --- a/docs/java-rest/high-level/security/change-password.asciidoc +++ b/docs/java-rest/high-level/security/change-password.asciidoc @@ -15,8 +15,7 @@ include-tagged::{doc-tests}/SecurityDocumentationIT.java[change-password-execute [[java-rest-high-change-password-response]] ==== Response -The returned `EmptyResponse` does not contain any fields. The return of this -response indicates a successful request. +The returned `Boolean` indicates the request status. [[java-rest-high-x-pack-security-change-password-async]] ==== Asynchronous Execution @@ -35,7 +34,7 @@ has completed the `ActionListener` is called back using the `onResponse` method if the execution successfully completed or using the `onFailure` method if it failed. -A typical listener for a `EmptyResponse` looks like: +A typical listener for a `Boolean` looks like: ["source","java",subs="attributes,callouts,macros"] -------------------------------------------------- diff --git a/docs/java-rest/high-level/security/disable-user.asciidoc b/docs/java-rest/high-level/security/disable-user.asciidoc index 8bb2299946c42..564b8699ebb8d 100644 --- a/docs/java-rest/high-level/security/disable-user.asciidoc +++ b/docs/java-rest/high-level/security/disable-user.asciidoc @@ -15,8 +15,7 @@ include-tagged::{doc-tests}/SecurityDocumentationIT.java[disable-user-execute] [[java-rest-high-security-disable-user-response]] ==== Response -The returned `EmptyResponse` does not contain any fields. The return of this -response indicates a successful request. +The returned `Boolean` indicates the request status. [[java-rest-high-security-disable-user-async]] ==== Asynchronous Execution @@ -35,7 +34,7 @@ has completed the `ActionListener` is called back using the `onResponse` method if the execution successfully completed or using the `onFailure` method if it failed. -A typical listener for a `EmptyResponse` looks like: +A typical listener for a `Boolean` looks like: ["source","java",subs="attributes,callouts,macros"] -------------------------------------------------- diff --git a/docs/java-rest/high-level/security/enable-user.asciidoc b/docs/java-rest/high-level/security/enable-user.asciidoc index 7601653269789..4be0f38e39fa6 100644 --- a/docs/java-rest/high-level/security/enable-user.asciidoc +++ b/docs/java-rest/high-level/security/enable-user.asciidoc @@ -15,8 +15,7 @@ include-tagged::{doc-tests}/SecurityDocumentationIT.java[enable-user-execute] [[java-rest-high-security-enable-user-response]] ==== Response -The returned `EmptyResponse` does not contain any fields. The return of this -response indicates a successful request. +The returned `Boolean` indicates the request status. [[java-rest-high-security-enable-user-async]] ==== Asynchronous Execution @@ -35,7 +34,7 @@ has completed the `ActionListener` is called back using the `onResponse` method if the execution successfully completed or using the `onFailure` method if it failed. -A typical listener for a `EmptyResponse` looks like: +A typical listener for a `Boolean` looks like: ["source","java",subs="attributes,callouts,macros"] -------------------------------------------------- From 56e482760e41932ac861e6e1c065480e2923f526 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Wed, 16 Jan 2019 20:17:17 -0600 Subject: [PATCH 2/2] Fix up from comments and the test cases that were broken --- .../elasticsearch/client/SecurityClient.java | 6 +-- .../client/security/EmptyResponse.java | 2 +- .../CustomRestHighLevelClientTests.java | 3 +- .../client/RestHighLevelClientTests.java | 44 ++++++++++++++----- 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java index 3636d19c9730d..4d8d1d5db43aa 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java @@ -252,7 +252,7 @@ public EmptyResponse enableUser(EnableUserRequest request, RequestOptions option * * @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 the response from the enable user call + * @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 { @@ -316,7 +316,7 @@ public EmptyResponse disableUser(DisableUserRequest request, RequestOptions opti * * @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 the response from the enable user call + * @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 { @@ -540,7 +540,7 @@ public EmptyResponse changePassword(ChangePasswordRequest request, RequestOption * * @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 the response from the change user password call + * @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 { diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java index c4f42c719140d..961a9cb3cdfb4 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/EmptyResponse.java @@ -26,7 +26,7 @@ /** * Response for a request which simply returns an empty object. - @deprecated Use a boolean to instead of this class + @deprecated Use a boolean instead of this class */ @Deprecated public final class EmptyResponse { diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java index 316de885fa136..afaf09755386c 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/CustomRestHighLevelClientTests.java +++ b/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", diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java index a94ab4541f0f9..299c7c8e37cff 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java @@ -711,6 +711,10 @@ public void testApiNamingConventions() throws Exception { "nodes.reload_secure_settings", "search_shards", }; + List booleanReturnMethods = Arrays.asList( + "security.enable_user", + "security.disable_user", + "security.change_password"); Set deprecatedMethods = new HashSet<>(); deprecatedMethods.add("indices.force_merge"); deprecatedMethods.add("multi_get"); @@ -730,6 +734,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.toMap(Tuple::v1, Tuple::v2)); Set apiNotFound = new HashSet<>(); @@ -748,7 +753,7 @@ public void testApiNamingConventions() throws Exception { } else if (isSubmitTaskMethod(apiName)) { assertSubmitTaskMethod(methods, method, apiName, restSpec); } else { - assertSyncMethod(method, apiName); + assertSyncMethod(method, apiName, booleanReturnMethods); boolean remove = apiSpec.remove(apiName); if (remove == false) { if (deprecatedMethods.contains(apiName)) { @@ -784,9 +789,9 @@ public void testApiNamingConventions() throws Exception { assertThat("Some API are not supported but they should be: " + apiSpec, apiSpec.size(), equalTo(0)); } - private static void assertSyncMethod(Method method, String apiName) { + private static void assertSyncMethod(Method method, String apiName, List 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 { @@ -805,10 +810,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)); + } } } @@ -823,10 +836,19 @@ private static void assertAsyncMethod(Map methods, Method method 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)); }