opendev/gerrit
Code Issues Proposed changes

Don't bind several REST endpoints under the same view name

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-07-02 14:41:05 +02:00
parent 4e933e0bc8
commit 57cc2c509d
4 changed files with 10 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
java/com/google/gerrit/server/restapi/account/AddSshKey.java
View File

@@ -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<AccountResource, SshKeyInput> {
public class AddSshKey
implements RestCollectionView<AccountResource, AccountResource.SshKey, SshKeyInput> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final Provider<CurrentUser> self;

4
java/com/google/gerrit/server/restapi/account/Module.java
View File

@@ -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);

2
java/com/google/gerrit/server/restapi/change/Module.java
View File

@@ -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);

6
java/com/google/gerrit/server/restapi/change/PostReviewers.java
View File

@@ -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<ChangeResource, AddReviewerInput, AddReviewerResult> {
extends RetryingRestCollectionView<
ChangeResource, ReviewerResource, AddReviewerInput, AddReviewerResult> {
public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
public static final int DEFAULT_MAX_REVIEWERS = 20;