Move permission filtering for All-Projects from ProjectState to ProjectConfig
Certain permissions cannot be used in All-Projects and are filtered
out before we evaluate them. This used to be done in ProjectState
and was cached when that entity was cached. Now, we cache immutable
data objects (mostly AutoValues) for better thread safety.
When the ProjectCache was refactored, this logic was left in
ProjectState. This means it has to be recomputed quite often when
permissions are evaluated. A recent profile showed that we can
save CPU time (hence request time) as well as heap if we cache
the outcome of this filtering.
This is what this commit does. Existing tests check that permissions
that are not allowed on All-Projects (e.g. OWNERS on refs/*) are
indeed not evaluated.
Since this filtering depends on if a project is considered to
be All-Projects or not, we have to invalidate the cache when
the name of All-Projects changes. We document this fact.
Release-Notes: Cache permission filtering for All-Projects
Change-Id: I01e08e70bf36aaf4be522564286ab4a86e7681a3
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index f088240..9dea08e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2143,6 +2143,9 @@
other project managed by the running server. The name is
relative to `gerrit.basePath`.
+
+The link:#cache_names[persisted_projects cache] must be
+flushed after this setting is changed.
++
Defaults to `All-Projects` if not set.
[[gerrit.defaultBranch]]gerrit.defaultBranch::
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 4a063a3..37c16a9 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -213,7 +213,8 @@
}
public ProjectConfig create(Project.NameKey projectName) {
- return new ProjectConfig(projectName, getBaseConfig(sitePaths, allProjects, projectName));
+ return new ProjectConfig(
+ projectName, getBaseConfig(sitePaths, allProjects, projectName), allProjects);
}
public ProjectConfig read(MetaDataUpdate update) throws IOException, ConfigInvalidException {
@@ -239,6 +240,7 @@
}
private final StoredConfig baseConfig;
+ private final AllProjectsName allProjectsName;
private Project project;
private AccountsSection accountsSection;
@@ -275,7 +277,6 @@
.setCheckReceivedObjects(checkReceivedObjects)
.setExtensionPanelSections(extensionPanelSections);
groupList.byUUID().values().forEach(g -> builder.addGroup(g));
- accessSections.values().forEach(a -> builder.addAccessSection(a));
contributorAgreements.values().forEach(c -> builder.addContributorAgreement(c));
notifySections.values().forEach(n -> builder.addNotifySection(n));
subscribeSections.values().forEach(s -> builder.addSubscribeSection(s));
@@ -287,6 +288,28 @@
projectLevelConfigs
.entrySet()
.forEach(c -> builder.addProjectLevelConfig(c.getKey(), c.getValue().toText()));
+
+ if (projectName.equals(allProjectsName)) {
+ // Filter out permissions that aren't allowed to be set on All-Projects
+ accessSections
+ .values()
+ .forEach(
+ a -> {
+ List<Permission.Builder> copy = new ArrayList<>();
+ for (Permission p : a.getPermissions()) {
+ if (Permission.canBeOnAllProjects(a.getName(), p.getName())) {
+ copy.add(p.toBuilder());
+ }
+ }
+ AccessSection section =
+ AccessSection.builder(a.getName())
+ .modifyPermissions(permissions -> permissions.addAll(copy))
+ .build();
+ builder.addAccessSection(section);
+ });
+ } else {
+ accessSections.values().forEach(a -> builder.addAccessSection(a));
+ }
return builder.build();
}
@@ -342,9 +365,13 @@
requireNonNull(commentLinkSections.remove(name));
}
- private ProjectConfig(Project.NameKey projectName, @Nullable StoredConfig baseConfig) {
+ private ProjectConfig(
+ Project.NameKey projectName,
+ @Nullable StoredConfig baseConfig,
+ AllProjectsName allProjectsName) {
this.projectName = projectName;
this.baseConfig = baseConfig;
+ this.allProjectsName = allProjectsName;
}
public void load(Repository repo) throws IOException, ConfigInvalidException {
diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java
index 249eb35..8fcfd49 100644
--- a/java/com/google/gerrit/server/project/ProjectState.java
+++ b/java/com/google/gerrit/server/project/ProjectState.java
@@ -266,35 +266,22 @@
/** Get the sections that pertain only to this project. */
List<SectionMatcher> getLocalAccessSections() {
- List<SectionMatcher> sm = localAccessSections;
- if (sm == null) {
- ImmutableList<AccessSection> fromConfig =
- cachedConfig.getAccessSections().values().stream()
- .sorted(comparing(AccessSection::getName))
- .collect(toImmutableList());
- sm = new ArrayList<>(fromConfig.size());
- for (AccessSection section : fromConfig) {
- if (isAllProjects) {
- List<Permission.Builder> copy = new ArrayList<>();
- for (Permission p : section.getPermissions()) {
- if (Permission.canBeOnAllProjects(section.getName(), p.getName())) {
- copy.add(p.toBuilder());
- }
- }
- section =
- AccessSection.builder(section.getName())
- .modifyPermissions(permissions -> permissions.addAll(copy))
- .build();
- }
-
- SectionMatcher matcher = SectionMatcher.wrap(getNameKey(), section);
- if (matcher != null) {
- sm.add(matcher);
- }
- }
- localAccessSections = sm;
+ if (localAccessSections != null) {
+ return localAccessSections;
}
- return sm;
+ ImmutableList<AccessSection> fromConfig =
+ cachedConfig.getAccessSections().values().stream()
+ .sorted(comparing(AccessSection::getName))
+ .collect(toImmutableList());
+ List<SectionMatcher> sm = new ArrayList<>(fromConfig.size());
+ for (AccessSection section : fromConfig) {
+ SectionMatcher matcher = SectionMatcher.wrap(getNameKey(), section);
+ if (matcher != null) {
+ sm.add(matcher);
+ }
+ }
+ localAccessSections = sm;
+ return localAccessSections;
}
/**
