Migrate GerritClientReview to direct GerritApi
The `GerritClientReview` uses ReviewInput to post ChatGPT review on a
change in question.
Note that in order to maintain existing unit tests the expected
body content is read and converted back to compact JSON representation
so that it is comparable with value that is captured from mock (that is
also turned to compact JSON) as `ReviewInput` has no `equals` method
implementation that could be used otherwise.
Bug: Isssue 336577745
Change-Id: I41bd1ef4d1f978843b29bfbd4942dda5d96d135b
diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java
index 4434b64..3a77620 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java
@@ -63,7 +63,7 @@
log.debug("ChatGPT response: {}", reviewReply);
retrieveReviewBatches(reviewReply, change);
- gerritClientReview.setReview(change.getFullChangeId(), reviewBatches, getReviewScore());
+ gerritClientReview.setReview(change, reviewBatches, getReviewScore());
}
private void setCommentBatchMap(ReviewBatch batchMap, Integer batchID) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientReview.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientReview.java
index 10451a0..2234dd1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientReview.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientReview.java
@@ -1,26 +1,24 @@
package com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit;
-import com.google.common.net.HttpHeaders;
+import com.google.common.base.Strings;
+import com.google.gerrit.extensions.client.Comment;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewResult;
+import com.google.gerrit.server.util.ManualRequestContext;
import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
-import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.UriResourceLocator;
-import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.api.gerrit.GerritComment;
-import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.api.gerrit.GerritReview;
import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.review.ReviewBatch;
import lombok.extern.slf4j.Slf4j;
-import org.apache.http.entity.ContentType;
-import java.net.URI;
-import java.net.http.HttpRequest;
-import java.net.http.HttpResponse;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import static com.googlesource.gerrit.plugins.chatgpt.mode.common.client.prompt.MessageSanitizer.sanitizeChatGptMessage;
import static com.googlesource.gerrit.plugins.chatgpt.settings.Settings.EMPTY_REVIEW_MESSAGE;
-import static com.googlesource.gerrit.plugins.chatgpt.utils.GsonUtils.getGson;
-import static java.net.HttpURLConnection.HTTP_OK;
@Slf4j
public class GerritClientReview extends GerritClientAccount {
@@ -28,39 +26,35 @@
super(config);
}
- public void setReview(String fullChangeId, List<ReviewBatch> reviewBatches, Integer reviewScore) throws Exception {
- GerritReview reviewMap = buildReview(reviewBatches, reviewScore);
- if (reviewMap.getComments() == null && reviewMap.getMessage() == null) {
+ public void setReview(GerritChange change, List<ReviewBatch> reviewBatches, Integer reviewScore) throws Exception {
+ ReviewInput reviewInput = buildReview(reviewBatches, reviewScore);
+ if (reviewInput.comments == null && reviewInput.message == null) {
return;
}
- URI uri = URI.create(config.getGerritAuthBaseUrl()
- + UriResourceLocator.gerritSetReviewUri(fullChangeId));
- log.debug("Set-Review uri: {}", uri);
- String auth = generateBasicAuth(config.getGerritUserName(),
- config.getGerritPassword());
- String json = getGson().toJson(reviewMap);
- log.debug("Set-Review JSON: {}", json);
- HttpRequest request = HttpRequest.newBuilder()
- .header(HttpHeaders.AUTHORIZATION, auth)
- .header(HttpHeaders.CONTENT_TYPE, ContentType.APPLICATION_JSON.toString())
- .uri(uri)
- .POST(HttpRequest.BodyPublishers.ofString(json))
- .build();
+ try (ManualRequestContext requestContext = config.openRequestContext()) {
+ ReviewResult result = config
+ .getGerritApi()
+ .changes()
+ .id(
+ change.getProjectName(),
+ change.getBranchNameKey().shortName(),
+ change.getChangeKey().get())
+ .current()
+ .review(reviewInput);
- HttpResponse<String> response = httpClientWithRetry.execute(request);
-
- if (response.statusCode() != HTTP_OK) {
- log.error("Review setting failed with status code: {}", response.statusCode());
+ if (!Strings.isNullOrEmpty(result.error)) {
+ log.error("Review setting failed with status code: {}", result.error);
+ }
}
}
- public void setReview(String fullChangeId, List<ReviewBatch> reviewBatches) throws Exception {
- setReview(fullChangeId, reviewBatches, null);
+ public void setReview(GerritChange change, List<ReviewBatch> reviewBatches) throws Exception {
+ setReview(change, reviewBatches, null);
}
- private GerritReview buildReview(List<ReviewBatch> reviewBatches, Integer reviewScore) {
- GerritReview reviewMap = new GerritReview();
- Map<String, List<GerritComment>> comments = new HashMap<>();
+ private ReviewInput buildReview(List<ReviewBatch> reviewBatches, Integer reviewScore) {
+ ReviewInput reviewInput = ReviewInput.create();
+ Map<String, List<CommentInput>> comments = new HashMap<>();
for (ReviewBatch reviewBatch : reviewBatches) {
String message = sanitizeChatGptMessage(reviewBatch.getContent());
if (message.trim().isEmpty()) {
@@ -69,32 +63,41 @@
}
boolean unresolved;
String filename = reviewBatch.getFilename();
- List<GerritComment> filenameComments = comments.getOrDefault(filename, new ArrayList<>());
- GerritComment filenameComment = new GerritComment();
- filenameComment.setMessage(message);
+ List<CommentInput> filenameComments = comments.getOrDefault(filename, new ArrayList<>());
+ CommentInput filenameComment = new CommentInput();
+ filenameComment.message = message;
if (reviewBatch.getLine() != null || reviewBatch.getRange() != null) {
- filenameComment.setLine(reviewBatch.getLine());
- filenameComment.setRange(reviewBatch.getRange());
- filenameComment.setInReplyTo(reviewBatch.getId());
+ filenameComment.line = reviewBatch.getLine();
+ Optional.ofNullable(reviewBatch.getRange())
+ .ifPresent(
+ r -> {
+ Comment.Range range = new Comment.Range();
+ range.startLine = r.startLine;
+ range.startCharacter = r.startCharacter;
+ range.endLine = r.endLine;
+ range.endCharacter = r.endCharacter;
+ filenameComment.range = range;
+ });
+ filenameComment.inReplyTo = reviewBatch.getId();
unresolved = !config.getInlineCommentsAsResolved();
}
else {
unresolved = !config.getPatchSetCommentsAsResolved();
}
- filenameComment.setUnresolved(unresolved);
+ filenameComment.unresolved = unresolved;
filenameComments.add(filenameComment);
comments.putIfAbsent(filename, filenameComments);
}
if (comments.isEmpty()) {
- reviewMap.setMessage(EMPTY_REVIEW_MESSAGE);
+ reviewInput.message(EMPTY_REVIEW_MESSAGE);
}
else {
- reviewMap.setComments(comments);
+ reviewInput.comments = comments;
}
if (reviewScore != null) {
- reviewMap.setLabels(new GerritReview.Labels(reviewScore));
+ reviewInput.label(LabelId.CODE_REVIEW, reviewScore);
}
- return reviewMap;
+ return reviewInput;
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewStatelessTest.java b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewStatelessTest.java
index 4a0da3d..de04876 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewStatelessTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewStatelessTest.java
@@ -3,7 +3,10 @@
import com.github.tomakehurst.wiremock.client.WireMock;
import com.google.common.net.HttpHeaders;
import com.googlesource.gerrit.plugins.chatgpt.listener.EventHandlerTask;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.json.OutputFormat;
+import com.google.gson.Gson;
import com.googlesource.gerrit.plugins.chatgpt.mode.stateless.client.api.UriResourceLocatorStateless;
import com.googlesource.gerrit.plugins.chatgpt.mode.stateless.client.prompt.ChatGptPromptStateless;
import lombok.extern.slf4j.Slf4j;
@@ -12,6 +15,7 @@
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
@@ -25,12 +29,12 @@
@Slf4j
@RunWith(MockitoJUnitRunner.class)
public class ChatGptReviewStatelessTest extends ChatGptReviewTestBase {
- private String expectedResponseStreamed;
+ private ReviewInput expectedResponseStreamed;
private String expectedSystemPromptReview;
private String promptTagReview;
private String promptTagComments;
private String diffContent;
- private String gerritPatchSetReview;
+ private ReviewInput gerritPatchSetReview;
private ChatGptPromptStateless chatGptPromptStateless;
@@ -81,14 +85,17 @@
.withStatus(HTTP_OK)
.withHeader(HttpHeaders.CONTENT_TYPE, ContentType.APPLICATION_JSON.toString())
.withBodyFile("chatGptResponseStreamed.txt")));
+
+ // Mock the GerritApi's revision API
+ when(changeApiMock.current()).thenReturn(revisionApiMock);
}
protected void initComparisonContent() {
super.initComparisonContent();
diffContent = readTestFile("reducePatchSet/patchSetDiffOutput.json");
- gerritPatchSetReview = readTestFile("__files/gerritPatchSetReview.json");
- expectedResponseStreamed = readTestFile("__files/chatGptExpectedResponseStreamed.json");
+ gerritPatchSetReview = readTestFileToClass("__files/gerritPatchSetReview.json", ReviewInput.class);
+ expectedResponseStreamed = readTestFileToClass("__files/chatGptExpectedResponseStreamed.json", ReviewInput.class);
promptTagReview = readTestFile("__files/chatGptPromptTagReview.json");
promptTagComments = readTestFile("__files/chatGptPromptTagRequests.json");
expectedSystemPromptReview = ChatGptPromptStateless.getDefaultGptReviewSystemPrompt();
@@ -114,13 +121,14 @@
handleEventBasedOnType(false);
- testRequestSent();
+ ArgumentCaptor<ReviewInput> captor = testRequestSent();
String systemPrompt = prompts.get(0).getAsJsonObject().get("content").getAsString();
Assert.assertEquals(expectedSystemPromptReview, systemPrompt);
String userPrompt = prompts.get(1).getAsJsonObject().get("content").getAsString();
Assert.assertEquals(reviewUserPrompt, userPrompt);
- String requestBody = loggedRequests.get(0).getBodyAsString();
- Assert.assertEquals(expectedResponseStreamed, requestBody);
+
+ Gson gson = OutputFormat.JSON_COMPACT.newGson();
+ Assert.assertEquals(gson.toJson(expectedResponseStreamed), gson.toJson(captor.getAllValues().get(0)));
}
@Test
@@ -141,11 +149,12 @@
handleEventBasedOnType(false);
- testRequestSent();
+ ArgumentCaptor<ReviewInput> captor = testRequestSent();
String userPrompt = prompts.get(1).getAsJsonObject().get("content").getAsString();
Assert.assertEquals(reviewUserPrompt, userPrompt);
- String requestBody = loggedRequests.get(0).getBodyAsString();
- Assert.assertEquals(gerritPatchSetReview, requestBody);
+
+ Gson gson = OutputFormat.JSON_COMPACT.newGson();
+ Assert.assertEquals(gson.toJson(gerritPatchSetReview), gson.toJson(captor.getAllValues().get(0)));
}
@Test
@@ -157,7 +166,7 @@
}
@Test
- public void gptMentionedInComment() {
+ public void gptMentionedInComment() throws RestApiException {
when(config.getGerritUserName()).thenReturn(GERRIT_GPT_USERNAME);
chatGptPromptStateless.setCommentEvent(true);
WireMock.stubFor(WireMock.post(WireMock.urlEqualTo(URI.create(config.getGptDomain()
diff --git a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java
index 7139ee6..a8ee958 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java
@@ -2,7 +2,6 @@
import com.github.tomakehurst.wiremock.client.WireMock;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
-import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
import com.github.tomakehurst.wiremock.verification.LoggedRequest;
import com.google.common.net.HttpHeaders;
import com.google.gerrit.entities.Account;
@@ -17,6 +16,8 @@
import com.google.gerrit.extensions.api.accounts.Accounts.QueryRequest;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.GroupInfo;
@@ -55,9 +56,9 @@
import com.googlesource.gerrit.plugins.chatgpt.mode.stateless.client.api.gerrit.GerritClientPatchSetStateless;
import lombok.NonNull;
import org.apache.http.entity.ContentType;
-import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
+import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.stubbing.Answer;
@@ -78,6 +79,7 @@
import static java.net.HttpURLConnection.HTTP_OK;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
public class ChatGptReviewTestBase {
@@ -113,6 +115,12 @@
protected OneOffRequestContext context;
@Mock
protected GerritApi gerritApi;
+ @Mock
+ protected Changes changesMock;
+ @Mock
+ protected ChangeApi changeApiMock;
+ @Mock
+ protected RevisionApi revisionApiMock;
protected PluginConfig globalConfig;
protected PluginConfig projectConfig;
@@ -184,6 +192,7 @@
// Mock the behavior of the gerritAccountGroups request
mockGerritAccountGroupsApiCall(accountsMock, GERRIT_USER_ACCOUNT_ID);
+ mockGerritChangeApiRestEndpoint();
// Mock the behavior of the gerritPatchSetRevisionsUri request
WireMock.stubFor(WireMock.get(gerritPatchSetRevisionsUri(fullChangeId))
.willReturn(WireMock.aResponse()
@@ -238,17 +247,22 @@
}
private void mockGerritChangeDetailsApiCall() throws RestApiException {
- Gson gson = OutputFormat.JSON.newGson();
- ChangeInfo changeInfo = gson.fromJson(readTestFile("__files/gerritPatchSetDetail.json"), ChangeInfo.class);
- Changes changesMock = mock(Changes.class);
- when(gerritApi.changes()).thenReturn(changesMock);
- ChangeApi changeApiMock = mock(ChangeApi.class);
- when(changesMock.id(PROJECT_NAME.get(), BRANCH_NAME.shortName(), CHANGE_ID.get())).thenReturn(changeApiMock);
+ ChangeInfo changeInfo = readTestFileToClass("__files/gerritPatchSetDetail.json", ChangeInfo.class);
when(changeApiMock.get()).thenReturn(changeInfo);
}
+ private void mockGerritChangeApiRestEndpoint() throws RestApiException {
+ when(gerritApi.changes()).thenReturn(changesMock);
+ when(changesMock.id(PROJECT_NAME.get(), BRANCH_NAME.shortName(), CHANGE_ID.get())).thenReturn(changeApiMock);
+ }
+
protected void initComparisonContent() {}
+ protected <T> T readTestFileToClass(String filename, Class<T> clazz) {
+ Gson gson = OutputFormat.JSON.newGson();
+ return gson.fromJson(readTestFile(filename), clazz);
+ }
+
protected String readTestFile(String filename) {
try {
return new String(Files.readAllBytes(basePath.resolve(filename)));
@@ -279,14 +293,13 @@
return task.execute();
}
- protected void testRequestSent() {
- RequestPatternBuilder requestPatternBuilder = WireMock.postRequestedFor(
- WireMock.urlEqualTo(gerritSetReviewUri(getGerritChange().getFullChangeId())));
- loggedRequests = WireMock.findAll(requestPatternBuilder);
- Assert.assertEquals(1, loggedRequests.size());
+ protected ArgumentCaptor<ReviewInput> testRequestSent() throws RestApiException {
+ ArgumentCaptor<ReviewInput> reviewInputCaptor = ArgumentCaptor.forClass(ReviewInput.class);
+ verify(revisionApiMock).review(reviewInputCaptor.capture());
JsonObject gptRequestBody = getGson().fromJson(patchSetReviewer.getChatGptClient().getRequestBody(),
JsonObject.class);
prompts = gptRequestBody.get("messages").getAsJsonArray();
+ return reviewInputCaptor;
}
private void initTest() {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/integration/CodeReviewPluginIT.java b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/integration/CodeReviewPluginIT.java
index 854de47..5485443 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/integration/CodeReviewPluginIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/integration/CodeReviewPluginIT.java
@@ -1,6 +1,7 @@
package com.googlesource.gerrit.plugins.chatgpt.integration;
import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
+import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritChange;
import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritClient;
import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritClientReview;
import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.data.ChangeSetData;
@@ -71,6 +72,6 @@
reviewBatches.get(0).setContent("message");
GerritClientReview gerritClientReview = new GerritClientReview(config);
- gerritClientReview.setReview("Your changeId", reviewBatches);
+ gerritClientReview.setReview(new GerritChange("Your changeId"), reviewBatches);
}
}
