Remove rescoping from PermissionBackend
Rescoping a permission backend to a new user is in place for performance
reasons. Looking at it from the software engineering perspective, we
would rather remove the rescoping to make the interface easier to use
and implement.
This commit refactors the callers of rescoping methods to construct
permission backends from scratch. On googlesource.com we have metrics in
place and will monitor the impact on performance. In case there is a
negative effect, we will revert the commit.
The commit leaves the interface definition as-is so that it can be
easily reverted. If the experiment is successful, we will remove the
interface definition of #user(CurrentUser user).
Change-Id: Ia6abfd735651b99942d5af82a322ddc845525c98
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index c6dbda3..622e06b 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -174,6 +174,7 @@
private final Config gerritConfig;
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
+ private final PermissionBackend permissionBackend;
private final boolean strictLabels;
@Inject
@@ -196,7 +197,8 @@
NotifyUtil notifyUtil,
@GerritServerConfig Config gerritConfig,
WorkInProgressOp.Factory workInProgressOpFactory,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ PermissionBackend permissionBackend) {
super(retryHelper);
this.db = db;
this.changeResourceFactory = changeResourceFactory;
@@ -216,6 +218,7 @@
this.gerritConfig = gerritConfig;
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
+ this.permissionBackend = permissionBackend;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -482,7 +485,11 @@
IdentifiedUser reviewer = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
try {
- perm.user(reviewer).check(ChangePermission.READ);
+ permissionBackend
+ .user(reviewer)
+ .database(db)
+ .change(rev.getNotes())
+ .check(ChangePermission.READ);
} catch (AuthException e) {
throw new UnprocessableEntityException(
String.format("on_behalf_of account %s cannot see change", reviewer.getAccountId()));
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index 4991c18..87e6163 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
@@ -40,6 +40,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
+import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -250,9 +251,7 @@
return null;
}
- PermissionBackend.ForRef perm =
- permissionBackend.absentUser(reviewerUser.getAccountId()).ref(rsrc.getChange().getDest());
- if (isValidReviewer(reviewerUser.getAccount(), perm)) {
+ if (isValidReviewer(rsrc.getChange().getDest(), reviewerUser.getAccount())) {
return new Addition(
reviewer,
rsrc,
@@ -333,10 +332,8 @@
ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
}
- PermissionBackend.ForRef perm =
- permissionBackend.user(rsrc.getUser()).ref(rsrc.getChange().getDest());
for (Account member : members) {
- if (isValidReviewer(member, perm)) {
+ if (isValidReviewer(rsrc.getChange().getDest(), member)) {
reviewers.add(member.getId());
}
}
@@ -373,7 +370,7 @@
reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
}
- private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm)
+ private boolean isValidReviewer(Branch.NameKey branch, Account member)
throws PermissionBackendException {
if (!member.isActive()) {
return false;
@@ -382,7 +379,11 @@
// Does not account for draft status as a user might want to let a
// reviewer see a draft.
try {
- perm.absentUser(member.getId()).check(RefPermission.READ);
+ permissionBackend
+ .absentUser(member.getId())
+ .database(dbProvider)
+ .ref(branch)
+ .check(RefPermission.READ);
return true;
} catch (AuthException e) {
return false;
diff --git a/java/com/google/gerrit/server/restapi/change/PutAssignee.java b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
index b7be2a8..21bd3ce 100644
--- a/java/com/google/gerrit/server/restapi/change/PutAssignee.java
+++ b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.SetAssigneeOp;
import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.restapi.account.AccountsCollection;
import com.google.gerrit.server.restapi.change.PostReviewers.Addition;
@@ -55,6 +56,7 @@
private final Provider<ReviewDb> db;
private final PostReviewers postReviewers;
private final AccountLoader.Factory accountLoaderFactory;
+ private final PermissionBackend permissionBackend;
@Inject
PutAssignee(
@@ -63,13 +65,15 @@
RetryHelper retryHelper,
Provider<ReviewDb> db,
PostReviewers postReviewers,
- AccountLoader.Factory accountLoaderFactory) {
+ AccountLoader.Factory accountLoaderFactory,
+ PermissionBackend permissionBackend) {
super(retryHelper);
this.accounts = accounts;
this.assigneeFactory = assigneeFactory;
this.db = db;
this.postReviewers = postReviewers;
this.accountLoaderFactory = accountLoaderFactory;
+ this.permissionBackend = permissionBackend;
}
@Override
@@ -89,9 +93,10 @@
throw new UnprocessableEntityException(input.assignee + " is not active");
}
try {
- rsrc.permissions()
- .database(db)
+ permissionBackend
.absentUser(assignee.getAccountId())
+ .database(db)
+ .change(rsrc.getNotes())
.check(ChangePermission.READ);
} catch (AuthException e) {
throw new AuthException("read not permitted for " + input.assignee);
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index a161767..f9a5087 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -468,7 +468,11 @@
CurrentUser caller = rsrc.getUser();
IdentifiedUser submitter = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
try {
- perm.user(submitter).check(ChangePermission.READ);
+ permissionBackend
+ .user(submitter)
+ .database(dbProvider)
+ .change(rsrc.getNotes())
+ .check(ChangePermission.READ);
} catch (AuthException e) {
throw new UnprocessableEntityException(
String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()));
diff --git a/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java b/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java
index bc3dfa7..66534b0 100644
--- a/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/SuggestChangeReviewers.java
@@ -85,14 +85,18 @@
}
private VisibilityControl getVisibility(ChangeResource rsrc) {
- // Use the destination reference, not the change, as private changes deny anyone who is not
- // already a reviewer.
- PermissionBackend.ForRef perm = permissionBackend.currentUser().ref(rsrc.getChange().getDest());
+
return new VisibilityControl() {
@Override
public boolean isVisibleTo(Account.Id account) throws OrmException {
+ // Use the destination reference, not the change, as private changes deny anyone who is not
+ // already a reviewer.
IdentifiedUser who = identifiedUserFactory.create(account);
- return perm.user(who).testOrFalse(RefPermission.READ);
+ return permissionBackend
+ .user(who)
+ .database(dbProvider)
+ .ref(rsrc.getChange().getDest())
+ .testOrFalse(RefPermission.READ);
}
};
}
