From b83a15511574afaacd74dbced3d189e3d2673308 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 1 Mar 2022 16:52:05 +0000 Subject: Change visibility: prefer test instead of check Do not rely to throwing AuthExceptions for checking change visibility but ForChange.test instead, which returns a boolean. Relying to exceptions to be thrown and catching them for a simple change filtering would cause the JVM to trigger multiple context switching and slowing down the overall ACL evaluation. A slow ACL evaluation for changes filtering may result in the increase of threads utilisation while the change query is executed. Release-Notes: skip Change-Id: Ie488a41b28b5c19710a11c2840793f04b1d3861b --- .../gerrit/server/query/change/ChangeIsVisibleToPredicate.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java index e3e0312cbc..60587da27b 100644 --- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java +++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.query.change; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Change; import com.google.gerrit.exceptions.StorageException; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.index.query.IsVisibleToPredicate; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; @@ -87,7 +86,10 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate .filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser) .orElseGet(anonymousUserProvider::get)); try { - withUser.change(cd).check(ChangePermission.READ); + if (!withUser.change(cd).test(ChangePermission.READ)) { + logger.atFine().log("Filter out non-visisble change: %s", cd); + return false; + } } catch (PermissionBackendException e) { Throwable cause = e.getCause(); if (cause instanceof RepositoryNotFoundException) { @@ -97,9 +99,6 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate return false; } throw new StorageException("unable to check permissions on change " + cd.getId(), e); - } catch (AuthException e) { - logger.atFine().log("Filter out non-visisble change: %s", cd); - return false; } cd.cacheVisibleTo(user); -- cgit v1.2.3