Cache additional *.config files in CachedProjectConfig

ProjectLevelConfigs represent additional configs stored in
refs/meta/config. They used to be loaded on demand and cached. This
wasn't thread safe and mutated a cached object.

This commit removes this behavior and loads all additional configs
when the project.config is loaded. Configs are stored as strings to
keep them immutable.

Change-Id: I2379e28b484550b2372b7f530ffda7639ed1a9d6

java/com/google/gerrit/entities/CachedProjectConfig.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.entities;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -23,6 +25,8 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import org.eclipse.jgit.annotations.Nullable;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -122,6 +126,34 @@ public abstract class CachedProjectConfig {
 
   public abstract ImmutableMap<String, String> getPluginConfigs();
 
+  /**
+   * Returns the {@link Config} that got parsed from the specified {@code fileName} on {@code
+   * refs/meta/config}. The returned instance is a defensive copy of the cached value.
+   *
+   * @param fileName the name of the file. Must end in {@code .config}.
+   * @return an {@link Optional} of the {@link Config}. {@link Optional#empty()} if the file was not
+   *     found or could not be parsed. {@link com.google.gerrit.server.project.ProjectConfig} will
+   *     surface validation errors in case of a parsing issue.
+   */
+  public Optional<Config> getProjectLevelConfig(String fileName) {
+    checkState(fileName.endsWith(".config"), "file name must end in .config");
+    if (getProjectLevelConfigs().containsKey(fileName)) {
+      Config config = new Config();
+      try {
+        config.fromText(getProjectLevelConfigs().get(fileName));
+      } catch (ConfigInvalidException e) {
+        // This is OK to propagate as IllegalStateException because it's a programmer error.
+        // The config was converted to a String using Config#toText. So #fromText must not
+        // throw a ConfigInvalidException
+        throw new IllegalStateException("invalid config for " + fileName, e);
+      }
+      return Optional.of(config);
+    }
+    return Optional.empty();
+  }
+
+  abstract ImmutableMap<String, String> getProjectLevelConfigs();
+
   public static Builder builder() {
     return new AutoValue_CachedProjectConfig.Builder();
   }
@@ -201,6 +233,13 @@ public abstract class CachedProjectConfig {
       return this;
     }
 
+    abstract ImmutableMap.Builder<String, String> projectLevelConfigsBuilder();
+
+    public Builder addProjectLevelConfig(String configFileName, String config) {
+      projectLevelConfigsBuilder().put(configFileName, config);
+      return this;
+    }
+
     public abstract CachedProjectConfig build();
 
     protected abstract ImmutableMap.Builder<AccountGroup.UUID, GroupReference> groupsBuilder();

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

@@ -30,6 +30,7 @@ import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
+import com.google.common.flogger.FluentLogger;
 import com.google.common.primitives.Shorts;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.UsedAt;
@@ -99,6 +100,8 @@ import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 
 public class ProjectConfig extends VersionedMetaData implements ValidationError.Sink {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   public static final String COMMENTLINK = "commentlink";
   public static final String LABEL = "label";
   public static final String KEY_FUNCTION = "function";
@@ -250,6 +253,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
   private ObjectId rulesId;
   private long maxObjectSizeLimit;
   private Map<String, Config> pluginConfigs;
+  private Map<String, Config> projectLevelConfigs;
   private boolean checkReceivedObjects;
   private Set<String> sectionsWithUnknownPermissions;
   private boolean hasLegacyPermissions;
@@ -278,6 +282,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     pluginConfigs
         .entrySet()
         .forEach(c -> builder.addPluginConfig(c.getKey(), c.getValue().toText()));
+    projectLevelConfigs
+        .entrySet()
+        .forEach(c -> builder.addProjectLevelConfig(c.getKey(), c.getValue().toText()));
     return builder.build();
   }
 
@@ -655,6 +662,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     loadSubscribeSections(rc);
     mimeTypes = ConfiguredMimeTypes.create(projectName.get(), rc);
     loadPluginSections(rc);
+    loadProjectLevelConfigs();
     loadReceiveSection(rc);
     loadExtensionPanelSections(rc);
   }
@@ -1176,6 +1184,25 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     return PluginConfig.create(pluginName, pluginConfig, getCacheable());
   }
 
+  private void loadProjectLevelConfigs() throws IOException {
+    projectLevelConfigs = new HashMap<>();
+    if (revision == null) {
+      return;
+    }
+    for (PathInfo pathInfo : getPathInfos(true)) {
+      if (pathInfo.path.endsWith(".config") && !PROJECT_CONFIG.equals(pathInfo.path)) {
+        String cfg = readUTF8(pathInfo.path);
+        Config parsedConfig = new Config();
+        try {
+          parsedConfig.fromText(cfg);
+          projectLevelConfigs.put(pathInfo.path, parsedConfig);
+        } catch (ConfigInvalidException e) {
+          logger.atWarning().withCause(e).log("Unable to parse config");
+        }
+      }
+    }
+  }
+
   private void readGroupList() throws IOException {
     groupList = GroupList.parse(projectName, readUTF8(GroupList.FILE_NAME), this);
   }

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

@@ -24,29 +24,61 @@ import java.io.IOException;
 import java.util.Arrays;
 import java.util.Set;
 import java.util.stream.Stream;
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
 
 /** Configuration file in the projects refs/meta/config branch. */
