From 59963899c2a9221473c1a0019736f8dfa727afae Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Tue, 16 Jun 2020 13:43:53 +0300 Subject: [PATCH] Remove RemoveFromAttentionSetInput RemoveFromAttentionSetInput was previously used by RemoveFromAttentionSet. Now, postReview uses AttentionSetInput for both adding and removing users, so it makes sense to use AttentionSetInput for both use cases here, as well. In this case, we will ensure that if a user is specified, it must match the user in the URL. The URL looks like this: POST /changes/{change-id}/attention/{account-id}/delete Thus, the user that should be removed is already specified there: if the caller specifies the user again, the users specified must match to the same user (both in the input object and in the URL). Change-Id: I68ee52d4e71fd733e0035bee14d3d38ebcdc9752 --- .../gerrit/entities/AttentionSetUpdate.java | 4 +- .../api/changes/AttentionSetApi.java | 4 +- .../api/changes/AttentionSetInput.java | 9 +++- .../changes/RemoveFromAttentionSetInput.java | 33 ------------- .../api/changes/AttentionSetApiImpl.java | 4 +- .../change/RemoveFromAttentionSet.java | 28 +++++++++-- .../rest/change/AttentionSetIT.java | 49 +++++++++++++------ 7 files changed, 71 insertions(+), 60 deletions(-) delete mode 100644 java/com/google/gerrit/extensions/api/changes/RemoveFromAttentionSetInput.java diff --git a/java/com/google/gerrit/entities/AttentionSetUpdate.java b/java/com/google/gerrit/entities/AttentionSetUpdate.java index 00af7a575c..2e586082a6 100644 --- a/java/com/google/gerrit/entities/AttentionSetUpdate.java +++ b/java/com/google/gerrit/entities/AttentionSetUpdate.java @@ -24,9 +24,7 @@ import java.time.Instant; * in reverse chronological order. Since each update contains all required information and * invalidates all previous state, only the most recent record is relevant for each user. * - *

See {@link AttentionSetInput} and {@link - * com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput} for the representation in - * the API. + *

