From 2beefab89a64a56e77aeb0e6850869d0a645da0a Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Fri, 23 Aug 2024 10:47:27 -0400 Subject: [PATCH 1/6] BB server 2 way diff fixes --- .../bitbucket_server_provider.py | 27 ++- tests/unittest/test_bitbucket_provider.py | 201 +++++++++++++++++- 2 files changed, 223 insertions(+), 5 deletions(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index b8358176f..f7e2f92b3 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -15,7 +15,8 @@ class BitbucketServerProvider(GitProvider): def __init__( - self, pr_url: Optional[str] = None, incremental: Optional[bool] = False + self, pr_url: Optional[str] = None, incremental: Optional[bool] = False, + bitbucket_client: Optional[Bitbucket] = None, ): self.bitbucket_server_url = None self.workspace_slug = None @@ -30,8 +31,9 @@ def __init__( self.bitbucket_pull_request_api_url = pr_url self.bitbucket_server_url = self._parse_bitbucket_server(url=pr_url) - self.bitbucket_client = Bitbucket(url=self.bitbucket_server_url, - token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN", None)) + self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url, + token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN", + None)) if pr_url: self.set_pr(pr_url) @@ -134,11 +136,28 @@ def get_files(self): diffstat = [change["path"]['toString'] for change in changes] return diffstat + #gets the best common ancestor: https://git-scm.com/docs/git-merge-base + @staticmethod + def get_best_common_ancestor(source_commits_list, destination_commits_list, guaranteed_common_ancestor) -> str: + destination_commit_hashes = {commit['id'] for commit in destination_commits_list} + destination_commit_hashes.add(guaranteed_common_ancestor) + + for commit in source_commits_list: + for parent_commit in commit['parents']: + if commit['id'] in destination_commit_hashes or parent_commit['id'] in destination_commit_hashes: + return parent_commit['id'] + + return guaranteed_common_ancestor + def get_diff_files(self) -> list[FilePatchInfo]: if self.diff_files: return self.diff_files - base_sha = self.pr.toRef['latestCommit'] + 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.get_best_common_ancestor(source_commits_list, destination_commits, guaranteed_common_ancestor) head_sha = self.pr.fromRef['latestCommit'] diff_files = [] diff --git a/tests/unittest/test_bitbucket_provider.py b/tests/unittest/test_bitbucket_provider.py index e17a26ce8..fb7eb2939 100644 --- a/tests/unittest/test_bitbucket_provider.py +++ b/tests/unittest/test_bitbucket_provider.py @@ -1,5 +1,8 @@ from pr_agent.git_providers import BitbucketServerProvider from pr_agent.git_providers.bitbucket_provider import BitbucketProvider +from unittest.mock import MagicMock +from atlassian.bitbucket import Bitbucket +from pr_agent.algo.types import EDIT_TYPE, FilePatchInfo class TestBitbucketProvider: @@ -10,9 +13,205 @@ def test_parse_pr_url(self): assert repo_slug == "MY_TEST_REPO" assert pr_number == 321 - def test_bitbucket_server_pr_url(self): + +class TestBitbucketServerProvider: + def test_parse_pr_url(self): url = "https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1" workspace_slug, repo_slug, pr_number = BitbucketServerProvider._parse_pr_url(url) assert workspace_slug == "AAA" assert repo_slug == "my-repo" assert pr_number == 1 + + def mock_get_content_of_file(self, project_key, repository_slug, filename, at=None, markup=None): + if at == '9c1cffdd9f276074bfb6fb3b70fbee62d298b058': + return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n' + elif at == '2a1165446bdf991caf114d01f7c88d84ae7399cf': + return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n' + elif at == 'f617708826cdd0b40abb5245eda71630192a17e3': + return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n' + elif at == 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': + return 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n' + elif at == '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': + return 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n' + elif at == 'ae4eca7f222c96d396927d48ab7538e2ee13ca63': + return 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile' + elif at == '548f8ba15abc30875a082156314426806c3f4d97': + return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile' + return '' + + ''' + 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 + + - o branch b + / + o - o - o main + ^ node c + ''' + + def test_get_diff_files_simple_diverge(self): + bitbucket_client = MagicMock(Bitbucket) + bitbucket_client.get_pull_request.return_value = { + 'toRef': {'latestCommit': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'}, + 'fromRef': {'latestCommit': '2a1165446bdf991caf114d01f7c88d84ae7399cf'} + } + bitbucket_client.get_pull_requests_commits.return_value = [ + {'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf', + 'parents': [{'id': 'f617708826cdd0b40abb5245eda71630192a17e3'}]} + ] + bitbucket_client.get_commits.return_value = [ + {'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'}, + {'id': 'dbca09554567d2e4bee7f07993390153280ee450'} + ] + bitbucket_client.get_pull_requests_changes.return_value = [ + { + 'path': {'toString': 'Readme.md'}, + 'type': 'MODIFY', + } + ] + + bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file + + 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\n', + 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n', + '--- \n+++ \n@@ -5,5 +5,5 @@\n to\n emulate\n a\n-real\n+fake\n file\n', + 'Readme.md', + edit_type=EDIT_TYPE.MODIFIED, + ) + ] + + actual = provider.get_diff_files() + + 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 + + - o - o - o branch b + / / + o - o -- o - o main + ^ node c + ''' + + def test_get_diff_files_diverge_with_merge_commit(self): + bitbucket_client = MagicMock(Bitbucket) + bitbucket_client.get_pull_request.return_value = { + 'toRef': {'latestCommit': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'}, + 'fromRef': {'latestCommit': '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b'} + } + bitbucket_client.get_pull_requests_commits.return_value = [ + {'id': '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b', + 'parents': [{'id': '692772f456c3db77a90b11ce39ea516f8c2bad93'}]}, + {'id': '692772f456c3db77a90b11ce39ea516f8c2bad93', 'parents': [ + {'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf'}, + {'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'}, + ]}, + {'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf', + 'parents': [{'id': 'f617708826cdd0b40abb5245eda71630192a17e3'}]} + ] + bitbucket_client.get_commits.return_value = [ + {'id': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'}, + {'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'}, + {'id': 'dbca09554567d2e4bee7f07993390153280ee450'} + ] + bitbucket_client.get_pull_requests_changes.return_value = [ + { + 'path': {'toString': 'Readme.md'}, + 'type': 'MODIFY', + } + ] + + bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file + + 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\n', + 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n', + '--- \n+++ \n@@ -5,5 +5,5 @@\n to\n emulate\n a\n-real\n-file\n+fake\n+test\n', + 'Readme.md', + edit_type=EDIT_TYPE.MODIFIED, + ) + ] + + actual = provider.get_diff_files() + + 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 + + ---- o - o branch c + / / + ---- o branch b + / / + o - o - o main + ^ node d + ''' + + def test_get_diff_files_multi_merge_diverge(self): + bitbucket_client = MagicMock(Bitbucket) + bitbucket_client.get_pull_request.return_value = { + 'toRef': {'latestCommit': '9569922b22fe4fd0968be6a50ed99f71efcd0504'}, + 'fromRef': {'latestCommit': 'ae4eca7f222c96d396927d48ab7538e2ee13ca63'} + } + bitbucket_client.get_pull_requests_commits.return_value = [ + {'id': 'ae4eca7f222c96d396927d48ab7538e2ee13ca63', + 'parents': [{'id': 'bbf300fb3af5129af8c44659f8cc7a526a6a6f31'}]}, + {'id': 'bbf300fb3af5129af8c44659f8cc7a526a6a6f31', 'parents': [ + {'id': '10b7b8e41cb370b48ceda8da4e7e6ad033182213'}, + {'id': 'd1bb183c706a3ebe4c2b1158c25878201a27ad8c'}, + ]}, + {'id': 'd1bb183c706a3ebe4c2b1158c25878201a27ad8c', 'parents': [ + {'id': '5bd76251866cb415fc5ff232f63a581e89223bda'}, + {'id': '548f8ba15abc30875a082156314426806c3f4d97'} + ]}, + {'id': '5bd76251866cb415fc5ff232f63a581e89223bda', + 'parents': [{'id': '0e898cb355a5170d8c8771b25d43fcaa1d2d9489'}]}, + {'id': '10b7b8e41cb370b48ceda8da4e7e6ad033182213', + 'parents': [{'id': '0e898cb355a5170d8c8771b25d43fcaa1d2d9489'}]} + ] + bitbucket_client.get_commits.return_value = [ + {'id': '9569922b22fe4fd0968be6a50ed99f71efcd0504'}, + {'id': '548f8ba15abc30875a082156314426806c3f4d97'} + ] + bitbucket_client.get_pull_requests_changes.return_value = [ + { + 'path': {'toString': 'Readme.md'}, + 'type': 'MODIFY', + } + ] + + bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file + + 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 From 2a9e3ee1ef1ab2ea22ef430e1d9955ca85700fb2 Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Fri, 23 Aug 2024 10:57:58 -0400 Subject: [PATCH 2/6] removing unnecessary if check --- pr_agent/git_providers/bitbucket_server_provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index f7e2f92b3..dcdedaddb 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -144,7 +144,7 @@ def get_best_common_ancestor(source_commits_list, destination_commits_list, guar for commit in source_commits_list: for parent_commit in commit['parents']: - if commit['id'] in destination_commit_hashes or parent_commit['id'] in destination_commit_hashes: + if parent_commit['id'] in destination_commit_hashes: return parent_commit['id'] return guaranteed_common_ancestor From a99ebf89533fdc410e69e869e57e7838bc5caa43 Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Fri, 23 Aug 2024 11:18:42 -0400 Subject: [PATCH 3/6] implementing PR bot feedback --- .../bitbucket_server_provider.py | 3 +-- tests/unittest/test_bitbucket_provider.py | 26 ++++++++----------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index dcdedaddb..d256a808a 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -139,8 +139,7 @@ def get_files(self): #gets the best common ancestor: https://git-scm.com/docs/git-merge-base @staticmethod def get_best_common_ancestor(source_commits_list, destination_commits_list, guaranteed_common_ancestor) -> str: - destination_commit_hashes = {commit['id'] for commit in destination_commits_list} - destination_commit_hashes.add(guaranteed_common_ancestor) + destination_commit_hashes = {commit['id'] for commit in destination_commits_list} | {guaranteed_common_ancestor} for commit in source_commits_list: for parent_commit in commit['parents']: diff --git a/tests/unittest/test_bitbucket_provider.py b/tests/unittest/test_bitbucket_provider.py index fb7eb2939..93c368e1c 100644 --- a/tests/unittest/test_bitbucket_provider.py +++ b/tests/unittest/test_bitbucket_provider.py @@ -23,21 +23,17 @@ def test_parse_pr_url(self): assert pr_number == 1 def mock_get_content_of_file(self, project_key, repository_slug, filename, at=None, markup=None): - if at == '9c1cffdd9f276074bfb6fb3b70fbee62d298b058': - return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n' - elif at == '2a1165446bdf991caf114d01f7c88d84ae7399cf': - return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n' - elif at == 'f617708826cdd0b40abb5245eda71630192a17e3': - return 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n' - elif at == 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': - return 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n' - elif at == '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': - return 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n' - elif at == 'ae4eca7f222c96d396927d48ab7538e2ee13ca63': - return 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile' - elif at == '548f8ba15abc30875a082156314426806c3f4d97': - return 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile' - return '' + content_map = { + '9c1cffdd9f276074bfb6fb3b70fbee62d298b058': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n', + '2a1165446bdf991caf114d01f7c88d84ae7399cf': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n', + 'f617708826cdd0b40abb5245eda71630192a17e3': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n', + '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' + } + + return content_map.get(at, '') ''' tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c From 0442cdcd3dcd9dcfb7b21562dfa47eba7f1f4727 Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Mon, 26 Aug 2024 16:07:21 -0400 Subject: [PATCH 4/6] adding config value for old Bitbucket Server diff functionality --- pr_agent/git_providers/bitbucket_server_provider.py | 4 +++- pr_agent/settings/configuration.toml | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index d256a808a..67e4cbaeb 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -156,8 +156,10 @@ def get_diff_files(self) -> list[FilePatchInfo]: 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.get_best_common_ancestor(source_commits_list, destination_commits, guaranteed_common_ancestor) + 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) diff_files = [] original_file_content_str = "" diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index b128aca08..108ae4e2d 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -263,6 +263,7 @@ 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 From 29c50758bc8892f517c0af38b42e9dd64e446a90 Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Wed, 28 Aug 2024 17:13:36 -0400 Subject: [PATCH 5/6] implementing more feedback, choosing a different Bitbucket diff strategy depending on API version, and expanded unit test cases --- .../bitbucket_server_provider.py | 47 ++++++-- pr_agent/settings/configuration.toml | 1 - tests/unittest/test_bitbucket_provider.py | 100 ++++++++++++++++-- 3 files changed, 129 insertions(+), 19 deletions(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index 67e4cbaeb..33b840c7e 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/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" diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 108ae4e2d..b128aca08 100644 --- a/pr_agent/settings/configuration.toml +++ b/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 diff --git a/tests/unittest/test_bitbucket_provider.py b/tests/unittest/test_bitbucket_provider.py index 93c368e1c..1ea99931c 100644 --- a/tests/unittest/test_bitbucket_provider.py +++ b/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 \ No newline at end of file From cf14e456748a829c17f1b228c13666686fdaa046 Mon Sep 17 00:00:00 2001 From: mhoecke1 Date: Fri, 30 Aug 2024 10:10:39 -0400 Subject: [PATCH 6/6] further cleaned up code based on feedback --- .../bitbucket_server_provider.py | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index 33b840c7e..7588075e5 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -156,33 +156,33 @@ 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 - )) - - # 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: + head_sha = self.pr.fromRef['latestCommit'] + + # if Bitbucket api version is >= 8.16 then use the merge-base api for 2-way diff calculation + if self.bitbucket_api_version is not None and self.bitbucket_api_version >= LooseVersion("8.16"): + try: + base_sha = self.bitbucket_client.get(self._get_merge_base())['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 + else: + source_commits_list = list(self.bitbucket_client.get_pull_requests_commits( + self.workspace_slug, + self.repo_slug, + self.pr_num + )) + # if Bitbucket api version is None or < 7.0 then do a simple diff with a guaranteed common ancestor + base_sha = source_commits_list[-1]['parents'][0]['id'] + # if Bitbucket api version is 7.0-8.15 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"): 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}") + 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 = [] @@ -452,5 +452,5 @@ 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): + def _get_merge_base(self): return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/merge-base"