AccountConfig: Return loaded account as Optional

This makes AccountConfig a little more consistent with GroupConfig.
Also rename getAccount() to getLoadedAccount() for consistency with
GroupConfig.

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

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

@@ -32,6 +32,7 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
@@ -78,8 +79,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   private final Account.Id accountId;
   private final String ref;
 
-  private boolean isLoaded;
+  private Optional<Account> loadedAccount;
-  private Account account;
   private Timestamp registeredOn;
   private List<ValidationError> validationErrors;
 
@@ -97,14 +97,13 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   /**
    * Get the loaded account.
    *
-   * @return the loaded account, {@code null} if load didn't find the account because it doesn't
+   * @return the loaded account, {@link Optional#empty()} if load didn't find the account because it
-   *     exist
+   *     doesn't exist
    * @throws IllegalStateException if the account was not loaded yet
    */
-  @Nullable
+  public Optional<Account> getLoadedAccount() {
-  public Account getAccount() {
     checkLoaded();
-    return account;
+    return loadedAccount;
   }
 
   /**
@@ -117,7 +116,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
    */
   public void setAccount(Account account) {
     checkLoaded();
-    this.account = account;
+    this.loadedAccount = Optional.of(account);
     this.registeredOn = account.getRegisteredOn();
   }
 
@@ -133,8 +132,8 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
       throw new OrmDuplicateKeyException(String.format("account %s already exists", accountId));
     }
     this.registeredOn = TimeUtil.nowTs();
-    this.account = new Account(accountId, registeredOn);
+    this.loadedAccount = Optional.of(new Account(accountId, registeredOn));
-    return account;
+    return loadedAccount.get();
   }
 
   @Override
@@ -147,14 +146,13 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
 
       Config cfg = readConfig(ACCOUNT_CONFIG);
 
-      account = parse(cfg);
+      loadedAccount = Optional.of(parse(cfg, revision.name()));
-      account.setMetaId(revision.name());
+    } else {
+      loadedAccount = Optional.empty();
     }
-
-    isLoaded = true;
   }
 
-  private Account parse(Config cfg) {
+  private Account parse(Config cfg, String metaId) {
     Account account = new Account(accountId, registeredOn);
     account.setActive(cfg.getBoolean(ACCOUNT, null, KEY_ACTIVE, true));
     account.setFullName(get(cfg, KEY_FULL_NAME));
@@ -168,13 +166,14 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
     }
 
     account.setStatus(get(cfg, KEY_STATUS));
+    account.setMetaId(metaId);
     return account;
   }
 
   @Override
   public RevCommit commit(MetaDataUpdate update) throws IOException {
     RevCommit c = super.commit(update);
-    account.setMetaId(c.name());
+    loadedAccount.get().setMetaId(c.name());
     return c;
   }
 
@@ -182,16 +181,20 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException {
     checkLoaded();
 
+    if (!loadedAccount.isPresent()) {
+      return false;
+    }
+
     if (revision != null) {
       commit.setMessage("Update account\n");
-    } else if (account != null) {
+    } else {
       commit.setMessage("Create account\n");
       commit.setAuthor(new PersonIdent(commit.getAuthor(), registeredOn));
       commit.setCommitter(new PersonIdent(commit.getCommitter(), registeredOn));
     }
 
     Config cfg = readConfig(ACCOUNT_CONFIG);
-    writeToConfig(account, cfg);
+    writeToConfig(loadedAccount.get(), cfg);
     saveConfig(ACCOUNT_CONFIG, cfg);
     return true;
   }
@@ -253,7 +256,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   }
 
   private void checkLoaded() {
-    checkState(isLoaded, "Account %s not loaded yet", accountId.get());
+    checkState(loadedAccount != null, "Account %s not loaded yet", accountId.get());
   }
 
   /**

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

@@ -31,6 +31,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -60,7 +61,7 @@ public class Accounts {
   @Nullable
   public Account get(Account.Id accountId) throws IOException, ConfigInvalidException {
     try (Repository repo = repoManager.openRepository(allUsersName)) {
-      return read(repo, accountId);
+      return read(repo, accountId).orElse(null);
     }
   }
 
@@ -69,7 +70,7 @@ public class Accounts {
     List<Account> accounts = new ArrayList<>(accountIds.size());
     try (Repository repo = repoManager.openRepository(allUsersName)) {
       for (Account.Id accountId : accountIds) {
-        accounts.add(read(repo, accountId));
+        read(repo, accountId).ifPresent(accounts::add);
       }
     }
     return accounts;
@@ -86,7 +87,7 @@ public class Accounts {
     try (Repository repo = repoManager.openRepository(allUsersName)) {
       for (Account.Id accountId : accountIds) {
         try {
-          accounts.add(read(repo, accountId));
+          read(repo, accountId).ifPresent(accounts::add);
         } catch (Exception e) {
           log.error(String.format("Ignoring invalid account %s", accountId.get()), e);
         }
@@ -135,12 +136,11 @@ public class Accounts {
     }
   }
 
-  @Nullable
+  private Optional<Account> read(Repository allUsersRepository, Account.Id accountId)
-  private Account read(Repository allUsersRepository, Account.Id accountId)
       throws IOException, ConfigInvalidException {
     AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
     accountConfig.load(allUsersRepository);
-    return accountConfig.getAccount();
+    return accountConfig.getLoadedAccount();
   }
 
   public static Stream<Account.Id> readUserRefs(Repository repo) throws IOException {

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

@@ -35,6 +35,7 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.List;
+import java.util.Optional;
 import java.util.function.Consumer;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
@@ -231,6 +232,7 @@ public class AccountsUpdate {
    * @throws IOException if updating the user branch fails
    * @throws ConfigInvalidException if any of the account fields has an invalid value
    */