See {@link AttentionSetInput} for the representation in the API. */ @AutoValue public abstract class AttentionSetUpdate { diff --git a/java/com/google/gerrit/extensions/api/changes/AttentionSetApi.java b/java/com/google/gerrit/extensions/api/changes/AttentionSetApi.java index 5086cd86af..da9a8c7420 100644 --- a/java/com/google/gerrit/extensions/api/changes/AttentionSetApi.java +++ b/java/com/google/gerrit/extensions/api/changes/AttentionSetApi.java @@ -20,7 +20,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; /** API for managing the attention set of a change. */ public interface AttentionSetApi { - void remove(RemoveFromAttentionSetInput input) throws RestApiException; + void remove(AttentionSetInput input) throws RestApiException; /** * A default implementation which allows source compatibility when adding new methods to the @@ -28,7 +28,7 @@ public interface AttentionSetApi { */ class NotImplemented implements AttentionSetApi { @Override - public void remove(RemoveFromAttentionSetInput input) throws RestApiException { + public void remove(AttentionSetInput input) throws RestApiException { throw new NotImplementedException(); } } diff --git a/java/com/google/gerrit/extensions/api/changes/AttentionSetInput.java b/java/com/google/gerrit/extensions/api/changes/AttentionSetInput.java index 1a173e9ce6..6bbb27f203 100644 --- a/java/com/google/gerrit/extensions/api/changes/AttentionSetInput.java +++ b/java/com/google/gerrit/extensions/api/changes/AttentionSetInput.java @@ -14,20 +14,25 @@ package com.google.gerrit.extensions.api.changes; +import com.google.gerrit.extensions.restapi.DefaultInput; + /** * Input at API level to add a user to the attention set. * - * @see RemoveFromAttentionSetInput * @see com.google.gerrit.extensions.common.AttentionSetEntry */ public class AttentionSetInput { public String user; - public String reason; + @DefaultInput public String reason; public AttentionSetInput(String user, String reason) { this.user = user; this.reason = reason; } + public AttentionSetInput(String reason) { + this.reason = reason; + } + public AttentionSetInput() {} } diff --git a/java/com/google/gerrit/extensions/api/changes/RemoveFromAttentionSetInput.java b/java/com/google/gerrit/extensions/api/changes/RemoveFromAttentionSetInput.java deleted file mode 100644 index b14c53d2bc..0000000000 --- a/java/com/google/gerrit/extensions/api/changes/RemoveFromAttentionSetInput.java +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright (C) 2020 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.extensions.api.changes; - -import com.google.gerrit.extensions.restapi.DefaultInput; - -/** - * Input at API level to remove a user from the attention set. - * - * @see AttentionSetInput - * @see com.google.gerrit.extensions.common.AttentionSetEntry - */ -public class RemoveFromAttentionSetInput { - @DefaultInput public String reason; - - public RemoveFromAttentionSetInput(String reason) { - this.reason = reason; - } - - public RemoveFromAttentionSetInput() {} -} diff --git a/java/com/google/gerrit/server/api/changes/AttentionSetApiImpl.java b/java/com/google/gerrit/server/api/changes/AttentionSetApiImpl.java index 8dc44b797c..6c792965cb 100644 --- a/java/com/google/gerrit/server/api/changes/AttentionSetApiImpl.java +++ b/java/com/google/gerrit/server/api/changes/AttentionSetApiImpl.java @@ -17,7 +17,7 @@ package com.google.gerrit.server.api.changes; import static com.google.gerrit.server.api.ApiUtil.asRestApiException; import com.google.gerrit.extensions.api.changes.AttentionSetApi; -import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput; +import com.google.gerrit.extensions.api.changes.AttentionSetInput; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.change.AttentionSetEntryResource; import com.google.gerrit.server.restapi.change.RemoveFromAttentionSet; @@ -41,7 +41,7 @@ public class AttentionSetApiImpl implements AttentionSetApi { } @Override - public void remove(RemoveFromAttentionSetInput input) throws RestApiException { + public void remove(AttentionSetInput input) throws RestApiException { try { removeFromAttentionSet.apply(attentionSetEntryResource, input); } catch (Exception e) { diff --git a/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java b/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java index 5d6ec4c284..fdee17ff5d 100644 --- a/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java +++ b/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java @@ -15,11 +15,13 @@ package com.google.gerrit.server.restapi.change; import com.google.common.base.Strings; -import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput; +import com.google.gerrit.entities.Account; +import com.google.gerrit.extensions.api.changes.AttentionSetInput; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.change.AttentionSetEntryResource; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.RemoveFromAttentionSetOp; @@ -33,20 +35,24 @@ import org.eclipse.jgit.errors.ConfigInvalidException; /** Removes a single user from the attention set. */ public class RemoveFromAttentionSet - implements RestModifyView { + implements RestModifyView { private final BatchUpdate.Factory updateFactory; private final RemoveFromAttentionSetOp.Factory opFactory; + private final AccountResolver accountResolver; @Inject RemoveFromAttentionSet( - BatchUpdate.Factory updateFactory, RemoveFromAttentionSetOp.Factory opFactory) { + BatchUpdate.Factory updateFactory, + RemoveFromAttentionSetOp.Factory opFactory, + AccountResolver accountResolver) { this.updateFactory = updateFactory; this.opFactory = opFactory; + this.accountResolver = accountResolver; } @Override public Response apply( - AttentionSetEntryResource attentionResource, RemoveFromAttentionSetInput input) + AttentionSetEntryResource attentionResource, AttentionSetInput input) throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException, UpdateException { if (input == null) { @@ -56,6 +62,20 @@ public class RemoveFromAttentionSet if (input.reason.isEmpty()) { throw new BadRequestException("missing field: reason"); } + input.user = Strings.nullToEmpty(input.user).trim(); + if (!input.user.isEmpty()) { + Account.Id attentionUserId = null; + try { + attentionUserId = accountResolver.resolve(input.user).asUnique().account().id(); + } catch (AccountResolver.UnresolvableAccountException ex) { + throw new BadRequestException( + "The user specified in the input body couldn't be found.", ex); + } + if (attentionUserId.get() != attentionResource.getAccountId().get()) { + throw new BadRequestException( + "The field \"user\" must be empty, or must match the user specified in the URL."); + } + } ChangeResource changeResource = attentionResource.getChangeResource(); try (BatchUpdate bu = updateFactory.create( diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index d88676a951..d43733687a 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java @@ -32,7 +32,6 @@ import com.google.gerrit.entities.AttentionSetUpdate; import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.AttentionSetInput; import com.google.gerrit.extensions.api.changes.HashtagsInput; -import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -134,7 +133,7 @@ public class AttentionSetIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); change(r).addToAttentionSet(new AttentionSetInput(user.email(), "added")); fakeClock.advance(Duration.ofSeconds(42)); - change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("removed")); + change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed")); AttentionSetUpdate expectedAttentionSetUpdate = AttentionSetUpdate.createFromRead( fakeClock.now(), user.id(), AttentionSetUpdate.Operation.REMOVE, "removed"); @@ -142,12 +141,35 @@ public class AttentionSetIT extends AbstractDaemonTest { // Second removal is ignored. fakeClock.advance(Duration.ofSeconds(42)); - change(r) - .attention(user.id().toString()) - .remove(new RemoveFromAttentionSetInput("removed again")); + change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed again")); assertThat(r.getChange().attentionSet()).containsExactly(expectedAttentionSetUpdate); } + @Test + public void removeUserWithInvalidUserInput() throws Exception { + PushOneCommit.Result r = createChange(); + BadRequestException exception = + assertThrows( + BadRequestException.class, + () -> + change(r) + .attention(user.id().toString()) + .remove(new AttentionSetInput("invalid user", "reason"))); + assertThat(exception.getMessage()) + .isEqualTo("The user specified in the input body couldn't be found."); + + exception = + assertThrows( + BadRequestException.class, + () -> + change(r) + .attention(user.id().toString()) + .remove(new AttentionSetInput(admin.email(), "reason"))); + assertThat(exception.getMessage()) + .isEqualTo( + "The field \"user\" must be empty, or must match the user specified in the URL."); + } + @Test public void changeMessageWhenAddedAndRemovedExplicitly() throws Exception { PushOneCommit.Result r = createChange(); @@ -155,7 +177,7 @@ public class AttentionSetIT extends AbstractDaemonTest { assertThat(Iterables.getLast(r.getChange().messages()).getMessage()) .contains("Added to attention set"); - change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo")); + change(r).attention(user.id().toString()).remove(new AttentionSetInput("foo")); assertThat(Iterables.getLast(r.getChange().messages()).getMessage()) .contains("Removed from attention set"); } @@ -163,7 +185,7 @@ public class AttentionSetIT extends AbstractDaemonTest { @Test public void removeUnrelatedUser() throws Exception { PushOneCommit.Result r = createChange(); - change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo")); + change(r).attention(user.id().toString()).remove(new AttentionSetInput("foo")); assertThat(r.getChange().attentionSet()).isEmpty(); } @@ -315,8 +337,7 @@ public class AttentionSetIT extends AbstractDaemonTest { change(r).addReviewer(user.id().toString()); change(r) .attention(user.id().toString()) - .remove( - new RemoveFromAttentionSetInput("removed and not re-added when re-adding as reviewer")); + .remove(new AttentionSetInput("removed and not re-added when re-adding as reviewer")); change(r).addReviewer(user.id().toString()); @@ -408,7 +429,7 @@ public class AttentionSetIT extends AbstractDaemonTest { public void reviewersAreNotAddedForNoReasonBecauseOfAnUpdate() throws Exception { PushOneCommit.Result r = createChange(); change(r).addToAttentionSet(new AttentionSetInput(user.email(), "user")); - change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("removed")); + change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed")); HashtagsInput hashtagsInput = new HashtagsInput(); hashtagsInput.add = ImmutableSet.of("tag"); @@ -571,7 +592,7 @@ public class AttentionSetIT extends AbstractDaemonTest { ReviewInput reviewInput = ReviewInput.create().setWorkInProgress(true).addUserToAttentionSet(user.email(), "reason"); - change(r).attention(user.email()).remove(new RemoveFromAttentionSetInput("removal")); + change(r).attention(user.email()).remove(new AttentionSetInput("removal")); change(r).current().review(reviewInput); // Attention set updates that relate to the admin (the person who replied) are filtered out. @@ -715,7 +736,7 @@ public class AttentionSetIT extends AbstractDaemonTest { TestAccount user2 = accountCreator.user2(); requestScopeOperations.setApiUser(user2.id()); - change(r).attention(user.email()).remove(new RemoveFromAttentionSetInput("reason")); + change(r).attention(user.email()).remove(new AttentionSetInput("reason")); ReviewInput reviewInput = new ReviewInput(); change(r).current().review(reviewInput); @@ -743,7 +764,7 @@ public class AttentionSetIT extends AbstractDaemonTest { change(r).addReviewer(user.email()); change(r) .attention(user.email()) - .remove(new RemoveFromAttentionSetInput("Reviewer is not in attention-set")); + .remove(new AttentionSetInput("Reviewer is not in attention-set")); TestAccount cc = accountCreator.admin2(); AddReviewerInput input = new AddReviewerInput(); @@ -809,7 +830,7 @@ public class AttentionSetIT extends AbstractDaemonTest { change(r).addReviewer(input); requestScopeOperations.setApiUser(user.id()); - change(r).attention(reviewer.email()).remove(new RemoveFromAttentionSetInput("reason")); + change(r).attention(reviewer.email()).remove(new AttentionSetInput("reason")); ReviewInput reviewInput = new ReviewInput(); change(r).current().review(reviewInput);