Google Git
Sign in
gerrit/plugins/ai-code-review/09d2c0a65c617f3723945189f7dacf91ec7d23be^!/.
commit
09d2c0a65c617f3723945189f7dacf91ec7d23be
[log]
author
Jacek Centkowski <geminica.programs@gmail.com>
Mon Apr 29 09:29:41 2024 +0200
committer
Dariusz Luksza <dariusz.luksza@gmail.com>
Sat May 11 14:15:37 2024 +0100
tree
ddaaf1d25119ec24eff7e6dad2ca38161c1893c3
parent
03b2f4066571f5b51b36d7071ef98738ebdb6381 [diff]
Use direct `accountCache` access instead if calling REST API

In `GerritClientAccount` and all derived classes get `Account.Id` from
cache instead of calling GerritApi.

Note that when classes are injected their constructor doesn't have to be
public unless they are created manually in tests. Such cases were marked
with `@VisibleForTesting` annotation.

Bug: Issue 336577745
Change-Id: I6c72043960de0b45c6d265f93a20fc9614142507

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 3a77620..fcbafd9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/PatchSetReviewer.java

@@ -1,6 +1,7 @@
 package com.googlesource.gerrit.plugins.chatgpt;
 
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.data.ChangeSetDataHandler;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritChange;
@@ -30,6 +31,7 @@
     private final Configuration config;
     private final GerritClient gerritClient;
     private final ChangeSetData changeSetData;
+    private final Provider<GerritClientReview> clientReviewProvider;
     @Getter
     private final IChatGptClient chatGptClient;
 
@@ -39,10 +41,16 @@
     private List<Integer> reviewScores;
 
     @Inject
