From 57cc2c509d59911dba15b1189e33d70ba8bbbd0f Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 2 Jul 2018 14:41:05 +0200 Subject: [PATCH] Don't bind several REST endpoints under the same view name For some view names we bind 2 REST endpoints classes: 1. one RestCollection class to handle listing/parsing of members 2. one RestModifyView class to handle POST on the collection E.g.: child(ACCOUNT_KIND, "sshkeys").to(SshKeys.class); post(ACCOUNT_KIND, "sshkeys").to(AddSshKey.class); Reusing the same view name for different REST endpoint classes is confusing. This only works because the classes are bound for different HTTP methods (GET and POST). Improve the code readability by making it more explicit that POST requests on a collections should be handled: child(ACCOUNT_KIND, "sshkeys").to(SshKeys.class); postOnCollection(SSH_KEY_KIND).to(AddSshKey.class); One might think that it might be better to always bind the endpoint that handles POST on the collection by using the post(...) method in favour of removing the postOnCollection(...) method and the overhead that is needed to support it. However this is not possible as it doesn't allow handling of POST requests on root collections (root collections are not bound in a RestApiModule, but as servlets via UrlModule). Change-Id: I0a9a0ae1c8897dc8c79c5f3aa5943aee6cb10a77 Signed-off-by: Edwin Kempin --- .../com/google/gerrit/server/restapi/account/AddSshKey.java | 5 +++-- java/com/google/gerrit/server/restapi/account/Module.java | 4 ++-- java/com/google/gerrit/server/restapi/change/Module.java | 2 +- .../google/gerrit/server/restapi/change/PostReviewers.java | 6 ++++-- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/account/AddSshKey.java b/java/com/google/gerrit/server/restapi/account/AddSshKey.java index ab06e25aed..9d9c1e305d 100644 --- a/java/com/google/gerrit/server/restapi/account/AddSshKey.java +++ b/java/com/google/gerrit/server/restapi/account/AddSshKey.java @@ -26,7 +26,7 @@ import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RawInput; import com.google.gerrit.extensions.restapi.Response; -import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.extensions.restapi.RestCollectionView; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; @@ -46,7 +46,8 @@ import java.io.InputStream; import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton -public class AddSshKey implements RestModifyView { +public class AddSshKey + implements RestCollectionView { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final Provider self; diff --git a/java/com/google/gerrit/server/restapi/account/Module.java b/java/com/google/gerrit/server/restapi/account/Module.java index f37687f107..be0924a6f5 100644 --- a/java/com/google/gerrit/server/restapi/account/Module.java +++ b/java/com/google/gerrit/server/restapi/account/Module.java @@ -66,12 +66,12 @@ public class Module extends RestApiModule { put(EMAIL_KIND, "preferred").to(PutPreferred.class); put(ACCOUNT_KIND, "password.http").to(PutHttpPassword.class); delete(ACCOUNT_KIND, "password.http").to(PutHttpPassword.class); - child(ACCOUNT_KIND, "sshkeys").to(SshKeys.class); - post(ACCOUNT_KIND, "sshkeys").to(AddSshKey.class); get(ACCOUNT_KIND, "watched.projects").to(GetWatchedProjects.class); post(ACCOUNT_KIND, "watched.projects").to(PostWatchedProjects.class); post(ACCOUNT_KIND, "watched.projects:delete").to(DeleteWatchedProjects.class); + child(ACCOUNT_KIND, "sshkeys").to(SshKeys.class); + postOnCollection(SSH_KEY_KIND).to(AddSshKey.class); get(SSH_KEY_KIND).to(GetSshKey.class); delete(SSH_KEY_KIND).to(DeleteSshKey.class); diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java index 4e05cee8db..dca64fdd7f 100644 --- a/java/com/google/gerrit/server/restapi/change/Module.java +++ b/java/com/google/gerrit/server/restapi/change/Module.java @@ -113,9 +113,9 @@ public class Module extends RestApiModule { post(CHANGE_KIND, "ready").to(SetReadyForReview.class); put(CHANGE_KIND, "message").to(PutMessage.class); - post(CHANGE_KIND, "reviewers").to(PostReviewers.class); get(CHANGE_KIND, "suggest_reviewers").to(SuggestChangeReviewers.class); child(CHANGE_KIND, "reviewers").to(Reviewers.class); + postOnCollection(REVIEWER_KIND).to(PostReviewers.class); get(REVIEWER_KIND).to(GetReviewer.class); delete(REVIEWER_KIND).to(DeleteReviewer.class); post(REVIEWER_KIND, "delete").to(DeleteReviewer.class); diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java index 65c7db7bfe..4e0cb60330 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java +++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java @@ -51,6 +51,7 @@ import com.google.gerrit.server.account.GroupMembers; import com.google.gerrit.server.change.ChangeMessages; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.NotifyUtil; +import com.google.gerrit.server.change.ReviewerResource; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.SystemGroupBackend; @@ -69,7 +70,7 @@ import com.google.gerrit.server.restapi.account.AccountsCollection; import com.google.gerrit.server.restapi.group.GroupsCollection; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryingRestModifyView; +import com.google.gerrit.server.update.RetryingRestCollectionView; import com.google.gerrit.server.update.UpdateException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -85,7 +86,8 @@ import org.eclipse.jgit.lib.Config; @Singleton public class PostReviewers - extends RetryingRestModifyView { + extends RetryingRestCollectionView< + ChangeResource, ReviewerResource, AddReviewerInput, AddReviewerResult> { public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10; public static final int DEFAULT_MAX_REVIEWERS = 20;