Make sure that `Needed by` changes actually depend on the change
If a commit message mentions Change-Ids of other changes, they got
treated as `Depends on`. But the Change-Ids might get mentioned for
other reasons as well.
So we make sure that only Changes that depend on the relevant Change
get listed as `Needed by`.
Change-Id: I52c74787f23842a222777905343c202551147e47
diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcher.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcher.java
index 8586be6..6bb2d4e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcher.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcher.java
@@ -15,6 +15,8 @@
package com.googlesource.gerrit.plugins.zuul.util;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import java.io.IOException;
@@ -40,4 +42,19 @@
return commit.getFullMessage();
}
}
+
+ /**
+ * Extracts the commit message of the most current revision of a change.
+ *
+ * <p>The ChangeInfo must have the {@link CommitInfo} of at least the most current revision
+ * loaded.
+ *
+ * @param changeInfo The ChangeInfo to extract the commit message from
+ * @return the extracted commit message
+ */
+ public String fetch(ChangeInfo changeInfo) {
+ String current = changeInfo.currentRevision;
+ CommitInfo commitInfo = changeInfo.revisions.get(current).commit;
+ return commitInfo.message;
+ }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcher.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcher.java
index 1d75a60..aa772f1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcher.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcher.java
@@ -13,8 +13,8 @@
// limitations under the License.
package com.googlesource.gerrit.plugins.zuul.util;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -29,29 +29,42 @@
/** Fetches the Needed-By part of cross repository dependencies. */
public class NeededByFetcher {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final ChangesCollection changes;
+ private final CommitMessageFetcher commitMessageFetcher;
+ private final DependsOnExtractor dependsOnExtractor;
@Inject
- public NeededByFetcher(ChangesCollection changes) {
+ public NeededByFetcher(
+ ChangesCollection changes,
+ CommitMessageFetcher commitMessageFetcher,
+ DependsOnExtractor dependsOnExtractor) {
this.changes = changes;
+ this.commitMessageFetcher = commitMessageFetcher;
+ this.dependsOnExtractor = dependsOnExtractor;
}
public List<String> fetchForChangeKey(Change.Key key)
throws BadRequestException, AuthException, PermissionBackendException {
+ String keyString = key.toString();
List<String> neededBy = new ArrayList<>();
QueryChanges query = changes.list();
- String neededByQuery = "message:" + key + " -change:" + key;
+ String neededByQuery = "message:" + keyString + " -change:" + keyString;
+ query.addOption(ListChangesOption.CURRENT_REVISION);
+ query.addOption(ListChangesOption.CURRENT_COMMIT);
query.addQuery(neededByQuery);
Response<List<?>> response = query.apply(TopLevelResource.INSTANCE);
@SuppressWarnings("unchecked")
List<ChangeInfo> changes = (List<ChangeInfo>) response.value();
- for (ChangeInfo other : changes) {
- String otherKey = other.changeId;
- logger.atFinest().log("Change %s needed by %s", key, otherKey);
- neededBy.add(otherKey);
+ for (ChangeInfo changeInfo : changes) {
+ // The search found the key somewhere in the commit message. But this need not be a
+ // `Depends-On`. `key` might be mentioned for a completely different reason. So we need to
+ // check if `key` occurs in a `Depends-On`.
+ String commitMessage = commitMessageFetcher.fetch(changeInfo);
+ List<String> dependencies = dependsOnExtractor.extract(commitMessage);
+ if (dependencies.contains(keyString)) {
+ neededBy.add(changeInfo.changeId);
+ }
}
return neededBy;
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcherTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcherTest.java
index db228f3..905683c 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcherTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/CommitMessageFetcherTest.java
@@ -21,9 +21,13 @@
import static org.mockito.Mockito.when;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.server.git.GitRepositoryManager;
import java.io.IOException;
import java.math.BigInteger;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;
import org.eclipse.jgit.errors.MissingObjectException;
@@ -84,6 +88,34 @@
assertThat(commitMessage).isEqualTo("CommitMsg\n");
}
+ @Test
+ public void testFetchChangeInfoSingleRevision() {
+ ChangeInfo changeInfo = new ChangeInfo();
+ changeInfo.currentRevision = "foo";
+ changeInfo.revisions = new HashMap<>();
+ changeInfo.revisions.put("foo", createRevisionInfo("CommitMsg"));
+
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetch(changeInfo);
+
+ assertThat(commitMessage).isEqualTo("CommitMsg");
+ }
+
+ @Test
+ public void testFetchChangeInfoMultipleRevisions() {
+ ChangeInfo changeInfo = new ChangeInfo();
+ changeInfo.currentRevision = "bar";
+ changeInfo.revisions = new HashMap<>();
+ changeInfo.revisions.put("foo", createRevisionInfo("CommitMsgFoo"));
+ changeInfo.revisions.put("bar", createRevisionInfo("CommitMsgBar"));
+ changeInfo.revisions.put("baz", createRevisionInfo("CommitMsgBaz"));
+
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetch(changeInfo);
+
+ assertThat(commitMessage).isEqualTo("CommitMsgBar");
+ }
+
@Before
public void setUp() throws Exception {
ObjectLoader objectLoaderBlob = mock(ObjectLoader.class);
@@ -117,6 +149,16 @@
when(repoManager.openRepository(eq(Project.nameKey("ProjectFoo")))).thenReturn(repo);
}
+ private RevisionInfo createRevisionInfo(String commitMessage) {
+ CommitInfo commitInfo = new CommitInfo();
+ commitInfo.message = commitMessage;
+
+ RevisionInfo revisionInfo = new RevisionInfo();
+ revisionInfo.commit = commitInfo;
+
+ return revisionInfo;
+ }
+
private CommitMessageFetcher createCommitMessageFetcher() {
return new CommitMessageFetcher(repoManager);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcherTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcherTest.java
index 1d51857..51b90c8 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcherTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcherTest.java
@@ -14,11 +14,14 @@
package com.googlesource.gerrit.plugins.zuul.util;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.TopLevelResource;
@@ -33,6 +36,8 @@
public class NeededByFetcherTest {
private ChangesCollection changes;
+ private CommitMessageFetcher commitMessageFetcher;
+ private DependsOnExtractor dependsOnExtractor;
private Change.Key changeKey = getChangeKey(1);
@Test
@@ -63,6 +68,38 @@
}
@Test
+ public void testFetchForChangeKeySingleResultUnmatchedEmpty() throws Exception {
+ List<ChangeInfo> searchResult = new ArrayList<>();
+ searchResult.add(getChangeInfo(2));
+
+ configureMocks(searchResult);
+ when(dependsOnExtractor.extract("commitMessage2")).thenReturn(new ArrayList<>());
+
+ NeededByFetcher fetcher = createFetcher();
+
+ List<String> neededBy = fetcher.fetchForChangeKey(changeKey);
+
+ assertThat(neededBy).isEmpty();
+ }
+
+ @Test
+ public void testFetchForChangeKeySingleResultUnmatchedDifferent() throws Exception {
+ List<ChangeInfo> searchResult = new ArrayList<>();
+ searchResult.add(getChangeInfo(2));
+
+ configureMocks(searchResult);
+ List<String> extracted = new ArrayList<>();
+ extracted.add(getChangeKey(3).toString());
+ when(dependsOnExtractor.extract("commitMessage2")).thenReturn(extracted);
+
+ NeededByFetcher fetcher = createFetcher();
+
+ List<String> neededBy = fetcher.fetchForChangeKey(changeKey);
+
+ assertThat(neededBy).isEmpty();
+ }
+
+ @Test
public void testFetchForChangeKeyMultipleResults() throws Exception {
List<ChangeInfo> searchResult = new ArrayList<>();
searchResult.add(getChangeInfo(2));
@@ -77,18 +114,46 @@
assertThat(neededBy).containsExactly(getChangeKey(2).toString(), getChangeKey(3).toString());
}
+ @Test
+ public void testFetchForChangeKeyMultipleResultsSomeUnmatched() throws Exception {
+ List<ChangeInfo> searchResult = new ArrayList<>();
+ searchResult.add(getChangeInfo(2));
+ searchResult.add(getChangeInfo(3));
+ searchResult.add(getChangeInfo(4));
+ searchResult.add(getChangeInfo(5));
+
+ configureMocks(searchResult);
+ when(dependsOnExtractor.extract("commitMessage3")).thenReturn(new ArrayList<>());
+ when(dependsOnExtractor.extract("commitMessage4")).thenReturn(new ArrayList<>());
+
+ NeededByFetcher fetcher = createFetcher();
+
+ List<String> neededBy = fetcher.fetchForChangeKey(changeKey);
+
+ assertThat(neededBy).containsExactly(getChangeKey(2).toString(), getChangeKey(5).toString());
+ }
+
+ /**
+ * Sets up mocks for a given search result.
+ *
+ * <p>Each search result is configured to have a `Depends-On` to the changeKey per default. To
+ * refine, override the extraction for ("commitMessage" + number) to the desired list of
+ * Change-Ids.
+ *
+ * @param searchResult The search result to configure.
+ * @throws Exception thrown upon issues.
+ */
public void configureMocks(final List<ChangeInfo> searchResult) throws Exception {
QueryChanges queryChanges = mock(QueryChanges.class);
final AtomicBoolean addedQuery = new AtomicBoolean(false);
- doAnswer(
- new Answer<Void>() {
- @Override
- public Void answer(InvocationOnMock invocation) throws Throwable {
- addedQuery.getAndSet(true);
- return null;
- }
- })
- .when(queryChanges)
+ final AtomicBoolean addedOptionCurrentRevision = new AtomicBoolean(false);
+ final AtomicBoolean addedOptionCurrentCommit = new AtomicBoolean(false);
+
+ mockQueryChangesWithSwitch(queryChanges, addedOptionCurrentCommit)
+ .addOption(ListChangesOption.CURRENT_COMMIT);
+ mockQueryChangesWithSwitch(queryChanges, addedOptionCurrentRevision)
+ .addOption(ListChangesOption.CURRENT_REVISION);
+ mockQueryChangesWithSwitch(queryChanges, addedQuery)
.addQuery("message:" + changeKey + " -change:" + changeKey);
when(queryChanges.apply(TopLevelResource.INSTANCE))
.thenAnswer(
@@ -97,12 +162,50 @@
@Override
public Response<List<ChangeInfo>> answer(InvocationOnMock invocation)
throws Throwable {
- return Response.ok(addedQuery.get() ? searchResult : null);
+ boolean ready =
+ addedOptionCurrentRevision.get()
+ && addedOptionCurrentCommit.get()
+ && addedQuery.get();
+ if (!ready) {
+ fail("executed query before all options were set");
+ }
+ return Response.ok(searchResult);
}
});
changes = mock(ChangesCollection.class);
when(changes.list()).thenReturn(queryChanges);
+
+ commitMessageFetcher = mock(CommitMessageFetcher.class);
+ when(commitMessageFetcher.fetch(any(ChangeInfo.class)))
+ .thenAnswer(
+ new Answer<String>() {
+ @Override
+ public String answer(InvocationOnMock invocation) throws Throwable {
+ ChangeInfo changeInfo = invocation.getArgument(0);
+ return "commitMessage" + changeInfo._number;
+ }
+ });
+
+ dependsOnExtractor = mock(DependsOnExtractor.class);
+ List<String> extractedMatchList = new ArrayList<>();
+ extractedMatchList.add(changeKey.toString());
+ when(dependsOnExtractor.extract(any(String.class))).thenReturn(extractedMatchList);
+ }
+
+ private QueryChanges mockQueryChangesWithSwitch(
+ QueryChanges queryChanges, AtomicBoolean booleanSwitch) {
+ return doAnswer(
+ new Answer<Void>() {
+ @Override
+ public Void answer(InvocationOnMock invocation) throws Throwable {
+ if (booleanSwitch.getAndSet(true)) {
+ fail("flag has already been set");
+ }
+ return null;
+ }
+ })
+ .when(queryChanges);
}
private Change.Key getChangeKey(int keyEnding) {
@@ -112,10 +215,11 @@
private ChangeInfo getChangeInfo(int keyEnding) {
ChangeInfo changeInfo = new ChangeInfo();
changeInfo.changeId = getChangeKey(keyEnding).toString();
+ changeInfo._number = keyEnding;
return changeInfo;
}
private NeededByFetcher createFetcher() {
- return new NeededByFetcher(changes);
+ return new NeededByFetcher(changes, commitMessageFetcher, dependsOnExtractor);
}
}