Merge changes Ib468eb4b,I3dfd8f91

* changes:
  Unmark DETAILED_LABELS as requiring lazy load
  Obsolete PermissionBackend#indexedChange

java/com/google/gerrit/server/change/ChangeJson.java

@@ -140,7 +140,6 @@ public class ChangeJson {
           COMMIT_FOOTERS,
           CURRENT_ACTIONS,
           CURRENT_COMMIT,
-          DETAILED_LABELS, // may need to load ChangeNotes to check remove reviewer permissions
           MESSAGES);
 
   @Singleton
@@ -722,7 +721,10 @@ public class ChangeJson {
     // testRemoveReviewer check for a specific reviewer in the loop saving potentially many
     // permission checks.
     boolean canRemoveAnyReviewer =
-        permissionBackendForChange(userProvider.get(), cd).test(ChangePermission.REMOVE_REVIEWER);
+        permissionBackend
+            .user(userProvider.get())
+            .change(cd)
+            .test(ChangePermission.REMOVE_REVIEWER);
     for (LabelInfo label : labels) {
       if (label.all == null) {
         continue;
@@ -817,16 +819,4 @@ public class ChangeJson {
     }
     return map;
   }
-
-  /**
-   * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed
-   *     from either an index-backed or a database-backed {@link ChangeData} depending on {@code
-   *     lazyload}.
-   */
-  private PermissionBackend.ForChange permissionBackendForChange(CurrentUser user, ChangeData cd) {
-    PermissionBackend.WithUser withUser = permissionBackend.user(user);
-    return lazyLoad
-        ? withUser.change(cd)
-        : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change()));
-  }
 }

java/com/google/gerrit/server/change/LabelsJson.java

@@ -131,7 +131,7 @@ public class LabelsJson {
 
     Map<String, Short> labels = null;
     Set<LabelPermission.WithValue> can =
-        permissionBackendForChange(filterApprovalsBy, cd).testLabels(toCheck.values());
+        permissionBackend.absentUser(filterApprovalsBy).change(cd).testLabels(toCheck.values());
     SetMultimap<String, String> permitted = LinkedHashMultimap.create();
     for (SubmitRecord rec : submitRecords(cd)) {
       if (rec.labels == null) {
@@ -452,7 +452,7 @@ public class LabelsJson {
 
     LabelTypes labelTypes = cd.getLabelTypes();
     for (Account.Id accountId : allUsers) {
-      PermissionBackend.ForChange perm = permissionBackendForChange(accountId, cd);
+      PermissionBackend.ForChange perm = permissionBackend.absentUser(accountId).change(cd);
       Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(accountId, cd));
       for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
         LabelType lt = labelTypes.byLabel(e.getKey());
@@ -492,18 +492,6 @@ public class LabelsJson {
     }
   }
 
-  /**
-   * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed
-   *     from either an index-backed or a database-backed {@link ChangeData} depending on {@code
-   *     lazyload}.
-   */
-  private PermissionBackend.ForChange permissionBackendForChange(Account.Id user, ChangeData cd) {
-    PermissionBackend.WithUser withUser = permissionBackend.absentUser(user);
-    return lazyLoad
-        ? withUser.change(cd)
-        : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change()));
-  }
-
   private List<SubmitRecord> submitRecords(ChangeData cd) {
     return cd.submitRecords(ChangeJson.SUBMIT_RULE_OPTIONS_LENIENT);
   }

java/com/google/gerrit/server/change/RevisionJson.java

@@ -60,7 +60,6 @@ import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.account.GpgApiAdapter;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -107,8 +106,6 @@ public class RevisionJson {
   private final AnonymousUser anonymous;
   private final GitRepositoryManager repoManager;
   private final PermissionBackend permissionBackend;
-  private final ChangeNotes.Factory notesFactory;
-  private final boolean lazyLoad;
 
   @Inject
   RevisionJson(
@@ -128,7 +125,6 @@ public class RevisionJson {
       ChangeKindCache changeKindCache,
       GitRepositoryManager repoManager,
       PermissionBackend permissionBackend,
-      ChangeNotes.Factory notesFactory,
       @Assisted Iterable<ListChangesOption> options) {
     this.userProvider = userProvider;
     this.anonymous = anonymous;
@@ -145,10 +141,8 @@ public class RevisionJson {
     this.changeResourceFactory = changeResourceFactory;
     this.changeKindCache = changeKindCache;
     this.permissionBackend = permissionBackend;
-    this.notesFactory = notesFactory;
     this.repoManager = repoManager;
     this.options = ImmutableSet.copyOf(options);
-    this.lazyLoad = containsAnyOf(this.options, ChangeJson.REQUIRE_LAZY_LOAD);
   }
 
   /**
@@ -346,22 +340,9 @@ public class RevisionJson {
     return options.contains(option);
   }
 
-  /**
-   * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed
-   *     from either an index-backed or a database-backed {@link ChangeData} depending on {@code
-   *     lazyload}.
-   */
-  private PermissionBackend.ForChange permissionBackendForChange(
-      PermissionBackend.WithUser withUser, ChangeData cd) {
-    return lazyLoad
-        ? withUser.change(cd)
-        : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change()));
-  }
-
   private boolean isWorldReadable(ChangeData cd) throws PermissionBackendException {
     try {
-      permissionBackendForChange(permissionBackend.user(anonymous), cd)
+      permissionBackend.user(anonymous).change(cd).check(ChangePermission.READ);
-          .check(ChangePermission.READ);
     } catch (AuthException ae) {
       return false;
     }

java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java

@@ -165,7 +165,7 @@ public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener {
         List<CachedChange> result = new ArrayList<>(cds.size());
         for (ChangeData cd : cds) {
           result.add(
-              new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.getReviewers()));
+              new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.reviewers()));
         }
         return Collections.unmodifiableList(result);
       }

