From cbc461050f2218cfceb61a6127d603c5cee7e57d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 26 Jun 2018 17:19:35 +0200 Subject: [PATCH] Let REST API binding tests fail if '405 Method Not Allowed' is returned For POST requests on a REST collection RestApiServlet returns '405 Method Not Allowed' if the RestCollection exists, but doesn't implement AcceptsPost. E.g. this means the 'POST /changes/' call continued to succeed even if ChangesCollection was no longer implementing AcceptsPost. To detect when 'implements AcceptsPost' was accidentally removed from a RestCollection treat '405 Method Not Allowed' as error and let the test fail. However for some tests we do expect '405 Method Not Allowed'. These tests must explicitly set '405 Method Not Allowed' as expected response code now: - PUT /accounts/%s/username: Changing the username is not allowed. - GET /projects/%s/branches/%s/reflog: The tests use DfsRepository which does not support getting the reflog. Some other tests must be adapted so that they no longer return '405 Method Not Allowed': - POST /plugins/%s/: The view name must be prefixed with 'gerrit~' - POST /changes/%s/hashtags: Setting hashtags is only supported with NoteDb. - PUT /plugins/: Requires plugins.allowRemoteAdmin = true Change-Id: Id7e778c28def5c59d65485d19dc957019f143be2 Signed-off-by: Edwin Kempin --- .../acceptance/rest/AbstractRestApiBindingsTest.java | 12 +++++++++++- .../acceptance/rest/AccountsRestApiBindingsIT.java | 9 ++++++++- .../acceptance/rest/ChangesRestApiBindingsIT.java | 4 ++-- .../acceptance/rest/PluginsRestApiBindingsIT.java | 6 +++--- .../acceptance/rest/ProjectsRestApiBindingsIT.java | 7 ++++++- .../rest/RootCollectionsRestApiBindingsIT.java | 2 ++ 6 files changed, 32 insertions(+), 8 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/rest/AbstractRestApiBindingsTest.java b/javatests/com/google/gerrit/acceptance/rest/AbstractRestApiBindingsTest.java index 26bc3453b9..2bb3dcaa77 100644 --- a/javatests/com/google/gerrit/acceptance/rest/AbstractRestApiBindingsTest.java +++ b/javatests/com/google/gerrit/acceptance/rest/AbstractRestApiBindingsTest.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertWithMessage; import static org.apache.http.HttpStatus.SC_FORBIDDEN; import static org.apache.http.HttpStatus.SC_INTERNAL_SERVER_ERROR; +import static org.apache.http.HttpStatus.SC_METHOD_NOT_ALLOWED; import static org.apache.http.HttpStatus.SC_NOT_FOUND; import com.google.auto.value.AutoValue; @@ -80,8 +81,13 @@ public abstract class AbstractRestApiBindingsTest extends AbstractDaemonTest { String msg = String.format("%s %s returned %d: %s", method, uri, status, body); if (restCall.expectedResponseCode().isPresent()) { assertWithMessage(msg).that(status).isEqualTo(restCall.expectedResponseCode().get()); + if (restCall.expectedMessage().isPresent()) { + assertWithMessage(msg).that(body).contains(restCall.expectedMessage().get()); + } } else { - assertWithMessage(msg).that(status).isNotIn(ImmutableList.of(SC_FORBIDDEN, SC_NOT_FOUND)); + assertWithMessage(msg) + .that(status) + .isNotIn(ImmutableList.of(SC_FORBIDDEN, SC_NOT_FOUND, SC_METHOD_NOT_ALLOWED)); assertWithMessage(msg).that(status).isLessThan(SC_INTERNAL_SERVER_ERROR); } } @@ -123,6 +129,8 @@ public abstract class AbstractRestApiBindingsTest extends AbstractDaemonTest { abstract Optional expectedResponseCode(); + abstract Optional expectedMessage(); + String uri(String... args) { String uriFormat = uriFormat(); int expectedArgNum = StringUtils.countMatches(uriFormat, "%s"); @@ -144,6 +152,8 @@ public abstract class AbstractRestApiBindingsTest extends AbstractDaemonTest { abstract Builder expectedResponseCode(int expectedResponseCode); + abstract Builder expectedMessage(String expectedMessage); + abstract RestCall build(); } } diff --git a/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java index 8de85a9510..e5c16de522 100644 --- a/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/AccountsRestApiBindingsIT.java @@ -14,6 +14,9 @@ package com.google.gerrit.acceptance.rest; +import static com.google.gerrit.acceptance.rest.AbstractRestApiBindingsTest.Method.PUT; +import static org.apache.http.HttpStatus.SC_METHOD_NOT_ALLOWED; + import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.UseSsh; import com.google.gerrit.extensions.common.ChangeInput; @@ -40,7 +43,11 @@ public class AccountsRestApiBindingsIT extends AbstractRestApiBindingsTest { RestCall.put("/accounts/%s/name"), RestCall.delete("/accounts/%s/name"), RestCall.get("/accounts/%s/username"), - RestCall.put("/accounts/%s/username"), + RestCall.builder(PUT, "/accounts/%s/username") + // Changing the username is not allowed. + .expectedResponseCode(SC_METHOD_NOT_ALLOWED) + .expectedMessage("Username cannot be changed.") + .build(), RestCall.get("/accounts/%s/active"), RestCall.put("/accounts/%s/active"), RestCall.delete("/accounts/%s/active"), diff --git a/javatests/com/google/gerrit/acceptance/rest/ChangesRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/ChangesRestApiBindingsIT.java index 3b7c931958..a7f13290fc 100644 --- a/javatests/com/google/gerrit/acceptance/rest/ChangesRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/ChangesRestApiBindingsIT.java @@ -59,7 +59,6 @@ public class ChangesRestApiBindingsIT extends AbstractRestApiBindingsTest { RestCall.delete("/changes/%s/topic"), RestCall.get("/changes/%s/in"), RestCall.get("/changes/%s/hashtags"), - RestCall.post("/changes/%s/hashtags"), RestCall.get("/changes/%s/comments"), RestCall.get("/changes/%s/robotcomments"), RestCall.get("/changes/%s/drafts"), @@ -116,7 +115,8 @@ public class ChangesRestApiBindingsIT extends AbstractRestApiBindingsTest { * identifier. */ private static final ImmutableList CHANGE_ENDPOINTS_NOTEDB = - ImmutableList.of(RestCall.post("/changes/%s/rebuild.notedb")); + ImmutableList.of( + RestCall.post("/changes/%s/hashtags"), RestCall.post("/changes/%s/rebuild.notedb")); /** * Reviewer REST endpoints to be tested, each URL contains placeholders for the change identifier diff --git a/javatests/com/google/gerrit/acceptance/rest/PluginsRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/PluginsRestApiBindingsIT.java index aba677f379..07ea3d02ac 100644 --- a/javatests/com/google/gerrit/acceptance/rest/PluginsRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/PluginsRestApiBindingsIT.java @@ -42,9 +42,9 @@ public class PluginsRestApiBindingsIT extends AbstractRestApiBindingsTest { RestCall.get("/plugins/%s/gerrit~status"), // POST (and PUT) requests don't require the 'gerrit~' prefix in front of the view name. - RestCall.post("/plugins/%s/enable"), - RestCall.post("/plugins/%s/disable"), - RestCall.post("/plugins/%s/reload"), + RestCall.post("/plugins/%s/gerrit~enable"), + RestCall.post("/plugins/%s/gerrit~disable"), + RestCall.post("/plugins/%s/gerrit~reload"), // Plugin deletion must be tested last RestCall.delete("/plugins/%s")); diff --git a/javatests/com/google/gerrit/acceptance/rest/ProjectsRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/ProjectsRestApiBindingsIT.java index d59f037be6..a86a7394fa 100644 --- a/javatests/com/google/gerrit/acceptance/rest/ProjectsRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/ProjectsRestApiBindingsIT.java @@ -19,6 +19,7 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.acceptance.rest.AbstractRestApiBindingsTest.Method.GET; import static com.google.gerrit.reviewdb.client.RefNames.REFS_DASHBOARDS; import static com.google.gerrit.server.restapi.project.DashboardsCollection.DEFAULT_DASHBOARD_NAME; +import static org.apache.http.HttpStatus.SC_METHOD_NOT_ALLOWED; import static org.apache.http.HttpStatus.SC_NOT_FOUND; import com.google.common.collect.ImmutableList; @@ -93,7 +94,11 @@ public class ProjectsRestApiBindingsIT extends AbstractRestApiBindingsTest { RestCall.get("/projects/%s/branches/%s"), RestCall.put("/projects/%s/branches/%s"), RestCall.get("/projects/%s/branches/%s/mergeable"), - RestCall.get("/projects/%s/branches/%s/reflog"), + RestCall.builder(GET, "/projects/%s/branches/%s/reflog") + // The tests use DfsRepository which does not support getting the reflog. + .expectedResponseCode(SC_METHOD_NOT_ALLOWED) + .expectedMessage("reflog not supported on") + .build(), RestCall.builder(GET, "/projects/%s/branches/%s/files") // GET /projects//branches//files is not implemented .expectedResponseCode(SC_NOT_FOUND) diff --git a/javatests/com/google/gerrit/acceptance/rest/RootCollectionsRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/RootCollectionsRestApiBindingsIT.java index 7a2945dd6e..a2c4ea697b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/RootCollectionsRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/RootCollectionsRestApiBindingsIT.java @@ -18,6 +18,7 @@ import static com.google.gerrit.acceptance.rest.AbstractRestApiBindingsTest.Meth import static org.apache.http.HttpStatus.SC_NOT_FOUND; import com.google.common.collect.ImmutableList; +import com.google.gerrit.acceptance.GerritConfig; import org.junit.Test; /** @@ -48,6 +49,7 @@ public class RootCollectionsRestApiBindingsIT extends AbstractRestApiBindingsTes RestCall.put("/projects/new-project")); @Test + @GerritConfig(name = "plugins.allowRemoteAdmin", value = "true") public void rootEndpoints() throws Exception { execute(ROOT_ENDPOINTS); }