Merge changes Ia856d964,I99fd4433,Ibd694fc2,I896edd3a
* changes:
CodeOwnerConfigScanner#containsAnyCodeOwnerConfigFile: Handle invalid files
CodeOwnerApprovalCheck: Fix exception when no old path is set for deletion
CodeOwnerApprovalCheck: Return 409 Conflict if destination branch was deleted
Avoid loading all accounts when code ownership is assigned to all users
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
index 3ead395..82655cf 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFile.java
@@ -137,6 +137,15 @@
public static ChangedFile create(PatchListEntry patchListEntry) {
requireNonNull(patchListEntry, "patchListEntry");
+ if (patchListEntry.getChangeType() == Patch.ChangeType.DELETED) {
+ // For deletions PatchListEntry sets the old path as new name and the old name is unset (see
+ // PatchListEntry constructor). This means to get the old path we need to read the new name.
+ return new AutoValue_ChangedFile(
+ Optional.empty(),
+ convertPathFromPatchListEntry(patchListEntry.getNewName()),
+ CHANGE_TYPE.get(patchListEntry.getChangeType()));
+ }
+
return new AutoValue_ChangedFile(
convertPathFromPatchListEntry(patchListEntry.getNewName()),
convertPathFromPatchListEntry(patchListEntry.getOldName()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 4ed19ce..5f55f62 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -14,7 +14,6 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Objects.requireNonNull;
@@ -27,6 +26,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.RequiredApproval;
@@ -106,7 +106,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -156,7 +156,7 @@
* returned
*/
public Stream<FileCodeOwnerStatus> getFileStatuses(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (TraceTimer traceTimer =
TraceContext.newTimer(
@@ -187,15 +187,13 @@
"patchSetUploader = %d, implicit approval from uploader is %s",
patchSetUploader.get(), enableImplicitApprovalFromUploader ? "enabled" : "disabled");
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds =
+ CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver
.get()
.enforceVisibility(false)
.resolve(
- codeOwnersPluginConfiguration.getGlobalCodeOwners(changeNotes.getProjectName()))
- .map(CodeOwner::accountId)
- .collect(toImmutableSet());
- logger.atFine().log("global code owner accounts = %s", globalCodeOwnerAccountIds);
+ codeOwnersPluginConfiguration.getGlobalCodeOwners(changeNotes.getProjectName()));
+ logger.atFine().log("global code owners = %s", globalCodeOwners);
// If the branch doesn't contain any code owner config file yet, we apply special logic
// (project
@@ -217,7 +215,7 @@
getFileStatus(
branch,
revision,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
enableImplicitApprovalFromUploader,
patchSetUploader,
reviewerAccountIds,
@@ -231,7 +229,7 @@
private FileCodeOwnerStatus getFileStatus(
BranchNameKey branch,
ObjectId revision,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -250,7 +248,7 @@
getPathCodeOwnerStatus(
branch,
revision,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
enableImplicitApprovalFromUploader,
patchSetUploader,
reviewerAccountIds,
@@ -271,7 +269,7 @@
getPathCodeOwnerStatus(
branch,
revision,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
enableImplicitApprovalFromUploader,
patchSetUploader,
reviewerAccountIds,
@@ -290,7 +288,7 @@
private PathCodeOwnerStatus getPathCodeOwnerStatus(
BranchNameKey branch,
ObjectId revision,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -310,7 +308,7 @@
return isBootstrapping
? getPathCodeOwnerStatusBootstrappingMode(
branch,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
enableImplicitApprovalFromUploader,
patchSetUploader,
reviewerAccountIds,
@@ -318,7 +316,7 @@
absolutePath)
: getPathCodeOwnerStatusRegularMode(
branch,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
enableImplicitApprovalFromUploader,
patchSetUploader,
revision,
@@ -336,7 +334,7 @@
*/
private PathCodeOwnerStatus getPathCodeOwnerStatusBootstrappingMode(
BranchNameKey branch,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -348,13 +346,13 @@
if (isApprovedBootstrappingMode(
branch.project(),
absolutePath,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
approverAccountIds,
enableImplicitApprovalFromUploader,
patchSetUploader)) {
codeOwnerStatus = CodeOwnerStatus.APPROVED;
} else if (isPendingBootstrappingMode(
- branch.project(), absolutePath, globalCodeOwnerAccountIds, reviewerAccountIds)) {
+ branch.project(), absolutePath, globalCodeOwners, reviewerAccountIds)) {
codeOwnerStatus = CodeOwnerStatus.PENDING;
}
@@ -367,21 +365,21 @@
private boolean isApprovedBootstrappingMode(
Project.NameKey projectName,
Path absolutePath,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader) {
return (enableImplicitApprovalFromUploader
&& isImplicitlyApprovedBootstrappingMode(
- projectName, absolutePath, globalCodeOwnerAccountIds, patchSetUploader))
+ projectName, absolutePath, globalCodeOwners, patchSetUploader))
|| isExplicitlyApprovedBootstrappingMode(
- projectName, absolutePath, globalCodeOwnerAccountIds, approverAccountIds);
+ projectName, absolutePath, globalCodeOwners, approverAccountIds);
}
private boolean isImplicitlyApprovedBootstrappingMode(
Project.NameKey projectName,
Path absolutePath,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
Account.Id patchSetUploader) {
if (isProjectOwner(projectName, patchSetUploader)) {
// The uploader of the patch set is a project owner and thus a code owner. This means there
@@ -393,7 +391,8 @@
return true;
}
- if (globalCodeOwnerAccountIds.contains(patchSetUploader)) {
+ if (globalCodeOwners.ownedByAllUsers()
+ || globalCodeOwners.codeOwnersAccountIds().contains(patchSetUploader)) {
// If the uploader of the patch set is a global code owner, there is an implicit code owner
// approval from the patch set uploader so that the path is automatically approved.
logger.atFine().log(
@@ -408,9 +407,10 @@
private boolean isExplicitlyApprovedBootstrappingMode(
Project.NameKey projectName,
Path absolutePath,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> approverAccountIds) {
- if (!Collections.disjoint(approverAccountIds, globalCodeOwnerAccountIds)) {
+ if (!Collections.disjoint(approverAccountIds, globalCodeOwners.codeOwnersAccountIds())
+ || (globalCodeOwners.ownedByAllUsers() && !approverAccountIds.isEmpty())) {
// At least one of the global code owners approved the change.
logger.atFine().log("%s was approved by a global code owner", absolutePath);
return true;
@@ -429,7 +429,7 @@
private boolean isPendingBootstrappingMode(
Project.NameKey projectName,
Path absolutePath,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> reviewerAccountIds) {
if (reviewerAccountIds.stream()
.anyMatch(reviewerAccountId -> isProjectOwner(projectName, reviewerAccountId))) {
@@ -438,7 +438,7 @@
return true;
}
- if (isPending(absolutePath, globalCodeOwnerAccountIds, reviewerAccountIds)) {
+ if (isPending(absolutePath, globalCodeOwners, reviewerAccountIds)) {
// At least one of the reviewers is a global code owner.
logger.atFine().log("%s is owned by a reviewer who is a global owner", absolutePath);
return true;
@@ -453,7 +453,7 @@
*/
private PathCodeOwnerStatus getPathCodeOwnerStatusRegularMode(
BranchNameKey branch,
- ImmutableSet<Account.Id> globalCodeOwnerAccountIds,
+ CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader,
ObjectId revision,
@@ -467,7 +467,7 @@
if (isApproved(
absolutePath,
- globalCodeOwnerAccountIds,
+ globalCodeOwners,
approverAccountIds,
enableImplicitApprovalFromUploader,
patchSetUploader)) {
@@ -476,7 +476,7 @@
} else {
logger.atFine().log("%s was not approved by a global code owner", absolutePath);
- if (isPending(absolutePath, globalCodeOwnerAccountIds, reviewerAccountIds)) {
+ if (isPending(absolutePath, globalCodeOwners, reviewerAccountIds)) {
logger.atFine().log("%s is owned by a reviewer who is a global owner", absolutePath);
codeOwnerStatus.set(CodeOwnerStatus.PENDING);
}
@@ -486,23 +486,22 @@
revision,
absolutePath,
codeOwnerConfig -> {
- ImmutableSet<Account.Id> codeOwnerAccountIds =
- getCodeOwnerAccountIds(codeOwnerConfig, absolutePath);
+ CodeOwnerResolverResult codeOwners = getCodeOwners(codeOwnerConfig, absolutePath);
logger.atFine().log(
"code owners = %s (code owner config folder path = %s, file name = %s)",
- codeOwnerAccountIds,
+ codeOwners,
codeOwnerConfig.key().folderPath(),
codeOwnerConfig.key().fileName().orElse("<default>"));
if (isApproved(
absolutePath,
- codeOwnerAccountIds,
+ codeOwners,
approverAccountIds,
enableImplicitApprovalFromUploader,
patchSetUploader)) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
return false;
- } else if (isPending(absolutePath, codeOwnerAccountIds, reviewerAccountIds)) {
+ } else if (isPending(absolutePath, codeOwners, reviewerAccountIds)) {
codeOwnerStatus.set(CodeOwnerStatus.PENDING);
// We need to continue to check if any of the higher-level code owners approved the
@@ -524,18 +523,21 @@
private boolean isApproved(
Path absolutePath,
- ImmutableSet<Account.Id> codeOwnerAccountIds,
+ CodeOwnerResolverResult codeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
Account.Id patchSetUploader) {
- if (enableImplicitApprovalFromUploader && codeOwnerAccountIds.contains(patchSetUploader)) {
+ if (enableImplicitApprovalFromUploader
+ && (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
+ || codeOwners.ownedByAllUsers())) {
// If the uploader of the patch set owns the path, there is an implicit code owner
// approval from the patch set uploader so that the path is automatically approved.
logger.atFine().log("%s was implicitly approved by the patch set uploader", absolutePath);
return true;
}
- if (!Collections.disjoint(approverAccountIds, codeOwnerAccountIds)) {
+ if (!Collections.disjoint(approverAccountIds, codeOwners.codeOwnersAccountIds())
+ || (codeOwners.ownedByAllUsers() && !approverAccountIds.isEmpty())) {
// At least one of the global code owners approved the change.
logger.atFine().log("%s was explicitly approved by a code owner", absolutePath);
return true;
@@ -546,9 +548,10 @@
private boolean isPending(
Path absolutePath,
- ImmutableSet<Account.Id> codeOwnerAccountIds,
+ CodeOwnerResolverResult codeOwners,
ImmutableSet<Account.Id> reviewerAccountIds) {
- if (!Collections.disjoint(codeOwnerAccountIds, reviewerAccountIds)) {
+ if (!Collections.disjoint(codeOwners.codeOwnersAccountIds(), reviewerAccountIds)
+ || (codeOwners.ownedByAllUsers() && !reviewerAccountIds.isEmpty())) {
logger.atFine().log("%s is owned by a reviewer", absolutePath);
return true;
}
@@ -578,17 +581,17 @@
}
/**
- * Gets the IDs of the accounts that own the given path according to the given code owner config.
+ * Gets the code owners that own the given path according to the given code owner config.
*
* @param codeOwnerConfig the code owner config from which the code owners should be retrieved
* @param absolutePath the path for which the code owners should be retrieved
*/
- private ImmutableSet<Account.Id> getCodeOwnerAccountIds(
+ private CodeOwnerResolverResult getCodeOwners(
CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
- return codeOwnerResolver.get().enforceVisibility(false)
- .resolvePathCodeOwners(codeOwnerConfig, absolutePath).stream()
- .map(CodeOwner::accountId)
- .collect(toImmutableSet());
+ return codeOwnerResolver
+ .get()
+ .enforceVisibility(false)
+ .resolvePathCodeOwners(codeOwnerConfig, absolutePath);
}
/**
@@ -633,16 +636,18 @@
*
* <p>This is the revision from which the code owner configs should be read when computing code
* owners for the files that are touched in the change.
+ *
+ * @throws ResourceConflictException thrown if the destination branch is not found, e.g. when the
+ * branch got deleted after the change was created
*/
- private ObjectId getDestBranchRevision(Change change) throws IOException {
+ private ObjectId getDestBranchRevision(Change change)
+ throws IOException, ResourceConflictException {
try (Repository repository = repoManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repository)) {
Ref ref = repository.exactRef(change.getDest().branch());
- checkNotNull(
- ref,
- "branch %s in repository %s not found",
- change.getDest().branch(),
- change.getProject().get());
+ if (ref == null) {
+ throw new ResourceConflictException("destination branch not found");
+ }
return rw.parseCommit(ref.getObjectId());
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
index 99ee653..06bd4ef 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -74,7 +74,7 @@
found.set(true);
return false;
},
- ignoreInvalidCodeOwnerConfigFiles());
+ (codeOwnerConfigFilePath, configInvalidException) -> found.set(true));
return found.get();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index b53afb2..fdd4da2 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -29,7 +29,6 @@
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.logging.Metadata;
@@ -44,6 +43,7 @@
import java.nio.file.Path;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Stream;
/** Class to resolve {@link CodeOwnerReference}s to {@link CodeOwner}s. */
@@ -57,7 +57,6 @@
private final Provider<CurrentUser> currentUser;
private final ExternalIds externalIds;
private final AccountCache accountCache;
- private final Accounts accounts;
private final AccountControl.Factory accountControlFactory;
private final PathCodeOwners.Factory pathCodeOwnersFactory;
@@ -76,7 +75,6 @@
Provider<CurrentUser> currentUser,
ExternalIds externalIds,
AccountCache accountCache,
- Accounts accounts,
AccountControl.Factory accountControlFactory,
PathCodeOwners.Factory pathCodeOwnersFactory) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
@@ -84,7 +82,6 @@
this.currentUser = currentUser;
this.externalIds = externalIds;
this.accountCache = accountCache;
- this.accounts = accounts;
this.accountControlFactory = accountControlFactory;
this.pathCodeOwnersFactory = pathCodeOwnersFactory;
}
@@ -123,6 +120,12 @@
return this;
}
+ /** Whether the given code owner reference can be resolved. */
+ public boolean isResolvable(CodeOwnerReference codeOwnerReference) {
+ CodeOwnerResolverResult result = resolve(ImmutableSet.of(codeOwnerReference));
+ return !result.codeOwners().isEmpty() || result.ownedByAllUsers();
+ }
+
/**
* Resolves the code owners from the given code owner config for the given path from {@link
* CodeOwnerReference}s to a {@link CodeOwner}s.
@@ -135,7 +138,7 @@
* absolute; can be the path of a file or folder; the path may or may not exist
* @return the resolved code owners
*/
- public ImmutableSet<CodeOwner> resolvePathCodeOwners(
+ public CodeOwnerResolverResult resolvePathCodeOwners(
CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
requireNonNull(absolutePath, "absolutePath");
@@ -150,9 +153,7 @@
.filePath(codeOwnerConfig.key().fileName().orElse("<default>"))
.build())) {
logger.atFine().log("resolving path code owners for path %s", absolutePath);
- return pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).get().stream()
- .flatMap(this::resolve)
- .collect(toImmutableSet());
+ return resolve(pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).get());
}
}
@@ -163,9 +164,22 @@
* @return the {@link CodeOwner} for the given code owner references
* @see #resolve(CodeOwnerReference)
*/
- public Stream<CodeOwner> resolve(Set<CodeOwnerReference> codeOwnerReferences) {
+ public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
- return codeOwnerReferences.stream().flatMap(this::resolve);
+ AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
+ ImmutableSet<CodeOwner> codeOwners =
+ codeOwnerReferences.stream()
+ .filter(
+ codeOwnerReference -> {
+ if (ALL_USERS_WILDCARD.equals(codeOwnerReference.email())) {
+ ownedByAllUsers.set(true);
+ return false;
+ }
+ return true;
+ })
+ .flatMap(this::resolve)
+ .collect(toImmutableSet());
+ return CodeOwnerResolverResult.create(codeOwners, ownedByAllUsers.get());
}
/**
@@ -201,18 +215,18 @@
* all accounts
* </ul>
*
+ * <p>This method does not resolve {@link CodeOwnerReference}s that assign the code ownership to
+ * all user by using {@link #ALL_USERS_WILDCARD} as email.
+ *
* @param codeOwnerReference the code owner reference that should be resolved
* @return the {@link CodeOwner} for the code owner reference if it was resolved, otherwise {@link
* Optional#empty()}
*/
+ @VisibleForTesting
public Stream<CodeOwner> resolve(CodeOwnerReference codeOwnerReference) {
String email = requireNonNull(codeOwnerReference, "codeOwnerReference").email();
logger.atFine().log("resolving code owner reference %s", codeOwnerReference);
- if (ALL_USERS_WILDCARD.equals(email)) {
- return resolveAllUsersWildcard();
- }
-
if (!isEmailDomainAllowed(email)) {
logger.atFine().log("domain of email %s is not allowed", email);
return Stream.of();
@@ -239,18 +253,6 @@
return Stream.of(codeOwner);
}
- private Stream<CodeOwner> resolveAllUsersWildcard() {
- try (TraceTimer traceTimer =
- TraceContext.newTimer("Resolve all users wildcard", Metadata.builder().build())) {
- return accounts.all().stream()
- .filter(accountState -> !enforceVisibility || canSee(accountState))
- .map(accountState -> CodeOwner.create(accountState.account().id()));
- } catch (IOException e) {
- throw new StorageException(
- String.format("cannot resolve code owner email %s", ALL_USERS_WILDCARD), e);
- }
- }
-
/** Whether the given account can be seen. */
private boolean canSee(AccountState accountState) {
AccountControl accountControl =
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
new file mode 100644
index 0000000..226d541
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -0,0 +1,59 @@
+// Copyright (C) 2020 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.plugins.codeowners.backend;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.Account;
+
+/** The result of resolving code owner references via {@link CodeOwnerResolver}. */
+@AutoValue
+public abstract class CodeOwnerResolverResult {
+ /**
+ * Returns the resolved code owners as stream.
+ *
+ * <p>Doesn't include code owners to which the code ownership was assigned by using the {@link
+ * CodeOwnerResolver#ALL_USERS_WILDCARD}.
+ */
+ public abstract ImmutableSet<CodeOwner> codeOwners();
+
+ /** Returns the account IDs of the resolved code owners as set. */
+ public ImmutableSet<Account.Id> codeOwnersAccountIds() {
+ return codeOwners().stream().map(CodeOwner::accountId).collect(toImmutableSet());
+ }
+
+ /**
+ * Whether the code ownership was assigned to all users by using the {@link
+ * CodeOwnerResolver#ALL_USERS_WILDCARD}.
+ */
+ public abstract boolean ownedByAllUsers();
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("codeOwners", codeOwners())
+ .add("ownedByAllUsers", ownedByAllUsers())
+ .toString();
+ }
+
+ /** Creates a {@link CodeOwnerResolverResult} instance. */
+ public static CodeOwnerResolverResult create(
+ ImmutableSet<CodeOwner> codeOwners, boolean ownedByAllUsers) {
+ return new AutoValue_CodeOwnerResolverResult(codeOwners, ownedByAllUsers);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index c1a21a1..f179a9c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -22,6 +22,7 @@
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -98,7 +99,7 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index dd4fd6a..fcc5543 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -21,9 +21,12 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ListAccountsOption;
import com.google.gerrit.extensions.client.ListOption;
+import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
@@ -31,17 +34,21 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolverResult;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountDirectory.FillOptions;
import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.inject.Provider;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.EnumSet;
@@ -60,6 +67,9 @@
@VisibleForTesting public static final int DEFAULT_LIMIT = 10;
+ private final AccountVisibility accountVisibility;
+ private final Accounts accounts;
+ private final AccountControl.Factory accountControlFactory;
private final PermissionBackend permissionBackend;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
@@ -96,12 +106,18 @@
}
protected AbstractGetCodeOwnersForPath(
+ AccountVisibility accountVisibility,
+ Accounts accounts,
+ AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
ServiceUserClassifier serviceUserClassifier,
CodeOwnerJson.Factory codeOwnerJsonFactory) {
+ this.accountVisibility = accountVisibility;
+ this.accounts = accounts;
+ this.accountControlFactory = accountControlFactory;
this.permissionBackend = permissionBackend;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
@@ -134,12 +150,31 @@
rsrc.getRevision(),
rsrc.getPath(),
codeOwnerConfig -> {
- ImmutableSet<CodeOwner> pathCodeOwners =
+ CodeOwnerResolverResult pathCodeOwners =
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
- codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners));
+ codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners()));
+
+ if (pathCodeOwners.ownedByAllUsers()) {
+ fillUpWithRandomUsers(rsrc, codeOwners, limit);
+
+ if (codeOwners.size() < limit) {
+ logger.atFine().log(
+ "tried to fill up the suggestion list with random users,"
+ + " but didn't find enough visible accounts"
+ + " (wanted number of suggestions = %d, got = %d",
+ limit, codeOwners.size());
+ }
+
+ // We already found that the path is owned by all users. Hence we do not need to check
+ // if there are further code owners in higher-level code owner configs.
+ return false;
+ }
+
int distance = rootDistance - codeOwnerConfig.key().folderPath().getNameCount();
- pathCodeOwners.forEach(
- localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+ pathCodeOwners
+ .codeOwners()
+ .forEach(
+ localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
// If codeOwners.size() >= limit we have gathered enough code owners and do not need to
// look at further code owner configs.
@@ -150,10 +185,15 @@
});
if (codeOwners.size() < limit) {
- ImmutableSet<CodeOwner> globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project());
- globalCodeOwners.forEach(
- codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, maxDistance));
- codeOwners.addAll(filterCodeOwners(rsrc, globalCodeOwners));
+ CodeOwnerResolverResult globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project());
+ globalCodeOwners
+ .codeOwners()
+ .forEach(codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, maxDistance));
+ codeOwners.addAll(filterCodeOwners(rsrc, globalCodeOwners.codeOwners()));
+
+ if (globalCodeOwners.ownedByAllUsers()) {
+ fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ }
}
return Response.ok(
@@ -162,12 +202,11 @@
.format(sortAndLimit(distanceScoring.build(), ImmutableSet.copyOf(codeOwners))));
}
- private ImmutableSet<CodeOwner> getGlobalCodeOwners(Project.NameKey projectName) {
- ImmutableSet<CodeOwner> globalCodeOwners =
+ private CodeOwnerResolverResult getGlobalCodeOwners(Project.NameKey projectName) {
+ CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver
.get()
- .resolve(codeOwnersPluginConfiguration.getGlobalCodeOwners(projectName))
- .collect(toImmutableSet());
+ .resolve(codeOwnersPluginConfiguration.getGlobalCodeOwners(projectName));
logger.atFine().log("including global code owners = %s", globalCodeOwners);
return globalCodeOwners;
}
@@ -288,14 +327,85 @@
}
/**
- * Returns the given code owners in a random order.
+ * Returns the entries from the given set in a random order.
*
- * @param codeOwners the code owners that should be returned in a random order
- * @return the given code owners in a random order
+ * @param set the set for which the entries should be returned in a random order
+ * @return the entries from the given set in a random order
*/
- private static Stream<CodeOwner> randomizeOrder(ImmutableSet<CodeOwner> codeOwners) {
- List<CodeOwner> randomlyOrderedCodeOwners = new ArrayList<>(codeOwners);
+ private static <T> Stream<T> randomizeOrder(Set<T> set) {
+ List<T> randomlyOrderedCodeOwners = new ArrayList<>(set);
Collections.shuffle(randomlyOrderedCodeOwners);
return randomlyOrderedCodeOwners.stream();
}
+
+ /**
+ * If the limit is not reached yet, add random visible users as code owners to the given code
+ * owner set.
+ *
+ * <p>Must be only used to complete the suggestion list when it is found that the path is owned by
+ * all user.
+ */
+ private void fillUpWithRandomUsers(
+ AbstractPathResource rsrc, Set<CodeOwner> codeOwners, int limit) {
+ if (codeOwners.size() >= limit) {
+ // limit is already reach, we don't need to add further suggestions
+ return;
+ }
+
+ logger.atFine().log("filling up with random users");
+ codeOwners.addAll(
+ filterCodeOwners(
+ rsrc,
+ // ask for 2 times the number of users that we need so that we still have enough
+ // suggestions when some users are removed by the filterCodeOwners call or if the
+ // returned users were already present in codeOwners
+ getRandomVisibleUsers(2 * limit - codeOwners.size())
+ .map(CodeOwner::create)
+ .collect(toImmutableSet())));
+ }
+
+ /**
+ * Returns random visible users, at most as many as specified by the limit.
+ *
+ * <p>It's possible that this method returns less users than the limit although further visible
+ * users exist. This is because we may inspect only a random set of users, instead of all users,
+ * for performance reasons.
+ *
+ * @param limit the max number of users that should be returned
+ * @return random visible users
+ */
+ private Stream<Account.Id> getRandomVisibleUsers(int limit) {
+ try {
+ if (permissionBackend.currentUser().test(GlobalPermission.VIEW_ALL_ACCOUNTS)) {
+ return getRandomUsers(limit);
+ }
+
+ switch (accountVisibility) {
+ case ALL:
+ return getRandomUsers(limit);
+ case SAME_GROUP:
+ case VISIBLE_GROUP:
+ // We cannot afford to inspect all relevant users and test their visibility for
+ // performance reasons, hence we use a random sample of users that is 3 times the limit.
+ return getRandomUsers(3 * limit)
+ .filter(accountId -> accountControlFactory.get().canSee(accountId))
+ .limit(limit);
+ case NONE:
+ return Stream.of();
+ }
+
+ throw new IllegalStateException("unknown account visibility setting: " + accountVisibility);
+ } catch (IOException | PermissionBackendException e) {
+ throw new StorageException("failed to get visible users", e);
+ }
+ }
+
+ /**
+ * Returns random users, at most as many as specified by the limit.
+ *
+ * <p>No visibility check is performed.
+ */
+ private Stream<Account.Id> getRandomUsers(int limit) throws IOException {
+ return randomizeOrder(accounts.allIds()).limit(limit);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
index baf260d..6e3d054 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -26,6 +27,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.server.account.AccountControl;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -65,6 +68,9 @@
@Inject
GetCodeOwnersForPathInBranch(
+ AccountVisibility accountVisibility,
+ Accounts accounts,
+ AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
@@ -73,6 +79,9 @@
CodeOwnerJson.Factory codeOwnerJsonFactory,
GitRepositoryManager repoManager) {
super(
+ accountVisibility,
+ accounts,
+ accountControlFactory,
permissionBackend,
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 43a5d21..3483614 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.restapi;
+import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
@@ -21,6 +22,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.server.account.AccountControl;
+import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -40,6 +43,9 @@
implements RestReadView<CodeOwnersInChangeCollection.PathResource> {
@Inject
GetCodeOwnersForPathInChange(
+ AccountVisibility accountVisibility,
+ Accounts accounts,
+ AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
@@ -47,6 +53,9 @@
ServiceUserClassifier serviceUserClassifier,
CodeOwnerJson.Factory codeOwnerJsonFactory) {
super(
+ accountVisibility,
+ accounts,
+ accountControlFactory,
permissionBackend,
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 4e883c5..f0023ff 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -674,7 +674,7 @@
}
// Check if the code owner reference is resolvable.
- if (codeOwnerResolver.resolve(codeOwnerReference).findAny().isPresent()) {
+ if (codeOwnerResolver.isResolvable(codeOwnerReference)) {
// The code owner reference was successfully resolved to at least one code owner.
return Optional.empty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index 360e2a7..4048684 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -15,10 +15,12 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
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.plugins.codeowners.testing.CodeOwnerInfoSubject.assertThatList;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.hasAccountId;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.hasAccountName;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
@@ -28,6 +30,7 @@
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
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.AccountGroup;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.extensions.client.ListAccountsOption;
@@ -721,4 +724,178 @@
.update();
assertThat(queryCodeOwners("/foo/bar/baz.md")).isEmpty();
}
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "ALL")
+ public void getAllUsersAsCodeOwners_allVisible() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // Add a code owner config that makes all users code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id(), admin.id());
+
+ // Query code owners with a limit.
+ codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).hasSize(2);
+ assertThatList(codeOwnerInfos)
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id(), admin.id());
+ assertThatList(codeOwnerInfos)
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id(), admin.id());
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void getAllUsersAsCodeOwners_sameGroupVisibility() throws Exception {
+ // Create 2 accounts that share a group.
+ TestAccount user2 = accountCreator.user2();
+ TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
+ groupOperations.newGroup().addMember(user2.id()).addMember(user3.id()).create();
+
+ // Add a code owner config that makes all users code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // user can only see itself
+ requestScopeOperations.setApiUser(user.id());
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(user.id());
+
+ // user2 can see user3 and itself
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user3.id());
+
+ // admin can see all users
+ requestScopeOperations.setApiUser(admin.id());
+ codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id(), user3.id());
+
+ // Query code owners with a limit, user2 can see user3 and itself
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).hasSize(1);
+ assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isAnyOf(user2.id(), user3.id());
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "VISIBLE_GROUP")
+ public void getAllUsersAsCodeOwners_visibleGroupVisibility() throws Exception {
+ // create a group that until contains user
+ AccountGroup.UUID userGroup = groupOperations.newGroup().addMember(user.id()).create();
+
+ // create user2 account and a group that only contains user2, but which is visible to user
+ // (since user owns the group)
+ TestAccount user2 = accountCreator.user2();
+ groupOperations.newGroup().addMember(user2.id()).ownerGroupUuid(userGroup).create();
+
+ // create user3 account and a group that only contains user3, but which is visible to all users
+ TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
+ groupOperations.newGroup().addMember(user3.id()).visibleToAll(true).create();
+
+ // Add a code owner config that makes all users code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // user can only see itself, user2 (because user is owner of a group that contains user2) and
+ // user3 (because user3 is member of a group that is visible to all users)
+ requestScopeOperations.setApiUser(user.id());
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), user2.id(), user3.id());
+
+ // user2 can see user3 and itself
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user3.id());
+
+ // admin can see all users
+ requestScopeOperations.setApiUser(admin.id());
+ codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id(), user3.id());
+
+ // Query code owners with a limit, user2 can see user3 and itself
+ requestScopeOperations.setApiUser(user2.id());
+ codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).hasSize(1);
+ assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isAnyOf(user2.id(), user3.id());
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void getAllUsersAsCodeOwners_noneVisible() throws Exception {
+ accountCreator.user2();
+
+ // Add a code owner config that makes all users code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // Use user, since admin is allowed to view all accounts.
+ requestScopeOperations.setApiUser(user.id());
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void getAllUsersAsCodeOwners_withViewAllAccounts() throws Exception {
+ // Allow all users to view all accounts.
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability(GlobalCapability.VIEW_ALL_ACCOUNTS).group(REGISTERED_USERS))
+ .update();
+
+ TestAccount user2 = accountCreator.user2();
+
+ // Add a code owner config that makes all users code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id());
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD
index 861a20f..4821121 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD
@@ -25,8 +25,10 @@
srcs = glob(["Abstract*.java"]),
deps = [
"//java/com/google/gerrit/acceptance:framework-lib",
+ "//java/com/google/gerrit/common:server",
"//java/com/google/gerrit/entities",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/server",
"//lib:guava",
"//lib/guice",
"//plugins/code-owners:code-owners__plugin",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
index 95fef12..a05799d 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFileTest.java
@@ -316,8 +316,14 @@
private void setupPatchListEntry(
@Nullable String newPath, @Nullable String oldPath, Patch.ChangeType changeType) {
- when(patchListEntry.getNewName()).thenReturn(newPath);
- when(patchListEntry.getOldName()).thenReturn(oldPath);
- when(patchListEntry.getChangeType()).thenReturn(changeType);
+ if (Patch.ChangeType.DELETED == changeType) {
+ // for deletions PatchListEntry sets the oldPath as new name
+ when(patchListEntry.getNewName()).thenReturn(oldPath);
+ when(patchListEntry.getChangeType()).thenReturn(changeType);
+ } else {
+ when(patchListEntry.getNewName()).thenReturn(newPath);
+ when(patchListEntry.getOldName()).thenReturn(oldPath);
+ when(patchListEntry.getChangeType()).thenReturn(changeType);
+ }
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
index 8694f92..19d5fdd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/ChangedFilesTest.java
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
@@ -243,6 +244,74 @@
}
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.mergeCommitStrategy", value = "ALL_CHANGED_FILES")
+ public void computeForMergeChangeThatContainsADeletedFileAsConflictResolution_allChangedFiles()
+ throws Exception {
+ testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void
+ computeForMergeChangeThatContainsADeletedFileAsConflictResolution_filesWithConflictResolution()
+ throws Exception {
+ testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution();
+ }
+
+ private void testComputeForMergeChangeThatContainsADeletedFileAsConflictResolution()
+ throws Exception {
+ String file = "foo/a.txt";
+
+ // Create a base change.
+ Change.Id baseChange =
+ changeOperations.newChange().branch("master").file(file).content("base content").create();
+ approveAndSubmit(baseChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id changeInMaster =
+ changeOperations.newChange().branch("master").file(file).content("master content").create();
+ approveAndSubmit(changeInMaster);
+
+ // Create a change in the other branch and that deleted file1.
+ PushOneCommit push =
+ pushFactory.create(admin.newIdent(), testRepo, "Change Deleting A File", file, "");
+ Result r = push.rm("refs/for/master");
+ r.assertOkStatus();
+ approveAndSubmit(r.getChange().getId());
+
+ // Create a merge change with resolving the conflict on file between the edit in master and the
+ // deletion in the other branch by deleting the file.
+ Change.Id mergeChange =
+ changeOperations
+ .newChange()
+ .branch("master")
+ .mergeOf()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .delete()
+ .create();
+
+ ImmutableSet<ChangedFile> changedFilesSet =
+ changedFiles.compute(getRevisionResource(Integer.toString(mergeChange.get())));
+ ImmutableSet<String> oldPaths =
+ changedFilesSet.stream()
+ .map(changedFile -> JgitPath.of(changedFile.oldPath().get()).get())
+ .collect(toImmutableSet());
+ assertThat(oldPaths).containsExactly(file);
+ }
+
private void approveAndSubmit(Change.Id changeId) throws Exception {
approve(Integer.toString(changeId.get()));
gApi.changes().id(changeId.get()).current().submit();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 69e5f97..1249301 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -22,13 +22,17 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
@@ -892,6 +896,136 @@
}
@Test
+ public void approvedByAnyoneWhenEveryoneIsCodeOwner() throws Exception {
+ // Create a code owner config file that makes everyone a code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // Create a change.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet (the change owner is a code owner, but
+ // implicit approvals are disabled).
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add an approval by a user that is a code owner only through the global code ownership.
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ public void everyoneIsCodeOwner_noImplicitApproval() throws Exception {
+ testImplicitlyApprovedWhenEveryoneIsCodeOwner(false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsCodeOwner_withImplicitApproval() throws Exception {
+ testImplicitlyApprovedWhenEveryoneIsCodeOwner(true);
+ }
+
+ private void testImplicitlyApprovedWhenEveryoneIsCodeOwner(boolean implicitApprovalsEnabled)
+ throws Exception {
+ // Create a code owner config file that makes everyone a code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // Create a change as a user that is a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void anyReviewerWhenEveryoneIsCodeOwner() throws Exception {
+ // Create a code owner config file that makes everyone a code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ // Create a change as a user that is a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
+ // approval by default).
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add a user as reviewer that is a code owner.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Check that the status of the file is PENDING now.
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.PENDING);
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
public void approvedByGlobalCodeOwner() throws Exception {
testApprovedByGlobalCodeOwner(false);
@@ -1114,6 +1248,198 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void approvedByAnyoneWhenEveryoneIsGlobalCodeOwner() throws Exception {
+ testApprovedByAnyoneWhenEveryoneIsGlobalCodeOwner(false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void approvedByAnyoneWhenEveryoneIsGlobalCodeOwner_bootstrappingMode() throws Exception {
+ testApprovedByAnyoneWhenEveryoneIsGlobalCodeOwner(true);
+ }
+
+ private void testApprovedByAnyoneWhenEveryoneIsGlobalCodeOwner(boolean bootstrappingMode)
+ throws Exception {
+ if (!bootstrappingMode) {
+ // Create a code owner config file so that we are not in the bootstrapping mode.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+ }
+
+ // Create a change.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet (the change owner is a global code owner, but
+ // implicit approvals are disabled).
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add an approval by a user that is a code owner only through the global code ownership.
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void everyoneIsGlobalCodeOwner_noImplicitApproval() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /** implicitApprovalsEnabled = */
+ false,
+ /** bootstrappingMode = */
+ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsGlobalCodeOwner_withImplicitApproval() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /** implicitApprovalsEnabled = */
+ true,
+ /** bootstrappingMode = */
+ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void everyoneIsGlobalCodeOwner_noImplicitApproval_bootstrappingMode() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /** implicitApprovalsEnabled = */
+ false,
+ /** bootstrappingMode = */
+ true);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsGlobalCodeOwner_withImplicitApproval_bootstrappingMode() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /** implicitApprovalsEnabled = */
+ true,
+ /** bootstrappingMode = */
+ true);
+ }
+
+ private void testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ boolean implicitApprovalsEnabled, boolean bootstrappingMode) throws Exception {
+ if (!bootstrappingMode) {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+ }
+
+ // Create a change as a user that is a code owner only through the global code ownership.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(
+ implicitApprovalsEnabled
+ ? CodeOwnerStatus.APPROVED
+ : CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void anyReviewerWhenEveryoneIsGlobalCodeOwner() throws Exception {
+ testAnyReviewerWhenEveryoneIsGlobalCodeOwner(false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void anyReviewerWhenEveryoneIsGlobalCodeOwner_bootstrappingMode() throws Exception {
+ testAnyReviewerWhenEveryoneIsGlobalCodeOwner(true);
+ }
+
+ private void testAnyReviewerWhenEveryoneIsGlobalCodeOwner(boolean bootstrappingMode)
+ throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ if (!bootstrappingMode) {
+ // Create a code owner config file so that we are not in the bootstrapping mode.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+ }
+
+ // Create a change as a user that is a code owner only through the global code ownership.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the status of the file is INSUFFICIENT_REVIEWERS (since there is no implicit
+ // approval by default).
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Add a user as reviewer that is a code owner only through the global code ownership.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Check that the status of the file is PENDING now.
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.PENDING);
+ }
+
+ @Test
public void parentCodeOwnerConfigsAreConsidered() throws Exception {
TestAccount user2 = accountCreator.user2();
TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
@@ -1526,6 +1852,24 @@
}
}
+ @Test
+ public void getStatus_branchDeleted() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ String changeId = createChange("refs/for/" + branchName).getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)));
+ assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
index ea0eab7..14bf652 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScannerTest.java
@@ -416,6 +416,16 @@
.isTrue();
}
+ @Test
+ public void containsOnlyInvalidCodeOwnerConfigFiles() throws Exception {
+ createInvalidCodeOwnerConfig("/OWNERS");
+
+ assertThat(
+ codeOwnerConfigScanner.containsAnyCodeOwnerConfigFile(
+ BranchNameKey.create(project, "master")))
+ .isTrue();
+ }
+
private void visit() {
codeOwnerConfigScanner.visit(
BranchNameKey.create(project, "master"), visitor, invalidCodeOwnerConfigCallback);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 8737bc6..f390336 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerSubject.assertThat;
-import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerSubject.hasAccountId;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableSet;
@@ -90,53 +89,12 @@
}
@Test
- public void resolveCodeOwnerReferenceForStarAsEmail() throws Exception {
- TestAccount user2 = accountCreator.user2();
+ public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception {
Stream<CodeOwner> codeOwner =
codeOwnerResolver
.get()
.resolve(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
- assertThat(codeOwner)
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), user.id(), user2.id());
- }
-
- @Test
- @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
- public void resolveCodeOwnerReferenceForStarAsEmailChecksAccountVisibility() throws Exception {
- // Create a new user that is not a member of any group. This means 'user' and 'admin' are not
- // visible to this user since they do not share any group.
- TestAccount user2 = accountCreator.user2();
-
- // Set user2 as current user.
- requestScopeOperations.setApiUser(user2.id());
-
- Stream<CodeOwner> codeOwner =
- codeOwnerResolver
- .get()
- .resolve(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
- assertThat(codeOwner).comparingElementsUsing(hasAccountId()).containsExactly(user2.id());
- }
-
- @Test
- @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
- public void nonVisibleCodeOwnerCanBeResolvedForStarAsEmailIfVisibilityIsNotEnforced()
- throws Exception {
- // Create a new user that is not a member of any group. This means 'user' and 'admin' are not
- // visible to this user since they do not share any group.
- TestAccount user2 = accountCreator.user2();
-
- // Set user2 as current user.
- requestScopeOperations.setApiUser(user2.id());
-
- Stream<CodeOwner> codeOwner =
- codeOwnerResolver
- .get()
- .enforceVisibility(false)
- .resolve(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD));
- assertThat(codeOwner)
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), user.id(), user2.id());
+ assertThat(codeOwner).isEmpty();
}
@Test
@@ -222,9 +180,10 @@
CodeOwnerConfig codeOwnerConfig =
CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
.build();
- assertThat(
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")))
- .isEmpty();
+ CodeOwnerResolverResult result =
+ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwners()).isEmpty();
+ assertThat(result.ownedByAllUsers()).isFalse();
}
@Test
@@ -233,10 +192,25 @@
CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
.addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(admin.email(), user.email()))
.build();
- assertThat(
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")))
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), user.id());
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
+ }
+
+ @Test
+ public void resolvePathCodeOwnersWhenStarIsUsedAsEmail() throws Exception {
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.createWithoutPathExpressions(CodeOwnerResolver.ALL_USERS_WILDCARD))
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).isEmpty();
+ assertThat(result.ownedByAllUsers()).isTrue();
}
@Test
@@ -247,10 +221,10 @@
CodeOwnerSet.createWithoutPathExpressions(
admin.email(), "non-existing@example.com"))
.build();
- assertThat(
- codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")))
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id());
+ CodeOwnerResolverResult result =
+ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
}
@Test
@@ -376,14 +350,27 @@
@Test
public void resolveCodeOwnerReferences() throws Exception {
+ CodeOwnerResolverResult result =
+ codeOwnerResolver
+ .get()
+ .resolve(
+ ImmutableSet.of(
+ CodeOwnerReference.create(admin.email()),
+ CodeOwnerReference.create(user.email())));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
+ }
+
+ @Test
+ public void isResolvable() throws Exception {
+ assertThat(codeOwnerResolver.get().isResolvable(CodeOwnerReference.create(admin.email())))
+ .isTrue();
+ }
+
+ @Test
+ public void isNotResolvable() throws Exception {
assertThat(
- codeOwnerResolver
- .get()
- .resolve(
- ImmutableSet.of(
- CodeOwnerReference.create(admin.email()),
- CodeOwnerReference.create(user.email()))))
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), user.id());
+ codeOwnerResolver.get().isResolvable(CodeOwnerReference.create("unknown@example.com")))
+ .isFalse();
}
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index abc4bfe..fdcd4da 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -340,6 +340,11 @@
}
```
+If the destination branch of a change no longer exists (e.g. because it was
+deleted), `409 Conflict` is returned. Since the code owners are retrieved from
+the destination branch, computing the code owner status is not possible, if the
+destination branch is missing.
+
## <a id="revision-endpoints"> Revision Endpoints
### <a id="list-code-owners-for-path-in-change"> List Code Owners for path in change
