From a5cd85a5b2d762f23450099af247dca1170868ba Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 27 Jan 2022 14:09:55 +0100 Subject: 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 --- Documentation/config-gerrit.txt | 3 ++ .../gerrit/server/project/ProjectConfig.java | 33 +++++++++++++++-- .../google/gerrit/server/project/ProjectState.java | 41 ++++++++-------------- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index ab0657a183..868d32cbc1 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2214,6 +2214,9 @@ access controls and settings. These are inherited into every 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 8f0b5357a4..c101ddf4e3 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -214,7 +214,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. projectName, projectName.equals(allProjectsName) ? allProjectsConfigProvider.get(allProjectsName) - : Optional.empty()); + : Optional.empty(), + allProjectsName); } public ProjectConfig read(MetaDataUpdate update) throws IOException, ConfigInvalidException { @@ -240,6 +241,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } private final Optional baseConfig; + private final AllProjectsName allProjectsName; private Project project; private AccountsSection accountsSection; @@ -277,7 +279,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. .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)); @@ -290,6 +291,28 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. 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 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(); } @@ -345,9 +368,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. requireNonNull(commentLinkSections.remove(name)); } - private ProjectConfig(Project.NameKey projectName, Optional baseConfig) { + private ProjectConfig( + Project.NameKey projectName, + Optional 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 69e6036d76..03a216d149 100644 --- a/java/com/google/gerrit/server/project/ProjectState.java +++ b/java/com/google/gerrit/server/project/ProjectState.java @@ -268,35 +268,22 @@ public class ProjectState { /** Get the sections that pertain only to this project. */ List getLocalAccessSections() { - List sm = localAccessSections; - if (sm == null) { - ImmutableList fromConfig = - cachedConfig.getAccessSections().values().stream() - .sorted(comparing(AccessSection::getName)) - .collect(toImmutableList()); - sm = new ArrayList<>(fromConfig.size()); - for (AccessSection section : fromConfig) { - if (isAllProjects) { - List 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); - } + if (localAccessSections != null) { + return localAccessSections; + } + ImmutableList fromConfig = + cachedConfig.getAccessSections().values().stream() + .sorted(comparing(AccessSection::getName)) + .collect(toImmutableList()); + List sm = new ArrayList<>(fromConfig.size()); + for (AccessSection section : fromConfig) { + SectionMatcher matcher = SectionMatcher.wrap(getNameKey(), section); + if (matcher != null) { + sm.add(matcher); } - localAccessSections = sm; } - return sm; + localAccessSections = sm; + return localAccessSections; } /** -- cgit v1.2.3