java/com/google/gerrit/server/permissions/ChangeControl.java

@@ -19,21 +19,16 @@ import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BE
 
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.conditions.BooleanCondition;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
 import com.google.gerrit.server.query.change.ChangeData;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
 import java.util.Collection;
 import java.util.EnumSet;
 import java.util.Map;
@@ -41,39 +36,16 @@ import java.util.Set;
 
 /** Access control management for a user accessing a single change. */
 class ChangeControl {
-  @Singleton
-  static class Factory {
-    private final ChangeData.Factory changeDataFactory;
-    private final ChangeNotes.Factory notesFactory;
-
-    @Inject
-    Factory(ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) {
-      this.changeDataFactory = changeDataFactory;
-      this.notesFactory = notesFactory;
-    }
-
-    ChangeControl create(RefControl refControl, Project.NameKey project, Change.Id changeId) {
-      return create(refControl, notesFactory.create(project, changeId));
-    }
-
-    ChangeControl create(RefControl refControl, ChangeNotes notes) {
-      return new ChangeControl(changeDataFactory, refControl, notes);
-    }
-  }
-
-  private final ChangeData.Factory changeDataFactory;
   private final RefControl refControl;
-  private final ChangeNotes notes;
+  private final ChangeData changeData;
 
-  private ChangeControl(
+  ChangeControl(RefControl refControl, ChangeData changeData) {
-      ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) {
-    this.changeDataFactory = changeDataFactory;
     this.refControl = refControl;
-    this.notes = notes;
+    this.changeData = changeData;
   }
 
-  ForChange asForChange(@Nullable ChangeData cd) {
+  ForChange asForChange() {
-    return new ForChangeImpl(cd);
+    return new ForChangeImpl();
   }
 
   private CurrentUser getUser() {
@@ -85,7 +57,7 @@ class ChangeControl {
   }
 
   private Change getChange() {
-    return notes.getChange();
+    return changeData.change();
   }
 
   /** Can this user see this change? */
@@ -218,19 +190,13 @@ class ChangeControl {
   }
 
   private class ForChangeImpl extends ForChange {
-    private ChangeData cd;
     private Map<String, PermissionRange> labels;
     private String resourcePath;
 
-    ForChangeImpl(@Nullable ChangeData cd) {
+    private ForChangeImpl() {}
-      this.cd = cd;
-    }
 
     private ChangeData changeData() {
-      if (cd == null) {
+      return changeData;
-        cd = changeDataFactory.create(notes);
-      }
-      return cd;
     }
 
     @Override

java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java

@@ -31,7 +31,6 @@ public class DefaultPermissionBackendModule extends AbstractModule {
       // TODO(hiesel) Hide ProjectControl, RefControl, ChangeControl related bindings.
       factory(ProjectControl.Factory.class);
       factory(DefaultRefFilter.Factory.class);
-      bind(ChangeControl.Factory.class);
     }
   }
 }

java/com/google/gerrit/server/permissions/DefaultRefFilter.java

@@ -448,12 +448,11 @@ class DefaultRefFilter {
     try {
       Map<Change.Id, BranchNameKey> visibleChanges = new HashMap<>();
       for (ChangeData cd : changeCache.getChangeData(project)) {
-        ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change());
         if (!projectState.statePermitsRead()) {
           continue;
         }
         try {
-          permissionBackendForProject.indexedChange(cd, notes).check(ChangePermission.READ);
+          permissionBackendForProject.change(cd).check(ChangePermission.READ);
           visibleChanges.put(cd.getId(), cd.change().getDest());
         } catch (AuthException e) {
           // Do nothing.

java/com/google/gerrit/server/permissions/FailedPermissionBackend.java

@@ -172,11 +172,6 @@ public class FailedPermissionBackend {
       return new FailedChange(message, cause);
     }
 
-    @Override
-    public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
-      return new FailedChange(message, cause);
-    }
-
     @Override
     public void check(RefPermission perm) throws PermissionBackendException {
       throw new PermissionBackendException(message, cause);

java/com/google/gerrit/server/permissions/PermissionBackend.java

@@ -173,15 +173,6 @@ public abstract class PermissionBackend {
       return ref(notes.getChange().getDest()).change(notes);
     }
 
-    /**
-     * Returns an instance scoped for the change loaded from index, and its destination ref and
-     * project. This method should only be used when database access is harmful and potentially
-     * stale data from the index is acceptable.
-     */
-    public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
-      return ref(notes.getChange().getDest()).indexedChange(cd, notes);
-    }
-
     /** Verify scoped user can {@code perm}, throwing if denied. */
     public abstract void check(GlobalOrPluginPermission perm)
         throws AuthException, PermissionBackendException;
@@ -289,15 +280,6 @@ public abstract class PermissionBackend {
       return ref(notes.getChange().getDest().branch()).change(notes);
     }
 
-    /**
-     * Returns an instance scoped for the change loaded from index, and its destination ref and
-     * project. This method should only be used when database access is harmful and potentially
-     * stale data from the index is acceptable.
-     */
-    public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
-      return ref(notes.getChange().getDest().branch()).indexedChange(cd, notes);
-    }
-
     /** Verify scoped user can {@code perm}, throwing if denied. */
     public abstract void check(CoreOrPluginProjectPermission perm)
         throws AuthException, PermissionBackendException;
@@ -386,12 +368,6 @@ public abstract class PermissionBackend {
     /** Returns an instance scoped to change. */
     public abstract ForChange change(ChangeNotes notes);
 
-    /**
-     * @return instance scoped to change loaded from index. This method should only be used when
-     *     database access is harmful and potentially stale data from the index is acceptable.
-     */
-    public abstract ForChange indexedChange(ChangeData cd, ChangeNotes notes);
-
     /** Verify scoped user can {@code perm}, throwing if denied. */
     public abstract void check(RefPermission perm) throws AuthException, PermissionBackendException;
 

java/com/google/gerrit/server/permissions/ProjectControl.java

@@ -70,9 +70,9 @@ class ProjectControl {
   private final PermissionBackend permissionBackend;
   private final CurrentUser user;
   private final ProjectState state;
-  private final ChangeControl.Factory changeControlFactory;
   private final PermissionCollection.Factory permissionFilter;
   private final DefaultRefFilter.Factory refFilterFactory;
+  private final ChangeData.Factory changeDataFactory;
 
   private List<SectionMatcher> allSections;
   private Map<String, RefControl> refControls;
@@ -83,17 +83,17 @@ class ProjectControl {
       @GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
       @GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
       PermissionCollection.Factory permissionFilter,
-      ChangeControl.Factory changeControlFactory,
       PermissionBackend permissionBackend,
       DefaultRefFilter.Factory refFilterFactory,
+      ChangeData.Factory changeDataFactory,
       @Assisted CurrentUser who,
       @Assisted ProjectState ps) {
-    this.changeControlFactory = changeControlFactory;
     this.uploadGroups = uploadGroups;
     this.receiveGroups = receiveGroups;
     this.permissionFilter = permissionFilter;
     this.permissionBackend = permissionBackend;
     this.refFilterFactory = refFilterFactory;
+    this.changeDataFactory = changeDataFactory;
     user = who;
     state = ps;
   }
@@ -102,13 +102,8 @@ class ProjectControl {
     return new ForProjectImpl();
   }
 
-  ChangeControl controlFor(Change change) {
+  ChangeControl controlFor(ChangeData cd) {
-    return changeControlFactory.create(
+    return new ChangeControl(controlForRef(cd.change().getDest()), cd);
-        controlForRef(change.getDest()), change.getProject(), change.getId());
-  }
-
-  ChangeControl controlFor(ChangeNotes notes) {
-    return changeControlFactory.create(controlForRef(notes.getChange().getDest()), notes);
   }
 
   RefControl controlForRef(BranchNameKey ref) {
@@ -122,7 +117,7 @@ class ProjectControl {
     RefControl ctl = refControls.get(refName);
     if (ctl == null) {
       PermissionCollection relevant = permissionFilter.filter(access(), refName, user);
-      ctl = new RefControl(this, refName, relevant);
+      ctl = new RefControl(changeDataFactory, this, refName, relevant);
       refControls.put(refName, ctl);
     }
     return ctl;

java/com/google/gerrit/server/permissions/RefControl.java

@@ -43,6 +43,7 @@ import java.util.Set;
 class RefControl {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  private final ChangeData.Factory changeDataFactory;
   private final ProjectControl projectControl;
   private final String refName;
 
@@ -58,7 +59,12 @@ class RefControl {
   private Boolean canForgeCommitter;
   private Boolean isVisible;
 
-  RefControl(ProjectControl projectControl, String ref, PermissionCollection relevant) {
+  RefControl(
+      ChangeData.Factory changeDataFactory,
+      ProjectControl projectControl,
+      String ref,
+      PermissionCollection relevant) {
+    this.changeDataFactory = changeDataFactory;
     this.projectControl = projectControl;
     this.refName = ref;
     this.relevant = relevant;
@@ -440,7 +446,7 @@ class RefControl {
     @Override
     public ForChange change(ChangeData cd) {
       try {
-        return getProjectControl().controlFor(cd.notes()).asForChange(cd);
+        return getProjectControl().controlFor(cd).asForChange();
       } catch (StorageException e) {
         return FailedPermissionBackend.change("unavailable", e);
       }
@@ -455,12 +461,9 @@ class RefControl {
           "expected change in project %s, not %s",
           project,
           change.getProject());
-      return getProjectControl().controlFor(notes).asForChange(null);
+      // Having ChangeNotes means it's OK to load values from NoteDb if needed.
-    }
+      // ChangeData.Factory will allow lazyLoading
-
+      return getProjectControl().controlFor(changeDataFactory.create(notes)).asForChange();
-    @Override
-    public ForChange indexedChange(ChangeData cd, ChangeNotes notes) {
-      return getProjectControl().controlFor(notes).asForChange(cd);
     }
 
     @Override

java/com/google/gerrit/server/query/change/ChangeData.java

@@ -674,7 +674,9 @@ public class ChangeData {
   public ReviewerSet reviewers() {
     if (reviewers == null) {
       if (!lazyLoad) {
-        return ReviewerSet.empty();
+        // We are not allowed to load values from NoteDb. Reviewers were not populated with values
+        // from the index. However, we need these values for permission checks.
+        throw new IllegalStateException("reviewers not populated");
       }
       reviewers = approvalsUtil.getReviewers(notes(), approvals().values());
     }
@@ -685,10 +687,6 @@ public class ChangeData {
     this.reviewers = reviewers;
   }
 
-  public ReviewerSet getReviewers() {
-    return reviewers;
-  }
-
   public ReviewerByEmailSet reviewersByEmail() {
     if (reviewersByEmail == null) {
       if (!lazyLoad) {

java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java

@@ -72,7 +72,6 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
       return false;
     }
 
-    ChangeNotes notes = notesFactory.createFromIndexedChange(change);
     Optional<ProjectState> projectState = projectCache.get(cd.project());
     if (!projectState.isPresent()) {
       logger.atFine().log("Filter out change %s of non-existing project %s", cd, cd.project());
@@ -88,7 +87,7 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
             ? permissionBackend.absentUser(user.getAccountId())
             : permissionBackend.user(anonymousUserProvider.get());
     try {
-      withUser.indexedChange(cd, notes).check(ChangePermission.READ);
+      withUser.change(cd).check(ChangePermission.READ);
     } catch (PermissionBackendException e) {
       Throwable cause = e.getCause();
       if (cause instanceof RepositoryNotFoundException) {

javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java

@@ -0,0 +1,70 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// 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.google.gerrit.acceptance.server.permissions;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.inject.Inject;
+import org.junit.Test;
+
+/** Asserts behavior on {@link PermissionBackend} using a fully-started Gerrit. */
+public class PermissionBackendIT extends AbstractDaemonTest {
+  @Inject PermissionBackend pb;
+  @Inject ChangeNotes.Factory changeNotesFactory;
+
+  @Test
+  public void changeDataFromIndex_canCheckReviewerState() throws Exception {
+    Change.Id changeId = createChange().getChange().getId();
+    gApi.changes().id(changeId.get()).setPrivate(true);
+    gApi.changes().id(changeId.get()).addReviewer(user.email());
+
+    ChangeData changeData =
+        Iterables.getOnlyElement(queryProvider.get().byLegacyChangeId(changeId));
+    boolean reviewerCanSee =
+        pb.absentUser(user.id()).change(changeData).test(ChangePermission.READ);
+    assertThat(reviewerCanSee).isTrue();
+  }
+
+  @Test
+  public void changeDataFromNoteDb_canCheckReviewerState() throws Exception {
+    Change.Id changeId = createChange().getChange().getId();
+    gApi.changes().id(changeId.get()).setPrivate(true);
+    gApi.changes().id(changeId.get()).addReviewer(user.email());
+
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+    ChangeData changeData = changeDataFactory.create(notes);
+    boolean reviewerCanSee =
+        pb.absentUser(user.id()).change(changeData).test(ChangePermission.READ);
+    assertThat(reviewerCanSee).isTrue();
+  }
+
+  @Test
+  public void changeNotes_canCheckReviewerState() throws Exception {
+    Change.Id changeId = createChange().getChange().getId();
+    gApi.changes().id(changeId.get()).setPrivate(true);
+    gApi.changes().id(changeId.get()).addReviewer(user.email());
+
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+    boolean reviewerCanSee = pb.absentUser(user.id()).change(notes).test(ChangePermission.READ);
+    assertThat(reviewerCanSee).isTrue();
+  }
+}

javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java

@@ -65,7 +65,7 @@ public class SetReviewersIT extends AbstractDaemonTest {
     session.exec(
         String.format("gerrit set-reviewers -%s %s %s", add ? "a" : "r", user.email(), id));
     session.assertSuccess();
-    ImmutableSet<Account.Id> reviewers = change.getChange().getReviewers().all();
+    ImmutableSet<Account.Id> reviewers = change.getChange().reviewers().all();
     if (add) {
       assertThat(reviewers).contains(user.id());
     } else {