Make GetOwnedPaths REST endpoint more useable
At the moment the GetOwnedPaths REST endpoint is rather simple, it
returns a plain list of all paths that were touched in the revision and
that are owned by the specified user.
This plain list of owned paths mixes old and new paths from additions,
deletions and renames.
The limit was applied to the number of owned paths, e.g. if a revision
contained a rename and a file addition and all paths were owned by the
user, and the REST endpoint was invoked with limit = 2, it was returning
the old and the new path for the rename, but not the path for the
addition (since the limit was already exhausted by the 2 paths of the
rename).
This made it difficult to use this REST endpoint in the frontend. E.g.
an intended use case is to highlight all files in the file list that are
owned by the current user. If a change touches many files, the change
screen only shows the first 250 files. This means the frontend would
like to check for these 250 files whether they are owned by the current
user. Setting the limit to 250 on the GetOwnedPath REST endpoint would
result in too few owned paths returned if the revision contains
renames where new and old path are both owned by the user (in the change
screen renames count as 1 entry, while the GetOwnedPath REST endpoint
may count renames as 2 entries).
Now the frontend could try to get owned paths with a higher limit to
compensate this, but this wouldn't work reliably.
Another use case would be to offer a UI that lists the files that are
owned by a user with pagination. Since the returned list of owned paths
mixes new and old paths, the ordering would not be consistent across
pages. E.g. the first page would contain all owned path from the first
n changed files and the second page would contain all owned paths from
the second n changed files. Now each page is sorted alphabetically but
the first page may include old paths from renames, which would logically
sort only after entries from the second page (e.g. imagine a rename from
xyz/foo.md to abc/foo.md). To implement paging properly, the UI would
need to show the changed files in the same order as on the change
screen, e.g. show only 1 entry for a rename, but doing this is difficult
since from the list of owned paths the caller doesn't know which ones
are new and old paths and which 2 paths belong to 1 rename.
To make this easier, we add a new field to the response that returns
owner information by changed files, which can be additions, deletions
but also renames. The entities in this field have the same order as the
changed files on the change screen. The limit on the request is now
appliying to the number of entities in this list rather than in the
owned paths list. This way renames count only as 1 entry, which matches
the counting on the change screen. Changed files where the user neither
owns the new or old path are omitted. For each changed file in the
response we return the new and the old path (if present), regardless of
whether the user owns them, so that the caller can detect renames. For
the new and old path we set a marker if the file is owned by the user so
that the caller can know for renames which of the 2 paths the user owns.
For backwards compatibility reasons we keep the old plain list of owned
paths. Now it just contains all new and old owned paths that are
returned as part of the new structure that represents owned paths. There
may even be some use-cases where renames do not matter and a consumption
of this plain list is just easier.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I2d538ba25323fcfecbd6346d4507293902548383
diff --git a/java/com/google/gerrit/plugins/codeowners/api/OwnedChangedFileInfo.java b/java/com/google/gerrit/plugins/codeowners/api/OwnedChangedFileInfo.java
new file mode 100644
index 0000000..971a823
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/api/OwnedChangedFileInfo.java
@@ -0,0 +1,35 @@
+// 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.api;
+
+/**
+ * JSON representation of a file that was changed in a change for which the user owns the new path,
+ * the old path or both paths.
+ */
+public class OwnedChangedFileInfo {
+ /**
+ * Owner information for the new path.
+ *
+ * <p>Not set for deletions.
+ */
+ public OwnedPathInfo newPath;
+
+ /**
+ * Owner information for the old path.
+ *
+ * <p>Only set for deletions and renames.
+ */
+ public OwnedPathInfo oldPath;
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathInfo.java b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathInfo.java
new file mode 100644
index 0000000..bda0e27
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathInfo.java
@@ -0,0 +1,28 @@
+// 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.api;
+
+/** JSON representation of a file path the may be owned by the user. */
+public class OwnedPathInfo {
+ /** The path of the file that may be owned by the user. */
+ public String path;
+
+ /**
+ * Whether the user owns this path.
+ *
+ * <p>Not set if {@code false}.
+ */
+ public Boolean owned;
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
index ba41418..98e9e90 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/OwnedPathsInfo.java
@@ -24,11 +24,25 @@
*/
public class OwnedPathsInfo {
/**
- * List of the owned paths.
+ * List of files that were changed in a change for which the user owns the new path, the old path
+ * or both paths.
+ *
+ * <p>The entries are sorted alphabetically by new path, and by old path if new path is not
+ * present.
+ *
+ * <p>Contains at most as many entries as the limit that was specified on the request.
+ */
+ public List<OwnedChangedFileInfo> ownedChangedFiles;
+
+ /**
+ * The list of the owned new and old paths that are contained in {@link #ownedChangedFiles}.
*
* <p>The paths are returned as absolute paths.
*
* <p>The paths are sorted alphabetically.
+ *
+ * <p>May contain more entries than the limit that was specified on the request (if the users owns
+ * new and old path of renamed files).
*/
public List<String> ownedPaths;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 6988acb..f015cb0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -129,7 +129,7 @@
* @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(
+ public ImmutableList<OwnedChangedFile> getOwnedPaths(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
throws ResourceConflictException {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
@@ -142,25 +142,45 @@
patchSet.id().get(),
start,
limit);
- Stream<Path> ownedPaths =
+
+ Stream<FileCodeOwnerStatus> fileStatuses =
getFileStatusesForAccount(changeNotes, patchSet, accountId)
- .flatMap(
- fileCodeOwnerStatus ->
- Stream.of(
- fileCodeOwnerStatus.newPathStatus(),
- fileCodeOwnerStatus.oldPathStatus())
- .filter(Optional::isPresent)
- .map(Optional::get))
.filter(
- pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
- .map(PathCodeOwnerStatus::path);
+ fileStatus ->
+ (fileStatus.newPathStatus().isPresent()
+ && fileStatus.newPathStatus().get().status()
+ == CodeOwnerStatus.APPROVED)
+ || (fileStatus.oldPathStatus().isPresent()
+ && fileStatus.oldPathStatus().get().status()
+ == CodeOwnerStatus.APPROVED));
if (start > 0) {
- ownedPaths = ownedPaths.skip(start);
+ fileStatuses = fileStatuses.skip(start);
}
if (limit > 0) {
- ownedPaths = ownedPaths.limit(limit);
+ fileStatuses = fileStatuses.limit(limit);
}
- return ownedPaths.collect(toImmutableList());
+
+ return fileStatuses
+ .map(
+ fileStatus ->
+ OwnedChangedFile.create(
+ fileStatus
+ .newPathStatus()
+ .map(
+ newPathStatus ->
+ OwnedPath.create(
+ newPathStatus.path(),
+ newPathStatus.status() == CodeOwnerStatus.APPROVED))
+ .orElse(null),
+ fileStatus
+ .oldPathStatus()
+ .map(
+ oldPathStatus ->
+ OwnedPath.create(
+ oldPathStatus.path(),
+ oldPathStatus.status() == CodeOwnerStatus.APPROVED))
+ .orElse(null)))
+ .collect(toImmutableList());
} catch (IOException | DiffNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 3cc8401..a694ce5 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -211,12 +211,13 @@
try {
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- reviewerAccountId,
- /* start= */ 0,
- limit + 1);
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ 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 2383de2..5b6f6dd 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -124,12 +124,13 @@
try {
// limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- user.getAccountId(),
- /* start= */ 0,
- limit + 1);
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ 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/backend/OwnedChangedFile.java b/java/com/google/gerrit/plugins/codeowners/backend/OwnedChangedFile.java
new file mode 100644
index 0000000..edb3894
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OwnedChangedFile.java
@@ -0,0 +1,79 @@
+// 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.backend;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
+import java.nio.file.Path;
+import java.util.Comparator;
+import java.util.Optional;
+import java.util.stream.Stream;
+
+/**
+ * Representation of a file that was changed in the revision for which the user owns the new path,
+ * the old path or both paths.
+ */
+@AutoValue
+public abstract class OwnedChangedFile {
+ /**
+ * Owner information for the new path.
+ *
+ * <p>{@link Optional#empty()} for deletions.
+ */
+ public abstract Optional<OwnedPath> newPath();
+
+ /**
+ * Owner information for the old path.
+ *
+ * <p>Present only for deletions and renames.
+ */
+ public abstract Optional<OwnedPath> oldPath();
+
+ public static OwnedChangedFile create(@Nullable OwnedPath newPath, @Nullable OwnedPath oldPath) {
+ return new AutoValue_OwnedChangedFile(
+ Optional.ofNullable(newPath), Optional.ofNullable(oldPath));
+ }
+
+ /**
+ * Returns the owned paths that are contained in the given {@link OwnedChangedFile}s as new or old
+ * path, as a sorted list.
+ *
+ * <p>New or old paths that are not owned by the user are filtered out.
+ */
+ public static ImmutableList<Path> getOwnedPaths(
+ ImmutableList<OwnedChangedFile> ownedChangedFiles) {
+ return asPathStream(ownedChangedFiles.stream()).collect(toImmutableList());
+ }
+
+ /**
+ * Returns the owned paths that are contained in the given {@link OwnedChangedFile}s as new or old
+ * path, as a sorted stream.
+ *
+ * <p>New or old paths that are not owned by the user are filtered out.
+ */
+ public static Stream<Path> asPathStream(Stream<OwnedChangedFile> ownedChangedFiles) {
+ return ownedChangedFiles
+ .flatMap(
+ ownedChangedFile -> Stream.of(ownedChangedFile.newPath(), ownedChangedFile.oldPath()))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .filter(OwnedPath::owned)
+ .map(ownedPath -> ownedPath.path())
+ .sorted(Comparator.comparing(path -> path.toString()));
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OwnedPath.java b/java/com/google/gerrit/plugins/codeowners/backend/OwnedPath.java
new file mode 100644
index 0000000..c6283a3
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OwnedPath.java
@@ -0,0 +1,32 @@
+// 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.backend;
+
+import com.google.auto.value.AutoValue;
+import java.nio.file.Path;
+
+/** Representation of a file path the may be owned by the user. */
+@AutoValue
+public abstract class OwnedPath {
+ /** The path of the file that may be owned by the user. */
+ public abstract Path path();
+
+ /** Whether the user owns this path. */
+ public abstract boolean owned();
+
+ public static OwnedPath create(Path path, boolean owned) {
+ return new AutoValue_OwnedPath(path, owned);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index 1f2298b..c6a447b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -24,8 +24,12 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.plugins.codeowners.api.OwnedChangedFileInfo;
+import com.google.gerrit.plugins.codeowners.api.OwnedPathInfo;
import com.google.gerrit.plugins.codeowners.api.OwnedPathsInfo;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerApprovalCheck;
+import com.google.gerrit.plugins.codeowners.backend.OwnedChangedFile;
+import com.google.gerrit.plugins.codeowners.backend.OwnedPath;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
import com.google.gerrit.server.change.RevisionResource;
@@ -89,7 +93,7 @@
Account.Id accountId = resolveAccount();
- ImmutableList<Path> ownedPaths =
+ ImmutableList<OwnedChangedFile> ownedChangedFiles =
codeOwnerApprovalCheck.getOwnedPaths(
revisionResource.getNotes(),
revisionResource.getPatchSet(),
@@ -98,9 +102,16 @@
limit + 1);
OwnedPathsInfo ownedPathsInfo = new OwnedPathsInfo();
- ownedPathsInfo.more = ownedPaths.size() > limit ? true : null;
+ ownedPathsInfo.more = ownedChangedFiles.size() > limit ? true : null;
+ ownedPathsInfo.ownedChangedFiles =
+ ownedChangedFiles.stream()
+ .limit(limit)
+ .map(GetOwnedPaths::toOwnedChangedFileInfo)
+ .collect(toImmutableList());
ownedPathsInfo.ownedPaths =
- ownedPaths.stream().limit(limit).map(Path::toString).collect(toImmutableList());
+ OwnedChangedFile.asPathStream(ownedChangedFiles.stream().limit(limit))
+ .map(Path::toString)
+ .collect(toImmutableList());
return Response.ok(ownedPathsInfo);
}
@@ -122,4 +133,22 @@
throw new BadRequestException("limit must be positive");
}
}
+
+ private static OwnedChangedFileInfo toOwnedChangedFileInfo(OwnedChangedFile ownedChangedFile) {
+ OwnedChangedFileInfo info = new OwnedChangedFileInfo();
+ if (ownedChangedFile.newPath().isPresent()) {
+ info.newPath = toOwnedPathInfo(ownedChangedFile.newPath().get());
+ }
+ if (ownedChangedFile.oldPath().isPresent()) {
+ info.oldPath = toOwnedPathInfo(ownedChangedFile.oldPath().get());
+ }
+ return info;
+ }
+
+ private static OwnedPathInfo toOwnedPathInfo(OwnedPath ownedPath) {
+ OwnedPathInfo info = new OwnedPathInfo();
+ info.path = ownedPath.path().toString();
+ info.owned = ownedPath.owned() ? true : null;
+ return info;
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java
new file mode 100644
index 0000000..7897058
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OwnedChangedFileInfoSubject.java
@@ -0,0 +1,88 @@
+// 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.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.Subject;
+import com.google.gerrit.plugins.codeowners.api.OwnedChangedFileInfo;
+
+/** {@link Subject} for doing assertions on {@link OwnedChangedFileInfo}s. */
+public class OwnedChangedFileInfoSubject extends Subject {
+ /**
+ * Starts fluent chain to do assertions on a {@link OwnedChangedFileInfo}.
+ *
+ * @param ownedChangedFileInfo the owned changed file info on which assertions should be done
+ * @return the created {@link OwnedChangedFileInfoSubject}
+ */
+ public static OwnedChangedFileInfoSubject assertThat(OwnedChangedFileInfo ownedChangedFileInfo) {
+ return assertAbout(ownedChangedFileInfos()).that(ownedChangedFileInfo);
+ }
+
+ private static Factory<OwnedChangedFileInfoSubject, OwnedChangedFileInfo>
+ ownedChangedFileInfos() {
+ return OwnedChangedFileInfoSubject::new;
+ }
+
+ private final OwnedChangedFileInfo ownedChangedFileInfo;
+
+ private OwnedChangedFileInfoSubject(
+ FailureMetadata metadata, OwnedChangedFileInfo ownedChangedFileInfo) {
+ super(metadata, ownedChangedFileInfo);
+ this.ownedChangedFileInfo = ownedChangedFileInfo;
+ }
+
+ public void hasEmptyNewPath() {
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath).isNull();
+ }
+
+ public void hasOwnedNewPath(String expectedOwnedNewPath) {
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath).isNotNull();
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath.path).isEqualTo(expectedOwnedNewPath);
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath.owned).isTrue();
+ }
+
+ public void hasNonOwnedNewPath(String expectedNonOwnedNewPath) {
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath).isNotNull();
+ check("ownedNewPath")
+ .that(ownedChangedFileInfo().newPath.path)
+ .isEqualTo(expectedNonOwnedNewPath);
+ check("ownedNewPath").that(ownedChangedFileInfo().newPath.owned).isNull();
+ }
+
+ public void hasEmptyOldPath() {
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath).isNull();
+ }
+
+ public void hasOwnedOldPath(String expectedOwnedOldPath) {
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath).isNotNull();
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath.path).isEqualTo(expectedOwnedOldPath);
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath.owned).isTrue();
+ }
+
+ public void hasNonOwnedOldPath(String expectedNonOwnedOldPath) {
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath).isNotNull();
+ check("ownedOldPath")
+ .that(ownedChangedFileInfo().oldPath.path)
+ .isEqualTo(expectedNonOwnedOldPath);
+ check("ownedOldPath").that(ownedChangedFileInfo().oldPath.owned).isNull();
+ }
+
+ private OwnedChangedFileInfo ownedChangedFileInfo() {
+ isNotNull();
+ return ownedChangedFileInfo;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
index d6a1072..08b762a 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/OwnedPathsInfoSubject.java
@@ -49,6 +49,10 @@
return check("ownedPaths()").that(ownedPathsInfo().ownedPaths);
}
+ public IterableSubject hasOwnedChangedFilesThat() {
+ return check("ownedChangedFiles()").that(ownedPathsInfo().ownedChangedFiles);
+ }
+
public BooleanSubject hasMoreThat() {
return check("more()").that(ownedPathsInfo().more);
}
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 681513b..5b19b9a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetOwnedPathsIT.java
@@ -15,13 +15,18 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.codeowners.testing.OwnedChangedFileInfoSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.OwnedPathsInfoSubject.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
+import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
@@ -29,6 +34,9 @@
import com.google.gerrit.plugins.codeowners.restapi.GetOwnedPaths;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
import org.junit.Test;
/**
@@ -130,11 +138,155 @@
.getOwnedPaths()
.forUser(user.email())
.get();
+
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(path1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasEmptyOldPath();
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedNewPath(path2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasEmptyOldPath();
+
assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
assertThat(ownedPathsInfo).hasMoreThat().isNull();
}
@Test
+ public void getOwnedPathsForDeletedFiles() throws Exception {
+ setAsCodeOwners("/foo/", user);
+
+ String path1 = "/foo/bar/baz.md";
+ String path2 = "/foo/baz/bar.md";
+ String path3 = "/bar/foo.md";
+
+ createChange(
+ "Change Adding Files",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content 1",
+ JgitPath.of(path2).get(),
+ "file content 2",
+ JgitPath.of(path3).get(),
+ "file content 3"))
+ .getChangeId();
+
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Change Deleting Files",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content 1",
+ JgitPath.of(path2).get(),
+ "file content 2",
+ JgitPath.of(path3).get(),
+ "file content 3"));
+ Result r = push.rm("refs/for/master");
+ r.assertOkStatus();
+ String changeId = r.getChangeId();
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId)
+ .current()
+ .getOwnedPaths()
+ .forUser(user.email())
+ .get();
+
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasEmptyNewPath();
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedOldPath(path1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasEmptyNewPath();
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedOldPath(path2);
+
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactly(path1, path2).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getOwnedPathsForRenamedFiles() throws Exception {
+ setAsCodeOwners("/foo/", user);
+
+ // Rename 1: user owns old and new path
+ String newPath1 = "/foo/test1.md";
+ String oldPath1 = "/foo/bar/test1.md";
+
+ // Rename 2: user owns only new path
+ String newPath2 = "/foo/test2.md";
+ String oldPath2 = "/other/test2.md";
+
+ // Rename 3: user owns only old path
+ String newPath3 = "/other/test3.md";
+ String oldPath3 = "/foo/test3.md";
+
+ // Rename 4: user owns neither old nor new path
+ String newPath4 = "/other/test4.md";
+ String oldPath4 = "/other/foo/test4.md";
+
+ String changeId1 =
+ createChange(
+ "Change Adding Files",
+ ImmutableMap.of(
+ JgitPath.of(oldPath1).get(),
+ "file content 1",
+ JgitPath.of(oldPath2).get(),
+ "file content 2",
+ JgitPath.of(oldPath3).get(),
+ "file content 3",
+ JgitPath.of(oldPath4).get(),
+ "file content 4"))
+ .getChangeId();
+
+ // The PushOneCommit test API doesn't support renaming files in a change. Use the change edit
+ // Java API instead.
+ ChangeInput changeInput = new ChangeInput();
+ changeInput.project = project.get();
+ changeInput.branch = "master";
+ changeInput.subject = "Change Renaming Files";
+ changeInput.baseChange = changeId1;
+ String changeId2 = gApi.changes().create(changeInput).get().changeId;
+ gApi.changes().id(changeId2).edit().create();
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath1).get(), JgitPath.of(newPath1).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath2).get(), JgitPath.of(newPath2).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath3).get(), JgitPath.of(newPath3).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath4).get(), JgitPath.of(newPath4).get());
+ gApi.changes().id(changeId2).edit().publish(new PublishChangeEditInput());
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId2)
+ .current()
+ .getOwnedPaths()
+ .forUser(user.email())
+ .get();
+
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(3);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(newPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedOldPath(oldPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedNewPath(newPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasNonOwnedOldPath(oldPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(2)).hasNonOwnedNewPath(newPath3);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(2)).hasOwnedOldPath(oldPath3);
+
+ List<String> ownedPaths = Arrays.asList(newPath1, oldPath1, newPath2, oldPath3);
+ Collections.sort(ownedPaths);
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactlyElementsIn(ownedPaths).inOrder();
+
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
public void getOwnedPathForOwnUser() throws Exception {
setAsRootCodeOwners(admin);
@@ -371,6 +523,203 @@
}
@Test
+ public void getOwnedPathsForRenamedFilesWithLimit() throws Exception {
+ setAsCodeOwners("/foo/", user);
+
+ // Rename 1: user owns old and new path
+ String newPath1 = "/foo/test1.md";
+ String oldPath1 = "/foo/bar/test1.md";
+
+ // Rename 2: user owns only new path
+ String newPath2 = "/foo/test2.md";
+ String oldPath2 = "/other/test2.md";
+
+ // Rename 3: user owns only old path
+ String newPath3 = "/other/test3.md";
+ String oldPath3 = "/foo/test3.md";
+
+ // Rename 4: user owns neither old nor new path
+ String newPath4 = "/other/test4.md";
+ String oldPath4 = "/other/foo/test4.md";
+
+ String changeId1 =
+ createChange(
+ "Change Adding Files",
+ ImmutableMap.of(
+ JgitPath.of(oldPath1).get(),
+ "file content 1",
+ JgitPath.of(oldPath2).get(),
+ "file content 2",
+ JgitPath.of(oldPath3).get(),
+ "file content 3",
+ JgitPath.of(oldPath4).get(),
+ "file content 4"))
+ .getChangeId();
+
+ // The PushOneCommit test API doesn't support renaming files in a change. Use the change edit
+ // Java API instead.
+ ChangeInput changeInput = new ChangeInput();
+ changeInput.project = project.get();
+ changeInput.branch = "master";
+ changeInput.subject = "Change Renaming Files";
+ changeInput.baseChange = changeId1;
+ String changeId2 = gApi.changes().create(changeInput).get().changeId;
+ gApi.changes().id(changeId2).edit().create();
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath1).get(), JgitPath.of(newPath1).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath2).get(), JgitPath.of(newPath2).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath3).get(), JgitPath.of(newPath3).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath4).get(), JgitPath.of(newPath4).get());
+ gApi.changes().id(changeId2).edit().publish(new PublishChangeEditInput());
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId2)
+ .current()
+ .getOwnedPaths()
+ .withLimit(1)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(newPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedOldPath(oldPath1);
+ List<String> ownedPaths = Arrays.asList(newPath1, oldPath1);
+ Collections.sort(ownedPaths);
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactlyElementsIn(ownedPaths);
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId2)
+ .current()
+ .getOwnedPaths()
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(newPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedOldPath(oldPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedNewPath(newPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasNonOwnedOldPath(oldPath2);
+ ownedPaths = Arrays.asList(newPath1, oldPath1, newPath2);
+ Collections.sort(ownedPaths);
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactlyElementsIn(ownedPaths).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isTrue();
+
+ ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId2)
+ .current()
+ .getOwnedPaths()
+ .withLimit(3)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(3);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(newPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedOldPath(oldPath1);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedNewPath(newPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasNonOwnedOldPath(oldPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(2)).hasNonOwnedNewPath(newPath3);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(2)).hasOwnedOldPath(oldPath3);
+ ownedPaths = Arrays.asList(newPath1, oldPath1, newPath2, oldPath3);
+ Collections.sort(ownedPaths);
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactlyElementsIn(ownedPaths).inOrder();
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
+ public void getOwnedPathsForRenamedFilesWithStartAndLimit() throws Exception {
+ setAsCodeOwners("/foo/", user);
+
+ // Rename 1: user owns old and new path
+ String newPath1 = "/foo/test1.md";
+ String oldPath1 = "/foo/bar/test1.md";
+
+ // Rename 2: user owns only new path
+ String newPath2 = "/foo/test2.md";
+ String oldPath2 = "/other/test2.md";
+
+ // Rename 3: user owns only old path
+ String newPath3 = "/other/test3.md";
+ String oldPath3 = "/foo/test3.md";
+
+ // Rename 4: user owns neither old nor new path
+ String newPath4 = "/other/test4.md";
+ String oldPath4 = "/other/foo/test4.md";
+
+ String changeId1 =
+ createChange(
+ "Change Adding Files",
+ ImmutableMap.of(
+ JgitPath.of(oldPath1).get(),
+ "file content 1",
+ JgitPath.of(oldPath2).get(),
+ "file content 2",
+ JgitPath.of(oldPath3).get(),
+ "file content 3",
+ JgitPath.of(oldPath4).get(),
+ "file content 4"))
+ .getChangeId();
+
+ // The PushOneCommit test API doesn't support renaming files in a change. Use the change edit
+ // Java API instead.
+ ChangeInput changeInput = new ChangeInput();
+ changeInput.project = project.get();
+ changeInput.branch = "master";
+ changeInput.subject = "Change Renaming Files";
+ changeInput.baseChange = changeId1;
+ String changeId2 = gApi.changes().create(changeInput).get().changeId;
+ gApi.changes().id(changeId2).edit().create();
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath1).get(), JgitPath.of(newPath1).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath2).get(), JgitPath.of(newPath2).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath3).get(), JgitPath.of(newPath3).get());
+ gApi.changes()
+ .id(changeId2)
+ .edit()
+ .renameFile(JgitPath.of(oldPath4).get(), JgitPath.of(newPath4).get());
+ gApi.changes().id(changeId2).edit().publish(new PublishChangeEditInput());
+
+ OwnedPathsInfo ownedPathsInfo =
+ changeCodeOwnersApiFactory
+ .change(changeId2)
+ .current()
+ .getOwnedPaths()
+ .withStart(1)
+ .withLimit(2)
+ .forUser(user.email())
+ .get();
+ assertThat(ownedPathsInfo).hasOwnedChangedFilesThat().hasSize(2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasOwnedNewPath(newPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(0)).hasNonOwnedOldPath(oldPath2);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasNonOwnedNewPath(newPath3);
+ assertThat(ownedPathsInfo.ownedChangedFiles.get(1)).hasOwnedOldPath(oldPath3);
+ List<String> ownedPaths = Arrays.asList(newPath2, oldPath3);
+ Collections.sort(ownedPaths);
+ assertThat(ownedPathsInfo).hasOwnedPathsThat().containsExactlyElementsIn(ownedPaths);
+ assertThat(ownedPathsInfo).hasMoreThat().isNull();
+ }
+
+ @Test
public void getOwnedPathsLimitedByDefault() throws Exception {
setAsRootCodeOwners(user);
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 6e3d739..d42cf50 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -729,7 +729,7 @@
| 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.
+| `limit`\|`n` | optional | Limit defining how many [OwnedChangedFileInfo](#owned-changed-file-info) entities should be returned at most. By default 50.
| `user` | mandatory | user for which the owned paths should be returned
#### Request
@@ -749,9 +749,45 @@
)]}'
{
+ "owned_changed_files": [
+ {
+ "new_path": {
+ "path": "/foo/bar/baz.md",
+ "owned": true
+ }
+ },
+ {
+ "old_path": {
+ "path": "/foo/baz/bar.md",
+ "owned": true
+ }
+ },
+ {
+ "new_path": {
+ "path": "/foo/new-name.md",
+ "owned": true
+ },
+ "old_path": {
+ "path": "/foo/old-name.md",
+ "owned": true
+ }
+ },
+ {
+ "new_path": {
+ "path": "/xyz/new-name.md"
+ },
+ "old_path": {
+ "path": "/abc/old-name.md",
+ "owned": true
+ }
+ }
+ ],
"owned_paths": [
+ "/abc/old-name.md",
"/foo/bar/baz.md",
"/foo/baz/bar.md",
+ "/foo/new-name.md",
+ "/foo/old-name.md"
]
}
```
@@ -1035,13 +1071,33 @@
| `invalid_code_owner_config_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to deal with invalid code owner config files.
|`fallback_code_owners` || Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `PROJECT_OWNERS`: Paths for which no code owners are defined are owned by the project owners. `ALL_USERS`: Paths for which no code owners are defined are owned by all users.
+### <a id="owned-changed-file-info"> OwnedChangedFileInfo
+The `OwnedChangedFileInfo` entity contains information about a file that was
+changed in a change for which the user owns the new path, the old path or both
+paths.
+
+| Field Name | | Description |
+| ---------- | -------- | ----------- |
+| `new_path` | optional | Owner information for the new path as a [OwnedPathInfo](#owned-path-info) entity. Not set for deletions.
+| `old_path` | optional | Owner information for the old path as a [OwnedPathInfo](#owned-path-info) entity. Only set for deletions and renames.
+
+### <a id="owned-path-info"> OwnedPathInfo
+The `OwnedPathInfo` entity contains information about a file path the may be
+owned by the user.
+
+| Field Name | | Description |
+| ---------- | -------- | ----------- |
+| `path` | | The absolute file path.
+| `owned` | optional | `true` is the path is owned by the user. Otherwise unset.
+
### <a id="owned-paths-info"> OwnedPathsInfo
The `OwnedPathsInfo` entity contains paths that are owned by a user.
| Field Name | | Description |
| ------------- | -------- | ----------- |
-| `owned_paths` | |The owned paths as absolute paths, sorted alphabetically.
+| `owned_changed_files` || List of files that were changed in the revision for which the user owns the new path, the old path or both paths. The entries are sorted alphabetically by new path, and by old path if new path is not present. Contains at most as many entries as the limit that was specified on the request.
+| `owned_paths` | | The list of the owned new and old paths that are contained in the `owned_changed_files` field. The paths are returned as absolute paths and are sorted alphabetically. May contain more entries than the limit that was specified on the request (if the users owns new and old path of renamed files).
| `more` | optional | Whether the request would deliver more results if not limited. Not set if `false`.
### <a id="path-code-owner-status-info"> PathCodeOwnerStatusInfo
