Add a /changes option to include detailed account info

Change-Id: I793e97762231372f223b069cff26c291ad767309

Documentation/rest-api-changes.txt

@@ -152,6 +152,9 @@ default. Optional fields are:
   `CURRENT_REVISION` was requested the only that commit's
   modified files will be output.
 
+* `DETAILED_ACCOUNTS`: include `_account_id` and `email` fields when
+  referencing accounts.
+
 ----
   GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES HTTP/1.0
 

gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java

@@ -30,7 +30,10 @@ public enum ListChangesOption {
 
   /** If a patch set is included, include the files of the patch set. */
   CURRENT_FILES(5),
-  ALL_FILES(6);
+  ALL_FILES(6),
+
+  /** If accounts are included, include detailed account info. */
+  DETAILED_ACCOUNTS(7);
 
   private final int value;
 

gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCache.java

@@ -20,6 +20,8 @@ import com.google.gerrit.reviewdb.client.Account;
 public interface AccountCache {
   public AccountState get(Account.Id accountId);
 
+  public AccountState getIfPresent(Account.Id accountId);
+
   public AccountState getByUsername(String username);
 
   public void evict(Account.Id accountId);

gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java

@@ -87,6 +87,10 @@ public class AccountCacheImpl implements AccountCache {
     }
   }
 
+  public AccountState getIfPresent(Account.Id accountId) {
+    return byId.getIfPresent(accountId);
+  }
+
   @Override
   public AccountState getByUsername(String username) {
     try {

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

@@ -0,0 +1,110 @@
+// Copyright (C) 2013 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.server.change;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.assistedinject.Assisted;
+
+import java.util.List;
+import java.util.Map;
+
+public class AccountInfo {
+  static class Loader {
+    interface Factory {
+      Loader create(boolean detailed);
+    }
+
+    private final Provider<ReviewDb> db;
+    private final AccountCache accountCache;
+    private final boolean detailed;
+    private final Map<Account.Id, AccountInfo> created;
+    private final List<AccountInfo> provided;
+
+    @Inject
+    Loader(Provider<ReviewDb> db,
+        AccountCache accountCache,
+        @Assisted boolean detailed) {
+      this.db = db;
+      this.accountCache = accountCache;
+      this.detailed = detailed;
+      created = Maps.newHashMap();
+      provided = Lists.newArrayList();
+    }
+
+    public AccountInfo get(Account.Id id) {
+      if (id == null) {
+        return null;
+      }
+      AccountInfo info = created.get(id);
+      if (info == null) {
+        info = new AccountInfo(id);
+        created.put(id, info);
+      }
+      return info;
+    }
+
+    public void put(AccountInfo info) {
+      provided.add(info);
+    }
+
+    public void fill() throws OrmException {
+      Multimap<Account.Id, AccountInfo> missing = ArrayListMultimap.create();
+      for (AccountInfo info : Iterables.concat(created.values(), provided)) {
+        AccountState state = accountCache.getIfPresent(info._id);
+        if (state != null) {
+          fill(info, state.getAccount());
+        } else {
+          missing.put(info._id, info);
+        }
+      }
+      if (!missing.isEmpty()) {
+        for (Account account : db.get().accounts().get(missing.keySet())) {
+          for (AccountInfo info : missing.get(account.getId())) {
+            fill(info, account);
+          }
+        }
+      }
+    }
+
+    private void fill(AccountInfo info, Account account) {
+      info.name = account.getFullName();
+      if (detailed) {
+        info._account_id = account.getId().get();
+        info.email = account.getPreferredEmail();
+      }
+    }
+  }
+
+  private transient Account.Id _id;
+
+  protected AccountInfo(Account.Id id) {
+    _id = id;
+  }
+
+  public Integer _account_id;
+  public String name;
+  public String email;
+}

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

@@ -20,6 +20,7 @@ import static com.google.gerrit.common.changes.ListChangesOption.ALL_REVISIONS;
 import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_COMMIT;
 import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_FILES;
 import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_REVISION;
+import static com.google.gerrit.common.changes.ListChangesOption.DETAILED_ACCOUNTS;
 import static com.google.gerrit.common.changes.ListChangesOption.LABELS;
 
 import com.google.common.base.Joiner;
@@ -47,7 +48,6 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.events.AccountAttribute;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListEntry;
@@ -106,13 +106,14 @@ public class ChangeJson {
   private final ChangeControl.GenericFactory changeControlGenericFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final PatchListCache patchListCache;
+  private final AccountInfo.Loader.Factory accountLoaderFactory;
   private final Provider<String> urlProvider;
   private final Urls urls;
   private ChangeControl.Factory changeControlUserFactory;
   private SshInfo sshInfo;
-  private Map<Account.Id, AccountAttribute> accounts;
   private Map<Change.Id, ChangeControl> controls;
   private EnumSet<ListChangesOption> options;
+  private AccountInfo.Loader accountLoader;
 
   @Inject
   ChangeJson(
@@ -123,6 +124,7 @@ public class ChangeJson {
       ChangeControl.GenericFactory gf,
       PatchSetInfoFactory psi,
       PatchListCache plc,
+      AccountInfo.Loader.Factory ailf,
       @CanonicalWebUrl Provider<String> curl,
       Urls urls) {
     this.db = db;
@@ -132,10 +134,10 @@ public class ChangeJson {
     this.changeControlGenericFactory = gf;
     this.patchSetInfoFactory = psi;
     this.patchListCache = plc;
+    this.accountLoaderFactory = ailf;
     this.urlProvider = curl;
     this.urls = urls;
 
-    accounts = Maps.newHashMap();
     controls = Maps.newHashMap();
     options = EnumSet.noneOf(ListChangesOption.class);
   }
@@ -179,6 +181,8 @@ public class ChangeJson {
 
   public List<List<ChangeInfo>> formatList2(List<List<ChangeData>> in)
       throws OrmException {
+    accountLoader =
+        accountLoaderFactory.create(options.contains(DETAILED_ACCOUNTS));
     List<List<ChangeInfo>> res = Lists.newArrayListWithCapacity(in.size());
     for (List<ChangeData> changes : in) {
       ChangeData.ensureChangeLoaded(db, changes);
@@ -186,12 +190,7 @@ public class ChangeJson {
       ChangeData.ensureCurrentApprovalsLoaded(db, changes);
       res.add(toChangeInfo(changes));
     }
-    if (!accounts.isEmpty()) {
-      for (Account account : db.get().accounts().get(accounts.keySet())) {
-        AccountAttribute a = accounts.get(account.getId());
-        a.name = Strings.emptyToNull(account.getFullName());
-      }
-    }
+    accountLoader.fill();
     return res;
   }
 
@@ -213,7 +212,7 @@ public class ChangeJson {
     out.changeId = in.getKey().get();
     out.subject = in.getSubject();
     out.status = in.getStatus();
-    out.owner = asAccountAttribute(in.getOwner());
+    out.owner = accountLoader.get(in.getOwner());
     out.created = in.getCreatedOn();
     out.updated = in.getLastUpdatedOn();
     out._number = in.getId().get();
@@ -236,18 +235,6 @@ public class ChangeJson {
     return out;
   }
 
-  private AccountAttribute asAccountAttribute(Account.Id user) {
-    if (user == null) {
-      return null;
-    }
-    AccountAttribute a = accounts.get(user);
-    if (a == null) {
-      a = new AccountAttribute();
-      accounts.put(user, a);
-    }
-    return a;
-  }
-
   private ChangeControl control(ChangeData cd) throws OrmException {
     ChangeControl ctrl = cd.changeControl();
     if (ctrl != null && ctrl.getCurrentUser() == user) {
@@ -295,10 +282,10 @@ public class ChangeJson {
           n._status = r.status;
           switch (r.status) {
             case OK:
-              n.approved = asAccountAttribute(r.appliedBy);
+              n.approved = accountLoader.get(r.appliedBy);
               break;
             case REJECT:
-              n.rejected = asAccountAttribute(r.appliedBy);
+              n.rejected = accountLoader.get(r.appliedBy);
               break;
             default:
               break;
@@ -336,10 +323,10 @@ public class ChangeJson {
         if (val != 0 && min < val && val < max
             && psa.getCategoryId().equals(type.getCategory().getId())) {
           if (0 < val) {
-            e.getValue().recommended = asAccountAttribute(psa.getAccountId());
+            e.getValue().recommended = accountLoader.get(psa.getAccountId());
             e.getValue().value = val != 1 ? val : null;
           } else {
-            e.getValue().disliked = asAccountAttribute(psa.getAccountId());
+            e.getValue().disliked = accountLoader.get(psa.getAccountId());
             e.getValue().value = val != -1 ? val : null;
           }
         }
@@ -542,7 +529,7 @@ public class ChangeJson {
     String _sortkey;
     int _number;
 
-    AccountAttribute owner;
+    AccountInfo owner;
     Map<String, LabelInfo> labels;
     String current_revision;
     Map<String, RevisionInfo> revisions;
@@ -601,11 +588,11 @@ public class ChangeJson {
 
   static class LabelInfo {
     transient SubmitRecord.Label.Status _status;
-    AccountAttribute approved;
-    AccountAttribute rejected;
+    AccountInfo approved;
+    AccountInfo rejected;
 
-    AccountAttribute recommended;
-    AccountAttribute disliked;
+    AccountInfo recommended;
+    AccountInfo disliked;
     Short value;
     Boolean optional;
   }

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

@@ -70,6 +70,7 @@ public class Module extends RestApiModule {
     install(new FactoryModule() {
       @Override
       protected void configure() {
+        factory(AccountInfo.Loader.Factory.class);
         factory(EmailReviewComments.Factory.class);
       }
     });