From 5836fc02f6f890fd1568d8290a2a87511c396243 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 19 Sep 2017 09:15:10 +0200 Subject: [PATCH] Reject invalid hashtags with 400 Bad Request Instead of failing with 500 Internal Server Error. Bug: Issue 7202 Change-Id: I9ba42bbea786220ff8324d37a9e15564d3f86b14 Signed-off-by: Edwin Kempin --- .../acceptance/rest/change/HashtagsIT.java | 10 +++++ .../gerrit/server/change/HashtagsUtil.java | 16 +++++++- .../gerrit/server/change/SetHashtagsOp.java | 39 ++++++++++--------- 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java index b8c2cf8a6f..08f0699e59 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/HashtagsIT.java @@ -30,6 +30,7 @@ import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.testutil.TestTimeUtil; import org.junit.AfterClass; import org.junit.Before; @@ -76,6 +77,15 @@ public class HashtagsIT extends AbstractDaemonTest { assertMessage(r, "Hashtag added: tag1"); } + @Test + public void addInvalidHashtag() throws Exception { + PushOneCommit.Result r = createChange(); + + exception.expect(BadRequestException.class); + exception.expectMessage("hashtags may not contain commas"); + addHashtags(r, "invalid,hashtag"); + } + @Test public void addMultipleHashtags() throws Exception { PushOneCommit.Result r = createChange(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/HashtagsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/HashtagsUtil.java index e9b0af28d6..42b56b696b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/HashtagsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/HashtagsUtil.java @@ -23,6 +23,18 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; public class HashtagsUtil { + public static class InvalidHashtagException extends Exception { + private static final long serialVersionUID = 1L; + + static InvalidHashtagException hashtagsMayNotContainCommas() { + return new InvalidHashtagException("hashtags may not contain commas"); + } + + InvalidHashtagException(String message) { + super(message); + } + } + private static final CharMatcher LEADER = CharMatcher.whitespace().or(CharMatcher.is('#')); private static final String PATTERN = "(?:\\s|\\A)#[\\p{L}[0-9]-_]+"; @@ -43,14 +55,14 @@ public class HashtagsUtil { return result; } - static Set extractTags(Set input) throws IllegalArgumentException { + static Set extractTags(Set input) throws InvalidHashtagException { if (input == null) { return Collections.emptySet(); } HashSet result = new HashSet<>(); for (String hashtag : input) { if (hashtag.contains(",")) { - throw new IllegalArgumentException("Hashtags may not contain commas"); + throw InvalidHashtagException.hashtagsMayNotContainCommas(); } hashtag = cleanupHashtag(hashtag); if (!hashtag.isEmpty()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java index 724598a921..1f17dd349c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java @@ -29,6 +29,7 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.change.HashtagsUtil.InvalidHashtagException; import com.google.gerrit.server.extensions.events.HashtagsEdited; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; @@ -99,31 +100,31 @@ public class SetHashtagsOp implements BatchUpdateOp { ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId()); ChangeNotes notes = update.getNotes().load(); - Set existingHashtags = notes.getHashtags(); - Set updated = new HashSet<>(); - toAdd = new HashSet<>(extractTags(input.add)); - toRemove = new HashSet<>(extractTags(input.remove)); - try { + Set existingHashtags = notes.getHashtags(); + Set updated = new HashSet<>(); + toAdd = new HashSet<>(extractTags(input.add)); + toRemove = new HashSet<>(extractTags(input.remove)); + for (HashtagValidationListener validator : validationListeners) { validator.validateHashtags(update.getChange(), toAdd, toRemove); } - } catch (ValidationException e) { + + updated.addAll(existingHashtags); + toAdd.removeAll(existingHashtags); + toRemove.retainAll(existingHashtags); + if (updated()) { + updated.addAll(toAdd); + updated.removeAll(toRemove); + update.setHashtags(updated); + addMessage(ctx, update); + } + + updatedHashtags = ImmutableSortedSet.copyOf(updated); + return true; + } catch (ValidationException | InvalidHashtagException e) { throw new BadRequestException(e.getMessage()); } - - updated.addAll(existingHashtags); - toAdd.removeAll(existingHashtags); - toRemove.retainAll(existingHashtags); - if (updated()) { - updated.addAll(toAdd); - updated.removeAll(toRemove); - update.setHashtags(updated); - addMessage(ctx, update); - } - - updatedHashtags = ImmutableSortedSet.copyOf(updated); - return true; } private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {