Split out Needed-By computation in helper class This splitting out simplifies the main logic a bit, but helps a lot with getting simpler tests. Change-Id: I12c12f1bbaaa725a58de3983f704edf14125739b
diff --git a/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java b/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java index 8194a2e..f9e23b7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java +++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/GetCrd.java
@@ -17,23 +17,18 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; -import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.permissions.PermissionBackendException; -import com.google.gerrit.server.restapi.change.ChangesCollection; -import com.google.gerrit.server.restapi.change.QueryChanges; import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.zuul.util.CommitMessageFetcher; - +import com.googlesource.gerrit.plugins.zuul.util.NeededByFetcher; import java.io.IOException; import java.util.ArrayList; -import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.eclipse.jgit.errors.RepositoryNotFoundException; @@ -42,27 +37,24 @@ public class GetCrd implements RestReadView<RevisionResource> { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private final ChangesCollection changes; private final CommitMessageFetcher commitMessageFetcher; + private final NeededByFetcher neededByFetcher; @Inject - GetCrd(ChangesCollection changes, CommitMessageFetcher commitMessageFetcher) { - this.changes = changes; + GetCrd(CommitMessageFetcher commitMessageFetcher, NeededByFetcher neededByFetcher) { this.commitMessageFetcher = commitMessageFetcher; + this.neededByFetcher = neededByFetcher; } @Override - @SuppressWarnings("unchecked") public Response<CrdInfo> apply(RevisionResource rsrc) throws RepositoryNotFoundException, IOException, BadRequestException, AuthException, PermissionBackendException { CrdInfo out = new CrdInfo(); - out.dependsOn = new ArrayList<>(); - out.neededBy = new ArrayList<>(); - Change.Key thisId = rsrc.getChange().getKey(); // get depends on info + out.dependsOn = new ArrayList<>(); Project.NameKey p = rsrc.getChange().getProject(); String rev = rsrc.getPatchSet().commitId().getName(); String commitMsg = commitMessageFetcher.fetch(p, rev); @@ -74,24 +66,15 @@ out.dependsOn.add(otherId); } - // get needed by info - QueryChanges query = changes.list(); - String neededByQuery = "message:" + thisId + " -change:" + thisId; - query.addQuery(neededByQuery); - Response<List<?>> response = query.apply(TopLevelResource.INSTANCE); - List<ChangeInfo> changes = (List<ChangeInfo>) response.value(); - // check for dependency cycles - for (ChangeInfo other : changes) { - String otherId = other.changeId; - logger.atFinest().log("Change %s needed by %s", thisId, otherId); - if (out.dependsOn.contains(otherId)) { - logger.atFiner().log( - "Detected dependency cycle between changes %s and %s", thisId, otherId); - out.cycle = true; - } - out.neededBy.add(otherId); - } + out.neededBy = neededByFetcher.fetchForChangeKey(thisId); + out.cycle = false; + for (String neededKey : out.neededBy) { + if (out.dependsOn.contains(neededKey)) { + out.cycle = true; + break; + } + } return Response.ok(out); } }
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 new file mode 100644 index 0000000..1d75a60 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcher.java
@@ -0,0 +1,58 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.googlesource.gerrit.plugins.zuul.util; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.Change; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.TopLevelResource; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.restapi.change.ChangesCollection; +import com.google.gerrit.server.restapi.change.QueryChanges; +import com.google.inject.Inject; +import java.util.ArrayList; +import java.util.List; + +/** Fetches the Needed-By part of cross repository dependencies. */ +public class NeededByFetcher { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final ChangesCollection changes; + + @Inject + public NeededByFetcher(ChangesCollection changes) { + this.changes = changes; + } + + public List<String> fetchForChangeKey(Change.Key key) + throws BadRequestException, AuthException, PermissionBackendException { + List<String> neededBy = new ArrayList<>(); + + QueryChanges query = changes.list(); + String neededByQuery = "message:" + key + " -change:" + key; + 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); + } + return neededBy; + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java b/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java index 3fb6b9f..bfa3cdb 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/GetCrdTest.java
@@ -14,7 +14,6 @@ package com.googlesource.gerrit.plugins.zuul; import static com.google.common.truth.Truth.assertThat; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -23,25 +22,18 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; -import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.Response; -import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.server.change.RevisionResource; -import com.google.gerrit.server.restapi.change.ChangesCollection; -import com.google.gerrit.server.restapi.change.QueryChanges; import com.googlesource.gerrit.plugins.zuul.util.CommitMessageFetcher; - +import com.googlesource.gerrit.plugins.zuul.util.NeededByFetcher; import java.sql.Timestamp; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; -import org.mockito.invocation.InvocationOnMock; -import org.mockito.stubbing.Answer; public class GetCrdTest { - private ChangesCollection changes; + private NeededByFetcher neededByFetcher; private CommitMessageFetcher commitMessageFetcher; private RevisionResource rsrc; @@ -94,9 +86,9 @@ @Test public void testSingleNeededBy() throws Exception { String commitMessage = "subject"; - List<ChangeInfo> searchResults = new ArrayList<>(); - searchResults.add(changeInfo("I00000001")); - configureMocks(commitMessage, searchResults); + List<String> neededBy = new ArrayList<>(); + neededBy.add("I00000001"); + configureMocks(commitMessage, neededBy); GetCrd getCrd = createGetCrd(); Response<CrdInfo> response = getCrd.apply(rsrc); @@ -111,11 +103,11 @@ @Test public void testMultipleNeededBy() throws Exception { String commitMessage = "subject"; - List<ChangeInfo> searchResults = new ArrayList<>(); - searchResults.add(changeInfo("I00000001")); - searchResults.add(changeInfo("I00000003")); - searchResults.add(changeInfo("I00000005")); - configureMocks(commitMessage, searchResults); + List<String> neededBy = new ArrayList<>(); + neededBy.add("I00000001"); + neededBy.add("I00000003"); + neededBy.add("I00000005"); + configureMocks(commitMessage, neededBy); GetCrd getCrd = createGetCrd(); Response<CrdInfo> response = getCrd.apply(rsrc); @@ -130,10 +122,10 @@ @Test public void testMixed() throws Exception { String commitMessage = "subject\nDepends-On: I00000002\nDepends-On: I00000004"; - List<ChangeInfo> searchResults = new ArrayList<>(); - searchResults.add(changeInfo("I00000001")); - searchResults.add(changeInfo("I00000003")); - configureMocks(commitMessage, searchResults); + List<String> neededBy = new ArrayList<>(); + neededBy.add("I00000001"); + neededBy.add("I00000003"); + configureMocks(commitMessage, neededBy); GetCrd getCrd = createGetCrd(); Response<CrdInfo> response = getCrd.apply(rsrc); @@ -148,9 +140,9 @@ @Test public void testSimpleCycle() throws Exception { String commitMessage = "subject\nDepends-On: I00000001"; - List<ChangeInfo> searchResults = new ArrayList<>(); - searchResults.add(changeInfo("I00000001")); - configureMocks(commitMessage, searchResults); + List<String> neededBy = new ArrayList<>(); + neededBy.add("I00000001"); + configureMocks(commitMessage, neededBy); GetCrd getCrd = createGetCrd(); Response<CrdInfo> response = getCrd.apply(rsrc); @@ -162,8 +154,7 @@ assertThat(crdInfo.cycle).isTrue(); } - public void configureMocks(String commitMessage, final List<ChangeInfo> searchResult) - throws Exception { + public void configureMocks(String commitMessage, final List<String> neededBy) throws Exception { String commitId = "0123456789012345678901234567890123456789"; Project.NameKey projectNameKey = Project.nameKey("projectFoo"); @@ -171,9 +162,11 @@ PatchSet patchSet = mock(PatchSet.class); when(patchSet.commitId()).thenReturn(ObjectId.fromString(commitId)); + Change.Key changeKey = Change.key("I0123456789"); + Change change = new Change( - Change.key("I0123456789"), + changeKey, Change.id(4711), Account.id(23), BranchNameKey.create(projectNameKey, "branchBar"), @@ -183,43 +176,14 @@ when(rsrc.getChange()).thenReturn(change); when(rsrc.getPatchSet()).thenReturn(patchSet); - 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) - .addQuery("message:I0123456789 -change:I0123456789"); - when(queryChanges.apply(TopLevelResource.INSTANCE)) - .thenAnswer( - new Answer<Response<List<ChangeInfo>>>() { - - @Override - public Response<List<ChangeInfo>> answer(InvocationOnMock invocation) - throws Throwable { - return Response.ok(addedQuery.get() ? searchResult : null); - } - }); - - changes = mock(ChangesCollection.class); - when(changes.list()).thenReturn(queryChanges); - commitMessageFetcher = mock(CommitMessageFetcher.class); when(commitMessageFetcher.fetch(projectNameKey, commitId)).thenReturn(commitMessage); - } - private ChangeInfo changeInfo(String changeId) { - ChangeInfo changeInfo = new ChangeInfo(); - changeInfo.changeId = changeId; - return changeInfo; + neededByFetcher = mock(NeededByFetcher.class); + when(neededByFetcher.fetchForChangeKey(changeKey)).thenReturn(neededBy); } private GetCrd createGetCrd() { - return new GetCrd(changes, commitMessageFetcher); + return new GetCrd(commitMessageFetcher, neededByFetcher); } }
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 new file mode 100644 index 0000000..1d51857 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/zuul/util/NeededByFetcherTest.java
@@ -0,0 +1,121 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.googlesource.gerrit.plugins.zuul.util; + +import static com.google.common.truth.Truth.assertThat; +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.common.ChangeInfo; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.TopLevelResource; +import com.google.gerrit.server.restapi.change.ChangesCollection; +import com.google.gerrit.server.restapi.change.QueryChanges; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +public class NeededByFetcherTest { + private ChangesCollection changes; + private Change.Key changeKey = getChangeKey(1); + + @Test + public void testFetchForChangeKeyNoResults() throws Exception { + List<ChangeInfo> searchResult = new ArrayList<>(); + + configureMocks(searchResult); + + NeededByFetcher fetcher = createFetcher(); + + List<String> neededBy = fetcher.fetchForChangeKey(changeKey); + + assertThat(neededBy).isEmpty(); + } + + @Test + public void testFetchForChangeKeySingleResult() throws Exception { + List<ChangeInfo> searchResult = new ArrayList<>(); + searchResult.add(getChangeInfo(2)); + + configureMocks(searchResult); + + NeededByFetcher fetcher = createFetcher(); + + List<String> neededBy = fetcher.fetchForChangeKey(changeKey); + + assertThat(neededBy).containsExactly(getChangeKey(2).toString()); + } + + @Test + public void testFetchForChangeKeyMultipleResults() throws Exception { + List<ChangeInfo> searchResult = new ArrayList<>(); + searchResult.add(getChangeInfo(2)); + searchResult.add(getChangeInfo(3)); + + configureMocks(searchResult); + + NeededByFetcher fetcher = createFetcher(); + + List<String> neededBy = fetcher.fetchForChangeKey(changeKey); + + assertThat(neededBy).containsExactly(getChangeKey(2).toString(), getChangeKey(3).toString()); + } + + 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) + .addQuery("message:" + changeKey + " -change:" + changeKey); + when(queryChanges.apply(TopLevelResource.INSTANCE)) + .thenAnswer( + new Answer<Response<List<ChangeInfo>>>() { + + @Override + public Response<List<ChangeInfo>> answer(InvocationOnMock invocation) + throws Throwable { + return Response.ok(addedQuery.get() ? searchResult : null); + } + }); + + changes = mock(ChangesCollection.class); + when(changes.list()).thenReturn(queryChanges); + } + + private Change.Key getChangeKey(int keyEnding) { + return Change.key("I0123456789abcdef0000000000000000000" + (10000 + keyEnding)); + } + + private ChangeInfo getChangeInfo(int keyEnding) { + ChangeInfo changeInfo = new ChangeInfo(); + changeInfo.changeId = getChangeKey(keyEnding).toString(); + return changeInfo; + } + + private NeededByFetcher createFetcher() { + return new NeededByFetcher(changes); + } +}