opendev/gerrit
Code Issues Proposed changes

Set hashtags using a BatchUpdate.Op

Browse Source 
Rather than having a utility method that creates a new ChangeUpdate on
invocation, get the update from the ChangeContext. Also split out the
post-update steps into postUpdate.

The end result is admittedly a little convoluted, requiring assisted
injection just to create the op to pass to the BatchUpdate. (It should
still be a little shorter than the original.) However, we don't expect
most new Ops to require assisted injection, since they can typically
get everything they need from the context objects.

Change-Id: I4d6024dd00b00bbc1825e700b1eda0923f29d278
This commit is contained in:
Dave Borowitz 2015-10-13 13:40:25 -04:00
parent ed1523094d
commit bce0e922c4
10 changed files with 203 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/HashtagsInput.java
View File

@ -22,4 +22,11 @@ public class HashtagsInput {
@DefaultInput
public Set<String> add;
public Set<String> remove;
public HashtagsInput(){
}
public HashtagsInput(Set<String> add) {
this.add = add;
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@ -307,7 +307,7 @@ class ChangeApiImpl implements ChangeApi {
public void setHashtags(HashtagsInput input) throws RestApiException {
try {
postHashtags.apply(change, input);
} catch (IOException | OrmException e) {
} catch (RestApiException | UpdateException e) {
throw new RestApiException("Cannot post hashtags", e);
}
}

35
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@ -21,8 +21,6 @@ import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@ -34,7 +32,6 @@ import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.BanCommit;
import com.google.gerrit.server.git.BatchUpdate;
@ -53,7 +50,6 @@ import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@ -83,8 +79,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
private final ApprovalsUtil approvalsUtil;
private final ChangeMessagesUtil cmUtil;
private final CreateChangeSender.Factory createChangeSenderFactory;
private final HashtagsUtil hashtagsUtil;
private final AccountCache accountCache;
private final WorkQueue workQueue;
private final CommitValidators.Factory commitValidatorsFactory;
@ -101,7 +95,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
private Set<Account.Id> reviewers;
private Set<Account.Id> extraCC;
private Map<String, Short> approvals;
private Set<String> hashtags;
private RequestScopePropagator requestScopePropagator;
private boolean runHooks;
private boolean sendMail;
@ -117,8 +110,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
CreateChangeSender.Factory createChangeSenderFactory,
HashtagsUtil hashtagsUtil,
AccountCache accountCache,
WorkQueue workQueue,
CommitValidators.Factory commitValidatorsFactory,
@Assisted RefControl refControl,
@ -136,8 +127,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
this.approvalsUtil = approvalsUtil;
this.cmUtil = cmUtil;
this.createChangeSenderFactory = createChangeSenderFactory;
this.hashtagsUtil = hashtagsUtil;
this.accountCache = accountCache;
this.workQueue = workQueue;
this.commitValidatorsFactory = commitValidatorsFactory;
@ -147,7 +136,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
this.reviewers = Collections.emptySet();
this.extraCC = Collections.emptySet();
this.approvals = Collections.emptyMap();
this.hashtags = Collections.emptySet();
this.runHooks = true;
this.sendMail = true;
this.updateRef = true;
@ -206,11 +194,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
return this;
}
public ChangeInserter setHashtags(Set<String> hashtags) {
this.hashtags = hashtags;
return this;
}
public ChangeInserter setRunHooks(boolean runHooks) {
this.runHooks = runHooks;
return this;
@ -287,18 +270,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
changeMessage.setMessage(message);
cmUtil.addChangeMessage(db, update, changeMessage);
}
if (hashtags != null && hashtags.size() > 0) {
try {
HashtagsInput input = new HashtagsInput();
input.add = hashtags;
// TODO(dborowitz): Migrate HashtagsUtil so it doesn't create another
// ChangeUpdate.
hashtagsUtil.setHashtags(ctl, input, false, false);
} catch (ValidationException | AuthException e) {
log.error("Cannot add hashtags to change " + change.getId(), e);
}
}
}
@Override
@ -335,12 +306,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
if (runHooks) {
ReviewDb db = ctx.getDb();
hooks.doPatchsetCreatedHook(change, patchSet, db);
if (hashtags != null && hashtags.size() > 0) {
hooks.doHashtagsChangedHook(change,
accountCache.get(change.getOwner()).getAccount(),
hashtags, null, hashtags, db);
}
if (approvals != null && !approvals.isEmpty()) {
hooks.doCommentAddedHook(change,
((IdentifiedUser) refControl.getCurrentUser()).getAccount(),

90
gerrit-server/src/main/java/com/google/gerrit/server/change/HashtagsUtil.java
View File

@ -16,56 +16,18 @@ package com.google.gerrit.server.change;
import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.validators.HashtagValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.TreeSet;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@Singleton
public class HashtagsUtil {
private static final CharMatcher LEADER =
CharMatcher.whitespace().or(CharMatcher.is('#'));
private static final String PATTERN = "(?:\\s|\\A)#[\\p{L}[0-9]-_]+";
private final ChangeUpdate.Factory updateFactory;
private final Provider<ReviewDb> dbProvider;
private final ChangeIndexer indexer;
private final ChangeHooks hooks;
private final DynamicSet<HashtagValidationListener> hashtagValidationListeners;
@Inject
HashtagsUtil(ChangeUpdate.Factory updateFactory,
Provider<ReviewDb> dbProvider,
ChangeIndexer indexer,
ChangeHooks hooks,
DynamicSet<HashtagValidationListener> hashtagValidationListeners) {
this.updateFactory = updateFactory;
this.dbProvider = dbProvider;
this.indexer = indexer;
this.hooks = hooks;
this.hashtagValidationListeners = hashtagValidationListeners;
}
public static String cleanupHashtag(String hashtag) {
hashtag = LEADER.trimLeadingFrom(hashtag);
hashtag = CharMatcher.whitespace().trimTrailingFrom(hashtag);
@ -83,7 +45,7 @@ public class HashtagsUtil {
return result;
}
private Set<String> extractTags(Set<String> input)
static Set<String> extractTags(Set<String> input)
throws IllegalArgumentException {
if (input == null) {
return Collections.emptySet();
@ -102,54 +64,6 @@ public class HashtagsUtil {
}
}
public TreeSet<String> setHashtags(ChangeControl control,
HashtagsInput input, boolean runHooks, boolean index)
throws IllegalArgumentException, IOException,
ValidationException, AuthException, OrmException {
if (input == null
|| (input.add == null && input.remove == null)) {
throw new IllegalArgumentException("Hashtags are required");
}
if (!control.canEditHashtags()) {
throw new AuthException("Editing hashtags not permitted");
}
ChangeUpdate update = updateFactory.create(control);
ChangeNotes notes = control.getNotes().load();
Set<String> existingHashtags = notes.getHashtags();
Set<String> updatedHashtags = new HashSet<>();
Set<String> toAdd = new HashSet<>(extractTags(input.add));
Set<String> toRemove = new HashSet<>(extractTags(input.remove));
for (HashtagValidationListener validator : hashtagValidationListeners) {
validator.validateHashtags(update.getChange(), toAdd, toRemove);
}
if (existingHashtags != null && !existingHashtags.isEmpty()) {
updatedHashtags.addAll(existingHashtags);
toAdd.removeAll(existingHashtags);
toRemove.retainAll(existingHashtags);
}
if (toAdd.size() > 0 || toRemove.size() > 0) {
updatedHashtags.addAll(toAdd);
updatedHashtags.removeAll(toRemove);
update.setHashtags(updatedHashtags);
update.commit();
if (index) {
indexer.index(dbProvider.get(), update.getChange());
}
if (runHooks) {
IdentifiedUser currentUser = ((IdentifiedUser) control.getCurrentUser());
hooks.doHashtagsChangedHook(
update.getChange(), currentUser.getAccount(),
toAdd, toRemove, updatedHashtags,
dbProvider.get());
}
}
return new TreeSet<>(updatedHashtags);
private HashtagsUtil() {
}
}

9
gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
View File

@ -119,12 +119,13 @@ public class Module extends RestApiModule {
get(CHANGE_EDIT_KIND, "/").to(ChangeEdits.Get.class);
get(CHANGE_EDIT_KIND, "meta").to(ChangeEdits.GetMeta.class);
factory(ReviewerResource.Factory.class);
factory(AccountLoader.Factory.class);
factory(EmailReviewComments.Factory.class);
factory(ChangeInserter.Factory.class);
factory(PatchSetInserter.Factory.class);
factory(ChangeEdits.Create.Factory.class);
factory(ChangeEdits.DeleteFile.Factory.class);
factory(ChangeInserter.Factory.class);
factory(EmailReviewComments.Factory.class);
factory(PatchSetInserter.Factory.class);
factory(ReviewerResource.Factory.class);
factory(SetHashtagsOp.Factory.class);
}
}

54
gerrit-server/src/main/java/com/google/gerrit/server/change/PostHashtags.java
View File

@ -14,43 +14,47 @@
package com.google.gerrit.server.change;
import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
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.extensions.webui.UiAction;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.UpdateException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Set;
@Singleton
public class PostHashtags implements RestModifyView<ChangeResource, HashtagsInput>,
UiAction<ChangeResource>{
private HashtagsUtil hashtagsUtil;
public class PostHashtags
implements RestModifyView<ChangeResource, HashtagsInput>,
UiAction<ChangeResource> {
private final Provider<ReviewDb> db;
private final BatchUpdate.Factory batchUpdateFactory;
private final SetHashtagsOp.Factory hashtagsFactory;
@Inject
PostHashtags(HashtagsUtil hashtagsUtil) {
this.hashtagsUtil = hashtagsUtil;
PostHashtags(Provider<ReviewDb> db,
BatchUpdate.Factory batchUpdateFactory,
SetHashtagsOp.Factory hashtagsFactory) {
this.db = db;
this.batchUpdateFactory = batchUpdateFactory;
this.hashtagsFactory = hashtagsFactory;
}
@Override
public Response<Set<String>> apply(ChangeResource req, HashtagsInput input)
throws AuthException, OrmException, IOException, BadRequestException,
ResourceConflictException {
try {
return Response.<Set<String>> ok(hashtagsUtil.setHashtags(
req.getControl(), input, true, true));
} catch (IllegalArgumentException e) {
throw new BadRequestException(e.getMessage());
} catch (ValidationException e) {
throw new ResourceConflictException(e.getMessage());
public Response<ImmutableSortedSet<String>> apply(ChangeResource req,
HashtagsInput input) throws RestApiException, UpdateException {
try (BatchUpdate bu = batchUpdateFactory.create(db.get(),
req.getChange().getProject(), req.getControl().getCurrentUser(),
TimeUtil.nowTs())) {
SetHashtagsOp op = hashtagsFactory.create(input);
bu.addOp(req.getChange().getId(), op);
bu.execute();
return Response.<ImmutableSortedSet<String>> ok(op.getUpdatedHashtags());
}
}
@ -60,4 +64,4 @@ public class PostHashtags implements RestModifyView<ChangeResource, HashtagsInpu
.setLabel("Edit Hashtags")
.setVisible(resource.getControl().canEditHashtags());
}
}
}

142
gerrit-server/src/main/java/com/google/gerrit/server/change/SetHashtagsOp.java Normal file
View File

@ -0,0 +1,142 @@
// Copyright (C) 2015 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.server.change;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.change.HashtagsUtil.extractTags;
import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.BatchUpdate.Context;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.validators.HashtagValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
public class SetHashtagsOp extends BatchUpdate.Op {
public interface Factory {
SetHashtagsOp create(HashtagsInput input);
}
private final ChangeHooks hooks;
private final DynamicSet<HashtagValidationListener> validationListeners;
private final HashtagsInput input;
private boolean runHooks = true;
private Change change;
private Set<String> toAdd;
private Set<String> toRemove;
private ImmutableSortedSet<String> updatedHashtags;
@AssistedInject
SetHashtagsOp(
ChangeHooks hooks,
DynamicSet<HashtagValidationListener> validationListeners,
@Assisted @Nullable HashtagsInput input) {
this.hooks = hooks;
this.validationListeners = validationListeners;
this.input = input;
}
public SetHashtagsOp setRunHooks(boolean runHooks) {
this.runHooks = runHooks;
return this;
}
@Override
public void updateChange(ChangeContext ctx)
throws AuthException, BadRequestException, OrmException, IOException {
if (input == null
|| (input.add == null && input.remove == null)) {
updatedHashtags = ImmutableSortedSet.of();
return;
}
if (!ctx.getChangeControl().canEditHashtags()) {
throw new AuthException("Editing hashtags not permitted");
}
ChangeUpdate update = ctx.getChangeUpdate();
ChangeNotes notes = update.getChangeNotes().load();
Set<String> existingHashtags = notes.getHashtags();
Set<String> updated = new HashSet<>();
toAdd = new HashSet<>(extractTags(input.add));
toRemove = new HashSet<>(extractTags(input.remove));
try {
for (HashtagValidationListener validator : validationListeners) {
validator.validateHashtags(update.getChange(), toAdd, toRemove);
}
} catch (ValidationException e) {
throw new BadRequestException(e.getMessage());
}
if (existingHashtags != null && !existingHashtags.isEmpty()) {
updated.addAll(existingHashtags);
toAdd.removeAll(existingHashtags);
toRemove.retainAll(existingHashtags);
}
if (updated()) {
updated.addAll(toAdd);
updated.removeAll(toRemove);
update.setHashtags(updated);
}
change = update.getChange();
updatedHashtags = ImmutableSortedSet.copyOf(updated);
}
@Override
public void postUpdate(Context ctx) throws OrmException {
if (updated() && runHooks) {
IdentifiedUser currentUser = (IdentifiedUser) ctx.getUser();
hooks.doHashtagsChangedHook(
change, currentUser.getAccount(),
toAdd, toRemove, updatedHashtags,
ctx.getDb());
}
}
public ImmutableSortedSet<String> getUpdatedHashtags() {
checkState(updatedHashtags != null,
"getUpdatedHashtags() only valid after executing op");
return updatedHashtags;
}
private boolean updated() {
return !isNullOrEmpty(toAdd) || !isNullOrEmpty(toRemove);
}
private static boolean isNullOrEmpty(Collection<?> coll) {
return coll == null || coll.isEmpty();
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
View File

@ -85,6 +85,10 @@ public class BatchUpdate implements AutoCloseable {
public ReviewDb getDb() {
return db;
}
public CurrentUser getUser() {
return user;
}
}
public class RepoContext extends Context {
@ -136,10 +140,6 @@ public class BatchUpdate implements AutoCloseable {
public Change getChange() {
return update.getChange();
}
public CurrentUser getUser() {
return update.getUser();
}
}
public static class Op {

14
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -62,6 +62,7 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicMap.Entry;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@ -90,6 +91,7 @@ import com.google.gerrit.server.change.ChangeKind;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.SetHashtagsOp;
import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.CanonicalWebUrl;
@ -308,6 +310,7 @@ public class ReceiveCommits {
private final ReceiveConfig receiveConfig;
private final ChangeKindCache changeKindCache;
private final BatchUpdate.Factory batchUpdateFactory;
private final SetHashtagsOp.Factory hashtagsFactory;
private final ProjectControl projectControl;
private final Project project;
@ -385,7 +388,8 @@ public class ReceiveCommits {
final DynamicMap<ProjectConfigEntry> pluginConfigEntries,
final NotesMigration notesMigration,
final ChangeEditUtil editUtil,
final BatchUpdate.Factory batchUpdateFactory) throws IOException {
final BatchUpdate.Factory batchUpdateFactory,
final SetHashtagsOp.Factory hashtagsFactory) throws IOException {
this.currentUser = (IdentifiedUser) projectControl.getCurrentUser();
this.db = db;
this.queryProvider = queryProvider;
@ -419,6 +423,7 @@ public class ReceiveCommits {
this.receiveConfig = config;
this.changeKindCache = changeKindCache;
this.batchUpdateFactory = batchUpdateFactory;
this.hashtagsFactory = hashtagsFactory;
this.projectControl = projectControl;
this.labelTypes = projectControl.getLabelTypes();
@ -1765,7 +1770,6 @@ public class ReceiveCommits {
if (magicBranch != null) {
recipients.add(magicBranch.getMailRecipients());
approvals = magicBranch.labels;
ins.setHashtags(magicBranch.hashtags);
}
recipients.add(getRecipientsFromFooters(accountResolver, ps, footerLines));
recipients.remove(me);
@ -1783,6 +1787,12 @@ public class ReceiveCommits {
.setRequestScopePropagator(requestScopePropagator)
.setSendMail(true)
.setUpdateRef(false));
if (magicBranch != null) {
bu.addOp(
ins.getChange().getId(),
hashtagsFactory.create(new HashtagsInput(magicBranch.hashtags))
.setRunHooks(false));
}
bu.execute();
}
created = true;

2
gerrit-server/src/test/java/com/google/gerrit/server/change/HashtagsTest.java
View File

@ -28,7 +28,7 @@ public class HashtagsTest {
@Test
public void nullCommitMessage() throws Exception {
assertThat(HashtagsUtil.extractTags(null)).isEmpty();
assertThat(HashtagsUtil.extractTags((String) null)).isEmpty();
}
@Test