Implement the PermissionBackend.filterQueryChanges() for queries speedup
When filtering a large site like GerritHub.io and selecting
a small number of projects, it is paramount to reduce the cardinality
of the results returned by Lucene and avoid CPU and memory overload
of post filtering a large number of changes.
Use the projects configuration in virtualhost.config for adding an
extra query filter and slash the number of entries to filter for
permissions.
Depends-On: Change 426537
Change-Id: I2dd78a6053baecd5d37fcce1ae5fe458b23e4a80
diff --git a/src/main/java/com/gerritforge/gerrit/modules/virtualhost/WithVirtualHostUser.java b/src/main/java/com/gerritforge/gerrit/modules/virtualhost/WithVirtualHostUser.java
index c49bcb1..28b41d2 100644
--- a/src/main/java/com/gerritforge/gerrit/modules/virtualhost/WithVirtualHostUser.java
+++ b/src/main/java/com/gerritforge/gerrit/modules/virtualhost/WithVirtualHostUser.java
@@ -14,6 +14,9 @@
package com.gerritforge.gerrit.modules.virtualhost;
+import static com.gerritforge.gerrit.modules.virtualhost.VirtualHostConfig.ALL_PROJECTS_REGEX;
+
+import com.google.gerrit.common.data.ParameterizedString;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.conditions.BooleanCondition;
@@ -26,8 +29,10 @@
import com.google.gerrit.server.project.RefPatternMatcher;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.Arrays;
import java.util.Collection;
import java.util.Set;
+import java.util.stream.Collectors;
public class WithVirtualHostUser extends WithUser {
private final CurrentUser user;
@@ -52,17 +57,22 @@
@Override
public ForProject project(Project.NameKey project) {
- if (!config.isEnabled()
- || matches(
- project.get(),
- CurrentServerName.get().map(config::getProjects).orElse(config.defaultProjects))) {
+ if (!config.isEnabled() || matches(project.get(), getVisibleProjects())) {
return wrapped.project(project);
}
return ForHiddenProject.INSTANCE;
}
+ private String[] getVisibleProjects() {
+ return CurrentServerName.get().map(config::getProjects).orElse(config.defaultProjects);
+ }
+
private boolean matches(String project, String[] projectsPatterns) {
+ if (projectsPatterns.length == 0) {
+ return true;
+ }
+
for (String projectPattern : projectsPatterns) {
if (RefPatternMatcher.getMatcher(projectPattern).match(project, user)) {
return true;
@@ -87,4 +97,38 @@
public BooleanCondition testCond(GlobalOrPluginPermission perm) {
return wrapped.testCond(perm);
}
+
+ @Override
+ public String filterQueryChanges() {
+ String[] visibleProjects = getVisibleProjects();
+ if (visibleProjects.length == 0) {
+ return null;
+ }
+
+ String[] queryChangesFilters = new String[visibleProjects.length];
+ for (int i = 0; i < visibleProjects.length; i++) {
+ String projectName = visibleProjects[i];
+
+ if (projectName.equals(ALL_PROJECTS_REGEX)) {
+ return null;
+ }
+ String expandedProjectName = projectName;
+ if (projectName.contains("${") && user.isIdentifiedUser()) {
+ expandedProjectName =
+ user.getUserName().map(u -> expandParameters(projectName, u)).orElse(projectName);
+ }
+ if (expandedProjectName.endsWith("*")) {
+ queryChangesFilters[i] =
+ "projects:" + expandedProjectName.substring(0, expandedProjectName.length() - 1);
+ } else {
+ queryChangesFilters[i] = "project:" + expandedProjectName;
+ }
+ }
+ return Arrays.stream(queryChangesFilters).collect(Collectors.joining(" OR ", "(", ")"));
+ }
+
+ private String expandParameters(String projectNamePattern, String username) {
+ ParameterizedString parameterizedString = new ParameterizedString(projectNamePattern);
+ return parameterizedString.replace("username", username).toString();
+ }
}
diff --git a/src/test/java/com/gerritforge/gerrit/modules/virtualhost/AbstractVirtualHostTest.java b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/AbstractVirtualHostTest.java
new file mode 100644
index 0000000..b95c5be
--- /dev/null
+++ b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/AbstractVirtualHostTest.java
@@ -0,0 +1,52 @@
+// Copyright (C) 2024 GerritForge Ltd.
+//
+// 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.gerritforge.gerrit.modules.virtualhost;
+
+import com.google.gerrit.server.config.SitePaths;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.util.FS;
+import org.junit.Before;
+import org.junit.Ignore;
+
+@Ignore
+public abstract class AbstractVirtualHostTest {
+ public static final String TEST_VIRTUAL_HOST = "testhost";
+ protected SitePaths testSitePaths;
+ protected FileBasedConfig virtualHostFileConfig;
+
+ @Before
+ public void setup() throws IOException {
+ Path tempPath = Files.createTempDirectory("virtualhost-test-" + System.nanoTime());
+ testSitePaths = new SitePaths(tempPath);
+ virtualHostFileConfig =
+ new FileBasedConfig(
+ testSitePaths.etc_dir.resolve("virtualhost.config").toFile(), FS.DETECTED);
+ }
+
+ protected void setVirtualHostConfig(String section, String subsection, String projectsValue)
+ throws IOException {
+ setVirtualHostConfig(section, subsection, List.of(projectsValue));
+ }
+
+ protected void setVirtualHostConfig(
+ String section, String subsection, List<String> projectsValues) throws IOException {
+ virtualHostFileConfig.setStringList(section, subsection, "projects", projectsValues);
+ virtualHostFileConfig.save();
+ }
+}
diff --git a/src/test/java/com/gerritforge/gerrit/modules/virtualhost/QueryChangesFilterTest.java b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/QueryChangesFilterTest.java
new file mode 100644
index 0000000..7b69815
--- /dev/null
+++ b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/QueryChangesFilterTest.java
@@ -0,0 +1,107 @@
+// Copyright (C) 2024 GerritForge Ltd.
+//
+// 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.gerritforge.gerrit.modules.virtualhost;
+
+import static com.gerritforge.gerrit.modules.virtualhost.VirtualHostConfig.ALL_PROJECTS_REGEX;
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.permissions.DefaultPermissionBackend;
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class QueryChangesFilterTest extends AbstractVirtualHostTest {
+ private static final String TEST_PROJECT1 = "testproject-1";
+ private static final String TEST_PROJECT2 = "testproject-2";
+ private static final String TEST_USER = "testuser";
+
+ @Mock private CurrentUser currentUserMock;
+
+ @Mock private DefaultPermissionBackend permissionBackendMock;
+
+ @Test
+ public void shouldBeNullWithEmtpyConfig() {
+ assertThat(newWithVirtualHostUser().filterQueryChanges()).isNull();
+ }
+
+ @Test
+ public void shouldBeNullWithDefaultCatchAllConfig() throws IOException {
+ setVirtualHostConfig("default", null, ALL_PROJECTS_REGEX);
+ assertThat(newWithVirtualHostUser().filterQueryChanges()).isNull();
+ }
+
+ @Test
+ public void shouldBeNullWithSpecificCatchAllConfig() throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, ALL_PROJECTS_REGEX);
+ CurrentServerName.set(TEST_VIRTUAL_HOST);
+ assertThat(newWithVirtualHostUser().filterQueryChanges()).isNull();
+ }
+
+ @Test
+ public void shouldBeSingleProjectConditionWhenFilteringBySingleProjectName() throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, TEST_PROJECT1);
+ CurrentServerName.set(TEST_VIRTUAL_HOST);
+ assertThat(newWithVirtualHostUser().filterQueryChanges())
+ .isEqualTo("(project:" + TEST_PROJECT1 + ")");
+ }
+
+ @Test
+ public void shouldBeSingleProjectConditionWhenFilteringByMultipleProjectNames()
+ throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, List.of(TEST_PROJECT1, TEST_PROJECT2));
+ CurrentServerName.set(TEST_VIRTUAL_HOST);
+ assertThat(newWithVirtualHostUser().filterQueryChanges())
+ .isEqualTo("(project:" + TEST_PROJECT1 + " OR project:" + TEST_PROJECT2 + ")");
+ }
+
+ @Test
+ public void shouldBeSingleProjectConditionWhenFilteringByProjectNameWildcard()
+ throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, TEST_PROJECT1 + "*");
+ CurrentServerName.set(TEST_VIRTUAL_HOST);
+ assertThat(newWithVirtualHostUser().filterQueryChanges())
+ .isEqualTo("(projects:" + TEST_PROJECT1 + ")");
+ }
+
+ @Test
+ public void shouldBeNullWhenFilteringBySingleProjectNameDoesNotMatchHost() throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, TEST_PROJECT1);
+ CurrentServerName.set(TEST_VIRTUAL_HOST + "_unmatched");
+ WithVirtualHostUser virtualHostPermission = newWithVirtualHostUser();
+ assertThat(virtualHostPermission.filterQueryChanges()).isNull();
+ }
+
+ @Test
+ public void shouldExpandCurrentUsernameInFilteringBySingleProjectName() throws IOException {
+ setVirtualHostConfig("server", TEST_VIRTUAL_HOST, "${username}");
+ when(currentUserMock.getUserName()).thenReturn(Optional.of(TEST_USER));
+ when(currentUserMock.isIdentifiedUser()).thenReturn(true);
+ CurrentServerName.set(TEST_VIRTUAL_HOST);
+ WithVirtualHostUser virtualHostPermission = newWithVirtualHostUser();
+ assertThat(virtualHostPermission.filterQueryChanges()).isEqualTo("(project:" + TEST_USER + ")");
+ }
+
+ private WithVirtualHostUser newWithVirtualHostUser() {
+ return new WithVirtualHostUser(
+ new VirtualHostConfig(testSitePaths), permissionBackendMock, currentUserMock);
+ }
+}
diff --git a/src/test/java/com/gerritforge/gerrit/modules/virtualhost/VirtualHostConfigTest.java b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/VirtualHostConfigTest.java
index d9ecbf2..aa5d791 100644
--- a/src/test/java/com/gerritforge/gerrit/modules/virtualhost/VirtualHostConfigTest.java
+++ b/src/test/java/com/gerritforge/gerrit/modules/virtualhost/VirtualHostConfigTest.java
@@ -18,29 +18,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
-import com.google.gerrit.server.config.SitePaths;
import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import org.eclipse.jgit.storage.file.FileBasedConfig;
-import org.eclipse.jgit.util.FS;
-import org.junit.Before;
import org.junit.Test;
-public class VirtualHostConfigTest {
-
- public static final String TEST_VIRTUAL_HOST = "testhost";
- private SitePaths testSitePaths;
- private FileBasedConfig virtualHostFileConfig;
-
- @Before
- public void setup() throws IOException {
- Path tempPath = Files.createTempDirectory("virtualhost-test-" + System.nanoTime());
- testSitePaths = new SitePaths(tempPath);
- virtualHostFileConfig =
- new FileBasedConfig(
- testSitePaths.etc_dir.resolve("virtualhost.config").toFile(), FS.DETECTED);
- }
+public class VirtualHostConfigTest extends AbstractVirtualHostTest {
@Test
public void projectsAllowsCatchAllRegExOnDefaults() throws IOException {
@@ -71,10 +52,4 @@
IllegalStateException.class,
() -> new VirtualHostConfig(testSitePaths).getProjects(TEST_VIRTUAL_HOST));
}
-
- private void setVirtualHostConfig(String section, String subsection, String projectsValue)
- throws IOException {
- virtualHostFileConfig.setString(section, subsection, "projects", projectsValue);
- virtualHostFileConfig.save();
- }
}
