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
diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
index e3e0312..60587da 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 @@
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 @@
.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 @@
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);
