diff --git a/java/com/google/gerrit/entities/CachedProjectConfig.java b/java/com/google/gerrit/entities/CachedProjectConfig.java index 0b755b785b..2a94bc8097 100644 --- a/java/com/google/gerrit/entities/CachedProjectConfig.java +++ b/java/com/google/gerrit/entities/CachedProjectConfig.java @@ -14,19 +14,17 @@ 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; import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.FluentLogger; import java.util.Collection; 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; /** @@ -37,6 +35,8 @@ import org.eclipse.jgit.lib.ObjectId; */ @AutoValue public abstract class CachedProjectConfig { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + public abstract Project getProject(); public abstract ImmutableMap getGroups(); @@ -126,34 +126,10 @@ public abstract class CachedProjectConfig { public abstract ImmutableMap 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 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(); - } - public abstract ImmutableMap getProjectLevelConfigs(); + public abstract ImmutableMap getParsedProjectLevelConfigs(); + public static Builder builder() { return new AutoValue_CachedProjectConfig.Builder(); } @@ -235,8 +211,15 @@ public abstract class CachedProjectConfig { abstract ImmutableMap.Builder projectLevelConfigsBuilder(); + abstract ImmutableMap.Builder parsedProjectLevelConfigsBuilder(); + public Builder addProjectLevelConfig(String configFileName, String config) { projectLevelConfigsBuilder().put(configFileName, config); + try { + parsedProjectLevelConfigsBuilder().put(configFileName, ImmutableConfig.parse(config)); + } catch (ConfigInvalidException e) { + logger.atInfo().withCause(e).log("Config for " + configFileName + " not parsable"); + } return this; } diff --git a/java/com/google/gerrit/entities/ImmutableConfig.java b/java/com/google/gerrit/entities/ImmutableConfig.java new file mode 100644 index 0000000000..a5efc1441f --- /dev/null +++ b/java/com/google/gerrit/entities/ImmutableConfig.java @@ -0,0 +1,91 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.entities; + +import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; + +/** + * Immutable parsed representation of a {@link org.eclipse.jgit.lib.Config} that can be cached. + * Supports only a limited set of operations. + */ +public class ImmutableConfig { + public static final ImmutableConfig EMPTY = new ImmutableConfig("", new Config()); + + private final String stringCfg; + private final Config cfg; + + private ImmutableConfig(String stringCfg, Config cfg) { + this.stringCfg = stringCfg; + this.cfg = cfg; + } + + public static ImmutableConfig parse(String stringCfg) throws ConfigInvalidException { + Config cfg = new Config(); + cfg.fromText(stringCfg); + return new ImmutableConfig(stringCfg, cfg); + } + + /** Returns a mutable copy of this config. */ + public Config mutableCopy() { + Config cfg = new Config(); + try { + cfg.fromText(this.cfg.toText()); + } catch (ConfigInvalidException e) { + // Can't happen as we used JGit to format that config. + throw new IllegalStateException(e); + } + return cfg; + } + + /** @see Config#getSections() */ + public Set getSections() { + return cfg.getSections(); + } + + /** @see Config#getNames(String) */ + public Set getNames(String section) { + return cfg.getNames(section); + } + + /** @see Config#getNames(String, String) */ + public Set getNames(String section, String subsection) { + return cfg.getNames(section, subsection); + } + + /** @see Config#getStringList(String, String, String) */ + public String[] getStringList(String section, String subsection, String name) { + return cfg.getStringList(section, subsection, name); + } + + /** @see Config#getSubsections(String) */ + public Set getSubsections(String section) { + return cfg.getSubsections(section); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof ImmutableConfig)) { + return false; + } + return ((ImmutableConfig) o).stringCfg.equals(stringCfg); + } + + @Override + public int hashCode() { + return stringCfg.hashCode(); + } +} diff --git a/java/com/google/gerrit/server/project/ProjectLevelConfig.java b/java/com/google/gerrit/server/project/ProjectLevelConfig.java index 482523378a..555cf4cd7b 100644 --- a/java/com/google/gerrit/server/project/ProjectLevelConfig.java +++ b/java/com/google/gerrit/server/project/ProjectLevelConfig.java @@ -16,14 +16,15 @@ package com.google.gerrit.server.project; import static java.util.stream.Collectors.toList; -import com.google.common.collect.Iterables; import com.google.common.collect.Streams; +import com.google.gerrit.entities.ImmutableConfig; import com.google.gerrit.entities.RefNames; import com.google.gerrit.server.git.meta.VersionedMetaData; import java.io.IOException; import java.util.Arrays; import java.util.Set; import java.util.stream.Stream; +import java.util.stream.StreamSupport; import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; @@ -73,19 +74,17 @@ public class ProjectLevelConfig { private final String fileName; private final ProjectState project; - private Config cfg; + private final ImmutableConfig immutableConfig; - public ProjectLevelConfig(String fileName, ProjectState project, Config cfg) { + public ProjectLevelConfig( + String fileName, ProjectState project, @Nullable ImmutableConfig immutableConfig) { this.fileName = fileName; this.project = project; - this.cfg = cfg; + this.immutableConfig = immutableConfig == null ? ImmutableConfig.EMPTY : immutableConfig; } public Config get() { - if (cfg == null) { - cfg = new Config(); - } - return cfg; + return immutableConfig.mutableCopy(); } public Config getWithInheritance() { @@ -105,58 +104,54 @@ public class ProjectLevelConfig { * @return a combined config. */ public Config getWithInheritance(boolean merge) { - Config cfgWithInheritance = new Config(); - try { - cfgWithInheritance.fromText(get().toText()); - } catch (ConfigInvalidException e) { - // cannot happen - } - ProjectState parent = Iterables.getFirst(project.parents(), null); - if (parent != null) { - Config parentCfg = parent.getConfig(fileName).getWithInheritance(); - for (String section : parentCfg.getSections()) { - Set allNames = get().getNames(section); - for (String name : parentCfg.getNames(section)) { - String[] parentValues = parentCfg.getStringList(section, null, name); - if (!allNames.contains(name)) { - cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues)); - } else if (merge) { - cfgWithInheritance.setStringList( - section, - null, - name, - Stream.concat( - Arrays.stream(cfg.getStringList(section, null, name)), - Arrays.stream(parentValues)) - .sorted() - .distinct() - .collect(toList())); - } - } + Config cfg = new Config(); + // Traverse from All-Projects down to the current project + StreamSupport.stream(project.treeInOrder().spliterator(), false) + .forEach( + parent -> { + ImmutableConfig levelCfg = parent.getConfig(fileName).immutableConfig; + for (String section : levelCfg.getSections()) { + Set allNames = cfg.getNames(section); + for (String name : levelCfg.getNames(section)) { + String[] levelValues = levelCfg.getStringList(section, null, name); + if (allNames.contains(name) && merge) { + cfg.setStringList( + section, + null, + name, + Stream.concat( + Arrays.stream(cfg.getStringList(section, null, name)), + Arrays.stream(levelValues)) + .sorted() + .distinct() + .collect(toList())); + } else { + cfg.setStringList(section, null, name, Arrays.asList(levelValues)); + } + } - for (String subsection : parentCfg.getSubsections(section)) { - allNames = get().getNames(section, subsection); - for (String name : parentCfg.getNames(section, subsection)) { - String[] parentValues = parentCfg.getStringList(section, subsection, name); - if (!allNames.contains(name)) { - cfgWithInheritance.setStringList( - section, subsection, name, Arrays.asList(parentValues)); - } else if (merge) { - cfgWithInheritance.setStringList( - section, - subsection, - name, - Streams.concat( - Arrays.stream(cfg.getStringList(section, subsection, name)), - Arrays.stream(parentValues)) - .sorted() - .distinct() - .collect(toList())); - } - } - } - } - } - return cfgWithInheritance; + for (String subsection : levelCfg.getSubsections(section)) { + allNames = cfg.getNames(section, subsection); + for (String name : levelCfg.getNames(section, subsection)) { + String[] levelValues = levelCfg.getStringList(section, subsection, name); + if (allNames.contains(name) && merge) { + cfg.setStringList( + section, + subsection, + name, + Streams.concat( + Arrays.stream(cfg.getStringList(section, subsection, name)), + Arrays.stream(levelValues)) + .sorted() + .distinct() + .collect(toList())); + } else { + cfg.setStringList(section, subsection, name, Arrays.asList(levelValues)); + } + } + } + } + }); + return cfg; } } diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java index 8c024ef74a..249eb351f9 100644 --- a/java/com/google/gerrit/server/project/ProjectState.java +++ b/java/com/google/gerrit/server/project/ProjectState.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.project; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.gerrit.entities.PermissionRule.Action.ALLOW; import static java.util.Comparator.comparing; @@ -176,8 +177,9 @@ public class ProjectState { } public ProjectLevelConfig getConfig(String fileName) { - Optional rawConfig = cachedConfig.getProjectLevelConfig(fileName); - return new ProjectLevelConfig(fileName, this, rawConfig.orElse(new Config())); + checkState(fileName.endsWith(".config"), "file name must end in .config. is: " + fileName); + return new ProjectLevelConfig( + fileName, this, cachedConfig.getParsedProjectLevelConfigs().get(fileName)); } public long getMaxObjectSizeLimit() {