AccountCache: Rename get to getEvenIfMissing

AccountCache#get returns an empty AccountState to represent a missing
account, but most callers are not aware of this. Make this behavior
obvious by having an explicit name.

Most callers of AccountCache should use maybeGet and handle the missing
account case explicitly. A follow-up change can now rename maybeGet to
get to make it more obvious that this is the method that should be used
by default.

Within this series quite some callers of AccountCache#get have been
migrated to AccountCache#maybeGet. The remaining usages of
AccountCache#get (now AccountCache#getEvenIfMissing) cannot easily be
migrated to AccountCache#maybeGet, at least not without changing
behavior. Ideally we still want to migrate all callers to
AccountCache#maybeGet and then get rid of AccountCache#get (now
AccountCache#getEvenIfMissing), but this work is left for future
changes.

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

java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java

@@ -123,10 +123,11 @@ public class ElasticAccountIndex extends AbstractElasticIndex<Account.Id, Accoun
     }
 
     Account.Id id = new Account.Id(source.getAsJsonObject().get(ID.getName()).getAsInt());
-    // Use the AccountCache rather than depending on any stored fields in the
-    // document (of which there shouldn't be any). The most expensive part to
-    // compute anyway is the effective group IDs, and we don't have a good way
-    // to reindex when those change.
-    return accountCache.get().get(id);
+    // Use the AccountCache rather than depending on any stored fields in the document (of which
+    // there shouldn't be any). The most expensive part to compute anyway is the effective group
+    // IDs, and we don't have a good way to reindex when those change.
+    // If the account doesn't exist return an empty AccountState to represent the missing account
+    // to account the fact that the account exists in the index.
+    return accountCache.get().getEvenIfMissing(id);
   }
 }

java/com/google/gerrit/lucene/LuceneAccountIndex.java

@@ -122,10 +122,11 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS
   @Override
   protected AccountState fromDocument(Document doc) {
     Account.Id id = new Account.Id(doc.getField(ID.getName()).numericValue().intValue());
-    // Use the AccountCache rather than depending on any stored fields in the
-    // document (of which there shouldn't be any). The most expensive part to
-    // compute anyway is the effective group IDs, and we don't have a good way
-    // to reindex when those change.
-    return accountCache.get().get(id);
+    // Use the AccountCache rather than depending on any stored fields in the document (of which
+    // there shouldn't be any). The most expensive part to compute anyway is the effective group
+    // IDs, and we don't have a good way to reindex when those change.
+    // If the account doesn't exist return an empty AccountState to represent the missing account
+    // to account the fact that the account exists in the index.
+    return accountCache.get().getEvenIfMissing(id);
   }
 }

java/com/google/gerrit/server/IdentifiedUser.java

@@ -277,9 +277,28 @@ public class IdentifiedUser extends CurrentUser {
     return true;
   }
 
+  /**
+   * Returns the account state of the identified user.
+   *
+   * @return the account state of the identified user, an empty account state if the account is
+   *     missing
+   */
   public AccountState state() {
     if (state == null) {
-      state = accountCache.get(getAccountId());
+      // TODO(ekempin):
+      // Ideally we would only create IdentifiedUser instances for existing accounts. To ensure
+      // this we could load the account state eagerly on the creation of IdentifiedUser and fail is
+      // the account is missing. In most cases, e.g. when creating an IdentifiedUser for a request
+      // context, we really want to fail early if the account is missing. However there are some
+      // usages where an IdentifiedUser may be instantiated for a missing account. We may go
+      // through all of them and ensure that they never try to create an IdentifiedUser for a
+      // missing account or make this explicit by adding a createEvenIfMissing method to
+      // IdentifiedUser.GenericFactory. However since this is a lot of effort we stick with calling
+      // AccountCache#getEvenIfMissing(Account.Id) for now.
+      // Alternatively we could be could also return an Optional<AccountState> from the state()
+      // method and let callers handle the missing account case explicitly. But this would be a lot
+      // of work too.
+      state = accountCache.getEvenIfMissing(getAccountId());
     }
     return state;
   }
