Merge "Only construct ChangeResources via ChangesCollection.parse"

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java

@@ -34,7 +34,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.change.Revisions;
 import com.google.gerrit.server.extensions.webui.UiActions;
@@ -76,6 +76,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
   private final ReviewDb db;
   private final PatchListCache patchListCache;
   private final ChangeControl.Factory changeControlFactory;
+  private final ChangesCollection changes;
   private final Revisions revisions;
 
   private Project.NameKey projectKey;
@@ -93,6 +94,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
   PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db,
       final PatchListCache patchListCache,
       final ChangeControl.Factory changeControlFactory,
+      final ChangesCollection changes,
       final Revisions revisions,
       @Assisted("psIdBase") @Nullable final PatchSet.Id psIdBase,
       @Assisted("psIdNew") final PatchSet.Id psIdNew,
@@ -101,6 +103,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
     this.db = db;
     this.patchListCache = patchListCache;
     this.changeControlFactory = changeControlFactory;
+    this.changes = changes;
     this.revisions = revisions;
 
     this.psIdBase = psIdBase;
@@ -179,7 +182,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
     detail.setCommands(Lists.newArrayList(Iterables.transform(
         UiActions.sorted(UiActions.plugins(UiActions.from(
           revisions,
-          new RevisionResource(new ChangeResource(control), patchSet),
+          new RevisionResource(changes.parse(control), patchSet),
           Providers.of(user)))),
         new Function<UiAction.Description, UiCommandDetail>() {
           @Override

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java

@@ -20,6 +20,7 @@ import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.PermissionRule;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -33,6 +34,7 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.MergeabilityChecker;
 import com.google.gerrit.server.change.PostReviewers;
 import com.google.gerrit.server.config.AllProjectsNameProvider;
@@ -41,7 +43,6 @@ import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.mail.CreateChangeSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.SetParent;
@@ -78,11 +79,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
   private final IdentifiedUser user;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final Provider<PostReviewers> reviewersProvider;
-  private final ChangeControl.GenericFactory changeFactory;
   private final MergeabilityChecker mergeabilityChecker;
   private final ChangeHooks hooks;
   private final CreateChangeSender.Factory createChangeSenderFactory;
   private final ProjectCache projectCache;
+  private final ChangesCollection changes;
 
   @Inject
   ReviewProjectAccess(final ProjectControl.Factory projectControlFactory,
@@ -90,11 +91,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
       MetaDataUpdate.User metaDataUpdateFactory, ReviewDb db,
       IdentifiedUser user, PatchSetInfoFactory patchSetInfoFactory,
       Provider<PostReviewers> reviewersProvider,
-      ChangeControl.GenericFactory changeFactory,
       MergeabilityChecker mergeabilityChecker, ChangeHooks hooks,
       CreateChangeSender.Factory createChangeSenderFactory,
       ProjectCache projectCache,
       AllProjectsNameProvider allProjects,
+      ChangesCollection changes,
       Provider<SetParent> setParent,
 
       @Assisted("projectName") Project.NameKey projectName,
@@ -109,11 +110,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
     this.user = user;
     this.patchSetInfoFactory = patchSetInfoFactory;
     this.reviewersProvider = reviewersProvider;
-    this.changeFactory = changeFactory;
     this.mergeabilityChecker = mergeabilityChecker;
     this.hooks = hooks;
     this.createChangeSenderFactory = createChangeSenderFactory;
     this.projectCache = projectCache;
+    this.changes = changes;
   }
 
   @Override
@@ -165,9 +166,15 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
     } catch (Exception err) {
       log.error("Cannot send email for new change " + change.getId(), err);
     }
-    addProjectOwnersAsReviewers(change);
+    ChangeResource rsrc;
+    try {
+      rsrc = changes.parse(changeId);
+    } catch (ResourceNotFoundException e) {
+      throw new IOException(e);
+    }
+    addProjectOwnersAsReviewers(rsrc);
     if (parentProjectUpdate) {
-      addAdministratorsAsReviewers(change);
+      addAdministratorsAsReviewers(rsrc);
     }
     return changeId;
   }
@@ -186,12 +193,10 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
     db.patchSetAncestors().insert(toInsert);
   }
 
-  private void addProjectOwnersAsReviewers(Change change) {
+  private void addProjectOwnersAsReviewers(ChangeResource rsrc) {
     final String projectOwners =
         groupBackend.get(SystemGroupBackend.PROJECT_OWNERS).getName();
     try {
-      ChangeResource rsrc =
-          new ChangeResource(changeFactory.controlFor(change, user));
       AddReviewerInput input = new AddReviewerInput();
       input.reviewer = projectOwners;
       reviewersProvider.get().apply(rsrc, input);
@@ -201,15 +206,13 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
     }
   }
 
-  private void addAdministratorsAsReviewers(Change change) {
+  private void addAdministratorsAsReviewers(ChangeResource rsrc) {
     List<PermissionRule> adminRules =
         projectCache.getAllProjects().getConfig()
             .getAccessSection(AccessSection.GLOBAL_CAPABILITIES)
             .getPermission(GlobalCapability.ADMINISTRATE_SERVER).getRules();
     for (PermissionRule r : adminRules) {
       try {
-        ChangeResource rsrc =
-            new ChangeResource(changeFactory.controlFor(change, user));
         AddReviewerInput input = new AddReviewerInput();
         input.reviewer = r.getGroup().getUUID().get();
         reviewersProvider.get().apply(rsrc, input);

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -133,11 +133,12 @@ public class ChangeJson {
   private final IdentifiedUser.GenericFactory userFactory;
   private final ProjectControl.GenericFactory projectControlFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
+  private final ChangesCollection changes;
   private final FileInfoJson fileInfoJson;
   private final AccountInfo.Loader.Factory accountLoaderFactory;
   private final DynamicMap<DownloadScheme> downloadSchemes;
   private final DynamicMap<DownloadCommand> downloadCommands;
-  private final DynamicMap<RestView<ChangeResource>> changes;
+  private final DynamicMap<RestView<ChangeResource>> changeViews;
   private final Revisions revisions;
   private final PatchListCache patchListCache;
 
@@ -156,11 +157,12 @@ public class ChangeJson {
       IdentifiedUser.GenericFactory uf,
       ProjectControl.GenericFactory pcf,
       PatchSetInfoFactory psi,
+      ChangesCollection changes,
       FileInfoJson fileInfoJson,
       AccountInfo.Loader.Factory ailf,
       DynamicMap<DownloadScheme> downloadSchemes,
       DynamicMap<DownloadCommand> downloadCommands,
-      DynamicMap<RestView<ChangeResource>> changes,
+      DynamicMap<RestView<ChangeResource>> changeViews,
       Revisions revisions,
       PatchListCache patchListCache) {
     this.db = db;
@@ -170,11 +172,12 @@ public class ChangeJson {
     this.userFactory = uf;
     this.projectControlFactory = pcf;
     this.patchSetInfoFactory = psi;
+    this.changes = changes;
     this.fileInfoJson = fileInfoJson;
     this.accountLoaderFactory = ailf;
     this.downloadSchemes = downloadSchemes;
     this.downloadCommands = downloadCommands;
-    this.changes = changes;
+    this.changeViews = changeViews;
     this.revisions = revisions;
     this.patchListCache = patchListCache;
 
@@ -322,8 +325,8 @@ public class ChangeJson {
     if (has(CURRENT_ACTIONS) && userProvider.get().isIdentifiedUser()) {
       out.actions = Maps.newTreeMap();
       for (UiAction.Description d : UiActions.from(
-          changes,
+          changeViews,
-          new ChangeResource(control(cd)),
+          changes.parse(control(cd)),
           userProvider)) {
         out.actions.put(d.getId(), new ActionInfo(d));
       }
@@ -821,7 +824,7 @@ public class ChangeJson {
       out.actions = Maps.newTreeMap();
       for (UiAction.Description d : UiActions.from(
           revisions,
-          new RevisionResource(new ChangeResource(control(cd)), in),
+          new RevisionResource(changes.parse(control(cd)), in),
           userProvider)) {
         out.actions.put(d.getId(), new ActionInfo(d));
       }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java

@@ -35,7 +35,7 @@ public class ChangeResource implements RestResource, HasETag {
 
   private final ChangeControl control;
 
-  public ChangeResource(ChangeControl control) {
+  ChangeResource(ChangeControl control) {
     this.control = control;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java

@@ -83,6 +83,16 @@ public class ChangesCollection implements
     return new ChangeResource(control);
   }
 
+  public ChangeResource parse(Change.Id id)
+      throws ResourceNotFoundException, OrmException {
+    return parse(TopLevelResource.INSTANCE,
+        IdString.fromUrl(Integer.toString(id.get())));
+  }
+
+  public ChangeResource parse(ChangeControl control) throws OrmException {
+    return new ChangeResource(control);
+  }
+
   private List<Change> findChanges(String id)
       throws OrmException, ResourceNotFoundException {
     // Try legacy id

gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java

@@ -37,7 +37,6 @@ import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.git.WorkQueue.Executor;
 import com.google.gerrit.server.index.ChangeIndexer;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.util.RequestContext;
 import com.google.gerrit.server.util.ThreadLocalRequestContext;
 import com.google.gwtorm.server.OrmException;
@@ -64,8 +63,8 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
 
   private final ThreadLocalRequestContext tl;
   private final SchemaFactory<ReviewDb> schemaFactory;
-  private final ChangeControl.GenericFactory changeControlFactory;
   private final IdentifiedUser.GenericFactory identifiedUserFactory;
+  private final ChangesCollection changes;
   private final Provider<Mergeable> mergeable;
   private final ChangeIndexer indexer;
   private final ListeningExecutorService executor;
@@ -75,16 +74,15 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
   @Inject
   public MergeabilityChecker(ThreadLocalRequestContext tl,
       SchemaFactory<ReviewDb> schemaFactory,
-      ChangeControl.GenericFactory changeControlFactory,
       IdentifiedUser.GenericFactory identifiedUserFactory,
-      Provider<Mergeable> mergeable, ChangeIndexer indexer,
+      ChangesCollection changes, Provider<Mergeable> mergeable,
-      @MergeabilityChecksExecutor Executor executor,
+      ChangeIndexer indexer, @MergeabilityChecksExecutor Executor executor,
       MergeabilityCheckQueue mergeabilityCheckQueue,
       MetaDataUpdate.Server metaDataUpdateFactory) {
     this.tl = tl;
     this.schemaFactory = schemaFactory;
-    this.changeControlFactory = changeControlFactory;
     this.identifiedUserFactory = identifiedUserFactory;
+    this.changes = changes;
     this.mergeable = mergeable;
     this.indexer = indexer;
     this.executor = MoreExecutors.listeningDecorator(executor);
@@ -263,8 +261,8 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
         PatchSet ps = db.patchSets().get(change.currentPatchSetId());
         Mergeable m = mergeable.get();
         m.setForce(force);
-        MergeableInfo info = m.apply(new RevisionResource(new ChangeResource(
+        MergeableInfo info = m.apply(
-            changeControlFactory.controlFor(change, context.getCurrentUser())), ps));
+            new RevisionResource(changes.parse(change.getId()), ps));
         return change.isMergeable() != info.mergeable;
       } catch (ResourceConflictException e) {
         // change is closed

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -73,6 +73,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   }
 
   private final Provider<ReviewDb> db;
+  private final ChangesCollection changes;
   private final ChangeIndexer indexer;
   private final AccountsCollection accounts;
   private final EmailReviewComments.Factory email;
@@ -87,11 +88,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
   @Inject
   PostReview(Provider<ReviewDb> db,
+      ChangesCollection changes,
       ChangeIndexer indexer,
       AccountsCollection accounts,
       EmailReviewComments.Factory email,
       ChangeHooks hooks) {
     this.db = db;
+    this.changes = changes;
     this.indexer = indexer;
     this.accounts = accounts;
     this.email = email;
@@ -193,7 +196,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     }
 
     ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
-    return new RevisionResource(new ChangeResource(target), rev.getPatchSet());
+    return new RevisionResource(changes.parse(target), rev.getPatchSet());
   }
 
   private void checkLabels(RevisionResource revision, boolean strict,

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -69,7 +69,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeInserter;
-import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.MergeabilityChecker;
 import com.google.gerrit.server.change.PatchSetInserter;
 import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
@@ -269,6 +269,7 @@ public class ReceiveCommits {
   private final CommitValidators.Factory commitValidatorsFactory;
   private final TagCache tagCache;
   private final AccountCache accountCache;
+  private final ChangesCollection changes;
   private final ChangeInserter.Factory changeInserterFactory;
   private final WorkQueue workQueue;
   private final ListeningExecutorService changeUpdateExector;
@@ -329,6 +330,7 @@ public class ReceiveCommits {
       final TagCache tagCache,
       final AccountCache accountCache,
       final ChangeCache changeCache,
+      final ChangesCollection changes,
       final ChangeInserter.Factory changeInserterFactory,
       final CommitValidators.Factory commitValidatorsFactory,
       @CanonicalWebUrl final String canonicalWebUrl,
@@ -364,6 +366,7 @@ public class ReceiveCommits {
     this.canonicalWebUrl = canonicalWebUrl;
     this.tagCache = tagCache;
     this.accountCache = accountCache;
+    this.changes = changes;
     this.changeInserterFactory = changeInserterFactory;
     this.commitValidatorsFactory = commitValidatorsFactory;
     this.workQueue = workQueue;
@@ -1561,7 +1564,7 @@ public class ReceiveCommits {
   private void submit(ChangeControl changeCtl, PatchSet ps)
       throws OrmException, IOException {
     Submit submit = submitProvider.get();
-    RevisionResource rsrc = new RevisionResource(new ChangeResource(changeCtl), ps);
+    RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
     Change c = submit.submit(rsrc, currentUser);
     if (c == null) {
       addError("Submitting change " + changeCtl.getChange().getChangeId()

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -42,7 +42,7 @@ import com.google.gerrit.server.account.AccountManager;
 import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
-import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.PostReview;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.project.ChangeControl;
@@ -79,6 +79,7 @@ public abstract class AbstractQueryChangesTest {
   @Inject protected AccountManager accountManager;
   @Inject protected ChangeControl.GenericFactory changeControlFactory;
   @Inject protected ChangeInserter.Factory changeFactory;
+  @Inject protected ChangesCollection changes;
   @Inject protected CreateProject.Factory projectFactory;
   @Inject protected IdentifiedUser.RequestFactory userFactory;
   @Inject protected InMemoryDatabase schemaFactory;
@@ -365,7 +366,7 @@ public abstract class AbstractQueryChangesTest {
     input.message = "toplevel";
     input.labels = ImmutableMap.<String, Short> of("Code-Review", (short) 1);
     postReview.apply(new RevisionResource(
-        new ChangeResource(ctl), ins.getPatchSet()), input);
+        changes.parse(ctl), ins.getPatchSet()), input);
 
     assertTrue(query("label:Code-Review=-2").isEmpty());
     assertTrue(query("label:Code-Review-2").isEmpty());
@@ -479,7 +480,7 @@ public abstract class AbstractQueryChangesTest {
     ReviewInput input = new ReviewInput();
     input.message = "toplevel";
     postReview.apply(new RevisionResource(
-        new ChangeResource(ctl1), ins1.getPatchSet()), input);
+        changes.parse(ctl1), ins1.getPatchSet()), input);
     change1 = db.changes().get(change1.getId());
 
     assertTrue(lastUpdatedMs(change1) > lastUpdatedMs(change2));
@@ -512,7 +513,7 @@ public abstract class AbstractQueryChangesTest {
     ReviewInput input = new ReviewInput();
     input.message = "toplevel";
     postReview.apply(new RevisionResource(
-        new ChangeResource(ctl1), ins1.getPatchSet()), input);
+        changes.parse(ctl1), ins1.getPatchSet()), input);
     change1 = db.changes().get(change1.getId());
 
     assertTrue(lastUpdatedMs(change1) > lastUpdatedMs(change2));
@@ -610,7 +611,7 @@ public abstract class AbstractQueryChangesTest {
     input.comments = ImmutableMap.<String, List<ReviewInput.Comment>> of(
         "Foo.java", ImmutableList.<ReviewInput.Comment> of(comment));
     postReview.apply(new RevisionResource(
-        new ChangeResource(ctl), ins.getPatchSet()), input);
+        changes.parse(ctl), ins.getPatchSet()), input);
 
     assertTrue(query("comment:foo").isEmpty());
     assertResultEquals(change, queryOne("comment:toplevel"));

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java

@@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.DeleteReviewer;
 import com.google.gerrit.server.change.PostReviewers;
 import com.google.gerrit.server.change.ReviewerResource;
@@ -88,6 +89,9 @@ public class SetReviewersCommand extends SshCommand {
   @Inject
   private ChangeControl.Factory changeControlFactory;
 
+  @Inject
+  private ChangesCollection changesCollection;
+
   private Set<Account.Id> toRemove = new HashSet<Account.Id>();
   private Set<Change.Id> changes = new HashSet<Change.Id>();
 
@@ -110,8 +114,7 @@ public class SetReviewersCommand extends SshCommand {
   }
 
   private boolean modifyOne(Change.Id changeId) throws Exception {
-    ChangeResource changeRsrc =
+    ChangeResource changeRsrc = changesCollection.parse(changeId);
-        new ChangeResource(changeControlFactory.validateFor(changeId));
     boolean ok = true;
 
     // Remove reviewers