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); } }