implementing more feedback, choosing a different Bitbucket diff strategy depending on API version, and expanded unit test cases

Author: MaxHoecker

pr_agent/git_providers/bitbucket_server_provider.py

@@ -1,9 +1,9 @@
+from distutils.version import LooseVersion
+from requests.exceptions import HTTPError
 from typing import Optional, Tuple
 from urllib.parse import quote_plus, urlparse
 
-from requests.exceptions import HTTPError
 from atlassian.bitbucket import Bitbucket
-from starlette_context import context
 
 from .git_provider import GitProvider
 from ..algo.types import EDIT_TYPE, FilePatchInfo
@@ -34,6 +34,10 @@ def __init__(
         self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url,
                                                               token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN",
                                                                                        None))
+        try:
+            self.bitbucket_api_version = LooseVersion(self.bitbucket_client.get("rest/api/1.0/application-properties").get('version'))
+        except Exception:
+            self.bitbucket_api_version = None
 
         if pr_url:
             self.set_pr(pr_url)
@@ -152,14 +156,34 @@ def get_diff_files(self) -> list[FilePatchInfo]:
         if self.diff_files:
             return self.diff_files
 
-        source_commits_list = list(self.bitbucket_client.get_pull_requests_commits(self.workspace_slug, self.repo_slug, self.pr_num))
-        guaranteed_common_ancestor = source_commits_list[-1]['parents'][0]['id']
-        destination_commits = list(self.bitbucket_client.get_commits(self.workspace_slug, self.repo_slug, guaranteed_common_ancestor, self.pr.toRef['latestCommit']))
-
-        base_sha = self.pr.toRef['latestCommit']
-        head_sha = self.pr.fromRef['latestCommit']
-        if not get_settings().bitbucket_server.get("legacy_diff_calculation", False):
-            base_sha = self.get_best_common_ancestor(source_commits_list, destination_commits, guaranteed_common_ancestor)
+        source_commits_list = list(self.bitbucket_client.get_pull_requests_commits(
+            self.workspace_slug,
+            self.repo_slug,
+            self.pr_num
+        ))
+
+        # defaults to basic diff functionality with a guaranteed common ancestor
+        base_sha, head_sha = source_commits_list[-1]['parents'][0]['id'], source_commits_list[0]['id']
+
+        # if Bitbucket api version is greater than or equal to 7.0 then use 2-way diff functionality for the base_sha
+        if self.bitbucket_api_version is not None and self.bitbucket_api_version >= LooseVersion("7.0"):
+            # Bitbucket endpoint for getting merge-base is available as of 8.16
+            if self.bitbucket_api_version >= LooseVersion("8.16"):
+                try:
+                    base_sha = self.bitbucket_client.get(self._get_best_common_ancestor())['id']
+                except Exception as e:
+                    get_logger().error(f"Failed to get the best common ancestor for PR: {self.pr_url}, \nerror: {e}")
+                    raise e
+            # for versions 7.0-8.15 try to calculate the merge-base on our own
+            else:
+                try:
+                    destination_commits = list(
+                        self.bitbucket_client.get_commits(self.workspace_slug, self.repo_slug, base_sha,
+                                                          self.pr.toRef['latestCommit']))
+                    base_sha = self.get_best_common_ancestor(source_commits_list, destination_commits, base_sha)
+                except Exception as e:
+                    get_logger().error(f"Failed to get the commit list for calculating best common ancestor for PR: {self.pr_url}, \nerror: {e}")
+                    raise e
 
         diff_files = []
         original_file_content_str = ""
@@ -427,3 +451,6 @@ def get_pr_labels(self, update=False):
 
     def _get_pr_comments_path(self):
         return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/comments"
+
+    def _get_best_common_ancestor(self):
+        return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/merge-base"

pr_agent/settings/configuration.toml

@@ -263,7 +263,6 @@ pr_commands = [
     "/review --pr_reviewer.num_code_suggestions=0",
     "/improve --pr_code_suggestions.commitable_code_suggestions=true --pr_code_suggestions.suggestions_score_threshold=7",
 ]
-legacy_diff_calculation = false
 
 [litellm]
 # use_client = false

tests/unittest/test_bitbucket_provider.py

@@ -30,11 +30,39 @@ def mock_get_content_of_file(self, project_key, repository_slug, filename, at=No
             'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n',
             '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
             'ae4eca7f222c96d396927d48ab7538e2ee13ca63': 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
-            '548f8ba15abc30875a082156314426806c3f4d97': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile'
+            '548f8ba15abc30875a082156314426806c3f4d97': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile',
+            '0e898cb355a5170d8c8771b25d43fcaa1d2d9489': 'file\nwith\nmultiple\nlines\nto\nemulate\na\nreal\nfile'
         }
-
         return content_map.get(at, '')
 
