Merge branch 'stable-3.0' into stable-3.1
* stable-3.0:
Extend CommitMessageFetcher to handle non-commit objects
Fix typo in documentation RevUpdatedEvents -> RefUpdatedEvents
Check if approvals array is null before iterating
Change-Id: I82d25c66ba2f8910390043bf5bdab7e5992a8ec9
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcher.java b/src/main/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcher.java
index 0fd523d..1d5188d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcher.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcher.java
@@ -8,6 +8,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
public class CommitMessageFetcher {
@@ -20,22 +21,33 @@
this.repoManager = repoManager;
}
- public String fetch(String projectName, String commitId) throws IOException {
+ public String fetch(String projectName, String objectId) throws IOException {
try (Repository repo = repoManager.openRepository(Project.nameKey(projectName))) {
try (RevWalk revWalk = new RevWalk(repo)) {
- RevCommit commit = revWalk.parseCommit(ObjectId.fromString(commitId));
- return commit.getFullMessage();
+ RevObject obj = revWalk.peel(revWalk.parseAny(ObjectId.fromString(objectId)));
+ if (obj instanceof RevCommit) {
+ return ((RevCommit) obj).getFullMessage();
+ }
+ // objectId was found, but it's not a commit.
+ // Since the objectId was found, it's nothing to worry about and we do not need to alert the
+ // user. We silently return the empty string as blobs, trees, ... do not have a proper
+ // commit message.
+ //
+ // Parsing a non-commit objectId (and reaching this point) will happen for example on NoteDB
+ // sites when Gerrit updates `refs/sequences/changes` (which does not point at a commit, but
+ // a blob) on All-Projects and the corresponding RefUpdatedEvent gets processed.
+ return "";
}
}
}
- public String fetchGuarded(String projectName, String commitId) {
+ public String fetchGuarded(String projectName, String objectId) {
String ret = "";
try {
- ret = fetch(projectName, commitId);
+ ret = fetch(projectName, objectId);
} catch (IOException e) {
logger.atSevere().withCause(e).log(
- "Could not fetch commit message for commit %s of project %s", commitId, projectName);
+ "Could not fetch commit message for commit %s of project %s", objectId, projectName);
}
return ret;
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractor.java b/src/main/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractor.java
index 926db56..9d6b2d9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractor.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractor.java
@@ -129,8 +129,9 @@
CommentAddedEvent event, Map<String, String> common) {
common.putAll(propertyAttributeExtractor.extractFrom(event.author.get(), "commenter"));
common.put("comment", event.comment);
- if (event.approvals != null) {
- for (ApprovalAttribute approvalAttribute : event.approvals.get()) {
+ ApprovalAttribute[] approvals = event.approvals.get();
+ if (approvals != null) {
+ for (ApprovalAttribute approvalAttribute : approvals) {
common.putAll(propertyAttributeExtractor.extractFrom(approvalAttribute));
}
}
diff --git a/src/main/resources/Documentation/config-rulebase-common.md b/src/main/resources/Documentation/config-rulebase-common.md
index 451846b..e877e9b 100644
--- a/src/main/resources/Documentation/config-rulebase-common.md
+++ b/src/main/resources/Documentation/config-rulebase-common.md
@@ -281,7 +281,7 @@
`added@<Association-Value>`
: (only for events that allow to determine the patch set number.
So for example, this `association` property is not set for
- RevUpdatedEvents)
+ RefUpdatedEvents)
issue id occurs at `<Association-Value>` in the most recent
patch set of the change, and either the event is for patch set
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcherTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcherTest.java
new file mode 100644
index 0000000..9a7faa2
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/its/base/util/CommitMessageFetcherTest.java
@@ -0,0 +1,160 @@
+// 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.its.base.util;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.googlesource.gerrit.plugins.its.base.testutil.LoggingMockingTestCase;
+import java.io.IOException;
+import java.math.BigInteger;
+import java.util.HashSet;
+import java.util.Set;
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Test;
+
+public class CommitMessageFetcherTest extends LoggingMockingTestCase {
+ private GitRepositoryManager repoManager;
+ private Repository repo;
+ private String objectIdBlob = "24c5735c3e8ce8fd18d312e9e58149a62236c01a";
+ private String objectIdTree = "3faaefce19558dfc8d9c976f09ae4897f45cb242";
+ private String objectIdCommit = "95aed53c03b6d3df0912bdd9bb1d0c6eaf619f58";
+ private String objectIdMissing = "0123456789012345678901234567890123456789";
+
+ private byte rawBlob[] = "def\n".getBytes();
+ private byte rawTree[] = sha1append("100644 abc\000", objectIdBlob);
+ private byte rawCommit[] =
+ ("tree\0003faaefce19558dfc8d9c976f09ae4897f45cb242\n"
+ + "author Author <author@example.org> 1592579853 +0200\n"
+ + "committer Committer <committer@example.org> 1592579853 +0200\n"
+ + "\n"
+ + "CommitMsg\n")
+ .getBytes();
+
+ private static byte[] sha1append(String left, String sha1sum) {
+ int leftLen = left.length();
+ byte[] right = (new BigInteger(sha1sum, 16)).toByteArray();
+ byte[] ret = new byte[leftLen + right.length];
+ System.arraycopy(left.getBytes(), 0, ret, 0, leftLen);
+ System.arraycopy(right, 0, ret, leftLen, right.length);
+ return ret;
+ }
+
+ @Test
+ public void testFetchBlob() throws IOException {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetch("ProjectFoo", objectIdBlob);
+
+ assertThat(commitMessage).isEmpty();
+ }
+
+ @Test
+ public void testFetchTree() throws IOException {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetch("ProjectFoo", objectIdTree);
+
+ assertThat(commitMessage).isEmpty();
+ }
+
+ @Test
+ public void testFetchCommit() throws IOException {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetch("ProjectFoo", objectIdCommit);
+
+ assertThat(commitMessage).isEqualTo("CommitMsg\n");
+ }
+
+ @Test
+ public void testFetchGuardedBlob() {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetchGuarded("ProjectFoo", objectIdBlob);
+
+ assertThat(commitMessage).isEmpty();
+ }
+
+ @Test
+ public void testFetchGuardedTree() {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetchGuarded("ProjectFoo", objectIdTree);
+
+ assertThat(commitMessage).isEmpty();
+ }
+
+ @Test
+ public void testFetchGuardedCommit() {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetchGuarded("ProjectFoo", objectIdCommit);
+
+ assertThat(commitMessage).isEqualTo("CommitMsg\n");
+ }
+
+ @Test
+ public void testFetchGuardedMissing() {
+ CommitMessageFetcher fetcher = createCommitMessageFetcher();
+ String commitMessage = fetcher.fetchGuarded("ProjectFoo", objectIdMissing);
+
+ assertThat(commitMessage).isEmpty();
+
+ assertLogMessageContains(objectIdMissing);
+ }
+
+ @Override
+ public void setUp() throws Exception {
+ super.setUp();
+
+ ObjectLoader objectLoaderBlob = mock(ObjectLoader.class);
+ when(objectLoaderBlob.getCachedBytes(anyInt())).thenReturn(rawBlob);
+ when(objectLoaderBlob.getType()).thenReturn(Constants.OBJ_BLOB);
+
+ ObjectLoader objectLoaderTree = mock(ObjectLoader.class);
+ when(objectLoaderTree.getCachedBytes(anyInt())).thenReturn(rawTree);
+ when(objectLoaderTree.getType()).thenReturn(Constants.OBJ_TREE);
+
+ ObjectLoader objectLoaderCommit = mock(ObjectLoader.class);
+ when(objectLoaderCommit.getCachedBytes(anyInt())).thenReturn(rawCommit);
+ when(objectLoaderCommit.getType()).thenReturn(Constants.OBJ_COMMIT);
+
+ Set<ObjectId> shallowCommits = new HashSet<>();
+ shallowCommits.add(ObjectId.fromString(objectIdCommit));
+
+ ObjectReader objectReader = mock(ObjectReader.class);
+ when(objectReader.getShallowCommits()).thenReturn(shallowCommits);
+ when(objectReader.open(ObjectId.fromString(objectIdBlob))).thenReturn(objectLoaderBlob);
+ when(objectReader.open(ObjectId.fromString(objectIdTree))).thenReturn(objectLoaderTree);
+ when(objectReader.open(ObjectId.fromString(objectIdCommit))).thenReturn(objectLoaderCommit);
+ when(objectReader.open(ObjectId.fromString(objectIdMissing)))
+ .thenThrow(
+ new MissingObjectException(ObjectId.fromString(objectIdMissing), Constants.OBJ_COMMIT));
+
+ repo = mock(Repository.class);
+ when(repo.newObjectReader()).thenReturn(objectReader);
+
+ repoManager = mock(GitRepositoryManager.class);
+ when(repoManager.openRepository(eq(Project.nameKey("ProjectFoo")))).thenReturn(repo);
+ }
+
+ private CommitMessageFetcher createCommitMessageFetcher() {
+ return new CommitMessageFetcher(repoManager);
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractorTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractorTest.java
index ac7ea92..bdc697b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/its/base/util/PropertyExtractorTest.java
@@ -210,6 +210,8 @@
ImmutableMap.of("revision", "testRevision", "patchSetNumber", "3");
when(propertyAttributeExtractor.extractFrom(patchSetAttribute)).thenReturn(patchSetProperties);
+ event.approvals = Suppliers.ofInstance(null);
+
event.comment = "testComment";
changeAttribute.project = "testProject";
changeAttribute.number = 176;