@@ -310,6 +329,11 @@ public class IdentifiedUser extends CurrentUser {
             () -> firstNonNull(getAccount().getPreferredEmail(), "a/" + getAccountId().get()));
   }
 
+  /**
+   * Returns the account of the identified user.
+   *
+   * @return the account of the identified user, an empty account if the account is missing
+   */
   public Account getAccount() {
     return state().getAccount();
   }

java/com/google/gerrit/server/account/AccountCache.java

@@ -21,26 +21,31 @@ import java.util.Optional;
 
 /** Caches important (but small) account state to avoid database hits. */
 public interface AccountCache {
+  /**
+   * Returns an {@code AccountState} instance for the given account ID. If not cached yet the
+   * account is loaded. Returns {@link Optional#empty()} if the account is missing.
+   *
+   * @param accountId ID of the account that should be retrieved
+   * @return {@code AccountState} instance for the given account ID, if no account with this ID
+   *     exists {@link Optional#empty()} is returned
+   */
+  Optional<AccountState> maybeGet(Account.Id accountId);
+
   /**
    * Returns an {@code AccountState} instance for the given account ID. If not cached yet the
    * account is loaded. Returns an empty {@code AccountState} instance to represent a missing
    * account.
    *
+   * <p>This method should only be used in exceptional cases where it is required to get an account
+   * state even if the account is missing. Callers should leave a comment with the method invocation
+   * explaining why this method is used. Most callers of {@link AccountCache} should use {@link
+   * #maybeGet(Account.Id)} instead and handle the missing account case explicitly.
+   *
    * @param accountId ID of the account that should be retrieved
    * @return {@code AccountState} instance for the given account ID, if no account with this ID
    *     exists an empty {@code AccountState} instance is returned to represent the missing account
    */
-  AccountState get(Account.Id accountId);
-
-  /**
-   * Returns an {@code AccountState} instance for the given account ID. If not cached yet the
-   * account is loaded. Returns {@link Optional#empty()} if the account is missing.
-   *
-   * @param accountId ID of the account that should be retrieved
-   * @return {@code AccountState} instance for the given account ID, if no account with this ID
-   *     exists {@link Optional#empty()}is returned
-   */
-  Optional<AccountState> maybeGet(Account.Id accountId);
+  AccountState getEvenIfMissing(Account.Id accountId);
 
   /**
    * Returns an {@code AccountState} instance for the given username.

java/com/google/gerrit/server/account/AccountCacheImpl.java

@@ -77,7 +77,7 @@ public class AccountCacheImpl implements AccountCache {
   }
 
   @Override
-  public AccountState get(Account.Id accountId) {
+  public AccountState getEvenIfMissing(Account.Id accountId) {
     try {
       return byId.get(accountId).orElse(missing(accountId));
     } catch (ExecutionException e) {

java/com/google/gerrit/server/account/AccountState.java

@@ -51,7 +51,7 @@ import org.slf4j.LoggerFactory;
  * and properties from the account config file. AccountState maps one-to-one to Account.
  *
  * <p>Most callers should not construct AccountStates directly but rather lookup accounts via the
- * account cache (see {@link AccountCache#get(Account.Id)}).
+ * account cache (see {@link AccountCache#maybeGet(Account.Id)}).
  */
 public class AccountState {
   private static final Logger logger = LoggerFactory.getLogger(AccountState.class);

java/com/google/gerrit/testing/FakeAccountCache.java

@@ -34,7 +34,7 @@ public class FakeAccountCache implements AccountCache {
   }
 
   @Override
-  public synchronized AccountState get(Account.Id accountId) {
+  public synchronized AccountState getEvenIfMissing(Account.Id accountId) {
     AccountState state = byId.get(accountId);
     if (state != null) {
       return state;