EventFactory: Don't require change notes, but only change

EventFactory only needed the change notes to load the patch set. But
the patch set was only used to retrieve the author from it. Instead of
loading the patch set, get the author directly from the patch set
commit that is already available.

As result of this ChangeHookRunner no longer needs to load the change
notes.

Change-Id: I6181788a2a76ac1d3359228757cc98818015f15f
Signed-off-by: Edwin Kempin <ekempin@google.com>

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -58,7 +58,6 @@ import com.google.gerrit.server.events.ReviewerAddedEvent;
 import com.google.gerrit.server.events.TopicChangedEvent;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.WorkQueue;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.project.ProjectState;
@@ -228,8 +227,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     /** Repository Manager. */
     private final GitRepositoryManager repoManager;
 
-    private final ChangeNotes.Factory notesFactory;
-
     /** Queue of hooks that need to run. */
     private final WorkQueue.Executor hookQueue;
 
@@ -259,7 +256,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     @Inject
     public ChangeHookRunner(WorkQueue queue,
       GitRepositoryManager repoManager,
-      ChangeNotes.Factory notesFactory,
       @GerritServerConfig Config config,
       @AnonymousCowardName String anonymousCowardName,
       SitePaths sitePath,
@@ -269,7 +265,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
       DynamicSet<EventListener> unrestrictedListeners) {
         this.anonymousCowardName = anonymousCowardName;
         this.repoManager = repoManager;
-        this.notesFactory = notesFactory;
         this.hookQueue = queue.createQueue(1, "hook");
         this.projectCache = projectCache;
         this.accountCache = accountCache;
@@ -309,11 +304,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
               .build());
     }
 