+    def mock_get_from_bitbucket_60(self, url):
+        response_map = {
+            "rest/api/1.0/application-properties": {
+                "version": "6.0"
+            }
+        }
+        return response_map.get(url, '')
+
+    def mock_get_from_bitbucket_70(self, url):
+        response_map = {
+            "rest/api/1.0/application-properties": {
+                "version": "7.0"
+            }
+        }
+        return response_map.get(url, '')
+
+    def mock_get_from_bitbucket_816(self, url):
+        response_map = {
+            "rest/api/1.0/application-properties": {
+                "version": "8.16"
+            },
+            "rest/api/latest/projects/AAA/repos/my-repo/pull-requests/1/merge-base": {
+                'id': '548f8ba15abc30875a082156314426806c3f4d97'
+            }
+        }
+        return response_map.get(url, '')
+
+
     '''
     tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c
     NOT between the HEAD of main and the HEAD of branch b
@@ -44,8 +72,7 @@ def mock_get_content_of_file(self, project_key, repository_slug, filename, at=No
     o - o - o  main
         ^ node c
     '''
-
-    def test_get_diff_files_simple_diverge(self):
+    def test_get_diff_files_simple_diverge_70(self):
         bitbucket_client = MagicMock(Bitbucket)
         bitbucket_client.get_pull_request.return_value = {
             'toRef': {'latestCommit': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
@@ -66,6 +93,7 @@ def test_get_diff_files_simple_diverge(self):
             }
         ]
 
+        bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
         bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
 
         provider = BitbucketServerProvider(
@@ -87,6 +115,7 @@ def test_get_diff_files_simple_diverge(self):
 
         assert actual == expected
 
+
     '''
     tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c
     NOT between the HEAD of main and the HEAD of branch b
@@ -96,8 +125,7 @@ def test_get_diff_files_simple_diverge(self):
     o - o -- o - o     main
              ^ node c
     '''
-
-    def test_get_diff_files_diverge_with_merge_commit(self):
+    def test_get_diff_files_diverge_with_merge_commit_70(self):
         bitbucket_client = MagicMock(Bitbucket)
         bitbucket_client.get_pull_request.return_value = {
             'toRef': {'latestCommit': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'},
@@ -125,6 +153,7 @@ def test_get_diff_files_diverge_with_merge_commit(self):
             }
         ]
 
+        bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
         bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
 
         provider = BitbucketServerProvider(
@@ -146,6 +175,7 @@ def test_get_diff_files_diverge_with_merge_commit(self):
 
         assert actual == expected
 
+
     '''
     tests the 2-way diff functionality where the diff should be between the HEAD of branch c and node d
     NOT between the HEAD of main and the HEAD of branch c
@@ -157,8 +187,7 @@ def test_get_diff_files_diverge_with_merge_commit(self):
         o - o - o      main
             ^ node d
     '''
-
-    def test_get_diff_files_multi_merge_diverge(self):
+    def get_multi_merge_diverge_mock_client(self, api_version):
         bitbucket_client = MagicMock(Bitbucket)
         bitbucket_client.get_pull_request.return_value = {
             'toRef': {'latestCommit': '9569922b22fe4fd0968be6a50ed99f71efcd0504'},
@@ -192,6 +221,39 @@ def test_get_diff_files_multi_merge_diverge(self):
         ]
 
         bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
+        if api_version == 60:
+            bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_60
+        elif api_version == 70:
+            bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
+        elif api_version == 816:
+            bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_816
+
+        return bitbucket_client
+
+    def test_get_diff_files_multi_merge_diverge_60(self):
+        bitbucket_client = self.get_multi_merge_diverge_mock_client(60)
+
+        provider = BitbucketServerProvider(
+            "https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
+            bitbucket_client=bitbucket_client
+        )
+
+        expected = [
+            FilePatchInfo(
+                'file\nwith\nmultiple\nlines\nto\nemulate\na\nreal\nfile',
+                'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
+                '--- \n+++ \n@@ -1,9 +1,9 @@\n-file\n-with\n-multiple\n+readme\n+without\n+some\n lines\n to\n-emulate\n+simulate\n a\n real\n file',
+                'Readme.md',
+                edit_type=EDIT_TYPE.MODIFIED,
+            )
+        ]
+
+        actual = provider.get_diff_files()
+
+        assert actual == expected
+
+    def test_get_diff_files_multi_merge_diverge_70(self):
+        bitbucket_client = self.get_multi_merge_diverge_mock_client(70)
 
         provider = BitbucketServerProvider(
             "https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
@@ -211,3 +273,25 @@ def test_get_diff_files_multi_merge_diverge(self):
         actual = provider.get_diff_files()
 
         assert actual == expected
+
+    def test_get_diff_files_multi_merge_diverge_816(self):
+        bitbucket_client = self.get_multi_merge_diverge_mock_client(816)
+
+        provider = BitbucketServerProvider(
+            "https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
+            bitbucket_client=bitbucket_client
+        )
+
+        expected = [
+            FilePatchInfo(
+                'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile',
+                'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
+                '--- \n+++ \n@@ -1,9 +1,9 @@\n-file\n-with\n+readme\n+without\n some\n lines\n to\n-emulate\n+simulate\n a\n real\n file',
+                'Readme.md',
+                edit_type=EDIT_TYPE.MODIFIED,
+            )
+        ]
+
+        actual = provider.get_diff_files()
+
+        assert actual == expected