AccountConfig: Remove unneeded validation of preferred email

AccountConfig was validating the preferred email and generated a
validation error if it was invalid, however the validation errors were
always ignored and never accessed.

Instead the validation of the preferred email is done in
AccountValidator. Here the validation only results into an error if the
preferred email was changed to an invalid email, but not if the email
already was invalid and stayed unchanged. This is something that
AccountConfig can't do since it doesn't know about the previous account
state.

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

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

@@ -18,20 +18,14 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.git.MetaDataUpdate;
-import com.google.gerrit.server.git.ValidationError;
 import com.google.gerrit.server.git.VersionedMetaData;
-import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 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;
@@ -67,7 +61,7 @@ import org.eclipse.jgit.revwalk.RevSort;
  * account. The first commit may be an empty commit (if no properties were set and 'account.config'
  * doesn't exist).
  */
-public class AccountConfig extends VersionedMetaData implements ValidationError.Sink {
+public class AccountConfig extends VersionedMetaData {
   public static final String ACCOUNT_CONFIG = "account.config";
   public static final String ACCOUNT = "account";
   public static final String KEY_ACTIVE = "active";
@@ -75,17 +69,14 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   public static final String KEY_PREFERRED_EMAIL = "preferredEmail";
   public static final String KEY_STATUS = "status";
 
-  @Nullable private final OutgoingEmailValidator emailValidator;
   private final Account.Id accountId;
   private final String ref;
 
   private Optional<Account> loadedAccount;
   private Optional<InternalAccountUpdate> accountUpdate = Optional.empty();
   private Timestamp registeredOn;
-  private List<ValidationError> validationErrors;
 
-  public AccountConfig(@Nullable OutgoingEmailValidator emailValidator, Account.Id accountId) {
-    this.emailValidator = emailValidator;
+  public AccountConfig(Account.Id accountId) {
     this.accountId = checkNotNull(accountId);
     this.ref = RefNames.refsUsers(accountId);
   }
@@ -182,11 +173,6 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
 
     String preferredEmail = get(cfg, KEY_PREFERRED_EMAIL);
     account.setPreferredEmail(preferredEmail);
-    if (emailValidator != null && !emailValidator.isValid(preferredEmail)) {
-      error(
-          new ValidationError(
-              ACCOUNT_CONFIG, String.format("Invalid preferred email: %s", preferredEmail)));
-    }
 
     account.setStatus(get(cfg, KEY_STATUS));
     account.setMetaId(metaId);
@@ -296,24 +282,4 @@ public class AccountConfig extends VersionedMetaData implements ValidationError.
   private void checkLoaded() {
     checkState(loadedAccount != null, "Account %s not loaded yet", accountId.get());
   }
-
-  /**
-   * Get the validation errors, if any were discovered during load.
-   *
-   * @return list of errors; empty list if there are no errors.
-   */
-  public List<ValidationError> getValidationErrors() {
-    if (validationErrors != null) {
-      return ImmutableList.copyOf(validationErrors);
-    }
-    return ImmutableList.of();
-  }
-
-  @Override
-  public void error(ValidationError error) {
-    if (validationErrors == null) {
-      validationErrors = new ArrayList<>(4);
-    }
-    validationErrors.add(error);
-  }
 }

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

@@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
@@ -46,16 +45,11 @@ public class Accounts {
 
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
-  private final OutgoingEmailValidator emailValidator;
 
   @Inject
-  Accounts(
-      GitRepositoryManager repoManager,
-      AllUsersName allUsersName,
-      OutgoingEmailValidator emailValidator) {
+  Accounts(GitRepositoryManager repoManager, AllUsersName allUsersName) {
     this.repoManager = repoManager;
     this.allUsersName = allUsersName;
-    this.emailValidator = emailValidator;
   }
 
   @Nullable
@@ -138,7 +132,7 @@ public class Accounts {
 
   private Optional<Account> read(Repository allUsersRepository, Account.Id accountId)
       throws IOException, ConfigInvalidException {
-    AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
+    AccountConfig accountConfig = new AccountConfig(accountId);
     accountConfig.load(allUsersRepository);
     return accountConfig.getLoadedAccount();
   }

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

@@ -33,7 +33,6 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.index.change.ReindexAfterRefUpdate;
-import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.update.RetryHelper.ActionType;
@@ -113,7 +112,6 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
-    private final OutgoingEmailValidator emailValidator;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
     private final RetryHelper retryHelper;
@@ -124,7 +122,6 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
-        OutgoingEmailValidator emailValidator,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
         RetryHelper retryHelper,
@@ -132,7 +129,6 @@ public class AccountsUpdate {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
-      this.emailValidator = emailValidator;
       this.serverIdentProvider = serverIdentProvider;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
       this.retryHelper = retryHelper;
@@ -146,7 +142,6 @@ public class AccountsUpdate {
           gitRefUpdated,
           null,
           allUsersName,
-          emailValidator,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -169,7 +164,6 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
-    private final OutgoingEmailValidator emailValidator;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
     private final RetryHelper retryHelper;
@@ -180,7 +174,6 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
-        OutgoingEmailValidator emailValidator,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
         RetryHelper retryHelper,
@@ -188,7 +181,6 @@ public class AccountsUpdate {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
-      this.emailValidator = emailValidator;
       this.serverIdentProvider = serverIdentProvider;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
       this.retryHelper = retryHelper;
@@ -202,7 +194,6 @@ public class AccountsUpdate {
           gitRefUpdated,
           null,
           allUsersName,
-          emailValidator,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -222,7 +213,6 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
-    private final OutgoingEmailValidator emailValidator;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<IdentifiedUser> identifiedUser;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
@@ -234,7 +224,6 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
-        OutgoingEmailValidator emailValidator,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<IdentifiedUser> identifiedUser,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
@@ -244,7 +233,6 @@ public class AccountsUpdate {
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
       this.serverIdentProvider = serverIdentProvider;
-      this.emailValidator = emailValidator;
       this.identifiedUser = identifiedUser;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
       this.retryHelper = retryHelper;
@@ -260,7 +248,6 @@ public class AccountsUpdate {
           gitRefUpdated,
           user,
           allUsersName,
-          emailValidator,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -277,7 +264,6 @@ public class AccountsUpdate {
   private final GitReferenceUpdated gitRefUpdated;
   @Nullable private final IdentifiedUser currentUser;
   private final AllUsersName allUsersName;
-  private final OutgoingEmailValidator emailValidator;
   private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
   private final RetryHelper retryHelper;
   private final ExternalIdNotesLoader extIdNotesLoader;
@@ -290,7 +276,6 @@ public class AccountsUpdate {
       GitReferenceUpdated gitRefUpdated,
       @Nullable IdentifiedUser currentUser,
       AllUsersName allUsersName,
-      OutgoingEmailValidator emailValidator,
       Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
       RetryHelper retryHelper,
       ExternalIdNotesLoader extIdNotesLoader,
@@ -301,7 +286,6 @@ public class AccountsUpdate {
         gitRefUpdated,
         currentUser,
         allUsersName,
-        emailValidator,
         metaDataUpdateInternalFactory,
         retryHelper,
         extIdNotesLoader,
@@ -316,7 +300,6 @@ public class AccountsUpdate {
       GitReferenceUpdated gitRefUpdated,
       @Nullable IdentifiedUser currentUser,
       AllUsersName allUsersName,
-      OutgoingEmailValidator emailValidator,
       Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
       RetryHelper retryHelper,
       ExternalIdNotesLoader extIdNotesLoader,
@@ -327,7 +310,6 @@ public class AccountsUpdate {
     this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated");
     this.currentUser = currentUser;
     this.allUsersName = checkNotNull(allUsersName, "allUsersName");
-    this.emailValidator = checkNotNull(emailValidator, "emailValidator");
     this.metaDataUpdateInternalFactory =
         checkNotNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory");
     this.retryHelper = checkNotNull(retryHelper, "retryHelper");
@@ -442,7 +424,7 @@ public class AccountsUpdate {
 
   private AccountConfig read(Repository allUsersRepo, Account.Id accountId)
       throws IOException, ConfigInvalidException {
-    AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
+    AccountConfig accountConfig = new AccountConfig(accountId);
     accountConfig.load(allUsersRepo);
 
     afterReadRevision.run();

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

@@ -90,7 +90,7 @@ public class AccountValidator {
   private Optional<Account> loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit)
       throws IOException, ConfigInvalidException {
     rw.reset();
-    AccountConfig accountConfig = new AccountConfig(null, accountId);
+    AccountConfig accountConfig = new AccountConfig(accountId);
     accountConfig.load(rw, commit);
     return accountConfig.getLoadedAccount();
   }

java/com/google/gerrit/server/schema/Schema_154.java

@@ -139,7 +139,7 @@ public class Schema_154 extends SchemaVersion {
     PersonIdent ident = serverIdent.get();
     md.getCommitBuilder().setAuthor(ident);
     md.getCommitBuilder().setCommitter(ident);
-    AccountConfig accountConfig = new AccountConfig(null, account.getId());
+    AccountConfig accountConfig = new AccountConfig(account.getId());
     accountConfig.load(allUsersRepo);
     accountConfig.setAccount(account);
     accountConfig.commit(md);

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -104,7 +104,6 @@ import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.index.account.AccountIndexer;
 import com.google.gerrit.server.index.account.StalenessChecker;
 import com.google.gerrit.server.mail.Address;
-import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
 import com.google.gerrit.server.project.RefPattern;
 import com.google.gerrit.server.query.account.InternalAccountQuery;
@@ -191,8 +190,6 @@ public class AccountIT extends AbstractDaemonTest {
 
   @Inject private AccountIndexer accountIndexer;
 
-  @Inject private OutgoingEmailValidator emailValidator;
-
   @Inject private GitReferenceUpdated gitReferenceUpdated;
 
   @Inject private RetryHelper.Metrics retryMetrics;
@@ -2013,7 +2010,6 @@ public class AccountIT extends AbstractDaemonTest {
             gitReferenceUpdated,
             null,
             allUsers,
-            emailValidator,
             metaDataUpdateInternalFactory,
             new RetryHelper(
                 cfg,
@@ -2062,7 +2058,6 @@ public class AccountIT extends AbstractDaemonTest {
             gitReferenceUpdated,
             null,
             allUsers,
-            emailValidator,
             metaDataUpdateInternalFactory,
             new RetryHelper(
                 cfg,

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

@@ -416,7 +416,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
       PersonIdent ident = serverIdent.get();
       md.getCommitBuilder().setAuthor(ident);
       md.getCommitBuilder().setCommitter(ident);
-      AccountConfig accountConfig = new AccountConfig(null, accountId);
+      AccountConfig accountConfig = new AccountConfig(accountId);
       accountConfig.load(repo);
       accountConfig.setAccountUpdate(InternalAccountUpdate.builder().setFullName(newName).build());
       accountConfig.commit(md);