Propagate failure if reading external IDs from NoteDb fails

When reading external IDs from NoteDb was enabled
ExternalIds#byAccount(...) and ExternalIds#byEmail(...) returned an
empty set if reading from NoteDb failed.

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

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java

@@ -25,6 +25,7 @@ import com.github.rholder.retry.Retryer;
 import com.github.rholder.retry.RetryerBuilder;
 import com.github.rholder.retry.StopStrategies;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
 import com.google.gerrit.acceptance.Sandboxed;
@@ -35,6 +36,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.account.externalids.DisabledExternalIdCache;
 import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdReader;
 import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
 import com.google.gerrit.server.config.AllUsersName;
@@ -54,15 +56,18 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.notes.NoteMap;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.Test;
 
 @Sandboxed
 public class ExternalIdIT extends AbstractDaemonTest {
   @Inject private AllUsersName allUsers;
-
   @Inject private ExternalIdsUpdate.Server extIdsUpdate;
-
   @Inject private ExternalIds externalIds;
+  @Inject private ExternalIdReader externalIdReader;
 
   @Test
   public void getExternalIDs() throws Exception {
@@ -267,4 +272,40 @@ public class ExternalIdIT extends AbstractDaemonTest {
     ExternalId extId = externalIds.get(db, extIdKey);
     assertThat(extId.accountId()).isEqualTo(accountId);
   }
+
+  @Test
+  @GerritConfig(name = "user.readExternalIdsFromGit", value = "true")
+  public void byAccountFailIfReadingExternalIdsFails() throws Exception {
+    externalIdReader.setFailOnLoad(true);
+
+    // update external ID branch so that external IDs need to be reloaded
+    insertExtIdBehindGerritsBack(ExternalId.create("foo", "bar", admin.id));
+
+    exception.expect(IOException.class);
+    externalIds.byAccount(db, admin.id);
+  }
+
+  @Test
+  @GerritConfig(name = "user.readExternalIdsFromGit", value = "true")
+  public void byEmailFailIfReadingExternalIdsFails() throws Exception {
+    externalIdReader.setFailOnLoad(true);
+
+    // update external ID branch so that external IDs need to be reloaded
+    insertExtIdBehindGerritsBack(ExternalId.create("foo", "bar", admin.id));
+
+    exception.expect(IOException.class);
+    externalIds.byEmail(db, admin.email);
+  }
+
+  private void insertExtIdBehindGerritsBack(ExternalId extId) throws Exception {
+    try (Repository repo = repoManager.openRepository(allUsers);
+        RevWalk rw = new RevWalk(repo);
+        ObjectInserter ins = repo.newObjectInserter()) {
+      ObjectId rev = ExternalIdReader.readRevision(repo);
+      NoteMap noteMap = ExternalIdReader.readNoteMap(rw, rev);
+      ExternalIdsUpdate.insert(rw, ins, noteMap, extId);
+      ExternalIdsUpdate.commit(
+          repo, rw, ins, rev, noteMap, "insert new ID", serverIdent.get(), serverIdent.get());
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java

@@ -28,7 +28,6 @@ import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import com.google.inject.name.Named;
 import java.io.IOException;
-import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.locks.Lock;
@@ -222,8 +221,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
     try {
       return extIdsByAccount.get(externalIdReader.readRevision()).get(accountId);
     } catch (ExecutionException e) {
-      log.warn("Cannot list external ids by account", e);
+      throw new IOException("Cannot list external ids by account", e);
-      return Collections.emptySet();
     }
   }
 
@@ -237,8 +235,7 @@ class ExternalIdCacheImpl implements ExternalIdCache {
           .filter(e -> email.equals(e.email()))
           .collect(toSet());
     } catch (ExecutionException e) {
-      log.warn("Cannot list external ids by email", e);
+      throw new IOException("Cannot list external ids by email", e);
-      return Collections.emptySet();
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdReader.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.account.externalids;
 
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -76,6 +77,7 @@ public class ExternalIdReader {
   private final boolean readFromGit;
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
+  private boolean failOnLoad = false;
 
   @Inject
   ExternalIdReader(
@@ -85,6 +87,11 @@ public class ExternalIdReader {
     this.allUsersName = allUsersName;
   }
 
+  @VisibleForTesting
+  public void setFailOnLoad(boolean failOnLoad) {
+    this.failOnLoad = failOnLoad;
+  }
+
   boolean readFromGit() {
     return readFromGit;
   }
@@ -97,6 +104,8 @@ public class ExternalIdReader {
 
   /** Reads and returns all external IDs. */
   Set<ExternalId> all(ReviewDb db) throws IOException, OrmException {
+    checkReadEnabled();
+
     if (readFromGit) {
       try (Repository repo = repoManager.openRepository(allUsersName)) {
         return all(repo, readRevision(repo));
@@ -111,6 +120,8 @@ public class ExternalIdReader {
    * branch.
    */
   Set<ExternalId> all(ObjectId rev) throws IOException {
+    checkReadEnabled();
+
     try (Repository repo = repoManager.openRepository(allUsersName)) {
       return all(repo, rev);
     }
@@ -142,6 +153,8 @@ public class ExternalIdReader {
   @Nullable
   ExternalId get(ReviewDb db, ExternalId.Key key)
       throws IOException, ConfigInvalidException, OrmException {
+    checkReadEnabled();
+
     if (readFromGit) {
       try (Repository repo = repoManager.openRepository(allUsersName);
           RevWalk rw = new RevWalk(repo)) {
@@ -159,6 +172,8 @@ public class ExternalIdReader {
   /** Reads and returns the specified external ID from the given revision. */
   @Nullable
   ExternalId get(ExternalId.Key key, ObjectId rev) throws IOException, ConfigInvalidException {
+    checkReadEnabled();
+
     if (rev.equals(ObjectId.zeroId())) {
       return null;
     }
@@ -181,4 +196,10 @@ public class ExternalIdReader {
         rw.getObjectReader().open(noteMap.get(noteId), OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
     return ExternalId.parse(noteId.name(), raw);
   }
+
+  private void checkReadEnabled() throws IOException {
+    if (failOnLoad) {
+      throw new IOException("Reading from external IDs is disabled");
+    }
+  }
 }