-public class ProjectLevelConfig extends VersionedMetaData {
+public class ProjectLevelConfig {
+  /**
+   * This class is a low-level API that allows callers to read the config directly from a repository
+   * and make updates to it.
+   */
+  public static class Bare extends VersionedMetaData {
+    private final String fileName;
+    @Nullable private Config cfg;
+
+    public Bare(String fileName) {
+      this.fileName = fileName;
+      this.cfg = null;
+    }
+
+    public Config getConfig() {
+      if (cfg == null) {
+        cfg = new Config();
+      }
+      return cfg;
+    }
+
+    @Override
+    protected String getRefName() {
+      return RefNames.REFS_CONFIG;
+    }
+
+    @Override
+    protected void onLoad() throws IOException, ConfigInvalidException {
+      cfg = readConfig(fileName);
+    }
+
+    @Override
+    protected boolean onSave(CommitBuilder commit) throws IOException {
+      if (commit.getMessage() == null || "".equals(commit.getMessage())) {
+        commit.setMessage("Updated configuration\n");
+      }
+      saveConfig(fileName, cfg);
+      return true;
+    }
+  }
+
   private final String fileName;
   private final ProjectState project;
   private Config cfg;
 
-  public ProjectLevelConfig(String fileName, ProjectState project) {
+  public ProjectLevelConfig(String fileName, ProjectState project, Config cfg) {
     this.fileName = fileName;
     this.project = project;
-  }
+    this.cfg = cfg;
-
-  @Override
-  protected String getRefName() {
-    return RefNames.REFS_CONFIG;
-  }
-
-  @Override
-  protected void onLoad() throws IOException, ConfigInvalidException {
-    cfg = readConfig(fileName);
   }
 
   public Config get() {
@@ -127,13 +159,4 @@ public class ProjectLevelConfig extends VersionedMetaData {
     }
     return cfgWithInheritance;
   }
-
-  @Override
-  protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException {
-    if (commit.getMessage() == null || "".equals(commit.getMessage())) {
-      commit.setMessage("Updated configuration\n");
-    }
-    saveConfig(fileName, cfg);
-    return true;
-  }
 }

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

@@ -43,16 +43,13 @@ import com.google.gerrit.server.account.CapabilityCollection;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.PluginConfig;
-import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -61,7 +58,6 @@ import java.util.Optional;
 import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.Repository;
 
 /**
  * Cached information on a project. Must not contain any data derived from parents other than it's
@@ -78,11 +74,9 @@ public class ProjectState {
   private final boolean isAllUsers;
   private final AllProjectsName allProjectsName;
   private final ProjectCache projectCache;
-  private final GitRepositoryManager gitMgr;
   private final List<CommentLinkInfo> commentLinks;
 
   private final CachedProjectConfig cachedConfig;
-  private final Map<String, ProjectLevelConfig> configs;
   private final Set<AccountGroup.UUID> localOwners;
   private final long globalMaxObjectSizeLimit;
   private final boolean inheritProjectMaxObjectSizeLimit;
@@ -98,7 +92,6 @@ public class ProjectState {
       ProjectCache projectCache,
       AllProjectsName allProjectsName,
       AllUsersName allUsersName,
-      GitRepositoryManager gitMgr,
       List<CommentLinkInfo> commentLinks,
       CapabilityCollection.Factory limitsFactory,
       TransferConfig transferConfig,
@@ -107,10 +100,8 @@ public class ProjectState {
     this.isAllProjects = cachedProjectConfig.getProject().getNameKey().equals(allProjectsName);
     this.isAllUsers = cachedProjectConfig.getProject().getNameKey().equals(allUsersName);
     this.allProjectsName = allProjectsName;
-    this.gitMgr = gitMgr;
     this.commentLinks = commentLinks;
     this.cachedConfig = cachedProjectConfig;
-    this.configs = new HashMap<>();
     this.capabilities =
         isAllProjects
             ? limitsFactory.create(
@@ -183,19 +174,8 @@ public class ProjectState {
   }
 
   public ProjectLevelConfig getConfig(String fileName) {
-    if (configs.containsKey(fileName)) {
+    Optional<Config> rawConfig = cachedConfig.getProjectLevelConfig(fileName);
-      return configs.get(fileName);
+    return new ProjectLevelConfig(fileName, this, rawConfig.orElse(new Config()));
-    }
-
-    ProjectLevelConfig cfg = new ProjectLevelConfig(fileName, this);
-    try (Repository git = gitMgr.openRepository(getNameKey())) {
-      cfg.load(getNameKey(), git, cachedConfig.getRevision().get());
-    } catch (IOException | ConfigInvalidException e) {
-      logger.atWarning().withCause(e).log("Failed to load %s for %s", fileName, getName());
-    }
-
-    configs.put(fileName, cfg);
-    return cfg;
   }
 
   public long getMaxObjectSizeLimit() {

javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java

@@ -173,4 +173,16 @@ public class ProjectLevelConfigIT extends AbstractDaemonTest {
 
     assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText());
   }
+
+  @Test
+  public void brokenConfigDoesNotBlockPush() throws Exception {
+    String configName = "test.config";
+    PushOneCommit push =
+        pushFactory.create(
+            admin.newIdent(), testRepo, "Create Project Level Config", configName, "\\\\///");
+    push.to(RefNames.REFS_CONFIG).assertOkStatus();
+
+    ProjectState state = projectCache.get(project).get();
+    assertThat(state.getConfig(configName).get().toText()).isEmpty();
+  }
 }