-    private ChangeNotes newNotes(ReviewDb db, Change change)
-        throws OrmException {
-      return notesFactory.create(db, change.getProject(), change.getId());
-    }
-
     private static Optional<Path> hook(Config config, Path path, String name) {
       String setting = name.replace("-", "") + "hook";
       String value = config.getString("hooks", null, setting);
@@ -404,14 +394,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     @Override
     public void doPatchsetCreatedHook(Change change,
         PatchSet patchSet, ReviewDb db) throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       PatchSetCreatedEvent event = new PatchSetCreatedEvent(change);
       Supplier<AccountState> uploader =
           getAccountSupplier(patchSet.getUploader());
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.uploader = accountAttributeSupplier(uploader);
 
       fireEvent(change, event, db);
@@ -442,14 +431,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     @Override
     public void doDraftPublishedHook(Change change, PatchSet patchSet,
           ReviewDb db) throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       DraftPublishedEvent event = new DraftPublishedEvent(change);
       Supplier<AccountState> uploader =
           getAccountSupplier(patchSet.getUploader());
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.uploader = accountAttributeSupplier(uploader);
 
       fireEvent(change, event, db);
@@ -479,13 +467,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     public void doCommentAddedHook(final Change change, Account account,
           PatchSet patchSet, String comment, final Map<String, Short> approvals,
           ReviewDb db) throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       CommentAddedEvent event = new CommentAddedEvent(change);
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
       event.author =  accountAttributeSupplier(account);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.comment = comment;
       event.approvals = Suppliers.memoize(
           new Supplier<ApprovalAttribute[]>() {
@@ -541,13 +528,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     public void doChangeMergedHook(Change change, Account account,
         PatchSet patchSet, ReviewDb db, String mergeResultRev)
         throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       ChangeMergedEvent event = new ChangeMergedEvent(change);
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
       event.submitter = accountAttributeSupplier(account);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.newRev = mergeResultRev;
 
       fireEvent(change, event, db);
@@ -577,13 +563,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     public void doMergeFailedHook(Change change, Account account,
           PatchSet patchSet, String reason,
           ReviewDb db) throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       MergeFailedEvent event = new MergeFailedEvent(change);
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
       event.submitter = accountAttributeSupplier(account);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
       fireEvent(change, event, db);
@@ -613,13 +598,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     public void doChangeAbandonedHook(Change change, Account account,
           PatchSet patchSet, String reason, ReviewDb db)
           throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       ChangeAbandonedEvent event = new ChangeAbandonedEvent(change);
       AccountState owner = accountCache.get(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
       event.abandoner = accountAttributeSupplier(account);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
       fireEvent(change, event, db);
@@ -649,13 +633,12 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     public void doChangeRestoredHook(Change change, Account account,
           PatchSet patchSet, String reason, ReviewDb db)
           throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       ChangeRestoredEvent event = new ChangeRestoredEvent(change);
       AccountState owner = accountCache.get(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
       event.restorer = accountAttributeSupplier(account);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reason = reason;
 
       fireEvent(change, event, db);
@@ -726,12 +709,11 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     @Override
     public void doReviewerAddedHook(Change change, Account account,
         PatchSet patchSet, ReviewDb db) throws OrmException {
-      ChangeNotes notes = newNotes(db, change);
       ReviewerAddedEvent event = new ReviewerAddedEvent(change);
       Supplier<AccountState> owner = getAccountSupplier(change.getOwner());
 
       event.change = changeAttributeSupplier(change);
-      event.patchSet = patchSetAttributeSupplier(notes, patchSet);
+      event.patchSet = patchSetAttributeSupplier(change, patchSet);
       event.reviewer = accountAttributeSupplier(account);
 
       fireEvent(change, event, db);
@@ -898,16 +880,16 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
     }
 
     private Supplier<PatchSetAttribute> patchSetAttributeSupplier(
-        final ChangeNotes notes, final PatchSet patchSet) {
+        final Change change, final PatchSet patchSet) {
       return Suppliers.memoize(
           new Supplier<PatchSetAttribute>() {
             @Override
             public PatchSetAttribute get() {
               try (Repository repo =
-                    repoManager.openRepository(notes.getProjectName());
+                    repoManager.openRepository(change.getProject());
                   RevWalk revWalk = new RevWalk(repo)) {
                 return eventFactory.asPatchSetAttribute(
-                    revWalk, notes, patchSet);
+                    revWalk, change, patchSet);
               } catch (IOException e) {
                 throw Throwables.propagate(e);
               }

gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java

@@ -36,6 +36,7 @@ import com.google.gerrit.reviewdb.client.UserIdentity;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.account.AccountByEmailCache;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.change.ChangeKindCache;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -56,8 +57,6 @@ import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListEntry;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gwtorm.server.OrmException;
@@ -74,11 +73,13 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 @Singleton
 public class EventFactory {
@@ -87,7 +88,7 @@ public class EventFactory {
   private final AccountCache accountCache;
   private final Provider<String> urlProvider;
   private final PatchListCache patchListCache;
-  private final PatchSetInfoFactory psInfoFactory;
+  private final AccountByEmailCache byEmailCache;
   private final PersonIdent myIdent;
   private final ChangeData.Factory changeDataFactory;
   private final ApprovalsUtil approvalsUtil;
@@ -98,7 +99,7 @@ public class EventFactory {
   @Inject
   EventFactory(AccountCache accountCache,
       @CanonicalWebUrl @Nullable Provider<String> urlProvider,
-      PatchSetInfoFactory psif,
+      AccountByEmailCache byEmailCache,
       PatchListCache patchListCache,
       @GerritPersonIdent PersonIdent myIdent,
       ChangeData.Factory changeDataFactory,
@@ -109,7 +110,7 @@ public class EventFactory {
     this.accountCache = accountCache;
     this.urlProvider = urlProvider;
     this.patchListCache = patchListCache;
-    this.psInfoFactory = psif;
+    this.byEmailCache = byEmailCache;
     this.myIdent = myIdent;
     this.changeDataFactory = changeDataFactory;
     this.approvalsUtil = approvalsUtil;
@@ -382,18 +383,17 @@ public class EventFactory {
   public void addPatchSets(ReviewDb db, RevWalk revWalk, ChangeAttribute ca,
       Collection<PatchSet> ps,
       Map<PatchSet.Id, Collection<PatchSetApproval>> approvals,
-      boolean includeFiles, ChangeNotes notes, LabelTypes labelTypes) {
+      boolean includeFiles, Change change, LabelTypes labelTypes) {
     if (!ps.isEmpty()) {
-      checkNotNull(notes, "notes may not be null");
       ca.patchSets = new ArrayList<>(ps.size());
       for (PatchSet p : ps) {
-        PatchSetAttribute psa = asPatchSetAttribute(db, revWalk, notes, p);
+        PatchSetAttribute psa = asPatchSetAttribute(db, revWalk, change, p);
         if (approvals != null) {
           addApprovals(psa, p.getId(), approvals, labelTypes);
         }
         ca.patchSets.add(psa);
         if (includeFiles) {
-          addPatchSetFileNames(psa, notes.getChange(), p);
+          addPatchSetFileNames(psa, change, p);
         }
       }
     }
@@ -453,9 +453,9 @@ public class EventFactory {
    * @return object suitable for serialization to JSON
    */
   public PatchSetAttribute asPatchSetAttribute(RevWalk revWalk,
-      ChangeNotes notes, PatchSet patchSet) {
+      Change change, PatchSet patchSet) {
     try (ReviewDb db = schema.open()) {
-      return asPatchSetAttribute(db, revWalk, notes, patchSet);
+      return asPatchSetAttribute(db, revWalk, change, patchSet);
     } catch (OrmException e) {
       log.error("Cannot open database connection", e);
       return new PatchSetAttribute();
@@ -471,7 +471,7 @@ public class EventFactory {
    * @return object suitable for serialization to JSON
    */
   public PatchSetAttribute asPatchSetAttribute(ReviewDb db, RevWalk revWalk,
-      ChangeNotes notes, PatchSet patchSet) {
+      Change change, PatchSet patchSet) {
     PatchSetAttribute p = new PatchSetAttribute();
     p.revision = patchSet.getRevision().get();
     p.number = Integer.toString(patchSet.getPatchSetId());
@@ -487,7 +487,7 @@ public class EventFactory {
         p.parents.add(parent.name());
       }
 
-      UserIdentity author = psInfoFactory.get(db, notes, pId).getAuthor();
+      UserIdentity author = toUserIdentity(c.getAuthorIdent());
       if (author.getAccount() == null) {
         p.author = new AccountAttribute();
         p.author.email = author.getEmail();
@@ -497,7 +497,6 @@ public class EventFactory {
         p.author = asAccountAttribute(author.getAccount());
       }
 
-      Change change = notes.getChange();
       List<Patch> list =
           patchListCache.get(change, patchSet).toPatchList(pId);
       for (Patch pe : list) {
@@ -509,14 +508,32 @@ public class EventFactory {
       p.kind = changeKindCache.getChangeKind(db, change, patchSet);
     } catch (IOException e) {
       log.error("Cannot load patch set data for " + patchSet.getId(), e);
-    } catch (PatchSetInfoNotAvailableException e) {
-      log.error(String.format("Cannot get authorEmail for %s.", pId), e);
     } catch (PatchListNotAvailableException e) {
       log.error(String.format("Cannot get size information for %s.", pId), e);
     }
     return p;
   }
 
+  // TODO: The same method exists in PatchSetInfoFactory, find a common place
+  // for it
+  private UserIdentity toUserIdentity(PersonIdent who) {
+    UserIdentity u = new UserIdentity();
+    u.setName(who.getName());
+    u.setEmail(who.getEmailAddress());
+    u.setDate(new Timestamp(who.getWhen().getTime()));
+    u.setTimeZone(who.getTimeZoneOffset());
+
+    // If only one account has access to this email address, select it
+    // as the identity of the user.
+    //
+    Set<Account.Id> a = byEmailCache.get(u.getEmail());
+    if (a.size() == 1) {
+      u.setAccount(a.iterator().next());
+    }
+
+    return u;
+  }
+
   public void addApprovals(PatchSetAttribute p, PatchSet.Id id,
       Map<PatchSet.Id, Collection<PatchSetApproval>> all,
       LabelTypes labelTypes) {

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java

@@ -98,6 +98,7 @@ public class PatchSetInfoFactory {
     }
   }
 
+  // TODO: The same method exists in EventFactory, find a common place for it
   private UserIdentity toUserIdentity(final PersonIdent who) {
     final UserIdentity u = new UserIdentity();
     u.setName(who.getName());

gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java

@@ -278,14 +278,14 @@ public class OutputStreamQuery {
     if (includePatchSets) {
       eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
           includeApprovals ? d.approvals().asMap() : null,
-          includeFiles, d.notes(), labelTypes);
+          includeFiles, d.change(), labelTypes);
     }
 
     if (includeCurrentPatchSet) {
       PatchSet current = d.currentPatchSet();
       if (current != null) {
         c.currentPatchSet =
-            eventFactory.asPatchSetAttribute(db.get(), rw, d.notes(), current);
+            eventFactory.asPatchSetAttribute(db.get(), rw, d.change(), current);
         eventFactory.addApprovals(c.currentPatchSet,
             d.currentApprovals(), labelTypes);
 
@@ -305,7 +305,7 @@ public class OutputStreamQuery {
       if (includePatchSets) {
         eventFactory.addPatchSets(db.get(), rw, c, d.patchSets(),
             includeApprovals ? d.approvals().asMap() : null,
-            includeFiles, d.notes(), labelTypes);
+            includeFiles, d.change(), labelTypes);
         for (PatchSetAttribute attribute : c.patchSets) {
           eventFactory.addPatchSetComments(
               attribute, d.publishedComments());