Don't use List in GerritConfigListener API

Currently GerritConfigListener's configUpdated method returns a List of
ConfigUpdatedEvent.Update.

There are only two types of updates (applied and rejected)
and the API returning lists of updates does not reflect this but
gives the impression that there can be an arbitrary amount
of update types.

Instead of returning a List of ConfigUpdatedEvent.Update that each have
a type, return a Multimap<UpdateResult, ConfigUpdateEntry>.

Removed the ConfigUpdatedEvent.Update class.

Change-Id: I81b4327a5878739087cd8a2ea8c0cff035b6ee57

java/com/google/gerrit/server/config/ConfigUpdatedEvent.java

@@ -13,10 +13,11 @@
 // limitations under the License.
 package com.google.gerrit.server.config;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.Multimap;
 import java.util.Collections;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import org.apache.commons.lang.StringUtils;
@@ -36,6 +37,8 @@ import org.eclipse.jgit.lib.Config;
  * (+ various overloaded versions of these)
  */
 public class ConfigUpdatedEvent {
+  public static final Multimap<UpdateResult, ConfigUpdateEntry> NO_UPDATES =
+      new ImmutableMultimap.Builder<UpdateResult, ConfigUpdateEntry>().build();
   private final Config oldConfig;
   private final Config newConfig;
 
@@ -52,25 +55,29 @@ public class ConfigUpdatedEvent {
     return this.newConfig;
   }
 
-  public Update accept(ConfigKey entry) {
+  private String getString(ConfigKey key, Config config) {
+    return config.getString(key.section(), key.subsection(), key.name());
+  }
+
+  public Multimap<UpdateResult, ConfigUpdateEntry> accept(ConfigKey entry) {
     return accept(Collections.singleton(entry));
   }
 
-  public Update accept(Set<ConfigKey> entries) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> accept(Set<ConfigKey> entries) {
     return createUpdate(entries, UpdateResult.APPLIED);
   }
 
-  public Update accept(String section) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> accept(String section) {
     Set<ConfigKey> entries = getEntriesFromSection(oldConfig, section);
     entries.addAll(getEntriesFromSection(newConfig, section));
     return createUpdate(entries, UpdateResult.APPLIED);
   }
 
-  public Update reject(ConfigKey entry) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> reject(ConfigKey entry) {
     return reject(Collections.singleton(entry));
   }
 
-  public Update reject(Set<ConfigKey> entries) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> reject(Set<ConfigKey> entries) {
     return createUpdate(entries, UpdateResult.REJECTED);
   }
 
@@ -87,20 +94,15 @@ public class ConfigUpdatedEvent {
     return res;
   }
 
-  private Update createUpdate(Set<ConfigKey> entries, UpdateResult updateResult) {
-    Update update = new Update(updateResult);
+  private Multimap<UpdateResult, ConfigUpdateEntry> createUpdate(
+      Set<ConfigKey> entries, UpdateResult updateResult) {
+    Multimap<UpdateResult, ConfigUpdateEntry> updates = ArrayListMultimap.create();
     entries
         .stream()
         .filter(this::isValueUpdated)
-        .forEach(
-            key -> {
-              update.addConfigUpdate(
-                  new ConfigUpdateEntry(
-                      key,
-                      oldConfig.getString(key.section(), key.subsection(), key.name()),
-                      newConfig.getString(key.section(), key.subsection(), key.name())));
-            });
-    return update;
+        .map(e -> new ConfigUpdateEntry(e, getString(e, oldConfig), getString(e, newConfig)))
+        .forEach(e -> updates.put(updateResult, e));
+    return updates;
   }
 
   public boolean isSectionUpdated(String section) {
@@ -142,31 +144,6 @@ public class ConfigUpdatedEvent {
     }
   }
 
-  /**
-   * One Accepted/Rejected Update have one or more config updates (ConfigUpdateEntry) tied to it.
-   */
-  public static class Update {
-    private UpdateResult result;
-    private final Set<ConfigUpdateEntry> configUpdates;
-
-    public Update(UpdateResult result) {
-      this.configUpdates = new LinkedHashSet<>();
-      this.result = result;
-    }
-
-    public UpdateResult getResult() {
-      return result;
-    }
-
-    public List<ConfigUpdateEntry> getConfigUpdates() {
-      return ImmutableList.copyOf(configUpdates);
-    }
-
-    public void addConfigUpdate(ConfigUpdateEntry entry) {
-      this.configUpdates.add(entry);
-    }
-  }
-
   public enum ConfigEntryType {
     ADDED,
     REMOVED,

java/com/google/gerrit/server/config/GerritConfigListener.java

@@ -14,9 +14,11 @@
 
 package com.google.gerrit.server.config;
 
+import com.google.common.collect.Multimap;
 import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import java.util.EventListener;
-import java.util.List;
 
 /**
  * Implementations of the GerritConfigListener interface expects to react GerritServerConfig
@@ -24,5 +26,5 @@ import java.util.List;
  */
 @ExtensionPoint
 public interface GerritConfigListener extends EventListener {
-  List<ConfigUpdatedEvent.Update> configUpdated(ConfigUpdatedEvent event);
+  Multimap<UpdateResult, ConfigUpdateEntry> configUpdated(ConfigUpdatedEvent event);
 }

java/com/google/gerrit/server/config/GerritConfigListenerHelper.java

@@ -15,13 +15,12 @@
 package com.google.gerrit.server.config;
 
 import com.google.common.collect.ImmutableSet;
-import java.util.Collections;
 
 public class GerritConfigListenerHelper {
   public static GerritConfigListener acceptIfChanged(ConfigKey... keys) {
     return e ->
         e.isEntriesUpdated(ImmutableSet.copyOf(keys))
-            ? Collections.singletonList(e.accept(ImmutableSet.copyOf(keys)))
-            : Collections.emptyList();
+            ? e.accept(ImmutableSet.copyOf(keys))
+            : ConfigUpdatedEvent.NO_UPDATES;
   }
 }

java/com/google/gerrit/server/config/GerritServerConfigReloader.java

@@ -14,12 +14,14 @@
 
 package com.google.gerrit.server.config;
 
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.Multimap;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
-import java.util.ArrayList;
-import java.util.List;
 
 /** Issues a configuration reload from the GerritServerConfigProvider and notify all listeners. */
 @Singleton
@@ -40,18 +42,20 @@ public class GerritServerConfigReloader {
    * Reloads the Gerrit Server Configuration from disk. Synchronized to ensure that one issued
    * reload is fully completed before a new one starts.
    */
-  public List<ConfigUpdatedEvent.Update> reloadConfig() {
+  public Multimap<UpdateResult, ConfigUpdateEntry> reloadConfig() {
     logger.atInfo().log("Starting server configuration reload");
-    List<ConfigUpdatedEvent.Update> updates = fireUpdatedConfigEvent(configProvider.updateConfig());
+    Multimap<UpdateResult, ConfigUpdateEntry> updates =
+        fireUpdatedConfigEvent(configProvider.updateConfig());
     logger.atInfo().log("Server configuration reload completed succesfully");
     return updates;
   }
 
-  public List<ConfigUpdatedEvent.Update> fireUpdatedConfigEvent(ConfigUpdatedEvent event) {
-    ArrayList<ConfigUpdatedEvent.Update> result = new ArrayList<>();
+  public Multimap<UpdateResult, ConfigUpdateEntry> fireUpdatedConfigEvent(
+      ConfigUpdatedEvent event) {
+    Multimap<UpdateResult, ConfigUpdateEntry> updates = ArrayListMultimap.create();
     for (GerritConfigListener configListener : configListeners) {
-      result.addAll(configListener.configUpdated(event));
+      updates.putAll(configListener.configUpdated(event));
     }
-    return result;
+    return updates;
   }
 }

java/com/google/gerrit/server/project/CommentLinkProvider.java

@@ -16,15 +16,17 @@ package com.google.gerrit.server.project;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Multimap;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.extensions.api.projects.CommentLinkInfo;
 import com.google.gerrit.server.config.ConfigUpdatedEvent;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.gerrit.server.config.GerritConfigListener;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
-import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 import org.eclipse.jgit.lib.Config;
@@ -64,11 +66,11 @@ public class CommentLinkProvider implements Provider<List<CommentLinkInfo>>, Ger
   }
 
   @Override
-  public List<ConfigUpdatedEvent.Update> configUpdated(ConfigUpdatedEvent event) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> configUpdated(ConfigUpdatedEvent event) {
     if (event.isSectionUpdated(ProjectConfig.COMMENTLINK)) {
       commentLinks = parseConfig(event.getNewConfig());
-      return Collections.singletonList(event.accept(ProjectConfig.COMMENTLINK));
+      return event.accept(ProjectConfig.COMMENTLINK);
     }
-    return Collections.emptyList();
+    return ConfigUpdatedEvent.NO_UPDATES;
   }
 }

java/com/google/gerrit/server/restapi/config/ReloadConfig.java

@@ -16,12 +16,12 @@ package com.google.gerrit.server.restapi.config;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
+import com.google.common.collect.Multimap;
 import com.google.gerrit.extensions.api.config.ConfigUpdateEntryInfo;
 import com.google.gerrit.extensions.common.Input;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.server.config.ConfigResource;
-import com.google.gerrit.server.config.ConfigUpdatedEvent;
 import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
 import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.gerrit.server.config.GerritServerConfigReloader;
@@ -29,10 +29,11 @@ import com.google.gerrit.server.permissions.GlobalPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
-import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 public class ReloadConfig implements RestModifyView<ConfigResource, Input> {
 
@@ -49,25 +50,22 @@ public class ReloadConfig implements RestModifyView<ConfigResource, Input> {
   public Map<String, List<ConfigUpdateEntryInfo>> apply(ConfigResource resource, Input input)
       throws RestApiException, PermissionBackendException {
     permissions.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
-
-    List<ConfigUpdatedEvent.Update> updates = config.reloadConfig();
-
-    Map<String, List<ConfigUpdateEntryInfo>> reply = new HashMap<>();
-    for (UpdateResult result : UpdateResult.values()) {
-      reply.put(result.name().toLowerCase(), new ArrayList<>());
-    }
+    Multimap<UpdateResult, ConfigUpdateEntry> updates = config.reloadConfig();
     if (updates.isEmpty()) {
-      return reply;
+      return Collections.emptyMap();
     }
-    updates
+    return updates
+        .asMap()
+        .entrySet()
         .stream()
-        .forEach(u -> reply.get(u.getResult().name().toLowerCase()).addAll(toEntryInfos(u)));
-    return reply;
+        .collect(
+            Collectors.toMap(
+                e -> e.getKey().name().toLowerCase(), e -> toEntryInfos(e.getValue())));
   }
 
-  private static List<ConfigUpdateEntryInfo> toEntryInfos(ConfigUpdatedEvent.Update update) {
-    return update
-        .getConfigUpdates()
+  private static List<ConfigUpdateEntryInfo> toEntryInfos(
+      Collection<ConfigUpdateEntry> updateEntries) {
+    return updateEntries
         .stream()
         .map(ReloadConfig::toConfigUpdateEntryInfo)
         .collect(toImmutableList());

java/com/google/gerrit/server/restapi/project/SetParent.java

@@ -19,6 +19,7 @@ import static java.util.Objects.requireNonNull;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Multimap;
 import com.google.gerrit.extensions.api.projects.ParentInput;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -32,6 +33,8 @@ import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.ConfigKey;
 import com.google.gerrit.server.config.ConfigUpdatedEvent;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.gerrit.server.config.GerritConfigListener;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.meta.MetaDataUpdate;
@@ -46,8 +49,6 @@ import com.google.gerrit.server.project.ProjectState;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
-import java.util.Collections;
-import java.util.List;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Config;
@@ -172,18 +173,18 @@ public class SetParent
   }
 
   @Override
-  public List<ConfigUpdatedEvent.Update> configUpdated(ConfigUpdatedEvent event) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> configUpdated(ConfigUpdatedEvent event) {
     ConfigKey receiveSetParent = ConfigKey.create("receive", "allowProjectOwnersToChangeParent");
     if (!event.isValueUpdated(receiveSetParent)) {
-      return Collections.emptyList();
+      return ConfigUpdatedEvent.NO_UPDATES;
     }
     try {
       boolean enabled =
           event.getNewConfig().getBoolean("receive", "allowProjectOwnersToChangeParent", false);
       this.allowProjectOwnersToChangeParent = enabled;
-      return Collections.singletonList(event.accept(receiveSetParent));
     } catch (IllegalArgumentException iae) {
-      return Collections.singletonList(event.reject(receiveSetParent));
+      return event.reject(receiveSetParent);
     }
+    return event.accept(receiveSetParent);
   }
 }

java/com/google/gerrit/sshd/SshLog.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.sshd;
 
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.server.CurrentUser;
@@ -24,6 +25,8 @@ import com.google.gerrit.server.audit.AuditService;
 import com.google.gerrit.server.audit.SshAuditEvent;
 import com.google.gerrit.server.config.ConfigKey;
 import com.google.gerrit.server.config.ConfigUpdatedEvent;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.gerrit.server.config.GerritConfigListener;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.ioutil.HexFormat;
@@ -33,8 +36,6 @@ import com.google.gerrit.sshd.SshScope.Context;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
-import java.util.Collections;
-import java.util.List;
 import org.apache.log4j.AsyncAppender;
 import org.apache.log4j.Level;
 import org.apache.log4j.Logger;
@@ -318,25 +319,22 @@ class SshLog implements LifecycleListener, GerritConfigListener {
   }
 
   @Override
-  public List<ConfigUpdatedEvent.Update> configUpdated(ConfigUpdatedEvent event) {
+  public Multimap<UpdateResult, ConfigUpdateEntry> configUpdated(ConfigUpdatedEvent event) {
     ConfigKey sshdRequestLog = ConfigKey.create("sshd", "requestLog");
     if (!event.isValueUpdated(sshdRequestLog)) {
-      return Collections.emptyList();
+      return ConfigUpdatedEvent.NO_UPDATES;
     }
     boolean stateUpdated;
     try {
       boolean enabled = event.getNewConfig().getBoolean("sshd", "requestLog", true);
-
       if (enabled) {
         stateUpdated = enableLogging();
       } else {
         stateUpdated = disableLogging();
       }
-      return stateUpdated
-          ? Collections.singletonList(event.accept(sshdRequestLog))
-          : Collections.emptyList();
+      return stateUpdated ? event.accept(sshdRequestLog) : ConfigUpdatedEvent.NO_UPDATES;
     } catch (IllegalArgumentException iae) {
-      return Collections.singletonList(event.reject(sshdRequestLog));
+      return event.reject(sshdRequestLog);
     }
   }
 }

java/com/google/gerrit/sshd/commands/ReloadConfig.java

@@ -16,16 +16,15 @@ package com.google.gerrit.sshd.commands;
 
 import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
 
+import com.google.common.collect.Multimap;
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.extensions.annotations.RequiresCapability;
-import com.google.gerrit.server.config.ConfigUpdatedEvent;
+import com.google.gerrit.server.config.ConfigUpdatedEvent.ConfigUpdateEntry;
 import com.google.gerrit.server.config.ConfigUpdatedEvent.UpdateResult;
 import com.google.gerrit.server.config.GerritServerConfigReloader;
 import com.google.gerrit.sshd.CommandMetaData;
 import com.google.gerrit.sshd.SshCommand;
 import com.google.inject.Inject;
-import java.util.List;
-import java.util.stream.Collectors;
 
 /** Issues a reload of gerrit.config. */
 @RequiresCapability(GlobalCapability.ADMINISTRATE_SERVER)
@@ -39,31 +38,16 @@ public class ReloadConfig extends SshCommand {
 
   @Override
   protected void run() throws Failure {
-    List<ConfigUpdatedEvent.Update> updates = gerritServerConfigReloader.reloadConfig();
+    Multimap<UpdateResult, ConfigUpdateEntry> updates = gerritServerConfigReloader.reloadConfig();
     if (updates.isEmpty()) {
       stdout.println("No config entries updated!");
       return;
     }
 
     // Print out UpdateResult.{ACCEPTED|REJECTED} entries grouped by their type
-    for (UpdateResult updateResult : UpdateResult.values()) {
-      List<ConfigUpdatedEvent.Update> filteredUpdates = filterUpdates(updates, updateResult);
-      if (filteredUpdates.isEmpty()) {
-        continue;
-      }
-      stdout.println(updateResult.toString() + " configuration changes:");
-      filteredUpdates
-          .stream()
-          .flatMap(update -> update.getConfigUpdates().stream())
-          .forEach(cfgEntry -> stdout.println(cfgEntry.toString()));
+    for (UpdateResult result : updates.keySet()) {
+      stdout.println(result.toString() + " configuration changes:");
+      updates.get(result).forEach(cfgEntry -> stdout.println(cfgEntry.toString()));
     }
   }
-
-  public static List<ConfigUpdatedEvent.Update> filterUpdates(
-      List<ConfigUpdatedEvent.Update> updates, UpdateResult result) {
-    return updates
-        .stream()
-        .filter(update -> update.getResult() == result)
-        .collect(Collectors.toList());
-  }
 }