Merge changes Ibebb6b55,Iafb1d32f,I2486c293,I2695d16d,If579300d, ...
* changes:
List code owners: Add option to only get code owners with the highest score
AbstractGetCodeOwnersForPath: Make code owner scorings accessible
AbstractGetCodeOwnersForPath: Cleanup how subclasses add further scorings
AbstractGetCodeOwnersForPath: Add a comment
Add distance scores for code owners that are added for resolving all users
Fix sorting in code owner suggestion if file is owned by all users
GetOwnedPath: Limit the number of returned paths
ChangeCodeOwners: Add request object for computing the code owner status
Adapt projectOwnersAreNoCodeOwnersIfDefaultCodeOwnerConfigExists test
diff --git a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
index eabf850..cd8aacb 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/ChangeCodeOwners.java
@@ -23,8 +23,8 @@
* <p>To create an instance for a change use {@code ChangeCodeOwnersFactory}.
*/
public interface ChangeCodeOwners {
- /** Returns the code owner status for the files in the change. */
- CodeOwnerStatusInfo getCodeOwnerStatus() throws RestApiException;
+ /** Creates a request to retrieve the code owner status for the files in the change. */
+ CodeOwnerStatusRequest getCodeOwnerStatus() throws RestApiException;
/** Returns the revision-level code owners API for the current revision. */
default RevisionCodeOwners current() throws RestApiException {
@@ -35,12 +35,26 @@
RevisionCodeOwners revision(String id) throws RestApiException;
/**
+ * Request to compute code owner status.
+ *
+ * <p>Allows to set parameters on the request before executing it by calling {@link #get()}.
+ */
+ abstract class CodeOwnerStatusRequest {
+ /**
+ * Executes this request and retrieves the code owner status.
+ *
+ * @return the code owner status
+ */
+ public abstract CodeOwnerStatusInfo get() throws RestApiException;
+ }
+
+ /**
* A default implementation which allows source compatibility when adding new methods to the
* interface.
*/
class NotImplemented implements ChangeCodeOwners {
@Override
- public CodeOwnerStatusInfo getCodeOwnerStatus() {
+ public CodeOwnerStatusRequest getCodeOwnerStatus() {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
index 578c1c3..655ef66 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
@@ -48,6 +48,7 @@
private String revision;
private Long seed;
private Boolean resolveAllUsers;
+ private Boolean highestScoreOnly;
/**
* Lists the code owners for the given path.
@@ -116,6 +117,17 @@
}
/**
+ * Sets whether only the code owners with the highest score should be returned.
+ *
+ * @param highestScoreOnly whether only the code owners with the highest score should be
+ * returned
+ */
+ public QueryRequest withHighestScoreOnly(boolean highestScoreOnly) {
+ this.highestScoreOnly = highestScoreOnly;
+ return this;
+ }
+
+ /**
* Sets the branch revision from which the code owner configs should be read.
*
* <p>Not supported for querying code owners for a path in a change.
@@ -149,6 +161,11 @@
return Optional.ofNullable(resolveAllUsers);
}
+ /** Whether only the code owners with the highest score should be returned. */
+ public Optional<Boolean> getHighestScoreOnly() {
+ return Optional.ofNullable(highestScoreOnly);
+ }
+
/** Returns the branch revision from which the code owner configs should be read. */
public Optional<String> getRevision() {
return Optional.ofNullable(revision);
diff --git a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
index 0fc1497..ba41418 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
@@ -31,4 +31,11 @@
* <p>The paths are sorted alphabetically.
*/
public List<String> ownedPaths;
+
+ /**
+ * Whether the request would deliver more results if not limited.
+ *
+ * <p>Not set if {@code false}.
+ */
+ public Boolean more;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
index 1b90340..7b2655b 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/RevisionCodeOwners.java
@@ -20,6 +20,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
/**
* Revision-level Java API of the code-owners plugin.
@@ -95,8 +96,40 @@
/** Request to check code owner config files. */
abstract class OwnedPathsRequest {
+ private Integer start;
+ private Integer limit;
private String user;
+ /**
+ * Sets a limit on the number of owned paths that should be returned.
+ *
+ * @param start number of owned paths to skip
+ */
+ public OwnedPathsRequest withStart(int start) {
+ this.start = start;
+ return this;
+ }
+
+ /** Returns the number of owned paths to skip. */
+ public Optional<Integer> getStart() {
+ return Optional.ofNullable(start);
+ }
+
+ /**
+ * Sets a limit on the number of owned paths that should be returned.
+ *
+ * @param limit the limit
+ */
+ public OwnedPathsRequest withLimit(int limit) {
+ this.limit = limit;
+ return this;
+ }
+
+ /** Returns the limit. */
+ public Optional<Integer> getLimit() {
+ return Optional.ofNullable(limit);
+ }
+
/** Sets the user for which the owned paths should be retrieved. */
public OwnedPathsRequest forUser(String user) {
this.user = user;
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
index 9f266f8..5a20f34 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/ChangeCodeOwnersImpl.java
@@ -52,12 +52,17 @@
}
@Override
- public CodeOwnerStatusInfo getCodeOwnerStatus() throws RestApiException {
- try {
- return getCodeOwnerStatus.apply(changeResource).value();
- } catch (Exception e) {
- throw asRestApiException("Cannot get code owner status", e);
- }
+ public CodeOwnerStatusRequest getCodeOwnerStatus() throws RestApiException {
+ return new CodeOwnerStatusRequest() {
+ @Override
+ public CodeOwnerStatusInfo get() throws RestApiException {
+ try {
+ return getCodeOwnerStatus.apply(changeResource).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot get code owner status", e);
+ }
+ }
+ };
}
@Override
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
index 9fba599..107fa61 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
@@ -59,6 +59,7 @@
getLimit().ifPresent(getCodeOwners::setLimit);
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
+ getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
getRevision().ifPresent(getCodeOwners::setRevision);
CodeOwnersInBranchCollection.PathResource pathInBranchResource =
codeOwnersInBranchCollection.parse(
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
index ed2bad9..b185172 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
@@ -64,6 +64,7 @@
getLimit().ifPresent(getCodeOwners::setLimit);
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
+ getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
CodeOwnersInChangeCollection.PathResource pathInChangeResource =
codeOwnersInChangeCollection.parse(
revisionResource, IdString.fromDecoded(path.toString()));
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
index d42327c..db8cb27 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/RevisionCodeOwnersImpl.java
@@ -75,6 +75,8 @@
public OwnedPathsInfo get() throws RestApiException {
try {
GetOwnedPaths getOwnedPaths = getOwnedPathsProvider.get();
+ getStart().ifPresent(getOwnedPaths::setStart);
+ getLimit().ifPresent(getOwnedPaths::setLimit);
getOwnedPaths.setUser(getUser());
return getOwnedPaths.apply(revisionResource).value();
} catch (Exception e) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index af7d9b7..b17651e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -121,21 +121,23 @@
* @param changeNotes the change notes for which the owned files should be returned
* @param patchSet the patch set for which the owned files should be returned
* @param accountId account ID of the code owner for which the owned files should be returned
+ * @param start number of owned paths to skip
* @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
* @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<Path> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int limit)
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
throws ResourceConflictException {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
"compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
- + " limit = %d)",
+ + " start = %d, limit = %d)",
accountId.get(),
changeNotes.getProjectName(),
changeNotes.getChangeId().get(),
patchSet.id().get(),
+ start,
limit);
Stream<Path> ownedPaths =
getFileStatusesForAccount(changeNotes, patchSet, accountId)
@@ -149,6 +151,9 @@
.filter(
pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
.map(PathCodeOwnerStatus::path);
+ if (start > 0) {
+ ownedPaths = ownedPaths.skip(start);
+ }
if (limit > 0) {
ownedPaths = ownedPaths.limit(limit);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
index 84a263b..84bddff 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
@@ -14,11 +14,13 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import java.util.Comparator;
-import java.util.HashSet;
import java.util.Set;
+import java.util.function.Function;
import java.util.stream.Collectors;
/**
@@ -40,21 +42,14 @@
return new AutoValue_CodeOwnerScorings(ImmutableSet.copyOf(codeOwnerScorings));
}
- public static CodeOwnerScorings appendScoring(
- CodeOwnerScorings codeOwnerScorings, CodeOwnerScoring codeOwnerScoringToBeAdded) {
- Set<CodeOwnerScoring> codeOwnerScoringSet = new HashSet<>(codeOwnerScorings.scorings());
- codeOwnerScoringSet.add(codeOwnerScoringToBeAdded);
- return create(codeOwnerScoringSet);
- }
-
/**
- * Returns a comparator to sort code owners by the collected scorings.
+ * Returns the total scorings for the given code owners.
*
- * <p>Code owners with higher scoring come first. The order of code owners with the same scoring
- * is undefined.
+ * @param codeOwners the code owners for which the scorings should be returned
*/
- public Comparator<CodeOwner> comparingByScorings() {
- return Comparator.comparingDouble(this::sumWeightedScorings).reversed();
+ public ImmutableMap<CodeOwner, Double> getScorings(ImmutableSet<CodeOwner> codeOwners) {
+ return codeOwners.stream()
+ .collect(toImmutableMap(Function.identity(), this::sumWeightedScorings));
}
/** Returns the sum of all weighted scorings that available for the given code owner. */
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index e6a55ee..403fe49 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -164,7 +164,11 @@
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId, limit + 1);
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ reviewerAccountId,
+ /* start= */ 0,
+ limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index e9e2c39..9803951 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -124,7 +124,11 @@
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId(), limit + 1);
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ user.getAccountId(),
+ /* start= */ 0,
+ limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 0644b4c..d1e6cd2 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
@@ -53,6 +54,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashSet;
import java.util.List;
@@ -87,6 +89,7 @@
private int limit = DEFAULT_LIMIT;
private Optional<Long> seed = Optional.empty();
private boolean resolveAllUsers;
+ private boolean highestScoreOnly;
@Option(
name = "-o",
@@ -128,6 +131,13 @@
this.resolveAllUsers = resolveAllUsers;
}
+ @Option(
+ name = "--highest-score-only",
+ usage = "whether only code owners with the highest score should be returned")
+ public void setHighestScoreOnly(boolean highestScoreOnly) {
+ this.highestScoreOnly = highestScoreOnly;
+ }
+
protected AbstractGetCodeOwnersForPath(
AccountVisibility accountVisibility,
Accounts accounts,
@@ -179,21 +189,6 @@
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners()));
- if (pathCodeOwners.ownedByAllUsers()) {
- ownedByAllUsers.set(true);
- 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());
- }
-
- return true;
- }
-
int distance =
codeOwnerConfig.key().branchNameKey().branch().equals(RefNames.REFS_CONFIG)
? defaultOwnersDistance
@@ -203,6 +198,27 @@
.forEach(
localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+ if (pathCodeOwners.ownedByAllUsers()) {
+ ownedByAllUsers.set(true);
+ ImmutableSet<CodeOwner> addedCodeOwners =
+ fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ addedCodeOwners.forEach(
+ localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
+
+ 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 always need to iterate over all relevant OWNERS files (even if the limit has already
+ // been reached).
+ // This is needed to collect distance scores for code owners that are mentioned in the
+ // more distant OWNERS files. Those become relevant if further scores are applied later
+ // (e.g. the score for current reviewers of the change).
return true;
});
@@ -216,23 +232,46 @@
if (globalCodeOwners.ownedByAllUsers()) {
ownedByAllUsers.set(true);
- fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ ImmutableSet<CodeOwner> addedCodeOwners = fillUpWithRandomUsers(rsrc, codeOwners, limit);
+ addedCodeOwners.forEach(
+ codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, globalOwnersDistance));
+ }
+ }
+
+ ImmutableSet<CodeOwner> immutableCodeOwners = ImmutableSet.copyOf(codeOwners);
+ CodeOwnerScorings codeOwnerScorings =
+ createScorings(rsrc, immutableCodeOwners, distanceScoring.build());
+ ImmutableMap<CodeOwner, Double> scoredCodeOwners =
+ codeOwnerScorings.getScorings(immutableCodeOwners);
+
+ ImmutableList<CodeOwner> sortedAndLimitedCodeOwners = sortAndLimit(rsrc, scoredCodeOwners);
+
+ if (highestScoreOnly) {
+ Optional<Double> highestScore =
+ scoredCodeOwners.values().stream().max(Comparator.naturalOrder());
+ if (highestScore.isPresent()) {
+ sortedAndLimitedCodeOwners =
+ sortedAndLimitedCodeOwners.stream()
+ .filter(codeOwner -> scoredCodeOwners.get(codeOwner).equals(highestScore.get()))
+ .collect(toImmutableList());
}
}
CodeOwnersInfo codeOwnersInfo = new CodeOwnersInfo();
codeOwnersInfo.codeOwners =
- codeOwnerJsonFactory
- .create(getFillOptions())
- .format(
- sortAndLimit(
- rsrc,
- CodeOwnerScorings.create(distanceScoring.build()),
- ImmutableSet.copyOf(codeOwners)));
+ codeOwnerJsonFactory.create(getFillOptions()).format(sortedAndLimitedCodeOwners);
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
return Response.ok(codeOwnersInfo);
}
+ private CodeOwnerScorings createScorings(
+ R rsrc, ImmutableSet<CodeOwner> codeOwners, CodeOwnerScoring distanceScoring) {
+ ImmutableSet.Builder<CodeOwnerScoring> codeOwnerScorings = ImmutableSet.builder();
+ codeOwnerScorings.add(distanceScoring);
+ codeOwnerScorings.addAll(getCodeOwnerScorings(rsrc, codeOwners));
+ return CodeOwnerScorings.create(codeOwnerScorings.build());
+ }
+
private CodeOwnerResolverResult getGlobalCodeOwners(Project.NameKey projectName) {
CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver
@@ -244,6 +283,19 @@
}
/**
+ * Get further code owner scorings.
+ *
+ * <p>To be overridden by subclasses to include further scorings.
+ *
+ * @param rsrc resource on which the request is being performed
+ * @param codeOwners the code owners
+ */
+ protected ImmutableSet<CodeOwnerScoring> getCodeOwnerScorings(
+ R rsrc, ImmutableSet<CodeOwner> codeOwners) {
+ return ImmutableSet.of();
+ }
+
+ /**
* Filters out code owners that should not be suggested.
*
* <p>The following code owners are filtered out:
@@ -349,26 +401,26 @@
}
private ImmutableList<CodeOwner> sortAndLimit(
- R rsrc, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return sortCodeOwners(rsrc, seed, scorings, codeOwners).limit(limit).collect(toImmutableList());
+ R rsrc, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return sortCodeOwners(rsrc, seed, scoredCodeOwners).limit(limit).collect(toImmutableList());
}
/**
* Sorts the code owners.
*
- * <p>Code owners with higher distance score are returned first.
+ * <p>Code owners with higher score are returned first.
*
- * <p>The order of code owners with the same distance score is random.
+ * <p>The order of code owners with the same score is random.
*
* @param rsrc resource on which this REST endpoint is invoked
* @param seed seed that should be used to randomize the order
- * @param scorings the scorings for the code owners
- * @param codeOwners the code owners that should be sorted
+ * @param scoredCodeOwners the code owners with their scores
* @return the sorted code owners
*/
- protected Stream<CodeOwner> sortCodeOwners(
- R rsrc, Optional<Long> seed, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
- return randomizeOrder(seed, codeOwners).sorted(scorings.comparingByScorings());
+ private Stream<CodeOwner> sortCodeOwners(
+ R rsrc, Optional<Long> seed, ImmutableMap<CodeOwner, Double> scoredCodeOwners) {
+ return randomizeOrder(seed, scoredCodeOwners.keySet())
+ .sorted(Comparator.comparingDouble(scoredCodeOwners::get).reversed());
}
/**
@@ -393,16 +445,19 @@
* all user.
*
* <p>No-op if code ownership for all users should not be resolved.
+ *
+ * @return the added code owners
*/
- private void fillUpWithRandomUsers(R rsrc, Set<CodeOwner> codeOwners, int limit) {
+ private ImmutableSet<CodeOwner> fillUpWithRandomUsers(
+ R rsrc, Set<CodeOwner> codeOwners, int limit) {
if (!resolveAllUsers || codeOwners.size() >= limit) {
// code ownership for all users should not be resolved or the limit has already been reached
// so that we don't need to add further suggestions
- return;
+ return ImmutableSet.of();
}
logger.atFine().log("filling up with random users");
- codeOwners.addAll(
+ ImmutableSet<CodeOwner> codeOwnersToAdd =
filterCodeOwners(
rsrc,
// ask for 2 times the number of users that we need so that we still have enough
@@ -414,7 +469,9 @@
.stream()
.filter(codeOwner -> !codeOwners.contains(codeOwner))
.limit(limit - codeOwners.size())
- .collect(toImmutableSet()));
+ .collect(toImmutableSet());
+ codeOwners.addAll(codeOwnersToAdd);
+ return codeOwnersToAdd;
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index f475550..e6a2f0d 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -29,7 +29,6 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
@@ -97,11 +96,8 @@
* only applies if code owners are suggested on changes.
*/
@Override
- protected Stream<CodeOwner> sortCodeOwners(
- CodeOwnersInChangeCollection.PathResource rsrc,
- Optional<Long> seed,
- CodeOwnerScorings scorings,
- ImmutableSet<CodeOwner> codeOwners) {
+ protected ImmutableSet<CodeOwnerScoring> getCodeOwnerScorings(
+ CodeOwnersInChangeCollection.PathResource rsrc, ImmutableSet<CodeOwner> codeOwners) {
// Add scorings for IS_REVIEWER score.
ImmutableSet<Account.Id> reviewers =
rsrc.getRevisionResource()
@@ -116,9 +112,8 @@
reviewers.contains(codeOwner.accountId())
? IS_REVIEWER_SCORING_VALUE
: NO_REVIEWER_SCORING_VALUE));
- scorings = CodeOwnerScorings.appendScoring(scorings, isReviewerScoring.build());
- return super.sortCodeOwners(rsrc, seed, scorings, codeOwners);
+ return ImmutableSet.of(isReviewerScoring.build());
}
@Override
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index 0430f94..1f2298b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Account;
@@ -41,11 +42,33 @@
* /changes/<change-id>/revisions/<revision-id>/owned_paths} requests.
*/
public class GetOwnedPaths implements RestReadView<RevisionResource> {
+ @VisibleForTesting public static final int DEFAULT_LIMIT = 50;
+
private final AccountResolver accountResolver;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private int start;
+ private int limit = DEFAULT_LIMIT;
private String user;
+ @Option(
+ name = "--limit",
+ aliases = {"-n"},
+ metaVar = "CNT",
+ usage = "maximum number of owned path to return (default = " + DEFAULT_LIMIT + ")")
+ public void setLimit(int limit) {
+ this.limit = limit;
+ }
+
+ @Option(
+ name = "--start",
+ aliases = {"-S"},
+ metaVar = "CNT",
+ usage = "number of owned paths to skip")
+ public void setStart(int start) {
+ this.start = start;
+ }
+
@Option(name = "--user", usage = "user for which the owned paths should be returned")
public void setUser(String user) {
this.user = user;
@@ -62,14 +85,22 @@
public Response<OwnedPathsInfo> apply(RevisionResource revisionResource)
throws BadRequestException, ResourceConflictException, UnresolvableAccountException,
ConfigInvalidException, IOException {
+ validateStartAndLimit();
+
Account.Id accountId = resolveAccount();
ImmutableList<Path> ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- revisionResource.getNotes(), revisionResource.getPatchSet(), accountId, /* limit= */ 0);
+ revisionResource.getNotes(),
+ revisionResource.getPatchSet(),
+ accountId,
+ start,
+ limit + 1);
OwnedPathsInfo ownedPathsInfo = new OwnedPathsInfo();
- ownedPathsInfo.ownedPaths = ownedPaths.stream().map(Path::toString).collect(toImmutableList());
+ ownedPathsInfo.more = ownedPaths.size() > limit ? true : null;
+ ownedPathsInfo.ownedPaths =
+ ownedPaths.stream().limit(limit).map(Path::toString).collect(toImmutableList());
return Response.ok(ownedPathsInfo);
}
@@ -82,4 +113,13 @@
return accountResolver.resolve(user).asUnique().account().id();
}
+
+ private void validateStartAndLimit() throws BadRequestException {
+ if (start < 0) {
+ throw new BadRequestException("start cannot be negative");
+ }
+ if (limit <= 0) {
+ throw new BadRequestException("limit must be positive");
+ }
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
index df6b1d0..d6a1072 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertAbout;
+import com.google.common.truth.BooleanSubject;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.IterableSubject;
import com.google.common.truth.Subject;
@@ -48,6 +49,10 @@
return check("ownedPaths()").that(ownedPathsInfo().ownedPaths);
}
+ public BooleanSubject hasMoreThat() {
+ return check("more()").that(ownedPathsInfo().more);
+ }
+
private OwnedPathsInfo ownedPathsInfo() {
isNotNull();
return ownedPathsInfo;
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 743bef0..1a659ae 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -1385,4 +1385,77 @@
.inOrder();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
}
+
+ @Test
+ public void getCodeOwnersWithHighestScoreOnly() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create some code owner configs
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ false),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(3);
+ // the first 2 code owners have the same scoring, so their order is random and we don't know
+ // which of them we get get first and second
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(2)
+ .hasAccountIdThat()
+ .isEqualTo(admin.id());
+
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ true),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
+ // the first 2 code owners have the same scoring, so their order is random and we don't know
+ // which of them we get get first and second
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(0)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .element(1)
+ .hasAccountIdThat()
+ .isAnyOf(user.id(), user2.id());
+ }
+
+ @Test
+ public void getCodeOwnersWithHighestScoreOnly_noCodeOwners() throws Exception {
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().withHighestScoreOnly(/* highestScoreOnly= */ true),
+ "/foo/bar/baz.md");
+ assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 1f213dc..cf6e515 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -105,7 +105,7 @@
// Verify that the code owner status for the changed file is INSUFFICIENT_REVIEWERS.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -158,7 +158,7 @@
// Verify that the code owner status for the changed file is PENDING.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -208,7 +208,7 @@
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
@@ -249,7 +249,7 @@
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.onlyElement()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
index 3cea3f4..79f235f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerStatusIT.java
@@ -56,7 +56,7 @@
recommend(changeId);
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasPatchSetNumberThat()
.isEqualTo(r.getChange().currentPatchSet().id().get());
@@ -79,7 +79,7 @@
String changeId = createChangeWithFileRename(oldPath, newPath);
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.comparingElementsUsing(isFileCodeOwnerStatus())
@@ -90,7 +90,7 @@
// Add a reviewer that is a code owner of the old path.
gApi.changes().id(changeId).addReviewer(user.email());
- codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.comparingElementsUsing(isFileCodeOwnerStatus())
@@ -101,7 +101,7 @@
// Add a reviewer that is a code owner of the new path.
gApi.changes().id(changeId).addReviewer(user2.email());
- codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ codeOwnerStatus = changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.comparingElementsUsing(isFileCodeOwnerStatus())
@@ -116,7 +116,7 @@
String path = "foo/bar.baz";
String changeId = createChange("Change Adding A File", path, "file content").getChangeId();
CodeOwnerStatusInfo codeOwnerStatus =
- changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus();
+ changeCodeOwnersApiFactory.change(changeId).getCodeOwnerStatus().get();
assertThat(codeOwnerStatus)
.hasFileCodeOwnerStatusesThat()
.comparingElementsUsing(isFileCodeOwnerStatus())
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index e0802a1..1feefc6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -36,6 +36,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import java.util.List;
@@ -389,4 +390,43 @@
.containsExactly(user2.id(), user.id(), admin.id())
.inOrder();
}
+
+ @Test
+ public void codeOwnersSortedByDistance_fileOwnedByAllUsers() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD)
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ // None of the code owners is a reviewer, hence the sorting is done only by distance.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user.id(), admin.id())
+ .inOrder();
+ assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
index 6040196..681513b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.api.OwnedPathsInfo;
+import com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
import org.junit.Test;
@@ -33,6 +34,10 @@
/**
* Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
* endpoint.
+ *
+ * <p>Further tests for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
+ * endpoint that require using the REST API are implemented in {@link
+ * com.google.gerrit.plugins.codeowners.acceptance.restapi.GetOwnedPathRestIT}.
*/
public class GetOwnedPathsIT extends AbstractCodeOwnersIT {
@Inject private RequestScopeOperations requestScopeOperations;
@@ -126,6 +131,7 @@
.forUser(user.email())
.get();
assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
}
@Test
@@ -179,4 +185,260 @@
.get();
assertThat(ownedPathsInfo).hasOwnedPathsThat().isEmpty();
}
+
+ @Test
+ public void getOwnedPathsWithStart() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(0)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo)
+ .hasOwnedPathsThat()
+ .containsExactly(path1, path2, path3, path4)
+ .inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(1)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path2, path3, path4).inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path3, path4).inOrder();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(3)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path4);
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(4)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().isEmpty();
+ }
+
+ @Test
+ public void getOwnedPathsWithLimit() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(1)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1);
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(3)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2, path3).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(4)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo)
+ .hasOwnedPathsThat()
+ .containsExactly(path1, path2, path3, path4)
+ .inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getOwnedPathsWithStartAndLimit() throws Exception {
+ setAsRootCodeOwners(user);
+
+ String path1 = "/bar/baz.md";
+ String path2 = "/bar/foo.md";
+ String path3 = "/foo/bar/baz.md";
+ String path4 = "/foo/baz/bar.md";
+
+ String changeId =
+ createChange(
+ "test change",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content",
+ JgitPath.of(path3).get(),
+ "file content",
+ JgitPath.of(path4).get(),
+ "file content"))
+ .getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(1)
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path2, path3).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+ }
+
+ @Test
+ public void getOwnedPathsLimitedByDefault() throws Exception {
+ setAsRootCodeOwners(user);
+
+ ImmutableMap.Builder<String, String> files = ImmutableMap.builder();
+ for (int i = 1; i <= GetOwnedPaths.DEFAULT_LIMIT + 1; i++) {
+ files.put(String.format("foo-%d.txt", i), "file content");
+ }
+
+ String changeId = createChange("test change", files.build()).getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().hasSize(GetOwnedPaths.DEFAULT_LIMIT);
+ }
+
+ @Test
+ public void startCannotBeNegative() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withStart(-1)
+ .forUser(user.email())
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("start cannot be negative");
+ }
+
+ @Test
+ public void limitCannotBeNegative() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(-1)
+ .forUser(user.email())
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("limit must be positive");
+ }
+
+ @Test
+ public void cannotGetOwnedPathWithoutLimit() throws Exception {
+ String changeId = createChange().getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .withLimit(0)
+ .forUser(user.email())
+ .get());
+ assertThat(exception).hasMessageThat().isEqualTo("limit must be positive");
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java
new file mode 100644
index 0000000..e5287df
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetOwnedPathRestIT.java
@@ -0,0 +1,72 @@
+// Copyright (C) 2021 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.acceptance.restapi;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.joining;
+
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import java.util.Arrays;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths} REST
+ * endpoint. that require using via REST.
+ *
+ * <p>Acceptance test for the {@link com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths}
+ * REST endpoint that can use the Java API are implemented in {@link
+ * com.google.gerrit.plugins.codeowners.acceptance.api.GetOwnedPathsIT}.
+ */
+public class GetOwnedPathRestIT extends AbstractCodeOwnersIT {
+ private String changeId;
+
+ @Before
+ public void createTestChange() throws Exception {
+ changeId = createChange().getChangeId();
+ }
+
+ @Test
+ public void cannotGetOwnedPathsWithInvalidStart() throws Exception {
+ RestResponse r = adminRestSession.get(getUrl("user=" + user.email(), "start=invalid"));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--start\"");
+ }
+
+ @Test
+ public void cannotGetOwnedPathsWithInvalidLimit() throws Exception {
+ RestResponse r = adminRestSession.get(getUrl("user=" + user.email(), "limit=invalid"));
+ r.assertBadRequest();
+ assertThat(r.getEntityContent()).contains("\"invalid\" is not a valid value for \"--limit\"");
+ }
+
+ private String getUrl(String... parameters) {
+ StringBuilder b = new StringBuilder();
+ b.append(getUrl());
+ String paramaterString = Arrays.stream(parameters).collect(joining("&"));
+ if (!paramaterString.isEmpty()) {
+ b.append('?').append(paramaterString);
+ }
+ return b.toString();
+ }
+
+ private String getUrl() {
+ return String.format(
+ "/changes/%s/revisions/%s/owned_paths",
+ IdString.fromDecoded(changeId), IdString.fromDecoded("current"));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 76510f4..596eead 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -1364,51 +1364,6 @@
}
@Test
- public void projectOwnersAreNoCodeOwnersIfDefaultCodeOwnerConfigExists() throws Exception {
- TestAccount user2 = accountCreator.user2();
-
- setAsDefaultCodeOwners(user);
-
- // Create a change as a user that is neither a code owner nor a project owner.
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
-
- // Verify that the file is not approved yet.
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
- codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Let the project owner approve the change.
- requestScopeOperations.setApiUser(admin.id());
- approve(changeId);
-
- // Verify that the file is not approved yet
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
-
- // Let the code owner approve the change.
- projectOperations
- .project(project)
- .forUpdate()
- .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
- .update();
- requestScopeOperations.setApiUser(user.id());
- approve(changeId);
-
- // Check that the file is approved now.
- requestScopeOperations.setApiUser(admin.id());
- fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
- }
-
- @Test
public void approvedByDefaultCodeOwner() throws Exception {
TestAccount user2 = accountCreator.user2();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
index 3878a27..d17d51b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
@@ -597,6 +597,51 @@
}
}
+ @Test
+ public void projectOwnersAreNotCodeOwnersIfDefaultCodeOwnerConfigExists() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ setAsDefaultCodeOwners(user);
+
+ // Create a change as a user that is neither a code owner nor a project owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Let the project owner approve the change.
+ requestScopeOperations.setApiUser(admin.id());
+ approve(changeId);
+
+ // Verify that the file is not approved yet
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Let the code owner approve the change.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatusesAsSet(getChangeNotes(changeId));
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(FileCodeOwnerStatus.addition(path, CodeOwnerStatus.APPROVED));
+ }
+
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/CodeOwnerScoringsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
index 1bb7090..daa2f41 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import java.util.ArrayList;
import org.junit.Test;
@@ -25,7 +26,7 @@
/** Tests for {@link CodeOwnerScorings}. */
public class CodeOwnerScoringsTest extends AbstractCodeOwnersTest {
@Test
- public void sortCodeOwnersByLowerValueIsBetterScore() throws Exception {
+ public void getScorings_lowerValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -43,12 +44,20 @@
.putValueForCodeOwner(codeOwner2, 100)
.putValueForCodeOwner(codeOwner3, 0)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner3, codeOwner1, codeOwner2).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5,
+ codeOwner2,
+ 0.0,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0);
}
@Test
- public void sortCodeOwnersByGreaterValueIsBetterScore() throws Exception {
+ public void getScorings_greaterValueIsBetterScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -63,12 +72,18 @@
.putValueForCodeOwner(codeOwner1, 0)
.putValueForCodeOwner(codeOwner2, 1)
.build();
- codeOwners.sort(CodeOwnerScorings.create(isReviewerScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByMultipleScore() throws Exception {
+ public void getScorings_multipleScore() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
@@ -95,9 +110,6 @@
.putValueForCodeOwner(codeOwner3, NO_REVIEWER_SCORING_VALUE)
.build();
- codeOwners.sort(
- CodeOwnerScorings.create(distanceScoring, isReviewerScoring).comparingByScorings());
-
// Expected scorings:
// codeOwner1: DISTANCE(weight=1)=0.5, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 0.5 * 1 + 0.0 * 2 = 0.5
@@ -105,12 +117,23 @@
// -> total scoring: 0.25 * 1 + 1.0 * 2 = 2.25
// codeOwner3: DISTANCE(weight=1)=1.0, IS_REVIEWER(weight=2)=0.0
// -> total scoring: 1.0 * 1 + 0.0 * 2 = 1.0
- // => expected order (highest total score first): codeOwner2, codeOwner3, codeOwner1
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner3, codeOwner1).inOrder();
+ CodeOwnerScorings codeOwnerScorings =
+ CodeOwnerScorings.create(distanceScoring, isReviewerScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2, codeOwner3)))
+ .containsExactly(
+ codeOwner1,
+ CodeOwnerScore.DISTANCE.weight() * 0.5
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE,
+ codeOwner2,
+ CodeOwnerScore.DISTANCE.weight() * 0.25
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.IS_REVIEWER_SCORING_VALUE,
+ codeOwner3,
+ CodeOwnerScore.DISTANCE.weight() * 1.0
+ + CodeOwnerScore.IS_REVIEWER.weight() * CodeOwnerScore.NO_REVIEWER_SCORING_VALUE);
}
@Test
- public void sortCodeOwnersByScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
+ public void getScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
CodeOwner codeOwner1 = CodeOwner.create(admin.id());
CodeOwner codeOwner2 = CodeOwner.create(user.id());
@@ -122,7 +145,9 @@
CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
.putValueForCodeOwner(codeOwner2, 50)
.build();
- codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+
+ CodeOwnerScorings codeOwnerScorings = CodeOwnerScorings.create(distanceScoring);
+ assertThat(codeOwnerScorings.getScorings(ImmutableSet.of(codeOwner1, codeOwner2)))
+ .containsExactly(codeOwner1, 0.0, codeOwner2, 0.5);
}
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 40fb545..c937cb2 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -459,6 +459,7 @@
| `limit`\|`n` | optional | Limit defining how many code owners should be returned at most. By default 10.
| `seed` | optional | Seed, as a long value, that should be used to shuffle code owners that have the same score. Can be used to make the sort order stable across several requests, e.g. to get the same set of random code owners for different file paths that have the same code owners. Important: the sort order is only stable if the requests use the same seed **and** the same limit. In addition, the sort order is not guaranteed to be stable if new accounts are created in between the requests, or if the account visibility is changed.
| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `false` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
+| `highest-score-only` | optional | Whether only code owners with the highest score should be returned. If not set, `false` by default.
| `revision` | optional | Revision from which the code owner configs should be read as commit SHA1. Can be used to read historic code owners from this branch, but imports from other branches or repositories as well as default and global code owners from `refs/meta/config` are still read from the current revisions. If not specified the code owner configs are read from the HEAD revision of the branch. Not supported for getting code owners for a path in a change.
As a response a [CodeOwnersInfo](#code-owners-info) entity is returned that
@@ -657,9 +658,11 @@
The following request parameters can be specified:
-| Field Name | | Description |
-| ----------- | --------- | ----------- |
-| `user` | mandatory | user for which the owned paths should be returned
+| Field Name | | Description |
+| ------------ | --------- | ----------- |
+| `start`\|`S` | optional | Number of owned paths to skip. Allows to page over the owned files. By default 0.
+| `limit`\|`n` | optional | Limit defining how many owned files should be returned at most. By default 50.
+| `user` | mandatory | user for which the owned paths should be returned
#### Request
@@ -960,9 +963,10 @@
The `OwnedPathsInfo` entity contains paths that are owned by a user.
-| Field Name | Description |
-| ------------- | ----------- |
-| `owned_paths` | The owned paths as absolute paths, sorted alphabetically.
+| Field Name | | Description |
+| ------------- | -------- | ----------- |
+| `owned_paths` | |The owned paths as absolute paths, sorted alphabetically.
+| `more` | optional | Whether the request would deliver more results if not limited. Not set if `false`.
### <a id="path-code-owner-status-info"> PathCodeOwnerStatusInfo
The `PathCodeOwnerStatusInfo` entity describes the code owner status for a path
