Merge "BatchUpdate: Support executing ops by different users"
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
index 1dcd662..5efcfc6 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java
@@ -150,6 +150,7 @@
String refName = RefNames.fullName(changeCreation.branch());
ChangeInserter inserter = getChangeInserter(changeId, refName, commitId);
+ inserter.setGroups(getGroups(changeCreation));
changeCreation.topic().ifPresent(t -> inserter.setTopic(t));
inserter.setApprovals(changeCreation.approvals());
@@ -243,9 +244,73 @@
return createCommit(objectInserter, tree, parentCommits, author, committer, commitMessage);
}
+ private ImmutableList<String> getGroups(TestChangeCreation changeCreation) {
+ return changeCreation
+ .parents()
+ .map(parents -> getGroups(parents))
+ .orElseGet(() -> ImmutableList.of());
+ }
+
+ private ImmutableList<String> getGroups(ImmutableList<TestCommitIdentifier> parents) {
+ return parents.stream()
+ .map(parent -> getGroups(parent))
+ .flatMap(groups -> groups.stream())
+ .collect(toImmutableList());
+ }
+
+ private ImmutableList<String> getGroups(TestCommitIdentifier parentCommit) {
+ switch (parentCommit.getKind()) {
+ case BRANCH:
+ return ImmutableList.of();
+ case CHANGE_ID:
+ return getGroupsFromChange(parentCommit.changeId());
+ case COMMIT_SHA_1:
+ return ImmutableList.of();
+ case PATCHSET_ID:
+ return getGroupsFromPatchset(parentCommit.patchsetId());
+ default:
+ throw new IllegalStateException(
+ String.format("No parent behavior implemented for %s.", parentCommit.getKind()));
+ }
+ }
+
+ private ImmutableList<String> getGroupsFromChange(Change.Id changeId) {
+ Optional<ChangeNotes> changeNotes = changeFinder.findOne(changeId);
+
+ if (changeNotes.isPresent() && changeNotes.get().getChange().isClosed()) {
+ return ImmutableList.of();
+ }
+
+ return changeNotes
+ .map(ChangeNotes::getCurrentPatchSet)
+ .map(PatchSet::groups)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Change %s not found and hence can't be used as parent.", changeId)));
+ }
+
+ private ImmutableList<String> getGroupsFromPatchset(PatchSet.Id patchsetId) {
+ Optional<ChangeNotes> changeNotes = changeFinder.findOne(patchsetId.changeId());
+
+ if (changeNotes.isPresent() && changeNotes.get().getChange().isClosed()) {
+ return ImmutableList.of();
+ }
+
+ return changeNotes
+ .map(ChangeNotes::getPatchSets)
+ .map(patchsets -> patchsets.get(patchsetId))
+ .map(PatchSet::groups)
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Patchset %s not found and hence can't be used as parent.", patchsetId)));
+ }
+
private ImmutableList<ObjectId> getParentCommits(
Repository repository, RevWalk revWalk, TestChangeCreation changeCreation) {
-
return changeCreation
.parents()
.map(parents -> resolveParents(repository, revWalk, parents))
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index b40e100..dc9bc32 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -174,4 +174,14 @@
submitted = Timestamp.from(when);
submitter = who;
}
+
+ public RevisionInfo getCurrentRevision() {
+ RevisionInfo currentRevisionInfo = revisions.get(currentRevision);
+ if (currentRevisionInfo.commit != null) {
+ // If all revisions are requested the commit.commit field is not populated because the commit
+ // SHA1 is already present as the key in the revisions map.
+ currentRevisionInfo.commit.commit = currentRevision;
+ }
+ return currentRevisionInfo;
+ }
}
diff --git a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
index 92788b7..fb28d30 100644
--- a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
@@ -101,6 +101,7 @@
IndexPreloadingUtil.computeChangeRequestsPath(requestedPath, page).get());
data.put("changeNum", IndexPreloadingUtil.computeChangeNum(requestedPath, page).get());
break;
+ case PROFILE:
case DASHBOARD:
// Dashboard is preloaded queries are added later when we check user is authenticated.
case PAGE_WITHOUT_PRELOADING:
diff --git a/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java b/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
index afaeaf6..402e48a 100644
--- a/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexPreloadingUtil.java
@@ -40,6 +40,7 @@
CHANGE,
DIFF,
DASHBOARD,
+ PROFILE,
PAGE_WITHOUT_PRELOADING,
}
@@ -50,6 +51,7 @@
public static final Pattern DIFF_URL_PATTERN =
Pattern.compile(CHANGE_CANONICAL_PATH + BASE_PATCH_NUM_PATH_PART + "(/(.+))" + "/?$");
public static final Pattern DASHBOARD_PATTERN = Pattern.compile("/dashboard/self$");
+ public static final Pattern PROFILE_PATTERN = Pattern.compile("/profile/self$");
public static final String ROOT_PATH = "/";
// These queries should be kept in sync with PolyGerrit:
@@ -130,6 +132,11 @@
return RequestedPage.DASHBOARD;
}
+ Matcher profileMatcher = IndexPreloadingUtil.PROFILE_PATTERN.matcher(requestedPath);
+ if (profileMatcher.matches()) {
+ return RequestedPage.PROFILE;
+ }
+
if (ROOT_PATH.equals(requestedPath)) {
return RequestedPage.DASHBOARD;
}
@@ -148,6 +155,7 @@
matcher = DIFF_URL_PATTERN.matcher(requestedURL);
break;
case DASHBOARD:
+ case PROFILE:
case PAGE_WITHOUT_PRELOADING:
default:
return Optional.empty();
@@ -172,6 +180,7 @@
matcher = DIFF_URL_PATTERN.matcher(requestedURL);
break;
case DASHBOARD:
+ case PROFILE:
case PAGE_WITHOUT_PRELOADING:
default:
return Optional.empty();
diff --git a/java/com/google/gerrit/httpd/raw/StaticModule.java b/java/com/google/gerrit/httpd/raw/StaticModule.java
index 961bf9b..8319d9d 100644
--- a/java/com/google/gerrit/httpd/raw/StaticModule.java
+++ b/java/com/google/gerrit/httpd/raw/StaticModule.java
@@ -77,6 +77,7 @@
"/x/*",
"/admin/*",
"/dashboard/*",
+ "/profile/*",
"/groups/self",
"/settings/*",
"/Documentation/q/*");
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 703693d..1818b1b 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -25,6 +25,7 @@
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -43,6 +44,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
@@ -421,13 +423,70 @@
private class ByEmail extends StringSearcher {
@Override
+ public boolean requiresContextUser() {
+ return true;
+ }
+
+ @Override
protected boolean matches(String input) {
return input.contains("@");
}
@Override
- public Stream<AccountState> search(String input) throws IOException {
- return toAccountStates(emails.getAccountFor(input));
+ public Stream<AccountState> search(String input, CurrentUser asUser) throws IOException {
+ boolean canSeeSecondaryEmails = false;
+ try {
+ if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ canSeeSecondaryEmails = true;
+ }
+ } catch (PermissionBackendException e) {
+ // remains false
+ }
+
+ if (canSeeSecondaryEmails) {
+ return toAccountStates(emails.getAccountFor(input));
+ }
+
+ // User cannot see secondary emails, hence search by preferred email only.
+ List<AccountState> accountStates = accountQueryProvider.get().byPreferredEmail(input);
+
+ if (accountStates.size() == 1) {
+ return Stream.of(Iterables.getOnlyElement(accountStates));
+ }
+
+ if (accountStates.size() > 1) {
+ // An email can only belong to a single account. If multiple accounts are found it means
+ // there is an inconsistency, i.e. some of the found accounts have a preferred email set
+ // that they do not own via an external ID. Hence in this case we return only the one
+ // account that actually owns the email via an external ID.
+ for (AccountState accountState : accountStates) {
+ if (accountState.externalIds().stream()
+ .map(ExternalId::email)
+ .filter(Objects::nonNull)
+ .anyMatch(email -> email.equals(input))) {
+ return Stream.of(accountState);
+ }
+ }
+
+ // None of the matched accounts owns the email, return all matches to be consistent with
+ // the behavior of Emails.getAccountFor(String) that is used above if the user can see
+ // secondary emails.
+ return accountStates.stream();
+ }
+
+ // No match by preferred email. Since users can always see their own secondary emails, check
+ // if the input matches a secondary email of the user and if yes, return the account of the
+ // user.
+ if (asUser.isIdentifiedUser()
+ && asUser.asIdentifiedUser().state().externalIds().stream()
+ .map(ExternalId::email)
+ .filter(Objects::nonNull)
+ .anyMatch(email -> email.equals(input))) {
+ return Stream.of(asUser.asIdentifiedUser().state());
+ }
+
+ // No match.
+ return Stream.empty();
}
@Override
@@ -671,6 +730,18 @@
input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::isActive);
}
+ public Result resolveAsUserIgnoreVisibility(CurrentUser asUser, String input)
+ throws ConfigInvalidException, IOException {
+ return resolveAsUserIgnoreVisibility(asUser, input, AccountResolver::isActive);
+ }
+
+ public Result resolveAsUserIgnoreVisibility(
+ CurrentUser asUser, String input, Predicate<AccountState> accountActivityPredicate)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(
+ input, searchers, asUser, this::allVisiblePredicate, accountActivityPredicate);
+ }
+
/**
* Resolves all accounts matching the input string by name or email.
*
diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java
index 8c3f033..13385d0 100644
--- a/java/com/google/gerrit/server/account/Emails.java
+++ b/java/com/google/gerrit/server/account/Emails.java
@@ -57,12 +57,11 @@
* are needed it is more efficient to use {@link #getAccountsFor(String...)} as this method reads
* the SHA1 of the refs/meta/external-ids branch only once (and not once per email).
*
- * <p>In addition accounts are included that have the given email as preferred email even if they
- * have no external ID for the preferred email. Having accounts with a preferred email that does
- * not exist as external ID is an inconsistency, but existing functionality relies on still
- * getting those accounts, which is why they are included. Accounts by preferred email are fetched
- * from the account index as a fallback for email addresses that could not be resolved using
- * {@link ExternalIds}.
+ * <p>If there is no account that owns the email via an external ID all accounts that have the
+ * email set as a preferred email are returned. Having accounts with a preferred email that does
+ * not exist as external ID is an inconsistency, but existing functionality relies on getting
+ * those accounts, which is why they are returned as a fall-back by fetching them from the account
+ * index.
*
* @see #getAccountsFor(String...)
*/
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index f6fc8db..56a0bbd 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -24,6 +24,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
@@ -230,6 +231,7 @@
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
+ public static final Account.Id NON_EXISTING_ACCOUNT_ID = Account.id(-1000);
public static final String OPERATOR_MERGED_BEFORE = "mergedbefore";
public static final String OPERATOR_MERGED_AFTER = "mergedafter";
@@ -1043,7 +1045,7 @@
} else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
- accounts = parseAccount(value);
+ accounts = parseAccountIgnoreVisibility(value);
}
} else if (key.equalsIgnoreCase(ARG_ID_GROUP)) {
group = parseGroup(value).getUUID();
@@ -1071,17 +1073,17 @@
if (accounts != null || group != null) {
throw new QueryParseException("more than one user/group specified (" + value + ")");
}
- try {
- if (value.equals(ARG_ID_OWNER)) {
- accounts = Collections.singleton(OWNER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_UPLOADER)) {
- accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
- } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
- accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
- } else {
- accounts = parseAccount(value);
- }
- } catch (QueryParseException qpex) {
+ if (value.equals(ARG_ID_OWNER)) {
+ accounts = Collections.singleton(OWNER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_UPLOADER)) {
+ accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
+ } else {
+ accounts = parseAccountIgnoreVisibility(value);
+ }
+
+ if (accounts.contains(NON_EXISTING_ACCOUNT_ID)) {
// If it doesn't match an account, see if it matches a group
// (accounts get precedence)
try {
@@ -1227,7 +1229,7 @@
@Operator
public Predicate<ChangeData> owner(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return owner(parseAccount(who, (AccountState s) -> true));
+ return owner(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> owner(Set<Account.Id> who) {
@@ -1240,7 +1242,7 @@
private Predicate<ChangeData> ownerDefaultField(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (accounts.size() > MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
return Predicate.any();
}
@@ -1251,7 +1253,7 @@
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
- return uploader(parseAccount(who, (AccountState s) -> true));
+ return uploader(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> uploader(Set<Account.Id> who) {
@@ -1266,7 +1268,7 @@
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
- return attention(parseAccount(who, (AccountState s) -> true));
+ return attention(parseAccountIgnoreVisibility(who, (AccountState s) -> true));
}
private Predicate<ChangeData> attention(Set<Account.Id> who) {
@@ -1403,7 +1405,7 @@
@Operator
public Predicate<ChangeData> commentby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return commentby(parseAccount(who));
+ return commentby(parseAccountIgnoreVisibility(who));
}
private Predicate<ChangeData> commentby(Set<Account.Id> who) {
@@ -1417,7 +1419,7 @@
@Operator
public Predicate<ChangeData> from(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- Set<Account.Id> ownerIds = parseAccount(who);
+ Set<Account.Id> ownerIds = parseAccountIgnoreVisibility(who);
return Predicate.or(owner(ownerIds), commentby(ownerIds));
}
@@ -1469,7 +1471,7 @@
@Operator
public Predicate<ChangeData> reviewedby(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- return ChangePredicates.reviewedBy(parseAccount(who));
+ return ChangePredicates.reviewedBy(parseAccountIgnoreVisibility(who));
}
@Operator
@@ -1722,18 +1724,42 @@
}
}
- private Set<Account.Id> parseAccount(
- String who, java.util.function.Predicate<AccountState> activityFilter)
- throws QueryParseException, IOException, ConfigInvalidException {
+ private Set<Account.Id> parseAccountIgnoreVisibility(String who)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
try {
return args.accountResolver
- .resolveAsUser(args.getUser(), who, activityFilter)
+ .resolveAsUserIgnoreVisibility(args.getUser(), who)
.asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
}
- throw new QueryParseException(e.getMessage(), e);
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
+ }
+ }
+
+ private Set<Account.Id> parseAccountIgnoreVisibility(
+ String who, java.util.function.Predicate<AccountState> activityFilter)
+ throws QueryRequiresAuthException, IOException, ConfigInvalidException {
+ try {
+ return args.accountResolver
+ .resolveAsUserIgnoreVisibility(args.getUser(), who, activityFilter)
+ .asNonEmptyIdSet();
+ } catch (UnresolvableAccountException e) {
+ // Thrown if no account was found.
+
+ // Users can always see their own account. This means if self was being resolved and there was
+ // no match the user wasn't logged it and the request was done anonymously.
+ if (e.isSelf()) {
+ throw new QueryRequiresAuthException(e.getMessage(), e);
+ }
+
+ // If no account is found, we don't want to fail with an error as this would allow users to
+ // probe the existence of accounts (error -> account doesn't exist, empty result -> account
+ // exists but didn't take part in any visible changes). Hence, we return a special account ID
+ // (NON_EXISTING_ACCOUNT_ID) that doesn't match any account so the query can be normally
+ // executed
+ return ImmutableSet.of(NON_EXISTING_ACCOUNT_ID);
}
}
@@ -1790,7 +1816,7 @@
Predicate<ChangeData> reviewerPredicate = null;
try {
- Set<Account.Id> accounts = parseAccount(who);
+ Set<Account.Id> accounts = parseAccountIgnoreVisibility(who);
if (!forDefaultField || accounts.size() <= MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
reviewerPredicate =
Predicate.or(
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 2c1739d..a9a0b70 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -2899,8 +2899,12 @@
requestScopeOperations.setApiUser(user.id());
assertThrows(ResourceNotFoundException.class, () -> gApi.accounts().id("secondary"));
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id("secondary@example.com"));
requestScopeOperations.setApiUser(admin.id());
assertThat(gApi.accounts().id("secondary").get()._accountId).isEqualTo(foo.id().get());
+ assertThat(gApi.accounts().id("secondary@example.com").get()._accountId)
+ .isEqualTo(foo.id().get());
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
index 3bfb573..43c8684 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -23,16 +24,20 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
@@ -40,9 +45,11 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.restapi.change.QueryChanges;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Arrays;
@@ -53,6 +60,7 @@
public class QueryChangesIT extends AbstractDaemonTest {
@Inject private AccountOperations accountOperations;
+ @Inject private ChangeOperations changeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private Provider<QueryChanges> queryChangesProvider;
@Inject private RequestScopeOperations requestScopeOperations;
@@ -364,6 +372,282 @@
assertThat(result3).hasSize(1);
}
+ /**
+ * This test verifies that querying by a non-visible account doesn't fail.
+ *
+ * <p>Change queries only return changes that are visible to the calling user. If a non-visible
+ * account participated in such a change the existence of this account is known to everyone who
+ * can see the change. Hence it's OK to that the account visibility check is skipped when querying
+ * changes by non-visible accounts. If the account is visible through any visible change these
+ * changes are returned, otherwise the result is empty (see
+ * emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()), same as for
+ * non-existing accounts (see test emptyResultWhenQueryingByNonExistingAccount()).
+ */
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void changesCanBeQueriesByNonVisibleAccounts() throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ /**
+ * This test verifies that an empty result is returned for a query by a non-existing account.
+ *
+ * <p>Such queries must not return an error so that users cannot probe whether an account exists.
+ * Since we return an empty result for non-visible accounts if there are no matched changes or non
+ * of the matched changes is visible, users could conclude the existence of a account if we would
+ * return an error for non-existing accounts.
+ */
+ @Test
+ public void emptyResultWhenQueryingByNonExistingAccount() throws Exception {
+ assertThat(gApi.changes().query("owner:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("reviewer:non-existing@example.com").get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=non-existing@example.com").get())
+ .isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void emptyResultWhenQueryingByNonVisibleAccountAndMatchingChangesAreNotVisible()
+ throws Exception {
+ String ownerEmail = "owner@example.com";
+ Account.Id nonVisibleOwner = accountOperations.newAccount().preferredEmail(ownerEmail).create();
+
+ String reviewerEmail = "reviewer@example.com";
+ Account.Id nonVisibleReviewer =
+ accountOperations.newAccount().preferredEmail(reviewerEmail).create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(nonVisibleOwner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(nonVisibleReviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Block read permission so that the change is not visible.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user cannot see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get()).isEmpty();
+
+ // Verify that user cannot see the other accounts.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleOwner.get()).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleReviewer.get()).get());
+
+ // Verify that the change is also found if user queries for changes owned/uploaded by
+ // nonVisibleOwner.
+ assertThat(gApi.changes().query("owner:" + ownerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + ownerEmail).get()).isEmpty();
+
+ // Verify that the change is also found if user queries for changes reviewed by
+ // nonVisibleReviewer.
+ assertThat(gApi.changes().query("reviewer:" + reviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + reviewerEmail).get()).isEmpty();
+ }
+
+ @Test
+ public void emptyResultWhenQueryingByNonVisibleSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the change.
+ assertThat(gApi.changes().query("change:" + changeId).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that user cannot see the other accounts by their secondary email.
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryOwnerEmail).get());
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(secondaryReviewerEmail).get());
+
+ // Verify that the change is not found if user queries for changes owned/uploaded by the
+ // secondary email of the owner that is not visible to user.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get()).isEmpty();
+
+ // Verify that the change is not found if user queries for changes reviewed by the secondary
+ // email of the reviewer that is not visible to user.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get()).isEmpty();
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .isEmpty();
+ }
+
+ @Test
+ public void changesFoundWhenQueryingBySecondaryEmailWithModifyAccountCapability()
+ throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // Verify that user can see the other accounts by their secondary email.
+ assertThat(gApi.accounts().id(secondaryOwnerEmail).get()._accountId).isEqualTo(owner.get());
+ assertThat(gApi.accounts().id(secondaryReviewerEmail).get()._accountId)
+ .isEqualTo(reviewer.get());
+
+ // Verify that the change is found if user queries for changes owned/uploaded by the secondary
+ // email.
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if user queries for changes reviewed by the secondary email.
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
+ @Test
+ public void changesFoundWhenQueryingByOwnSecondaryEmail() throws Exception {
+ String secondaryOwnerEmail = "owner-secondary@example.com";
+ Account.Id owner =
+ accountOperations
+ .newAccount()
+ .preferredEmail("owner@example.com")
+ .addSecondaryEmail(secondaryOwnerEmail)
+ .create();
+
+ String secondaryReviewerEmail = "reviewer-secondary@example.com";
+ Account.Id reviewer =
+ accountOperations
+ .newAccount()
+ .preferredEmail("reviewer@example.com")
+ .addSecondaryEmail(secondaryReviewerEmail)
+ .create();
+
+ // Create the change.
+ Change.Id changeId = changeOperations.newChange().owner(owner).create();
+
+ // Add a review.
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.recommend());
+
+ // Verify that the change is found if owner queries for changes owned/uploaded by their
+ // secondary email.
+ requestScopeOperations.setApiUser(owner);
+ assertThat(gApi.changes().query("owner:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("uploader:" + secondaryOwnerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+
+ // Verify that the change is found if reviewer queries for changes reviewed by their secondary
+ // email.
+ requestScopeOperations.setApiUser(reviewer);
+ assertThat(gApi.changes().query("reviewer:" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ assertThat(gApi.changes().query("label:Code-Review+1,user=" + secondaryReviewerEmail).get())
+ .comparingElementsUsing(hasChangeId())
+ .containsExactly(changeId);
+ }
+
private static void assertNoChangeHasMoreChangesSet(List<ChangeInfo> results) {
for (ChangeInfo info : results) {
assertThat(info._moreChanges).isNull();
@@ -383,4 +667,9 @@
}
}
}
+
+ private static Correspondence<ChangeInfo, Change.Id> hasChangeId() {
+ return NullAwareCorrespondence.transforming(
+ changeInfo -> Change.id(changeInfo._number), "hasChangeId");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index 3531234..b7b55ee 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -322,8 +322,8 @@
testRepo.reset("HEAD~1");
PushOneCommit.Result r2 = createChange();
- ChangeInfo ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
+ RevisionInfo ri2 =
+ get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
Change.Id id1 = r1.getChange().getId();
@@ -332,8 +332,7 @@
rebaseCallWithInput.call(r2.getChangeId(), in);
Change.Id id2 = r2.getChange().getId();
- ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- ri2 = ci2.revisions.get(ci2.currentRevision);
+ ri2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
List<RelatedChangeAndCommitInfo> related =
@@ -352,8 +351,8 @@
testRepo.reset("HEAD~1");
PushOneCommit.Result r2 = createChange();
- ChangeInfo ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
+ RevisionInfo ri2 =
+ get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
// Submit first change.
@@ -367,8 +366,7 @@
rebaseCallWithInput.call(r2.getChangeId(), in);
Change.Id id2 = r2.getChange().getId();
- ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- ri2 = ci2.revisions.get(ci2.currentRevision);
+ ri2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(gApi.changes().id(id2.get()).revision(ri2._number).related().changes).isEmpty();
@@ -539,7 +537,7 @@
ChangeInfo info =
gApi.changes().id(changeId.get()).get(CURRENT_REVISION, CURRENT_COMMIT, DETAILED_LABELS);
- RevisionInfo r = info.revisions.get(info.currentRevision);
+ RevisionInfo r = info.getCurrentRevision();
assertThat(r._number).isEqualTo(expectedNumRevisions);
assertThat(r.realUploader).isNull();
@@ -550,10 +548,10 @@
.isEqualTo(baseCommit);
// ...and the committer and description should be correct
- GitPerson committer = info.revisions.get(info.currentRevision).commit.committer;
+ GitPerson committer = r.commit.committer;
assertThat(committer.name).isEqualTo(admin.fullName());
assertThat(committer.email).isEqualTo(admin.email());
- String description = info.revisions.get(info.currentRevision).description;
+ String description = r.description;
assertThat(description).isEqualTo("Rebase");
if (shouldHaveApproval) {
@@ -676,7 +674,7 @@
ChangeInfo changeInfo =
gApi.changes().id(changeId).get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
assertThat(changeInfo.revisions).hasSize(2);
- assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+ assertThat(changeInfo.getCurrentRevision().commit.parents.get(0).commit)
.isEqualTo(base.name());
// Verify that the file content in the created patch set is correct.
@@ -767,8 +765,8 @@
rebaseCallWithInput.call(r3.getChangeId(), in);
Change.Id id3 = r3.getChange().getId();
- ChangeInfo ci3 = get(r3.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
- RevisionInfo ri3 = ci3.revisions.get(ci3.currentRevision);
+ RevisionInfo ri3 =
+ get(r3.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT).getCurrentRevision();
assertThat(ri3.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(gApi.changes().id(id3.get()).revision(ri3._number).related().changes).isEmpty();
@@ -843,22 +841,20 @@
fileName2,
fileContent2Ps2)
.assertOkStatus();
- ChangeInfo changeInfo2 = gApi.changes().id(changeId2).get();
- assertThat(changeInfo2.revisions.get(changeInfo2.currentRevision)._number).isEqualTo(2);
+ assertThat(gApi.changes().id(changeId2).get().getCurrentRevision()._number).isEqualTo(2);
// Rebase the first patch set of the second change
gApi.changes().id(changeId2).revision(1).rebase();
// Second change should have 3 patch sets
- changeInfo2 = gApi.changes().id(changeId2).get();
- assertThat(changeInfo2.revisions.get(changeInfo2.currentRevision)._number).isEqualTo(3);
+ assertThat(gApi.changes().id(changeId2).get().getCurrentRevision()._number).isEqualTo(3);
// ... and the committer and description should be correct
ChangeInfo info = gApi.changes().id(changeId2).get(CURRENT_REVISION, CURRENT_COMMIT);
- GitPerson committer = info.revisions.get(info.currentRevision).commit.committer;
+ GitPerson committer = info.getCurrentRevision().commit.committer;
assertThat(committer.name).isEqualTo(admin.fullName());
assertThat(committer.email).isEqualTo(admin.email());
- String description = info.revisions.get(info.currentRevision).description;
+ String description = info.getCurrentRevision().description;
assertThat(description).isEqualTo("Rebase");
// ... and the file contents should match with patch set 1 based on change1
@@ -936,8 +932,13 @@
verifyChangeIsUpToDate(r4);
// r5 wasn't rebased.
- ChangeInfo r5info = gApi.changes().id(r5.getChangeId()).get(CURRENT_REVISION);
- assertThat(r5info.revisions.get(r5info.currentRevision)._number).isEqualTo(1);
+ assertThat(
+ gApi.changes()
+ .id(r5.getChangeId())
+ .get(CURRENT_REVISION)
+ .getCurrentRevision()
+ ._number)
+ .isEqualTo(1);
// Rebasing r5
verifyRebaseChainResponse(
@@ -1081,7 +1082,7 @@
.id(changeWithConflictId)
.get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
assertThat(changeInfo.revisions).hasSize(2);
- assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+ assertThat(changeInfo.getCurrentRevision().commit.parents.get(0).commit)
.isEqualTo(base.name());
// Verify that the file content in the created patch set is correct.
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
index 5a5402b..9219ede 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseOnBehalfOfUploaderIT.java
@@ -215,7 +215,7 @@
}
ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 2 patch sets before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -278,7 +278,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -297,7 +297,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 2 patch sets before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -317,7 +317,7 @@
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ currentRevisionInfo = changeInfo2.getCurrentRevision();
// The change had 3 patch sets before the rebase, now it should be 4
assertThat(currentRevisionInfo._number).isEqualTo(4);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -432,8 +432,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -531,8 +531,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 2 patch set before the rebase, now it should be 3
assertThat(currentRevisionInfo._number).isEqualTo(3);
assertThat(currentRevisionInfo.uploader._accountId).isEqualTo(uploader.get());
@@ -632,8 +632,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.author.email).isEqualTo(authorEmail);
@@ -720,8 +720,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.committer.email).isEqualTo(uploaderEmail);
@@ -773,8 +773,8 @@
rebaseInput.onBehalfOfUploader = true;
gApi.changes().id(changeToBeRebased.get()).rebase(rebaseInput);
- ChangeInfo changeInfo2 = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo currentRevisionInfo = changeInfo2.revisions.get(changeInfo2.currentRevision);
+ RevisionInfo currentRevisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
// The change had 1 patch set before the rebase, now it should be 2
assertThat(currentRevisionInfo._number).isEqualTo(2);
assertThat(currentRevisionInfo.commit.author.email)
@@ -871,8 +871,8 @@
gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
requestScopeOperations.setApiUser(rebaser);
- ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ RevisionInfo revisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
assertThat(revisionInfo.actions).containsKey("rebase");
ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
assertThat(rebaseActionInfo.enabled).isTrue();
@@ -900,8 +900,8 @@
gApi.changes().id(changeToBeTheNewBase.get()).current().submit();
requestScopeOperations.setApiUser(changeOwner);
- ChangeInfo changeInfo = gApi.changes().id(changeToBeRebased.get()).get();
- RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+ RevisionInfo revisionInfo =
+ gApi.changes().id(changeToBeRebased.get()).get().getCurrentRevision();
assertThat(revisionInfo.actions).containsKey("rebase");
ActionInfo rebaseActionInfo = revisionInfo.actions.get("rebase");
assertThat(rebaseActionInfo.enabled).isTrue();
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
index c89e11a..b21f97d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/GetAccountDetailIT.java
@@ -91,16 +91,26 @@
Account.Id id =
accountOperations
.newAccount()
- .preferredEmail("preferred@email")
- .addSecondaryEmail("secondary@email")
+ .preferredEmail("preferred@eexample.com")
+ .addSecondaryEmail("secondary@example.com")
.create();
+
RestResponse r = userRestSession.get("/accounts/secondary/detail/");
r.assertStatus(404);
+
+ r = userRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(404);
+
// The admin has MODIFY_ACCOUNT permission and can see the user.
r = adminRestSession.get("/accounts/secondary/detail/");
r.assertStatus(200);
AccountDetailInfo info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
assertThat(info._accountId).isEqualTo(id.get());
+
+ r = adminRestSession.get("/accounts/secondary@example.com/detail/");
+ r.assertStatus(200);
+ info = newGson().fromJson(r.getReader(), AccountDetailInfo.class);
+ assertThat(info._accountId).isEqualTo(id.get());
}
private static class CustomAccountTagProvider implements AccountTagProvider {
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index cf1eee0..432a6c6 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -450,8 +450,18 @@
.to("refs/for/master");
r.assertOkStatus();
- // assert no email notifications for user
- assertThat(sender.getMessages()).isEmpty();
+ // Assert email notification for user.
+ // The non-visible account participated in a change that is visible to user, hence through this
+ // change user can see the non-visible account.
+ // Even if watching by the non-visible account was not possible, user could just watch all
+ // changes that are visible to them and then filter them by the non-visible account locally.
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
index 3b97372..5bdf91f 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java
@@ -24,6 +24,7 @@
import static com.google.gerrit.truth.MapSubject.assertThatMap;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.truth.Correspondence;
@@ -37,6 +38,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeType;
@@ -147,6 +149,124 @@
}
@Test
+ public void createdChangeHasDefaultGroupsByDefault() throws Exception {
+ Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
+ Change.Id changeId =
+ changeOperations.newChange().project(project).branch("test-branch").create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfBranchTipIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .tipOfBranch("refs/heads/test-branch")
+ .create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasSameGroupsAsOpenParentChange() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+
+ ChangeInfo parentChange = getChangeFromServer(parentChangeId);
+ ImmutableList<String> parentGroups = getGroups(project, parentChangeId);
+ assertThat(parentGroups).containsExactly(parentChange.currentRevision);
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().change(parentChangeId).create();
+
+ assertThat(getGroups(project, changeId)).isEqualTo(parentGroups);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfClosedChangeIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ gApi.changes().id(parentChangeId.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(parentChangeId.get()).current().submit();
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().change(parentChangeId).create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasSameGroupsAsPatchSetOfOpenParentChange() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ TestPatchset parentPatchset = changeOperations.change(parentChangeId).currentPatchset().get();
+ changeOperations.change(parentChangeId).newPatchset().create();
+
+ ImmutableList<String> parentGroups = getGroups(project, parentPatchset.patchsetId());
+ assertThat(parentGroups).containsExactly(parentPatchset.commitId().name());
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .patchset(parentPatchset.patchsetId())
+ .create();
+
+ assertThat(getGroups(project, changeId)).isEqualTo(parentGroups);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfPatchSetOfClosedChangeIsSpecifiedAsParent()
+ throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ Change.Id parentChangeId = changeOperations.newChange().project(project).create();
+ TestPatchset parentPatchset = changeOperations.change(parentChangeId).currentPatchset().get();
+ changeOperations.change(parentChangeId).newPatchset().create();
+ gApi.changes().id(parentChangeId.get()).current().review(ReviewInput.approve());
+ gApi.changes().id(parentChangeId.get()).current().submit();
+
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .childOf()
+ .patchset(parentPatchset.patchsetId())
+ .create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
+ public void createdChangeHasDefaultGroupsIfCommitIsSpecifiedAsParent() throws Exception {
+ Project.NameKey project = projectOperations.newProject().create();
+
+ // Currently, the easiest way to create a commit is by creating another change.
+ Change.Id anotherChangeId = changeOperations.newChange().project(project).create();
+ ObjectId parentCommitId =
+ changeOperations.change(anotherChangeId).currentPatchset().get().commitId();
+
+ Change.Id changeId =
+ changeOperations.newChange().project(project).childOf().commit(parentCommitId).create();
+
+ ChangeInfo change = getChangeFromServer(changeId);
+ assertThat(getGroups(project, changeId)).containsExactly(change.currentRevision);
+ }
+
+ @Test
public void createdChangeUsesTipOfTargetBranchAsParentByDefault() throws Exception {
Project.NameKey project = projectOperations.newProject().branches("test-branch").create();
ObjectId parentCommitId = projectOperations.project(project).getHead("test-branch").getId();
@@ -1634,6 +1754,17 @@
return gApi.changes().id(changeId.get()).revision(patchsetId.get()).file(filePath).content();
}
+ private ImmutableList<String> getGroups(Project.NameKey projectName, Change.Id changeId) {
+ return changeDataFactory.create(projectName, changeId).currentPatchSet().groups();
+ }
+
+ private ImmutableList<String> getGroups(Project.NameKey projectName, PatchSet.Id patchSetId) {
+ return changeDataFactory
+ .create(projectName, patchSetId.changeId())
+ .patchSet(patchSetId)
+ .groups();
+ }
+
private Correspondence<CommitInfo, String> hasSha1() {
return NullAwareCorrespondence.transforming(commitInfo -> commitInfo.commit, "hasSha1");
}
diff --git a/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java b/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
index e1cccf8..9f8f494 100644
--- a/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
+++ b/javatests/com/google/gerrit/httpd/raw/IndexPreloadingUtilTest.java
@@ -52,6 +52,7 @@
@Test
public void preloadOnlyForSelfDashboard() throws Exception {
assertThat(parseRequestedPage("/dashboard/self")).isEqualTo(RequestedPage.DASHBOARD);
+ assertThat(parseRequestedPage("/profile/self")).isEqualTo(RequestedPage.PROFILE);
assertThat(parseRequestedPage("/dashboard/1085901"))
.isEqualTo(RequestedPage.PAGE_WITHOUT_PRELOADING);
assertThat(parseRequestedPage("/dashboard/gerrit"))
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
index 41ad3f8..a11951d 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
@@ -11,7 +11,6 @@
import '../../shared/gr-list-view/gr-list-view';
import '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import '../gr-confirm-delete-item-dialog/gr-confirm-delete-item-dialog';
-import {encodeURL} from '../../../utils/url-util';
import {GrCreatePointerDialog} from '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import {
BranchInfo,
@@ -28,12 +27,16 @@
import {formStyles} from '../../../styles/gr-form-styles';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
-import {LitElement, PropertyValues, css, html} from 'lit';
+import {LitElement, PropertyValues, css, html, nothing} from 'lit';
import {customElement, query, property, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {assertIsDefined} from '../../../utils/common-util';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {RepoDetailView, RepoViewState} from '../../../models/views/repo';
+import {
+ createRepoUrl,
+ RepoDetailView,
+ RepoViewState,
+} from '../../../models/views/repo';
import {modalStyles} from '../../../styles/gr-modal-styles';
const PGP_START = '-----BEGIN PGP SIGNATURE-----';
@@ -132,6 +135,8 @@
}
override render() {
+ if (!this.repo) return nothing;
+ if (!this.detailType) return nothing;
return html`
<gr-list-view
.createNew=${this.loggedIn}
@@ -140,7 +145,7 @@
.items=${this.items}
.loading=${this.loading}
.offset=${this.offset}
- .path=${this.getPath(this.repo, this.detailType)}
+ .path=${this.getPath()}
@create-clicked=${() => {
this.handleCreateClicked();
}}
@@ -434,11 +439,10 @@
return Promise.reject(new Error('unknown detail type'));
}
- private getPath(repo?: RepoName, detailType?: RepoDetailView) {
- // TODO: Replace with `createRepoUrl()`, but be aware that `encodeURL()`
- // gets `false` as a second parameter here. The router pattern in gr-router
- // does not handle the filter URLs, if the repo is not encoded!
- return `/admin/repos/${encodeURL(repo ?? '')},${detailType}`;
+ private getPath() {
+ if (!this.repo) return '';
+ if (!this.detailType) return '';
+ return createRepoUrl({repo: this.repo, detail: this.detailType});
}
private computeWeblink(repo: ProjectInfo | BranchInfo | TagInfo) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 36ac307..ccb750d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -47,7 +47,7 @@
isQuickLabelInfo,
LabelInfo,
NumericChangeId,
- PatchSetNum,
+ PatchSetNumber,
RequestPayload,
RevertSubmissionInfo,
ReviewInput,
@@ -110,6 +110,7 @@
import {whenVisible} from '../../../utils/dom-util';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {subscribe} from '../../lit/subscription-controller';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -410,8 +411,7 @@
@property({type: Boolean})
hasParent?: boolean;
- @property({type: String})
- latestPatchNum?: PatchSetNum;
+ @state() latestPatchNum?: PatchSetNumber;
@property({type: String})
commitMessage = '';
@@ -533,6 +533,11 @@
constructor() {
super();
+ subscribe(
+ this,
+ () => this.getChangeModel().latestPatchNum$,
+ x => (this.latestPatchNum = x)
+ );
}
override connectedCallback() {
@@ -1327,7 +1332,7 @@
}
// private but used in test
- getRevision(change: ChangeViewChangeInfo, patchNum?: PatchSetNum) {
+ getRevision(change: ChangeViewChangeInfo, patchNum?: PatchSetNumber) {
for (const rev of Object.values(change.revisions)) {
if (rev._number === patchNum) {
return rev;
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 82900a3..9c01f2f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -34,7 +34,6 @@
ChangeSubmissionId,
CommitId,
NumericChangeId,
- PatchSetNum,
PatchSetNumber,
RepoName,
ReviewInput,
@@ -135,7 +134,7 @@
},
};
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
element.account = {
_account_id: 123 as AccountId,
};
@@ -514,7 +513,7 @@
rev2: {...createRevision(), _number: 2 as PatchSetNumber},
},
};
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
queryAndAssert<GrButton>(
element,
@@ -1808,7 +1807,7 @@
element.change!.is_private = false;
element.changeNum = 2 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
await element.updateComplete;
await element.reload();
@@ -1862,7 +1861,7 @@
element.change!.is_private = true;
element.changeNum = 2 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
await element.updateComplete;
await element.reload();
@@ -2331,7 +2330,7 @@
const reloadStub = sinon.stub(element, 'reload');
element.changeNum = 123 as NumericChangeId;
assert.isFalse(reloadStub.called);
- element.latestPatchNum = 456 as PatchSetNum;
+ element.latestPatchNum = 456 as PatchSetNumber;
assert.isFalse(reloadStub.called);
});
@@ -2427,7 +2426,7 @@
setup(async () => {
cleanup = sinon.stub();
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 12 as PatchSetNum;
+ element.latestPatchNum = 12 as PatchSetNumber;
element.change = {
...createChangeViewChange(),
revisions: createRevisions(element.latestPatchNum as number),
@@ -2695,7 +2694,7 @@
// set the following properties
element.change = createChangeViewChange();
element.changeNum = 42 as NumericChangeId;
- element.latestPatchNum = 2 as PatchSetNum;
+ element.latestPatchNum = 2 as PatchSetNumber;
stubRestApi('getRepoBranches').returns(Promise.resolve([]));
await element.updateComplete;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 2246671..d07a6ac 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1345,7 +1345,6 @@
.changeNum=${this.changeNum}
.changeStatus=${this.change?.status}
.commitNum=${this.commitInfo?.commit}
- .latestPatchNum=${computeLatestPatchNum(this.allPatchSets)}
.commitMessage=${this.latestCommitMessage}
.editPatchsetLoaded=${this.patchRange
? hasEditPatchsetLoaded(this.patchRange)