-    PatchSetReviewer(GerritClient gerritClient, Configuration config, ChangeSetData changeSetData, IChatGptClient chatGptClient) {
+    PatchSetReviewer(
+            GerritClient gerritClient,
+            Configuration config,
+            ChangeSetData changeSetData,
+            Provider<GerritClientReview> clientReviewProvider,
+            IChatGptClient chatGptClient) {
         this.config = config;
         this.gerritClient = gerritClient;
         this.changeSetData = changeSetData;
+        this.clientReviewProvider = clientReviewProvider;
         this.chatGptClient = chatGptClient;
     }
 
@@ -51,7 +59,6 @@
         reviewScores = new ArrayList<>();
         commentProperties = gerritClient.getClientData(change).getCommentProperties();
         gerritCommentRange = new GerritCommentRange(gerritClient, change);
-        GerritClientReview gerritClientReview = new GerritClientReview(config);
         String patchSet = gerritClient.getPatchSet(change);
         if (patchSet.isEmpty()) {
             log.info("No file to review has been found in the PatchSet");
@@ -63,7 +70,7 @@
         log.debug("ChatGPT response: {}", reviewReply);
 
         retrieveReviewBatches(reviewReply, change);
-        gerritClientReview.setReview(change, reviewBatches, getReviewScore());
+        clientReviewProvider.get().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/GerritClientAccount.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientAccount.java
index 8004845..563ca6d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientAccount.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientAccount.java

@@ -1,7 +1,7 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit;
 
-import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.GroupInfo;
+import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.util.ManualRequestContext;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import lombok.extern.slf4j.Slf4j;
@@ -14,9 +14,11 @@
 
 @Slf4j
 public class GerritClientAccount extends GerritClientBase {
+    private final AccountCache accountCache;
 
-    public GerritClientAccount(Configuration config) {
+    public GerritClientAccount(Configuration config, AccountCache accountCache) {
         super(config);
+        this.accountCache = accountCache;
     }
 
     public boolean isDisabledUser(String authorUsername) {
@@ -37,12 +39,10 @@
     }
 
     protected Optional<Integer> getAccountId(String authorUsername) {
-        try (ManualRequestContext requestContext = config.openRequestContext()) {
-            List<AccountInfo> accounts = config.getGerritApi().accounts().query(authorUsername).get();
-            if (accounts.isEmpty()) {
-                return Optional.empty();
-            }
-            return Optional.of(accounts.get(0)).map(a -> a._accountId);
+        try {
+            return accountCache
+                .getByUsername(authorUsername)
+                .map(accountState -> accountState.account().id().get());
         }
         catch (Exception e) {
             log.error("Could not find account ID for username '{}'", authorUsername);

diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientComments.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientComments.java
index 1a3d968..26df1f2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientComments.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientComments.java

@@ -1,8 +1,11 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.events.CommentAddedEvent;
 import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.inject.Inject;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.messages.ClientMessage;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.api.gerrit.GerritCodeRange;
@@ -33,8 +36,10 @@
     @Getter
     private List<GerritComment> commentProperties;
 
-    public GerritClientComments(Configuration config, ChangeSetData changeSetData) {
-        super(config);
+    @VisibleForTesting
+    @Inject
+    public GerritClientComments(Configuration config, AccountCache accountCache, ChangeSetData changeSetData) {
+        super(config, accountCache);
         this.changeSetData = changeSetData;
         commentProperties = new ArrayList<>();
         commentMap = new HashMap<>();

diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientFacade.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientFacade.java
index ff1a404..c1b4dac 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientFacade.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientFacade.java

@@ -1,5 +1,6 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.inject.Inject;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.patch.diff.FileDiffProcessed;
@@ -15,15 +16,20 @@
 public class GerritClientFacade {
     private final ChangeSetData changeSetData;
     private final GerritClientDetail gerritClientDetail;
-    private final IGerritClientPatchSet gerritClientPatchSet;
     private final GerritClientComments gerritClientComments;
+    private final IGerritClientPatchSet gerritClientPatchSet;
 
+    @VisibleForTesting
     @Inject
-    public GerritClientFacade(Configuration config, ChangeSetData changeSetData, IGerritClientPatchSet gerritClientPatchSet) {
+    public GerritClientFacade(
+            Configuration config,
+            ChangeSetData changeSetData,
+            GerritClientComments gerritClientComments,
+            IGerritClientPatchSet gerritClientPatchSet) {
         gerritClientDetail = new GerritClientDetail(config, changeSetData);
         this.gerritClientPatchSet = gerritClientPatchSet;
         this.changeSetData = changeSetData;
-        gerritClientComments = new GerritClientComments(config, changeSetData);
+        this.gerritClientComments = gerritClientComments;
     }
 
     public GerritPermittedVotingRange getPermittedVotingRange(GerritChange change) {

diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientPatchSet.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientPatchSet.java
index 6caa8b7..c409cc5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientPatchSet.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/common/client/api/gerrit/GerritClientPatchSet.java

@@ -4,6 +4,7 @@
 
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.util.ManualRequestContext;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.data.ChangeSetDataHandler;
@@ -16,8 +17,8 @@
     @Getter
     protected Integer revisionBase = 0;
 
-    public GerritClientPatchSet(Configuration config) {
-        super(config);
+    public GerritClientPatchSet(Configuration config, AccountCache accountCache) {
+        super(config, accountCache);
     }
 
     public void retrieveRevisionBase(GerritChange change) {

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 2234dd1..556789d 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,12 +1,15 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit;
 
+import com.google.common.annotations.VisibleForTesting;
 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.account.AccountCache;
 import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.inject.Inject;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.review.ReviewBatch;
 import lombok.extern.slf4j.Slf4j;
@@ -22,8 +25,10 @@
 
 @Slf4j
 public class GerritClientReview extends GerritClientAccount {
-    public GerritClientReview(Configuration config) {
-        super(config);
+    @VisibleForTesting
+    @Inject
+    public GerritClientReview(Configuration config, AccountCache accountCache) {
+        super(config, accountCache);
     }
 
     public void setReview(GerritChange change, List<ReviewBatch> reviewBatches, Integer reviewScore) throws Exception {

diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateful/client/api/gerrit/GerritClientPatchSetStateful.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateful/client/api/gerrit/GerritClientPatchSetStateful.java
index 257cddf..a987954 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateful/client/api/gerrit/GerritClientPatchSetStateful.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateful/client/api/gerrit/GerritClientPatchSetStateful.java

@@ -1,5 +1,7 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.stateful.client.api.gerrit;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.server.account.AccountCache;
 import com.google.inject.Inject;
 import com.googlesource.gerrit.plugins.chatgpt.config.Configuration;
 import com.googlesource.gerrit.plugins.chatgpt.data.PluginDataHandler;
@@ -16,9 +18,14 @@
     private final GitRepoFiles gitRepoFiles;
     private final PluginDataHandler pluginDataHandler;
 
+    @VisibleForTesting
     @Inject
-    public GerritClientPatchSetStateful(Configuration config, GitRepoFiles gitRepoFiles, PluginDataHandler pluginDataHandler) {
-        super(config);
+    public GerritClientPatchSetStateful(
+            Configuration config,
+            AccountCache accountCache,
+            GitRepoFiles gitRepoFiles,
+            PluginDataHandler pluginDataHandler) {
+        super(config, accountCache);
         this.gitRepoFiles = gitRepoFiles;
         this.pluginDataHandler = pluginDataHandler;
     }

diff --git a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateless/client/api/gerrit/GerritClientPatchSetStateless.java b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateless/client/api/gerrit/GerritClientPatchSetStateless.java
index de313da..bbba27f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateless/client/api/gerrit/GerritClientPatchSetStateless.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/chatgpt/mode/stateless/client/api/gerrit/GerritClientPatchSetStateless.java

@@ -1,8 +1,10 @@
 package com.googlesource.gerrit.plugins.chatgpt.mode.stateless.client.api.gerrit;
 
 import com.google.inject.Inject;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.extensions.common.DiffInfo;
 import com.google.gerrit.extensions.common.FileInfo;
+import com.google.gerrit.server.account.AccountCache;
 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.gerrit.GerritChange;
@@ -29,9 +31,10 @@
     private final List<String> diffs;
     private boolean isCommitMessage;
 
+    @VisibleForTesting
     @Inject
-    public GerritClientPatchSetStateless(Configuration config) {
-        super(config);
+    public GerritClientPatchSetStateless(Configuration config, AccountCache accountCache) {
+        super(config, accountCache);
         diffs = new ArrayList<>();
     }
 

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 0c024f8..9362896 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/chatgpt/ChatGptReviewTestBase.java

@@ -11,13 +11,11 @@
 import com.google.gerrit.extensions.api.GerritApi;
 import com.google.gerrit.extensions.api.accounts.AccountApi;
 import com.google.gerrit.extensions.api.accounts.Accounts;
-import com.google.gerrit.extensions.api.accounts.Accounts.QueryRequest;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.ChangeApi.CommentsRequest;
 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.CommentInfo;
 import com.google.gerrit.extensions.common.GroupInfo;
@@ -43,9 +41,12 @@
 import com.googlesource.gerrit.plugins.chatgpt.listener.EventHandlerTask;
 import com.googlesource.gerrit.plugins.chatgpt.listener.GerritEventContextModule;
 import com.google.inject.TypeLiteral;
+import com.google.inject.util.Providers;
 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.GerritClientComments;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritClientFacade;
+import com.googlesource.gerrit.plugins.chatgpt.mode.common.client.api.gerrit.GerritClientReview;
 import com.googlesource.gerrit.plugins.chatgpt.mode.common.model.data.ChangeSetData;
 import com.googlesource.gerrit.plugins.chatgpt.mode.interfaces.client.api.chatgpt.IChatGptClient;
 import com.googlesource.gerrit.plugins.chatgpt.mode.interfaces.client.api.gerrit.IGerritClientPatchSet;
@@ -122,6 +123,8 @@
     protected RevisionApi revisionApiMock;
     @Mock
     protected CommentsRequest commentsRequestMock;
+    @Mock
+    protected AccountCache accountCacheMock;
 
     protected PluginConfig globalConfig;
     protected PluginConfig projectConfig;
@@ -177,10 +180,10 @@
     protected void setupMockRequests() throws RestApiException {
         Accounts accountsMock = mockGerritAccountsRestEndpoint();
         // Mock the behavior of the gerritAccountIdUri request
-        mockGerritAccountsQueryApiCall(accountsMock, GERRIT_GPT_USERNAME, GERRIT_GPT_ACCOUNT_ID);
+        mockGerritAccountsQueryApiCall(GERRIT_GPT_USERNAME, GERRIT_GPT_ACCOUNT_ID);
 
         // Mock the behavior of the gerritAccountIdUri request
-        mockGerritAccountsQueryApiCall(accountsMock, GERRIT_USER_USERNAME, GERRIT_USER_ACCOUNT_ID);
+        mockGerritAccountsQueryApiCall(GERRIT_USER_USERNAME, GERRIT_USER_ACCOUNT_ID);
 
         // Mock the behavior of the gerritAccountGroups request
         mockGerritAccountGroupsApiCall(accountsMock, GERRIT_USER_ACCOUNT_ID);
@@ -201,10 +204,12 @@
     }
 
     private void mockGerritAccountsQueryApiCall(
-        Accounts accountsMock, String username, int expectedAccountId) throws RestApiException {
-        QueryRequest queryRequestMock = mock(QueryRequest.class);
-        when(accountsMock.query(username)).thenReturn(queryRequestMock);
-        when(queryRequestMock.get()).thenReturn(List.of(new AccountInfo(expectedAccountId)));
+        String username, int expectedAccountId) throws RestApiException {
+        AccountState accountStateMock = mock(AccountState.class);
+        Account accountMock = mock(Account.class);
+        when(accountStateMock.account()).thenReturn(accountMock);
+        when(accountMock.id()).thenReturn(Account.id(expectedAccountId));
+        when(accountCacheMock.getByUsername(username)).thenReturn(Optional.of(accountStateMock));
     }
 
     private void mockGerritAccountGroupsApiCall(Accounts accountsMock, int accountId)
@@ -291,8 +296,20 @@
 
     private void initTest() {
         ChangeSetData changeSetData = new ChangeSetData(GPT_USER_ACCOUNT_ID, config.getVotingMinScore(), config.getMaxReviewFileSize());
-        gerritClient = new GerritClient(new GerritClientFacade(config, changeSetData, getGerritClientPatchSet()));
-        patchSetReviewer = new PatchSetReviewer(gerritClient, config, changeSetData, getChatGptClient());
+        gerritClient =
+            new GerritClient(
+                new GerritClientFacade(
+                    config,
+                    changeSetData,
+                    new GerritClientComments(config, accountCacheMock, changeSetData),
+                    getGerritClientPatchSet()));
+        patchSetReviewer =
+            new PatchSetReviewer(
+                gerritClient,
+                config,
+                changeSetData,
+                Providers.of(new GerritClientReview(config, accountCacheMock)),
+                getChatGptClient());
         mockConfigCreator = mock(ConfigCreator.class);
     }
 
@@ -357,8 +374,8 @@
 
     private IGerritClientPatchSet getGerritClientPatchSet() {
         return switch (config.getGptMode()) {
-            case stateful -> new GerritClientPatchSetStateful(config, gitRepoFiles, pluginDataHandler);
-            case stateless -> new GerritClientPatchSetStateless(config);
+            case stateful -> new GerritClientPatchSetStateful(config, accountCacheMock, gitRepoFiles, pluginDataHandler);
+            case stateless -> new GerritClientPatchSetStateless(config, accountCacheMock);
         };
     }
 }

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 fa33a2d..4cf4436 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,5 +1,6 @@
 package com.googlesource.gerrit.plugins.chatgpt.integration;
 
+import com.google.gerrit.server.account.AccountCache;
 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;
@@ -36,6 +37,9 @@
     @InjectMocks
     private IChatGptClient chatGptClient;
 
+    @InjectMocks
+    private AccountCache accountCache;
+
     @Test
     public void sayHelloToGPT() throws Exception {
         ChangeSetData changeSetData = new ChangeSetData(1, config.getVotingMinScore(), config.getMaxReviewFileSize());
@@ -67,7 +71,7 @@
         reviewBatches.add(new ReviewBatch());
         reviewBatches.get(0).setContent("message");
 
-        GerritClientReview gerritClientReview = new GerritClientReview(config);
+        GerritClientReview gerritClientReview = new GerritClientReview(config, accountCache);
         gerritClientReview.setReview(new GerritChange("Your changeId"), reviewBatches);
     }
 }