opendev/gerrit
Code Issues Proposed changes

Refactor NotifyUtil analogously to AccountResolver

Browse Source 
NotifyUtil#resolveAccounts was basically a wrapper around
AccountResolver. Give the class a new name, NotifyResolver, and have it
resolve inputs to a Result type. This Result type encapsulates both the
NotifyHandling enum and the ListMultimap that got passed around
everywhere. This has several significant advantages.

First, this pair of objects is used in many places, so cutting down two
arguments (including the unwieldy ListMultimap type) to one is a win on
its own. This change fixes some such methods, and the rest will be fixed
in future cleanups.

Second and more subtly, this change results in more explicit and less
error-prone behavior when a null notify field is passed in a REST API
object. Callers have to explicitly specify a non-null NotifyHandling
input to NotifyResolver#resolve, which typically means they have to do a
firstNonNull(input.notify, <some default>). It turns out that in the
REST API docs, there are several different defaults for the different
*Input types, including some that have different behavior for WIP/ready
changes. The documentation is structured per Input type, which means
it's always clear from the documentation what the default should be if
you have in front of you a FooInput. In other words, this pattern makes
it easier to inspect the code for correctness.

Change-Id: I609674d22b6a16b8fac32aeea1a57d293ee601d5
This commit is contained in:
Dave Borowitz
2019-01-30 12:13:22 -08:00
parent a9fc40ccdc
commit 3949eea76e
22 changed files with 262 additions and 299 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
java/com/google/gerrit/server/change/AbandonOp.java
View File

@@ -15,13 +15,9 @@
package com.google.gerrit.server.change;
import com.google.common.base.Strings;
import com.google.common.collect.ListMultimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -49,8 +45,7 @@ public class AbandonOp implements BatchUpdateOp {
private final ChangeAbandoned changeAbandoned;
private final String msgTxt;
private final NotifyHandling notifyHandling;
private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
private final NotifyResolver.Result notify;
private final AccountState accountState;
private Change change;
@@ -61,8 +56,7 @@ public class AbandonOp implements BatchUpdateOp {
AbandonOp create(
@Assisted @Nullable AccountState accountState,
@Assisted @Nullable String msgTxt,
@Assisted NotifyHandling notifyHandling,
@Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify);
@Assisted NotifyResolver.Result notify);
}
@Inject
@@ -73,8 +67,7 @@ public class AbandonOp implements BatchUpdateOp {
ChangeAbandoned changeAbandoned,
@Assisted @Nullable AccountState accountState,
@Assisted @Nullable String msgTxt,
@Assisted NotifyHandling notifyHandling,
@Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify) {
@Assisted NotifyResolver.Result notify) {
this.abandonedSenderFactory = abandonedSenderFactory;
this.cmUtil = cmUtil;
this.psUtil = psUtil;
@@ -82,8 +75,7 @@ public class AbandonOp implements BatchUpdateOp {
this.accountState = accountState;
this.msgTxt = Strings.nullToEmpty(msgTxt);
this.notifyHandling = notifyHandling;
this.accountsToNotify = accountsToNotify;
this.notify = notify;
}
@Nullable
@@ -128,12 +120,12 @@ public class AbandonOp implements BatchUpdateOp {
cm.setFrom(accountState.getAccount().getId());
}
cm.setChangeMessage(message.getMessage(), ctx.getWhen());
cm.setNotify(notifyHandling);
cm.setAccountsToNotify(accountsToNotify);
cm.setNotify(notify.handling());
cm.setAccountsToNotify(notify.accounts());
cm.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notifyHandling);
changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling());
}
}