Make collections in AccountState immutable

The account cache stores AccountState objects. If the collections in
AccountState are mutable callers can polute the cache by making updates
to the collections in AccountState without updating the underlying
storage.

Change-Id: I1141fd8211d4a081c92efe0848d90efe174487e4
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/pgm/init/InitAdminUser.java

@@ -17,6 +17,8 @@ package com.google.gerrit.pgm.init;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.errors.NoSuchGroupException;
@@ -45,7 +47,6 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Optional;
 import org.apache.commons.validator.routines.EmailValidator;
@@ -155,8 +156,8 @@ public class InitAdminUser implements InitStep {
               new AccountState(
                   new AllUsersName(allUsers.get()),
                   a,
-                  extIds,
-                  new HashMap<>(),
+                  ImmutableSet.copyOf(extIds),
+                  ImmutableMap.of(),
                   GeneralPreferencesInfo.defaults());
           for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) {
             accountIndex.replace(as);

java/com/google/gerrit/server/account/AccountCacheImpl.java

@@ -18,6 +18,8 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USE
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
@@ -34,8 +36,6 @@ import com.google.inject.Singleton;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Named;
 import java.io.IOException;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -134,8 +134,8 @@ public class AccountCacheImpl implements AccountCache {
     return new AccountState(
         allUsersName,
         account,
-        Collections.emptySet(),
-        new HashMap<>(),
+        ImmutableSet.of(),
+        ImmutableMap.of(),
         GeneralPreferencesInfo.defaults());
   }
 

java/com/google/gerrit/server/account/AccountConfig.java

@@ -19,6 +19,8 @@ import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
@@ -33,6 +35,7 @@ import com.google.gwtorm.server.OrmDuplicateKeyException;
 import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -157,7 +160,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
    *
    * @return the project watches of the loaded account
    */
-  public Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches() {
+  public ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> getProjectWatches() {
     checkLoaded();
     return watchConfig.getProjectWatches();
   }
@@ -334,7 +337,8 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
     if (accountUpdate.isPresent()
         && (!accountUpdate.get().getDeletedProjectWatches().isEmpty()
             || !accountUpdate.get().getUpdatedProjectWatches().isEmpty())) {
-      Map<ProjectWatchKey, Set<NotifyType>> projectWatches = watchConfig.getProjectWatches();
+      Map<ProjectWatchKey, Set<NotifyType>> projectWatches =
+          new HashMap<>(watchConfig.getProjectWatches());
       accountUpdate.get().getDeletedProjectWatches().forEach(pw -> projectWatches.remove(pw));
       accountUpdate
           .get()

java/com/google/gerrit/server/account/AccountState.java

@@ -20,6 +20,8 @@ import com.google.common.base.Function;
 import com.google.common.base.Strings;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
@@ -29,9 +31,6 @@ import com.google.gerrit.server.account.WatchConfig.NotifyType;
 import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.config.AllUsersName;
-import java.util.Collection;
-import java.util.Map;
-import java.util.Set;
 import org.apache.commons.codec.DecoderException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -48,16 +47,16 @@ public class AccountState {
 
   private final AllUsersName allUsersName;
   private final Account account;
-  private final Collection<ExternalId> externalIds;
-  private final Map<ProjectWatchKey, Set<NotifyType>> projectWatches;
+  private final ImmutableSet<ExternalId> externalIds;
+  private final ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches;
   private final GeneralPreferencesInfo generalPreferences;
   private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
 
   public AccountState(
       AllUsersName allUsersName,
       Account account,
-      Collection<ExternalId> externalIds,
-      Map<ProjectWatchKey, Set<NotifyType>> projectWatches,
+      ImmutableSet<ExternalId> externalIds,
+      ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches,
       GeneralPreferencesInfo generalPreferences) {
     this.allUsersName = allUsersName;
     this.account = account;
@@ -110,12 +109,12 @@ public class AccountState {
   }
 
   /** The external identities that identify the account holder. */
-  public Collection<ExternalId> getExternalIds() {
+  public ImmutableSet<ExternalId> getExternalIds() {
     return externalIds;
   }
 
   /** The project watches of the account. */
-  public Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches() {
+  public ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> getProjectWatches() {
     return projectWatches;
   }
 

java/com/google/gerrit/server/account/WatchConfig.java

@@ -23,6 +23,7 @@ import com.google.common.base.Enums;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
@@ -104,7 +105,7 @@ public class WatchConfig {
   private final Config cfg;
   private final ValidationError.Sink validationErrorSink;
 
-  private Map<ProjectWatchKey, Set<NotifyType>> projectWatches;
+  private ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches;
 
   public WatchConfig(Account.Id accountId, Config cfg, ValidationError.Sink validationErrorSink) {
     this.accountId = checkNotNull(accountId, "accountId");
@@ -112,7 +113,7 @@ public class WatchConfig {
     this.validationErrorSink = checkNotNull(validationErrorSink, "validationErrorSink");
   }
 
-  public Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches() {
+  public ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> getProjectWatches() {
     if (projectWatches == null) {
       parse();
     }
@@ -124,7 +125,7 @@ public class WatchConfig {
   }
 
   @VisibleForTesting
-  public static Map<ProjectWatchKey, Set<NotifyType>> parse(
+  public static ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> parse(
       Account.Id accountId, Config cfg, ValidationError.Sink validationErrorSink) {
     Map<ProjectWatchKey, Set<NotifyType>> projectWatches = new HashMap<>();
     for (String projectName : cfg.getSubsections(PROJECT)) {
@@ -148,11 +149,11 @@ public class WatchConfig {
         projectWatches.get(key).addAll(notifyValue.notifyTypes());
       }
     }
-    return projectWatches;
+    return immutableCopyOf(projectWatches);
   }
 
   public Config save(Map<ProjectWatchKey, Set<NotifyType>> projectWatches) {
-    this.projectWatches = projectWatches;
+    this.projectWatches = immutableCopyOf(projectWatches);
 
     for (String projectName : cfg.getSubsections(PROJECT)) {
       cfg.unsetSection(PROJECT, projectName);
@@ -172,6 +173,16 @@ public class WatchConfig {
     return cfg;
   }
 
+  private static ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> immutableCopyOf(
+      Map<ProjectWatchKey, Set<NotifyType>> projectWatches) {
+    ImmutableMap.Builder<ProjectWatchKey, ImmutableSet<NotifyType>> b = ImmutableMap.builder();
+    projectWatches
+        .entrySet()
+        .stream()
+        .forEach(e -> b.put(e.getKey(), ImmutableSet.copyOf(e.getValue())));
+    return b.build();
+  }
+
   @AutoValue
   public abstract static class NotifyValue {
     public static NotifyValue parse(

java/com/google/gerrit/server/mail/send/ProjectWatch.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.mail.send;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.index.query.Predicate;
@@ -69,7 +70,8 @@ public class ProjectWatch {
 
     for (AccountState a : args.accountQueryProvider.get().byWatchedProject(project)) {
       Account.Id accountId = a.getAccount().getId();
-      for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : a.getProjectWatches().entrySet()) {
+      for (Map.Entry<ProjectWatchKey, ImmutableSet<NotifyType>> e :
+          a.getProjectWatches().entrySet()) {
         if (project.equals(e.getKey().project())
             && add(matching, accountId, e.getKey(), e.getValue(), type)) {
           // We only want to prevent matching All-Projects if this filter hits
@@ -79,7 +81,8 @@ public class ProjectWatch {
     }
 
     for (AccountState a : args.accountQueryProvider.get().byWatchedProject(args.allProjectsName)) {
-      for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : a.getProjectWatches().entrySet()) {
+      for (Map.Entry<ProjectWatchKey, ImmutableSet<NotifyType>> e :
+          a.getProjectWatches().entrySet()) {
         if (args.allProjectsName.equals(e.getKey().project())) {
           Account.Id accountId = a.getAccount().getId();
           if (!projectWatchers.contains(accountId)) {

java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.account;
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -40,7 +41,6 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
@@ -71,7 +71,8 @@ public class GetWatchedProjects implements RestReadView<AccountResource> {
       throw new ResourceNotFoundException();
     }
     List<ProjectWatchInfo> projectWatchInfos = new ArrayList<>();
-    for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : account.getProjectWatches().entrySet()) {
+    for (Map.Entry<ProjectWatchKey, ImmutableSet<NotifyType>> e :
+        account.getProjectWatches().entrySet()) {
       ProjectWatchInfo pwi = new ProjectWatchInfo();
       pwi.filter = e.getKey().filter();
       pwi.project = e.getKey().project().get();

java/com/google/gerrit/testing/FakeAccountCache.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.testing;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
@@ -83,7 +84,7 @@ public class FakeAccountCache implements AccountCache {
         new AllUsersName(AllUsersNameProvider.DEFAULT),
         account,
         ImmutableSet.of(),
-        new HashMap<>(),
+        ImmutableMap.of(),
         GeneralPreferencesInfo.defaults());
   }
 }

javatests/com/google/gerrit/server/account/WatchConfigTest.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.account;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.account.WatchConfig.NotifyType;
@@ -52,7 +53,7 @@ public class WatchConfigTest implements ValidationError.Sink {
             + "[project \"otherProject\"]\n"
             + "  notify = [NEW_PATCHSETS]\n"
             + "  notify = * [NEW_PATCHSETS, ALL_COMMENTS]\n");
-    Map<ProjectWatchKey, Set<NotifyType>> projectWatches =
+    Map<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches =
         WatchConfig.parse(new Account.Id(1000000), cfg, this);
 
     assertThat(validationErrors).isEmpty();

javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java

@@ -21,6 +21,8 @@ import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.verify;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
@@ -30,8 +32,6 @@ import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.mail.Address;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -388,8 +388,8 @@ public class FromAddressGeneratorProviderTest {
     return new AccountState(
         new AllUsersName(AllUsersNameProvider.DEFAULT),
         account,
-        Collections.emptySet(),
-        new HashMap<>(),
+        ImmutableSet.of(),
+        ImmutableMap.of(),
         GeneralPreferencesInfo.defaults());
   }
 }