Merge "Don't use List in GerritConfigListener API" into stable-2.16

9 files changed, 79 insertions, 114 deletions

diff --git a/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java b/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java
index 66d65552c8..0bfe5fdc9a 100644
--- a/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java
+++ b/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,
diff --git a/java/com/google/gerrit/server/config/GerritConfigListener.java b/java/com/google/gerrit/server/config/GerritConfigListener.java
index 337a962679..f5b29768b6 100644
--- a/java/com/google/gerrit/server/config/GerritConfigListener.java
+++ b/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);
 }
diff --git a/java/com/google/gerrit/server/config/GerritConfigListenerHelper.java b/java/com/google/gerrit/server/config/GerritConfigListenerHelper.java
index 1dfa3fc21c..d21e1c302d 100644
--- a/java/com/google/gerrit/server/config/GerritConfigListenerHelper.java
+++ b/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;
   }
 }
diff --git a/java/com/google/gerrit/server/config/GerritServerConfigReloader.java b/java/com/google/gerrit/server/config/GerritServerConfigReloader.java
index 1890de86e7..09c1074054 100644
--- a/java/com/google/gerrit/server/config/GerritServerConfigReloader.java
+++ b/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;
   }
 }
diff --git a/java/com/google/gerrit/server/project/CommentLinkProvider.java b/java/com/google/gerrit/server/project/CommentLinkProvider.java
index 56cf51e3dd..4987d0019f 100644
--- a/java/com/google/gerrit/server/project/CommentLinkProvider.java
+++ b/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;
   }
 }
diff --git a/java/com/google/gerrit/server/restapi/config/ReloadConfig.java b/java/com/google/gerrit/server/restapi/config/ReloadConfig.java
index de3c3ee15d..cab07e3ae0 100644
--- a/java/com/google/gerrit/server/restapi/config/ReloadConfig.java
+++ b/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());
diff --git a/java/com/google/gerrit/server/restapi/project/SetParent.java b/java/com/google/gerrit/server/restapi/project/SetParent.java
index ca7e7aa2f1..d02d04a6c2 100644
--- a/java/com/google/gerrit/server/restapi/project/SetParent.java
+++ b/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);
   }
 }
diff --git a/java/com/google/gerrit/sshd/SshLog.java b/java/com/google/gerrit/sshd/SshLog.java
index 0e34889f08..df3242cf7d 100644
--- a/java/com/google/gerrit/sshd/SshLog.java
+++ b/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);
     }
   }
 }
diff --git a/java/com/google/gerrit/sshd/commands/ReloadConfig.java b/java/com/google/gerrit/sshd/commands/ReloadConfig.java
index 1b21230b58..cbe3c57031 100644
--- a/java/com/google/gerrit/sshd/commands/ReloadConfig.java
+++ b/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());
-  }
 }