Merge changes I4f8d4e8a,Ib54dcf81,I8fe3adc0 * changes: Migrate project watches to git (part 1) AccountManager: Use account index to lookup external IDs AccountCacheImpl#ByNameLoader: Use account index to lookup usernames
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/WatchedProjectsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/WatchedProjectsIT.java index 5dfae50..32cfc9b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/WatchedProjectsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/WatchedProjectsIT.java
@@ -119,6 +119,22 @@ } @Test + public void setAndGetEmptyWatch() throws Exception { + String projectName = createProject(NEW_PROJECT_NAME).get(); + + List<ProjectWatchInfo> projectsToWatch = new LinkedList<>(); + + ProjectWatchInfo pwi = new ProjectWatchInfo(); + pwi.project = projectName; + projectsToWatch.add(pwi); + + gApi.accounts().self().setWatchedProjects(projectsToWatch); + List<ProjectWatchInfo> persistedWatchedProjects = + gApi.accounts().self().getWatchedProjects(); + assertThat(persistedWatchedProjects).containsAllIn(projectsToWatch); + } + + @Test public void watchNonExistingProject() throws Exception { String projectName = NEW_PROJECT_NAME + "3";
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ProjectWatchInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ProjectWatchInfo.java index beb869e..556dddc 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ProjectWatchInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ProjectWatchInfo.java
@@ -47,4 +47,30 @@ .hash(project, filter, notifyNewChanges, notifyNewPatchSets, notifyAllComments, notifySubmittedChanges, notifyAbandonedChanges); } + + @Override + public String toString() { + StringBuilder b = new StringBuilder(); + b.append(project); + if (filter != null) { + b.append("%filter=") + .append(filter); + } + b.append("(notifyAbandonedChanges=") + .append(toBoolean(notifyAbandonedChanges)) + .append(", notifyAllComments=") + .append(toBoolean(notifyAllComments)) + .append(", notifyNewChanges=") + .append(toBoolean(notifyNewChanges)) + .append(", notifyNewPatchSets=") + .append(toBoolean(notifyNewPatchSets)) + .append(", notifySubmittedChanges=") + .append(toBoolean(notifySubmittedChanges)) + .append(")"); + return b.toString(); + } + + private boolean toBoolean(Boolean b) { + return b == null ? false : b; + } }
diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java index cd172a7..2deae3f 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java
@@ -294,12 +294,7 @@ msg.append("GPG key ").append(externalId) .append(" associated with multiple accounts: "); Joiner.on(", ").appendTo(msg, - Lists.transform(accountStates, new Function<AccountState, String>() { - @Override - public String apply(AccountState accountState) { - return accountState.getAccount().getId().toString(); - } - })); + Lists.transform(accountStates, AccountState.ACCOUNT_ID_FUNCTION)); log.error(msg.toString()); throw new IllegalStateException(msg.toString()); }
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java index c7f52b0..a6796e7 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java
@@ -22,8 +22,14 @@ public final class AccountProjectWatch { public enum NotifyType { - NEW_CHANGES, NEW_PATCHSETS, ALL_COMMENTS, SUBMITTED_CHANGES, - ABANDONED_CHANGES, ALL + // sort by name, except 'ALL' which should stay last + ABANDONED_CHANGES, + ALL_COMMENTS, + NEW_CHANGES, + NEW_PATCHSETS, + SUBMITTED_CHANGES, + + ALL } public static final String FILTER_ALL = "*";
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java index af0ee4c..fb6bda2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -24,10 +24,14 @@ import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.cache.CacheModule; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.index.account.AccountIndexCollection; import com.google.gerrit.server.index.account.AccountIndexer; +import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; @@ -38,13 +42,16 @@ import com.google.inject.name.Named; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -133,9 +140,9 @@ Account account = new Account(accountId, TimeUtil.nowTs()); account.setActive(false); Collection<AccountExternalId> ids = Collections.emptySet(); - Collection<AccountProjectWatch> projectWatches = Collections.emptySet(); Set<AccountGroup.UUID> anon = ImmutableSet.of(); - return new AccountState(account, anon, ids, projectWatches); + return new AccountState(account, anon, ids, + new HashMap<ProjectWatchKey, Set<NotifyType>>()); } static class ByIdLoader extends CacheLoader<Account.Id, AccountState> { @@ -143,17 +150,24 @@ private final GroupCache groupCache; private final GeneralPreferencesLoader loader; private final LoadingCache<String, Optional<Account.Id>> byName; + private final boolean readFromGit; + private final Provider<WatchConfig.Accessor> watchConfig; @Inject ByIdLoader(SchemaFactory<ReviewDb> sf, GroupCache groupCache, GeneralPreferencesLoader loader, @Named(BYUSER_NAME) LoadingCache<String, - Optional<Account.Id>> byUsername) { + Optional<Account.Id>> byUsername, + @GerritServerConfig Config cfg, + Provider<WatchConfig.Accessor> watchConfig) { this.schema = sf; this.groupCache = groupCache; this.loader = loader; this.byName = byUsername; + this.readFromGit = + cfg.getBoolean("user", null, "readProjectWatchesFromGit", true); + this.watchConfig = watchConfig; } @Override @@ -169,7 +183,7 @@ } private AccountState load(final ReviewDb db, final Account.Id who) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { Account account = db.accounts().get(who); if (account == null) { // Account no longer exists? They are anonymous. @@ -198,9 +212,10 @@ account.setGeneralPreferences(GeneralPreferencesInfo.defaults()); } - Collection<AccountProjectWatch> projectWatches = - Collections.unmodifiableCollection( - db.accountProjectWatches().byAccount(who).toList()); + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + readFromGit + ? watchConfig.get().getProjectWatches(who) + : GetWatchedProjects.readProjectWatchesFromDb(db, who); return new AccountState(account, internalGroups, externalIds, projectWatches); @@ -209,19 +224,33 @@ static class ByNameLoader extends CacheLoader<String, Optional<Account.Id>> { private final SchemaFactory<ReviewDb> schema; + private final AccountIndexCollection accountIndexes; + private final Provider<InternalAccountQuery> accountQueryProvider; @Inject - ByNameLoader(final SchemaFactory<ReviewDb> sf) { + ByNameLoader(SchemaFactory<ReviewDb> sf, + AccountIndexCollection accountIndexes, + Provider<InternalAccountQuery> accountQueryProvider) { this.schema = sf; + this.accountIndexes = accountIndexes; + this.accountQueryProvider = accountQueryProvider; } @Override public Optional<Account.Id> load(String username) throws Exception { - try (ReviewDb db = schema.open()) { - final AccountExternalId.Key key = new AccountExternalId.Key( // + AccountExternalId.Key key = new AccountExternalId.Key( // AccountExternalId.SCHEME_USERNAME, // username); - final AccountExternalId id = db.accountExternalIds().get(key); + if (accountIndexes.getSearchIndex() != null) { + AccountState accountState = + accountQueryProvider.get().oneByExternalId(key.get()); + return accountState != null + ? Optional.of(accountState.getAccount().getId()) + : Optional.<Account.Id>absent(); + } + + try (ReviewDb db = schema.open()) { + AccountExternalId id = db.accountExternalIds().get(key); if (id != null) { return Optional.of(id.getAccountId()); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index ac4d2ba..69295b3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
@@ -27,11 +27,14 @@ import com.google.gerrit.reviewdb.client.AccountGroupMember; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.index.account.AccountIndexCollection; import com.google.gerrit.server.project.ProjectCache; +import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import org.slf4j.Logger; @@ -58,6 +61,8 @@ private final ProjectCache projectCache; private final AtomicBoolean awaitsFirstAccountCheck; private final AuditService auditService; + private final AccountIndexCollection accountIndexes; + private final Provider<InternalAccountQuery> accountQueryProvider; @Inject AccountManager(SchemaFactory<ReviewDb> schema, @@ -67,7 +72,9 @@ IdentifiedUser.GenericFactory userFactory, ChangeUserName.Factory changeUserNameFactory, ProjectCache projectCache, - AuditService auditService) { + AuditService auditService, + AccountIndexCollection accountIndexes, + Provider<InternalAccountQuery> accountQueryProvider) { this.schema = schema; this.byIdCache = byIdCache; this.byEmailCache = byEmailCache; @@ -77,6 +84,8 @@ this.projectCache = projectCache; this.awaitsFirstAccountCheck = new AtomicBoolean(true); this.auditService = auditService; + this.accountIndexes = accountIndexes; + this.accountQueryProvider = accountQueryProvider; } /** @@ -84,6 +93,14 @@ */ public Account.Id lookup(String externalId) throws AccountException { try { + if (accountIndexes.getSearchIndex() != null) { + AccountState accountState = + accountQueryProvider.get().oneByExternalId(externalId); + return accountState != null + ? accountState.getAccount().getId() + : null; + } + try (ReviewDb db = schema.open()) { AccountExternalId ext = db.accountExternalIds().get(new AccountExternalId.Key(externalId)); @@ -132,16 +149,26 @@ private AccountExternalId getAccountExternalId(ReviewDb db, AccountExternalId.Key key) throws OrmException { - String keyValue = key.get(); - String keyScheme = keyValue.substring(0, keyValue.indexOf(':') + 1); + if (accountIndexes.getSearchIndex() != null) { + AccountState accountState = + accountQueryProvider.get().oneByExternalId(key.get()); + if (accountState != null) { + for (AccountExternalId extId : accountState.getExternalIds()) { + if (extId.getKey().equals(key)) { + return extId; + } + } + } + return null; + } // We don't have at the moment an account_by_external_id cache // but by using the accounts cache we get the list of external_ids // without having to query the DB every time - if (keyScheme.equals(AccountExternalId.SCHEME_GERRIT) - || keyScheme.equals(AccountExternalId.SCHEME_USERNAME)) { + if (key.getScheme().equals(AccountExternalId.SCHEME_GERRIT) + || key.getScheme().equals(AccountExternalId.SCHEME_USERNAME)) { AccountState state = byIdCache.getByUsername( - keyValue.substring(keyScheme.length())); + key.get().substring(key.getScheme().length())); if (state != null) { for (AccountExternalId accountExternalId : state.getExternalIds()) { if (accountExternalId.getKey().equals(key)) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java index 145f34b..5c1513e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
@@ -17,31 +17,42 @@ import static com.google.gerrit.reviewdb.client.AccountExternalId.SCHEME_MAILTO; import static com.google.gerrit.reviewdb.client.AccountExternalId.SCHEME_USERNAME; +import com.google.common.base.Function; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.CurrentUser.PropertyKey; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import java.util.Collection; import java.util.HashSet; +import java.util.Map; import java.util.Set; public class AccountState { + public static Function<AccountState, Account.Id> ACCOUNT_ID_FUNCTION = + new Function<AccountState, Account.Id>() { + @Override + public Account.Id apply(AccountState in) { + return in.getAccount().getId(); + } + }; + private final Account account; private final Set<AccountGroup.UUID> internalGroups; private final Collection<AccountExternalId> externalIds; - private final Collection<AccountProjectWatch> projectWatches; + private final Map<ProjectWatchKey, Set<NotifyType>> projectWatches; private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties; public AccountState(Account account, Set<AccountGroup.UUID> actualGroups, Collection<AccountExternalId> externalIds, - Collection<AccountProjectWatch> projectWatches) { + Map<ProjectWatchKey, Set<NotifyType>> projectWatches) { this.account = account; this.internalGroups = actualGroups; this.externalIds = externalIds; @@ -81,7 +92,7 @@ } /** The project watches of the account. */ - public Collection<AccountProjectWatch> getProjectWatches() { + public Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches() { return projectWatches; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java index 2c66846..e2fbc3c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java
@@ -14,6 +14,8 @@ package com.google.gerrit.server.account; +import com.google.common.base.Function; +import com.google.common.collect.Lists; import com.google.gerrit.extensions.client.ProjectWatchInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.Response; @@ -24,12 +26,15 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import org.eclipse.jgit.errors.ConfigInvalidException; + import java.io.IOException; import java.util.HashMap; import java.util.LinkedList; @@ -38,52 +43,75 @@ @Singleton public class DeleteWatchedProjects implements RestModifyView<AccountResource, List<ProjectWatchInfo>> { - private final Provider<ReviewDb> dbProvider; private final Provider<IdentifiedUser> self; private final AccountCache accountCache; + private final WatchConfig.Accessor watchConfig; @Inject DeleteWatchedProjects(Provider<ReviewDb> dbProvider, Provider<IdentifiedUser> self, - AccountCache accountCache) { + AccountCache accountCache, + WatchConfig.Accessor watchConfig) { this.dbProvider = dbProvider; this.self = self; this.accountCache = accountCache; + this.watchConfig = watchConfig; } @Override public Response<?> apply(AccountResource rsrc, List<ProjectWatchInfo> input) throws AuthException, UnprocessableEntityException, OrmException, - IOException { - if (self.get() != rsrc.getUser() - && !self.get().getCapabilities().canAdministrateServer()) { + IOException, ConfigInvalidException { + if (self.get() != rsrc.getUser() + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("It is not allowed to edit project watches " + "of other users"); } + if (input == null) { + return Response.none(); + } + Account.Id accountId = rsrc.getUser().getAccountId(); + deleteFromDb(accountId, input); + deleteFromGit(accountId, input); + accountCache.evict(accountId); + return Response.none(); + } + + private void deleteFromDb(Account.Id accountId, List<ProjectWatchInfo> input) + throws OrmException, IOException { ResultSet<AccountProjectWatch> watchedProjects = dbProvider.get().accountProjectWatches().byAccount(accountId); - HashMap<AccountProjectWatch.Key, AccountProjectWatch> - watchedProjectsMap = new HashMap<>(); + HashMap<AccountProjectWatch.Key, AccountProjectWatch> watchedProjectsMap = + new HashMap<>(); for (AccountProjectWatch watchedProject : watchedProjects) { watchedProjectsMap.put(watchedProject.getKey(), watchedProject); } - if (input != null) { - List<AccountProjectWatch> watchesToDelete = new LinkedList<>(); - for (ProjectWatchInfo projectInfo : input) { - AccountProjectWatch.Key key = new AccountProjectWatch.Key(accountId, - new Project.NameKey(projectInfo.project), projectInfo.filter); - if (watchedProjectsMap.containsKey(key)) { - watchesToDelete.add(watchedProjectsMap.get(key)); - } - } - if (!watchesToDelete.isEmpty()) { - dbProvider.get().accountProjectWatches().delete(watchesToDelete); - accountCache.evict(accountId); + List<AccountProjectWatch> watchesToDelete = new LinkedList<>(); + for (ProjectWatchInfo projectInfo : input) { + AccountProjectWatch.Key key = new AccountProjectWatch.Key(accountId, + new Project.NameKey(projectInfo.project), projectInfo.filter); + if (watchedProjectsMap.containsKey(key)) { + watchesToDelete.add(watchedProjectsMap.get(key)); } } - return Response.none(); + if (!watchesToDelete.isEmpty()) { + dbProvider.get().accountProjectWatches().delete(watchesToDelete); + accountCache.evict(accountId); + } + } + + private void deleteFromGit(Account.Id accountId, List<ProjectWatchInfo> input) + throws IOException, ConfigInvalidException { + watchConfig.deleteProjectWatches(accountId, Lists.transform(input, + new Function<ProjectWatchInfo, ProjectWatchKey>() { + @Override + public ProjectWatchKey apply(ProjectWatchInfo info) { + return ProjectWatchKey.create(new Project.NameKey(info.project), + info.filter); + } + })); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java index 0c81b38..7cda472 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java
@@ -14,67 +14,120 @@ package com.google.gerrit.server.account; +import com.google.common.base.Strings; +import com.google.common.collect.ComparisonChain; import com.google.gerrit.extensions.client.ProjectWatchInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; + +import java.io.IOException; +import java.util.Collections; +import java.util.Comparator; +import java.util.EnumSet; +import java.util.HashMap; import java.util.LinkedList; import java.util.List; +import java.util.Map; +import java.util.Set; @Singleton public class GetWatchedProjects implements RestReadView<AccountResource> { private final Provider<ReviewDb> dbProvider; private final Provider<IdentifiedUser> self; + private final boolean readFromGit; + private final WatchConfig.Accessor watchConfig; @Inject public GetWatchedProjects(Provider<ReviewDb> dbProvider, - Provider<IdentifiedUser> self) { + Provider<IdentifiedUser> self, + @GerritServerConfig Config cfg, + WatchConfig.Accessor watchConfig) { this.dbProvider = dbProvider; this.self = self; + this.readFromGit = + cfg.getBoolean("user", null, "readProjectWatchesFromGit", true); + this.watchConfig = watchConfig; } @Override public List<ProjectWatchInfo> apply(AccountResource rsrc) - throws OrmException, AuthException { + throws OrmException, AuthException, IOException, ConfigInvalidException { if (self.get() != rsrc.getUser() - && !self.get().getCapabilities().canAdministrateServer()) { + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("It is not allowed to list project watches " + "of other users"); } + Account.Id accountId = rsrc.getUser().getAccountId(); + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + readFromGit + ? watchConfig.getProjectWatches(accountId) + : readProjectWatchesFromDb(dbProvider.get(), accountId); + List<ProjectWatchInfo> projectWatchInfos = new LinkedList<>(); - Iterable<AccountProjectWatch> projectWatches = - dbProvider.get().accountProjectWatches() - .byAccount(rsrc.getUser().getAccountId()); - for (AccountProjectWatch a : projectWatches) { + for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : projectWatches + .entrySet()) { ProjectWatchInfo pwi = new ProjectWatchInfo(); - pwi.filter = a.getFilter(); - pwi.project = a.getProjectNameKey().get(); + pwi.filter = e.getKey().filter(); + pwi.project = e.getKey().project().get(); pwi.notifyAbandonedChanges = - toBoolean( - a.isNotify(AccountProjectWatch.NotifyType.ABANDONED_CHANGES)); + toBoolean(e.getValue().contains(NotifyType.ABANDONED_CHANGES)); pwi.notifyNewChanges = - toBoolean(a.isNotify(AccountProjectWatch.NotifyType.NEW_CHANGES)); + toBoolean(e.getValue().contains(NotifyType.NEW_CHANGES)); pwi.notifyNewPatchSets = - toBoolean(a.isNotify(AccountProjectWatch.NotifyType.NEW_PATCHSETS)); + toBoolean(e.getValue().contains(NotifyType.NEW_PATCHSETS)); pwi.notifySubmittedChanges = - toBoolean( - a.isNotify(AccountProjectWatch.NotifyType.SUBMITTED_CHANGES)); + toBoolean(e.getValue().contains(NotifyType.SUBMITTED_CHANGES)); pwi.notifyAllComments = - toBoolean(a.isNotify(AccountProjectWatch.NotifyType.ALL_COMMENTS)); + toBoolean(e.getValue().contains(NotifyType.ALL_COMMENTS)); projectWatchInfos.add(pwi); } + Collections.sort(projectWatchInfos, new Comparator<ProjectWatchInfo>() { + @Override + public int compare(ProjectWatchInfo pwi1, ProjectWatchInfo pwi2) { + return ComparisonChain.start() + .compare(pwi1.project, pwi2.project) + .compare(Strings.nullToEmpty(pwi1.filter), + Strings.nullToEmpty(pwi2.filter)) + .result(); + } + }); return projectWatchInfos; } private static Boolean toBoolean(boolean value) { return value ? true : null; } + + public static Map<ProjectWatchKey, Set<NotifyType>> readProjectWatchesFromDb( + ReviewDb db, Account.Id who) throws OrmException { + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + new HashMap<>(); + for (AccountProjectWatch apw : db.accountProjectWatches().byAccount(who)) { + ProjectWatchKey key = + ProjectWatchKey.create(apw.getProjectNameKey(), apw.getFilter()); + Set<NotifyType> notifyValues = EnumSet.noneOf(NotifyType.class); + for (NotifyType notifyType : NotifyType.values()) { + if (apw.isNotify(notifyType)) { + notifyValues.add(notifyType); + } + } + projectWatches.put(key, notifyValues); + } + return projectWatches; + } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java index 863a1e4..d54ec50 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java
@@ -22,19 +22,26 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.project.ProjectsCollection; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import org.eclipse.jgit.errors.ConfigInvalidException; + import java.io.IOException; +import java.util.EnumSet; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Set; @Singleton @@ -45,39 +52,41 @@ private final GetWatchedProjects getWatchedProjects; private final ProjectsCollection projectsCollection; private final AccountCache accountCache; + private final WatchConfig.Accessor watchConfig; @Inject public PostWatchedProjects(Provider<ReviewDb> dbProvider, Provider<IdentifiedUser> self, GetWatchedProjects getWatchedProjects, ProjectsCollection projectsCollection, - AccountCache accountCache) { + AccountCache accountCache, + WatchConfig.Accessor watchConfig) { this.dbProvider = dbProvider; this.self = self; this.getWatchedProjects = getWatchedProjects; this.projectsCollection = projectsCollection; this.accountCache = accountCache; + this.watchConfig = watchConfig; } @Override public List<ProjectWatchInfo> apply(AccountResource rsrc, - List<ProjectWatchInfo> input) - throws OrmException, RestApiException, IOException { + List<ProjectWatchInfo> input) throws OrmException, RestApiException, + IOException, ConfigInvalidException { if (self.get() != rsrc.getUser() - && !self.get().getCapabilities().canAdministrateServer()) { + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to edit project watches"); } Account.Id accountId = rsrc.getUser().getAccountId(); - List<AccountProjectWatch> accountProjectWatchList = - getAccountProjectWatchList(input, accountId); - dbProvider.get().accountProjectWatches().upsert(accountProjectWatchList); + updateInDb(accountId, input); + updateInGit(accountId, input); accountCache.evict(accountId); return getWatchedProjects.apply(rsrc); } - private List<AccountProjectWatch> getAccountProjectWatchList( - List<ProjectWatchInfo> input, Account.Id accountId) - throws UnprocessableEntityException, BadRequestException, IOException { + private void updateInDb(Account.Id accountId, List<ProjectWatchInfo> input) + throws BadRequestException, UnprocessableEntityException, IOException, + OrmException { Set<AccountProjectWatch.Key> keys = new HashSet<>(); List<AccountProjectWatch> watchedProjects = new LinkedList<>(); for (ProjectWatchInfo a : input) { @@ -87,15 +96,11 @@ Project.NameKey projectKey = projectsCollection.parse(a.project).getNameKey(); - AccountProjectWatch.Key key = new AccountProjectWatch.Key(accountId, projectKey, a.filter); if (!keys.add(key)) { - throw new BadRequestException( - "duplicate entry for project " + key.getProjectName().get() - + (!AccountProjectWatch.FILTER_ALL.equals(key.getFilter().get()) - ? " and filter " + key.getFilter().get() - : "")); + throw new BadRequestException("duplicate entry for project " + + format(key.getProjectName().get(), key.getFilter().get())); } AccountProjectWatch apw = new AccountProjectWatch(key); apw.setNotify(AccountProjectWatch.NotifyType.ABANDONED_CHANGES, @@ -110,10 +115,61 @@ toBoolean(a.notifySubmittedChanges)); watchedProjects.add(apw); } - return watchedProjects; + dbProvider.get().accountProjectWatches().upsert(watchedProjects); + } + + private void updateInGit(Account.Id accountId, List<ProjectWatchInfo> input) + throws BadRequestException, UnprocessableEntityException, IOException, + ConfigInvalidException { + watchConfig.upsertProjectWatches(accountId, asMap(input)); + } + + private Map<ProjectWatchKey, Set<NotifyType>> asMap( + List<ProjectWatchInfo> input) throws BadRequestException, + UnprocessableEntityException, IOException { + Map<ProjectWatchKey, Set<NotifyType>> m = new HashMap<>(); + for (ProjectWatchInfo info : input) { + if (info.project == null) { + throw new BadRequestException("project name must be specified"); + } + + ProjectWatchKey key = ProjectWatchKey.create( + projectsCollection.parse(info.project).getNameKey(), info.filter); + if (m.containsKey(key)) { + throw new BadRequestException( + "duplicate entry for project " + format(info.project, info.filter)); + } + + Set<NotifyType> notifyValues = EnumSet.noneOf(NotifyType.class); + if (toBoolean(info.notifyAbandonedChanges)) { + notifyValues.add(NotifyType.ABANDONED_CHANGES); + } + if (toBoolean(info.notifyAllComments)) { + notifyValues.add(NotifyType.ALL_COMMENTS); + } + if (toBoolean(info.notifyNewChanges)) { + notifyValues.add(NotifyType.NEW_CHANGES); + } + if (toBoolean(info.notifyNewPatchSets)) { + notifyValues.add(NotifyType.NEW_PATCHSETS); + } + if (toBoolean(info.notifySubmittedChanges)) { + notifyValues.add(NotifyType.SUBMITTED_CHANGES); + } + + m.put(key, notifyValues); + } + return m; } private boolean toBoolean(Boolean b) { return b == null ? false : b; } + + private static String format(String project, String filter) { + return project + + (filter != null && !AccountProjectWatch.FILTER_ALL.equals(filter) + ? " and filter " + filter + : ""); + } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java index dc96d49..befccb6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAuthorizedKeys.java
@@ -14,7 +14,6 @@ package com.google.gerrit.server.account; -import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.base.Function; @@ -322,6 +321,6 @@ } private void checkLoaded() { - checkNotNull(keys, "SSH keys not loaded yet"); + checkState(keys != null, "SSH keys not loaded yet"); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java new file mode 100644 index 0000000..eae3349 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java
@@ -0,0 +1,340 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.account; + +import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Preconditions.checkState; + +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Enums; +import com.google.common.base.Joiner; +import com.google.common.base.Optional; +import com.google.common.base.Splitter; +import com.google.common.base.Strings; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimap; +import com.google.common.collect.Sets; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.VersionedMetaData; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.Repository; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.EnumSet; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * ‘watch.config’ file in the user branch in the All-Users repository that + * contains the watch configuration of the user. + * <p> + * The 'watch.config' file is a git config file that has one 'project' section + * for all project watches of a project. + * <p> + * The project name is used as subsection name and the filters with the notify + * types that decide for which events email notifications should be sent are + * represented as 'notify' values in the subsection. A 'notify' value is + * formatted as '<filter> [<comma-separated-list-of-notify-types>]': + * + * <pre> + * [project "foo"] + * notify = * [ALL_COMMENTS] + * notify = branch:master [ALL_COMMENTS, NEW_PATCHSETS] + * notify = branch:master owner:self [SUBMITTED_CHANGES] + * </pre> + * <p> + * If two notify values in the same subsection have the same filter they are + * merged on the next save, taking the union of the notify types. + * <p> + * For watch configurations that notify on no event the list of notify types is + * empty: + * + * <pre> + * [project "foo"] + * notify = branch:master [] + * </pre> + * <p> + * Unknown notify types are ignored and removed on save. + */ +public class WatchConfig extends VersionedMetaData implements AutoCloseable { + @Singleton + public static class Accessor { + private final GitRepositoryManager repoManager; + private final AllUsersName allUsersName; + private final Provider<MetaDataUpdate.User> metaDataUpdateFactory; + private final IdentifiedUser.GenericFactory userFactory; + + @Inject + Accessor( + GitRepositoryManager repoManager, + AllUsersName allUsersName, + Provider<MetaDataUpdate.User> metaDataUpdateFactory, + IdentifiedUser.GenericFactory userFactory) { + this.repoManager = repoManager; + this.allUsersName = allUsersName; + this.metaDataUpdateFactory = metaDataUpdateFactory; + this.userFactory = userFactory; + } + + public Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches( + Account.Id accountId) throws IOException, ConfigInvalidException { + try (Repository git = repoManager.openRepository(allUsersName); + WatchConfig watchConfig = new WatchConfig(accountId)) { + watchConfig.load(git); + return watchConfig.getProjectWatches(); + } + } + + public void upsertProjectWatches(Account.Id accountId, + Map<ProjectWatchKey, Set<NotifyType>> newProjectWatches) + throws IOException, ConfigInvalidException { + try (WatchConfig watchConfig = open(accountId)) { + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + watchConfig.getProjectWatches(); + projectWatches.putAll(newProjectWatches); + commit(watchConfig); + } + } + + public void deleteProjectWatches(Account.Id accountId, + Collection<ProjectWatchKey> projectWatchKeys) + throws IOException, ConfigInvalidException { + try (WatchConfig watchConfig = open(accountId)) { + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + watchConfig.getProjectWatches(); + boolean commit = false; + for (ProjectWatchKey key : projectWatchKeys) { + if (projectWatches.remove(key) != null) { + commit = true; + } + } + if (commit) { + commit(watchConfig); + } + } + } + + private WatchConfig open(Account.Id accountId) + throws IOException, ConfigInvalidException { + Repository git = repoManager.openRepository(allUsersName); + WatchConfig watchConfig = new WatchConfig(accountId); + watchConfig.load(git); + return watchConfig; + } + + private void commit(WatchConfig watchConfig) + throws IOException { + try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName, + userFactory.create(watchConfig.accountId))) { + watchConfig.commit(md); + } + } + } + + @AutoValue + public abstract static class ProjectWatchKey { + public static ProjectWatchKey create(Project.NameKey project, + @Nullable String filter) { + return new AutoValue_WatchConfig_ProjectWatchKey(project, + Strings.emptyToNull(filter)); + } + + public abstract Project.NameKey project(); + public abstract @Nullable String filter(); + } + + private static final String WATCH_CONFIG = "watch.config"; + private static final String PROJECT = "project"; + private static final String KEY_NOTIFY = "notify"; + + private final Account.Id accountId; + private final String ref; + + private Repository git; + private Map<ProjectWatchKey, Set<NotifyType>> projectWatches; + + public WatchConfig(Account.Id accountId) { + this.accountId = accountId; + this.ref = RefNames.refsUsers(accountId); + } + + @Override + protected String getRefName() { + return ref; + } + + @Override + public void load(Repository git) throws IOException, ConfigInvalidException { + checkState(this.git == null); + this.git = git; + super.load(git); + } + + @Override + protected void onLoad() throws IOException, ConfigInvalidException { + Config cfg = readConfig(WATCH_CONFIG); + projectWatches = parse(accountId, cfg); + } + + @VisibleForTesting + public static Map<ProjectWatchKey, Set<NotifyType>> parse( + Account.Id accountId, Config cfg) throws ConfigInvalidException { + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = new HashMap<>(); + for (String projectName : cfg.getSubsections(PROJECT)) { + String[] notifyValues = + cfg.getStringList(PROJECT, projectName, KEY_NOTIFY); + for (String nv : notifyValues) { + if (Strings.isNullOrEmpty(nv)) { + continue; + } + + NotifyValue notifyValue = NotifyValue.parse(accountId, projectName, nv); + ProjectWatchKey key = ProjectWatchKey + .create(new Project.NameKey(projectName), notifyValue.filter()); + if (!projectWatches.containsKey(key)) { + projectWatches.put(key, EnumSet.noneOf(NotifyType.class)); + } + projectWatches.get(key).addAll(notifyValue.notifyTypes()); + } + } + return projectWatches; + } + + Map<ProjectWatchKey, Set<NotifyType>> getProjectWatches() { + checkLoaded(); + return projectWatches; + } + + @Override + protected boolean onSave(CommitBuilder commit) + throws IOException, ConfigInvalidException { + checkLoaded(); + + if (Strings.isNullOrEmpty(commit.getMessage())) { + commit.setMessage("Updated watch configuration\n"); + } + + Config cfg = readConfig(WATCH_CONFIG); + + for (String projectName : cfg.getSubsections(PROJECT)) { + cfg.unset(PROJECT, projectName, KEY_NOTIFY); + } + + Multimap<String, String> notifyValuesByProject = ArrayListMultimap.create(); + for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : projectWatches + .entrySet()) { + NotifyValue notifyValue = + NotifyValue.create(e.getKey().filter(), e.getValue()); + notifyValuesByProject.put(e.getKey().project().get(), + notifyValue.toString()); + } + + for (Map.Entry<String, Collection<String>> e : notifyValuesByProject.asMap() + .entrySet()) { + cfg.setStringList(PROJECT, e.getKey(), KEY_NOTIFY, + new ArrayList<>(e.getValue())); + } + + saveConfig(WATCH_CONFIG, cfg); + return true; + } + + @Override + public void close() { + if (git != null) { + git.close(); + } + } + + private void checkLoaded() { + checkState(projectWatches != null, "project watches not loaded yet"); + } + + @AutoValue + abstract static class NotifyValue { + public static NotifyValue parse(Account.Id accountId, String project, + String notifyValue) throws ConfigInvalidException { + notifyValue = notifyValue.trim(); + int i = notifyValue.lastIndexOf('['); + if (i < 0 || notifyValue.charAt(notifyValue.length() - 1) != ']') { + throw new ConfigInvalidException(String.format( + "Invalid project watch of account %d for project %s: %s", + accountId.get(), project, notifyValue)); + } + String filter = notifyValue.substring(0, i).trim(); + if (filter.isEmpty() || AccountProjectWatch.FILTER_ALL.equals(filter)) { + filter = null; + } + + Set<NotifyType> notifyTypes = EnumSet.noneOf(NotifyType.class); + if (i + 1 < notifyValue.length() - 2) { + for (String nt : Splitter.on(',').trimResults().splitToList( + notifyValue.substring(i + 1, notifyValue.length() - 1))) { + Optional<NotifyType> notifyType = + Enums.getIfPresent(NotifyType.class, nt); + if (!notifyType.isPresent()) { + throw new ConfigInvalidException(String.format( + "Invalid notify type %s in project watch " + + "of account %d for project %s: %s", + nt, accountId.get(), project, notifyValue)); + } + notifyTypes.add(notifyType.get()); + } + } + return create(filter, notifyTypes); + } + + public static NotifyValue create(@Nullable String filter, + Set<NotifyType> notifyTypes) { + return new AutoValue_WatchConfig_NotifyValue(Strings.emptyToNull(filter), + Sets.immutableEnumSet(notifyTypes)); + } + + public abstract @Nullable String filter(); + public abstract ImmutableSet<NotifyType> notifyTypes(); + + @Override + public String toString() { + List<NotifyType> notifyTypes = new ArrayList<>(notifyTypes()); + StringBuilder notifyValue = new StringBuilder(); + notifyValue.append(firstNonNull(filter(), AccountProjectWatch.FILTER_ALL)) + .append(" ["); + Joiner.on(", ").appendTo(notifyValue, notifyTypes); + notifyValue.append("]"); + return notifyValue.toString(); + } + } +}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java index c678a2b..78e31a9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java
@@ -221,7 +221,7 @@ public List<ProjectWatchInfo> getWatchedProjects() throws RestApiException { try { return getWatchedProjects.apply(account); - } catch (OrmException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { throw new RestApiException("Cannot get watched projects", e); } } @@ -231,7 +231,7 @@ List<ProjectWatchInfo> in) throws RestApiException { try { return postWatchedProjects.apply(account, in); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { throw new RestApiException("Cannot update watched projects", e); } } @@ -241,7 +241,7 @@ throws RestApiException { try { deleteWatchedProjects.apply(account, in); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | ConfigInvalidException e) { throw new RestApiException("Cannot delete watched projects", e); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountField.java index 3b0d5e6..824739e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountField.java
@@ -20,8 +20,8 @@ import com.google.common.collect.FluentIterable; import com.google.common.collect.Iterables; import com.google.gerrit.reviewdb.client.AccountExternalId; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.index.FieldDef; import com.google.gerrit.server.index.FieldType; import com.google.gerrit.server.index.SchemaUtil; @@ -152,11 +152,11 @@ "watchedproject", FieldType.EXACT, false) { @Override public Iterable<String> get(AccountState input, FillArgs args) { - return FluentIterable.from(input.getProjectWatches()) - .transform(new Function<AccountProjectWatch, String>() { + return FluentIterable.from(input.getProjectWatches().keySet()) + .transform(new Function<ProjectWatchKey, String>() { @Override - public String apply(AccountProjectWatch in) { - return in.getProjectNameKey().get(); + public String apply(ProjectWatchKey in) { + return in.project().get(); } }).toSet(); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java index c558422..c1ead9f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java
@@ -28,6 +28,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.git.NotifyConfig; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.Predicate; @@ -43,6 +44,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; public class ProjectWatch { @@ -94,19 +96,23 @@ for (AccountState a : args.accountQueryProvider.get() .byWatchedProject(project)) { - for (AccountProjectWatch w : a.getProjectWatches()) { - if (add(matching, w, type)) { + Account.Id accountId = a.getAccount().getId(); + for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : + a.getProjectWatches().entrySet()) { + if (add(matching, accountId, e.getKey(), e.getValue(), type)) { // We only want to prevent matching All-Projects if this filter hits - projectWatchers.add(w.getAccountId()); + projectWatchers.add(accountId); } } } for (AccountState a : args.accountQueryProvider.get() .byWatchedProject(args.allProjectsName)) { - for (AccountProjectWatch w : a.getProjectWatches()) { - if (!projectWatchers.contains(w.getAccountId())) { - add(matching, w, type); + for (Map.Entry<ProjectWatchKey, Set<NotifyType>> e : + a.getProjectWatches().entrySet()) { + Account.Id accountId = a.getAccount().getId(); + if (!projectWatchers.contains(accountId)) { + add(matching, accountId, e.getKey(), e.getValue(), type); } } } @@ -210,6 +216,26 @@ } } + private boolean add(Watchers matching, Account.Id accountId, + ProjectWatchKey key, Set<NotifyType> watchedTypes, NotifyType type) + throws OrmException { + IdentifiedUser user = args.identifiedUserFactory.create(accountId); + + try { + if (filterMatch(user, key.filter())) { + // If we are set to notify on this type, add the user. + // Otherwise, still return true to stop notifications for this user. + if (watchedTypes.contains(type)) { + matching.bcc.accounts.add(accountId); + } + return true; + } + } catch (QueryParseException e) { + // Ignore broken filter expressions. + } + return false; + } + private boolean add(Watchers matching, AccountProjectWatch w, NotifyType type) throws OrmException { IdentifiedUser user = args.identifiedUserFactory.create(w.getAccountId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java index 2e3aa59..7bc3144 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
@@ -14,6 +14,8 @@ package com.google.gerrit.server.query.account; +import com.google.common.base.Joiner; +import com.google.common.collect.Lists; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.index.IndexConfig; @@ -22,10 +24,16 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.util.List; import java.util.Set; public class InternalAccountQuery extends InternalQuery<AccountState> { + private static final Logger log = + LoggerFactory.getLogger(InternalAccountQuery.class); + @Inject InternalAccountQuery(AccountQueryProcessor queryProcessor, AccountIndexCollection indexes, @@ -67,6 +75,22 @@ return query(AccountPredicates.externalId(externalId)); } + public AccountState oneByExternalId(String externalId) throws OrmException { + List<AccountState> accountStates = byExternalId(externalId); + if (accountStates.size() == 1) { + return accountStates.get(0); + } else if (accountStates.size() > 0) { + StringBuilder msg = new StringBuilder(); + msg.append("Ambiguous external ID ") + .append(externalId) + .append("for accounts: "); + Joiner.on(", ").appendTo(msg, + Lists.transform(accountStates, AccountState.ACCOUNT_ID_FUNCTION)); + log.warn(msg.toString()); + } + return null; + } + public List<AccountState> byFullName(String fullName) throws OrmException { return query(AccountPredicates.fullName(fullName));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java index 0090cf4..0b7a2f0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
@@ -15,8 +15,8 @@ package com.google.gerrit.server.query.change; import com.google.common.collect.ImmutableList; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.query.AndPredicate; import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryBuilder; @@ -48,11 +48,11 @@ boolean checkIsVisible) throws QueryParseException { List<Predicate<ChangeData>> r = new ArrayList<>(); ChangeQueryBuilder builder = new ChangeQueryBuilder(args); - for (AccountProjectWatch w : getWatches(args)) { + for (ProjectWatchKey w : getWatches(args)) { Predicate<ChangeData> f = null; - if (w.getFilter() != null) { + if (w.filter() != null) { try { - f = builder.parse(w.getFilter()); + f = builder.parse(w.filter()); if (QueryBuilder.find(f, IsWatchedByPredicate.class) != null) { // If the query is going to infinite loop, assume it // will never match and return null. Yes this test @@ -66,10 +66,10 @@ } Predicate<ChangeData> p; - if (w.getProjectNameKey().equals(args.allProjectsName)) { + if (w.project().equals(args.allProjectsName)) { p = null; } else { - p = builder.project(w.getProjectNameKey().get()); + p = builder.project(w.project().get()); } if (p != null && f != null) { @@ -91,14 +91,14 @@ } } - private static Collection<AccountProjectWatch> getWatches( + private static Collection<ProjectWatchKey> getWatches( ChangeQueryBuilder.Arguments args) throws QueryParseException { CurrentUser user = args.getUser(); if (user.isIdentifiedUser()) { return args.accountCache.get(args.getUser().getAccountId()) - .getProjectWatches(); + .getProjectWatches().keySet(); } - return Collections.<AccountProjectWatch> emptySet(); + return Collections.<ProjectWatchKey> emptySet(); } private static List<Predicate<ChangeData>> none() {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java new file mode 100644 index 0000000..0133ec2 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java
@@ -0,0 +1,166 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.account; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; + +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; +import com.google.gerrit.server.account.WatchConfig.NotifyValue; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; + +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Config; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import java.util.EnumSet; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +public class WatchConfigTest { + @Rule + public ExpectedException exception = ExpectedException.none(); + + @Test + public void parseWatchConfig() throws Exception { + Config cfg = new Config(); + cfg.fromText("[project \"myProject\"]\n" + + " notify = * [ALL_COMMENTS, NEW_PATCHSETS]\n" + + " notify = branch:master [NEW_CHANGES]\n" + + " notify = branch:master [NEW_PATCHSETS]\n" + + " notify = branch:foo []\n" + + "[project \"otherProject\"]\n" + + " notify = [NEW_PATCHSETS]\n" + + " notify = * [NEW_PATCHSETS, ALL_COMMENTS]\n"); + Map<ProjectWatchKey, Set<NotifyType>> projectWatches = + WatchConfig.parse(new Account.Id(1000000), cfg); + + Project.NameKey myProject = new Project.NameKey("myProject"); + Project.NameKey otherProject = new Project.NameKey("otherProject"); + Map<ProjectWatchKey, Set<NotifyType>> expectedProjectWatches = + new HashMap<>(); + expectedProjectWatches.put(ProjectWatchKey.create(myProject, null), + EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS)); + expectedProjectWatches.put( + ProjectWatchKey.create(myProject, "branch:master"), + EnumSet.of(NotifyType.NEW_CHANGES, NotifyType.NEW_PATCHSETS)); + expectedProjectWatches.put(ProjectWatchKey.create(myProject, "branch:foo"), + EnumSet.noneOf(NotifyType.class)); + expectedProjectWatches.put(ProjectWatchKey.create(otherProject, null), + EnumSet.of(NotifyType.NEW_PATCHSETS)); + expectedProjectWatches.put(ProjectWatchKey.create(otherProject, null), + EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS)); + assertThat(projectWatches).containsExactlyEntriesIn(expectedProjectWatches); + } + + @Test + public void parseInvalidWatchConfig() throws Exception { + Config cfg = new Config(); + cfg.fromText("[project \"myProject\"]\n" + + " notify = * [ALL_COMMENTS, NEW_PATCHSETS]\n" + + " notify = branch:master [INVALID, NEW_CHANGES]\n" + + "[project \"otherProject\"]\n" + + " notify = [NEW_PATCHSETS]\n"); + + exception.expect(ConfigInvalidException.class); + exception.expectMessage( + "Invalid notify type INVALID in project watch of account 1000000" + + " for project myProject: branch:master [INVALID, NEW_CHANGES]"); + WatchConfig.parse(new Account.Id(1000000), cfg); + } + + @Test + public void parseNotifyValue() throws Exception { + assertParseNotifyValue("* []", null, EnumSet.noneOf(NotifyType.class)); + assertParseNotifyValue("* [ALL_COMMENTS]", null, + EnumSet.of(NotifyType.ALL_COMMENTS)); + assertParseNotifyValue("[]", null, EnumSet.noneOf(NotifyType.class)); + assertParseNotifyValue("[ALL_COMMENTS, NEW_PATCHSETS]", null, + EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS)); + assertParseNotifyValue("branch:master []", "branch:master", + EnumSet.noneOf(NotifyType.class)); + assertParseNotifyValue("branch:master || branch:stable []", + "branch:master || branch:stable", EnumSet.noneOf(NotifyType.class)); + assertParseNotifyValue("branch:master [ALL_COMMENTS]", "branch:master", + EnumSet.of(NotifyType.ALL_COMMENTS)); + assertParseNotifyValue("branch:master [ALL_COMMENTS, NEW_PATCHSETS]", + "branch:master", + EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS)); + assertParseNotifyValue("* [ALL]", null, EnumSet.of(NotifyType.ALL)); + } + + @Test + public void parseInvalidNotifyValue() { + assertParseNotifyValueFails("* [] illegal-characters-at-the-end"); + assertParseNotifyValueFails("* [INVALID]"); + assertParseNotifyValueFails("* [ALL_COMMENTS, UNKNOWN]"); + assertParseNotifyValueFails("* [ALL_COMMENTS NEW_CHANGES]"); + assertParseNotifyValueFails("* [ALL_COMMENTS, NEW_CHANGES"); + assertParseNotifyValueFails("* ALL_COMMENTS, NEW_CHANGES]"); + } + + @Test + public void toNotifyValue() throws Exception { + assertToNotifyValue(null, EnumSet.noneOf(NotifyType.class), "* []"); + assertToNotifyValue("*", EnumSet.noneOf(NotifyType.class), "* []"); + assertToNotifyValue(null, EnumSet.of(NotifyType.ALL_COMMENTS), + "* [ALL_COMMENTS]"); + assertToNotifyValue("branch:master", EnumSet.noneOf(NotifyType.class), + "branch:master []"); + assertToNotifyValue("branch:master", + EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS), + "branch:master [ALL_COMMENTS, NEW_PATCHSETS]"); + assertToNotifyValue("branch:master", + EnumSet.of(NotifyType.ABANDONED_CHANGES, NotifyType.ALL_COMMENTS, + NotifyType.NEW_CHANGES, NotifyType.NEW_PATCHSETS, + NotifyType.SUBMITTED_CHANGES), + "branch:master [ABANDONED_CHANGES, ALL_COMMENTS, NEW_CHANGES," + + " NEW_PATCHSETS, SUBMITTED_CHANGES]"); + assertToNotifyValue("*", EnumSet.of(NotifyType.ALL), "* [ALL]"); + } + + private static void assertParseNotifyValue(String notifyValue, + String expectedFilter, Set<NotifyType> expectedNotifyTypes) + throws ConfigInvalidException { + NotifyValue nv = parseNotifyValue(notifyValue); + assertThat(nv.filter()).isEqualTo(expectedFilter); + assertThat(nv.notifyTypes()).containsExactlyElementsIn(expectedNotifyTypes); + } + + private static void assertToNotifyValue(String filter, + Set<NotifyType> notifyTypes, String expectedNotifyValue) { + NotifyValue nv = NotifyValue.create(filter, notifyTypes); + assertThat(nv.toString()).isEqualTo(expectedNotifyValue); + } + + private static void assertParseNotifyValueFails(String notifyValue) { + try { + parseNotifyValue(notifyValue); + fail("expected ConfigInvalidException for notifyValue: " + notifyValue); + } catch (ConfigInvalidException e) { + // Expected. + } + } + + private static NotifyValue parseNotifyValue(String notifyValue) + throws ConfigInvalidException { + return NotifyValue.parse(new Account.Id(1000000), "project", notifyValue); + } +}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java index fab21ab..11f1d54 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java
@@ -25,9 +25,10 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; @@ -35,6 +36,8 @@ import org.junit.Test; import java.util.Collections; +import java.util.HashMap; +import java.util.Set; public class FromAddressGeneratorProviderTest { private Config config; @@ -300,6 +303,6 @@ account.setPreferredEmail(email); return new AccountState(account, Collections.<AccountGroup.UUID> emptySet(), Collections.<AccountExternalId> emptySet(), - Collections.<AccountProjectWatch> emptySet()); + new HashMap<ProjectWatchKey, Set<NotifyType>>()); } }
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java index ca322a7..f809d11 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeAccountCache.java
@@ -19,12 +19,14 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import java.util.HashMap; import java.util.Map; +import java.util.Set; /** Fake implementation of {@link AccountCache} for testing. */ public class FakeAccountCache implements AccountCache { @@ -76,6 +78,6 @@ private static AccountState newState(Account account) { return new AccountState(account, ImmutableSet.<AccountGroup.UUID> of(), ImmutableSet.<AccountExternalId> of(), - ImmutableSet.<AccountProjectWatch> of()); + new HashMap<ProjectWatchKey, Set<NotifyType>>()); } }