+  @Nullable
   public Account update(Account.Id accountId, List<Consumer<Account>> consumers)
       throws IOException, ConfigInvalidException {
     if (consumers.isEmpty()) {
@@ -238,13 +240,13 @@ public class AccountsUpdate {
     }
 
     AccountConfig accountConfig = read(accountId);
-    Account account = accountConfig.getAccount();
+    Optional<Account> account = accountConfig.getLoadedAccount();
-    if (account != null) {
+    if (account.isPresent()) {
-      consumers.stream().forEach(c -> c.accept(account));
+      consumers.stream().forEach(c -> c.accept(account.get()));
       commit(accountConfig);
     }
 
-    return account;
+    return account.orElse(null);
   }
 
   /**

java/com/google/gerrit/server/git/validators/AccountValidator.java

@@ -25,6 +25,7 @@ import com.google.inject.Provider;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -43,7 +44,7 @@ public class AccountValidator {
   public List<String> validate(
       Account.Id accountId, RevWalk rw, @Nullable ObjectId oldId, ObjectId newId)
       throws IOException {
-    Account oldAccount = null;
+    Optional<Account> oldAccount = Optional.empty();
     if (oldId != null && !ObjectId.zeroId().equals(oldId)) {
       try {
         oldAccount = loadAccount(accountId, rw, oldId);
@@ -52,7 +53,7 @@ public class AccountValidator {
       }
     }
 
-    Account newAccount;
+    Optional<Account> newAccount;
     try {
       newAccount = loadAccount(accountId, rw, newId);
     } catch (ConfigInvalidException e) {
@@ -62,35 +63,35 @@ public class AccountValidator {
               newId.name(), AccountConfig.ACCOUNT_CONFIG, accountId.get(), e.getMessage()));
     }
 
-    if (newAccount == null) {
+    if (!newAccount.isPresent()) {
       return ImmutableList.of(String.format("account '%s' does not exist", accountId.get()));
     }
 
     List<String> messages = new ArrayList<>();
-    if (accountId.equals(self.get().getAccountId()) && !newAccount.isActive()) {
+    if (accountId.equals(self.get().getAccountId()) && !newAccount.get().isActive()) {
       messages.add("cannot deactivate own account");
     }
 
-    if (newAccount.getPreferredEmail() != null
+    String newPreferredEmail = newAccount.get().getPreferredEmail();
-        && (oldAccount == null
+    if (newPreferredEmail != null
-            || !newAccount.getPreferredEmail().equals(oldAccount.getPreferredEmail()))) {
+        && (!oldAccount.isPresent()
-      if (!emailValidator.isValid(newAccount.getPreferredEmail())) {
+            || !newPreferredEmail.equals(oldAccount.get().getPreferredEmail()))) {
+      if (!emailValidator.isValid(newPreferredEmail)) {
         messages.add(
             String.format(
                 "invalid preferred email '%s' for account '%s'",
-                newAccount.getPreferredEmail(), accountId.get()));
+                newPreferredEmail, accountId.get()));
       }
     }
 
     return ImmutableList.copyOf(messages);
   }
 
-  @Nullable
+  private Optional<Account> loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit)
-  private Account loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit)
       throws IOException, ConfigInvalidException {
     rw.reset();
     AccountConfig accountConfig = new AccountConfig(null, accountId);
     accountConfig.load(rw, commit);
-    return accountConfig.getAccount();
+    return accountConfig.getLoadedAccount();
   }
 }

javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java

@@ -417,7 +417,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
       md.getCommitBuilder().setCommitter(ident);
       AccountConfig accountConfig = new AccountConfig(null, accountId);
       accountConfig.load(repo);
-      accountConfig.getAccount().setFullName(newName);
+      accountConfig.getLoadedAccount().get().setFullName(newName);
       accountConfig.commit(md);
     }