mirror of
https://github.com/qodo-ai/pr-agent.git
synced 2025-07-04 21:00:40 +08:00
implementing more feedback, choosing a different Bitbucket diff strategy depending on API version, and expanded unit test cases
This commit is contained in:
@ -1,9 +1,9 @@
|
|||||||
|
from distutils.version import LooseVersion
|
||||||
|
from requests.exceptions import HTTPError
|
||||||
from typing import Optional, Tuple
|
from typing import Optional, Tuple
|
||||||
from urllib.parse import quote_plus, urlparse
|
from urllib.parse import quote_plus, urlparse
|
||||||
|
|
||||||
from requests.exceptions import HTTPError
|
|
||||||
from atlassian.bitbucket import Bitbucket
|
from atlassian.bitbucket import Bitbucket
|
||||||
from starlette_context import context
|
|
||||||
|
|
||||||
from .git_provider import GitProvider
|
from .git_provider import GitProvider
|
||||||
from ..algo.types import EDIT_TYPE, FilePatchInfo
|
from ..algo.types import EDIT_TYPE, FilePatchInfo
|
||||||
@ -34,6 +34,10 @@ class BitbucketServerProvider(GitProvider):
|
|||||||
self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url,
|
self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url,
|
||||||
token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN",
|
token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN",
|
||||||
None))
|
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:
|
if pr_url:
|
||||||
self.set_pr(pr_url)
|
self.set_pr(pr_url)
|
||||||
@ -152,14 +156,34 @@ class BitbucketServerProvider(GitProvider):
|
|||||||
if self.diff_files:
|
if self.diff_files:
|
||||||
return 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))
|
source_commits_list = list(self.bitbucket_client.get_pull_requests_commits(
|
||||||
guaranteed_common_ancestor = source_commits_list[-1]['parents'][0]['id']
|
self.workspace_slug,
|
||||||
destination_commits = list(self.bitbucket_client.get_commits(self.workspace_slug, self.repo_slug, guaranteed_common_ancestor, self.pr.toRef['latestCommit']))
|
self.repo_slug,
|
||||||
|
self.pr_num
|
||||||
|
))
|
||||||
|
|
||||||
base_sha = self.pr.toRef['latestCommit']
|
# defaults to basic diff functionality with a guaranteed common ancestor
|
||||||
head_sha = self.pr.fromRef['latestCommit']
|
base_sha, head_sha = source_commits_list[-1]['parents'][0]['id'], source_commits_list[0]['id']
|
||||||
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)
|
# 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 = []
|
diff_files = []
|
||||||
original_file_content_str = ""
|
original_file_content_str = ""
|
||||||
@ -427,3 +451,6 @@ class BitbucketServerProvider(GitProvider):
|
|||||||
|
|
||||||
def _get_pr_comments_path(self):
|
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"
|
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"
|
||||||
|
@ -263,7 +263,6 @@ pr_commands = [
|
|||||||
"/review --pr_reviewer.num_code_suggestions=0",
|
"/review --pr_reviewer.num_code_suggestions=0",
|
||||||
"/improve --pr_code_suggestions.commitable_code_suggestions=true --pr_code_suggestions.suggestions_score_threshold=7",
|
"/improve --pr_code_suggestions.commitable_code_suggestions=true --pr_code_suggestions.suggestions_score_threshold=7",
|
||||||
]
|
]
|
||||||
legacy_diff_calculation = false
|
|
||||||
|
|
||||||
[litellm]
|
[litellm]
|
||||||
# use_client = false
|
# use_client = false
|
||||||
|
@ -30,11 +30,39 @@ class TestBitbucketServerProvider:
|
|||||||
'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': 'file\nwith\nsome\nchanges\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',
|
'1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
|
||||||
'ae4eca7f222c96d396927d48ab7538e2ee13ca63': 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
|
'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, '')
|
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
|
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
|
NOT between the HEAD of main and the HEAD of branch b
|
||||||
@ -44,8 +72,7 @@ class TestBitbucketServerProvider:
|
|||||||
o - o - o main
|
o - o - o main
|
||||||
^ node c
|
^ node c
|
||||||
'''
|
'''
|
||||||
|
def test_get_diff_files_simple_diverge_70(self):
|
||||||
def test_get_diff_files_simple_diverge(self):
|
|
||||||
bitbucket_client = MagicMock(Bitbucket)
|
bitbucket_client = MagicMock(Bitbucket)
|
||||||
bitbucket_client.get_pull_request.return_value = {
|
bitbucket_client.get_pull_request.return_value = {
|
||||||
'toRef': {'latestCommit': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
|
'toRef': {'latestCommit': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
|
||||||
@ -66,6 +93,7 @@ class TestBitbucketServerProvider:
|
|||||||
}
|
}
|
||||||
]
|
]
|
||||||
|
|
||||||
|
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
|
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
|
||||||
|
|
||||||
provider = BitbucketServerProvider(
|
provider = BitbucketServerProvider(
|
||||||
@ -87,6 +115,7 @@ class TestBitbucketServerProvider:
|
|||||||
|
|
||||||
assert actual == expected
|
assert actual == expected
|
||||||
|
|
||||||
|
|
||||||
'''
|
'''
|
||||||
tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c
|
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
|
NOT between the HEAD of main and the HEAD of branch b
|
||||||
@ -96,8 +125,7 @@ class TestBitbucketServerProvider:
|
|||||||
o - o -- o - o main
|
o - o -- o - o main
|
||||||
^ node c
|
^ node c
|
||||||
'''
|
'''
|
||||||
|
def test_get_diff_files_diverge_with_merge_commit_70(self):
|
||||||
def test_get_diff_files_diverge_with_merge_commit(self):
|
|
||||||
bitbucket_client = MagicMock(Bitbucket)
|
bitbucket_client = MagicMock(Bitbucket)
|
||||||
bitbucket_client.get_pull_request.return_value = {
|
bitbucket_client.get_pull_request.return_value = {
|
||||||
'toRef': {'latestCommit': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'},
|
'toRef': {'latestCommit': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'},
|
||||||
@ -125,6 +153,7 @@ class TestBitbucketServerProvider:
|
|||||||
}
|
}
|
||||||
]
|
]
|
||||||
|
|
||||||
|
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
|
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
|
||||||
|
|
||||||
provider = BitbucketServerProvider(
|
provider = BitbucketServerProvider(
|
||||||
@ -146,6 +175,7 @@ class TestBitbucketServerProvider:
|
|||||||
|
|
||||||
assert actual == expected
|
assert actual == expected
|
||||||
|
|
||||||
|
|
||||||
'''
|
'''
|
||||||
tests the 2-way diff functionality where the diff should be between the HEAD of branch c and node d
|
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
|
NOT between the HEAD of main and the HEAD of branch c
|
||||||
@ -157,8 +187,7 @@ class TestBitbucketServerProvider:
|
|||||||
o - o - o main
|
o - o - o main
|
||||||
^ node d
|
^ node d
|
||||||
'''
|
'''
|
||||||
|
def get_multi_merge_diverge_mock_client(self, api_version):
|
||||||
def test_get_diff_files_multi_merge_diverge(self):
|
|
||||||
bitbucket_client = MagicMock(Bitbucket)
|
bitbucket_client = MagicMock(Bitbucket)
|
||||||
bitbucket_client.get_pull_request.return_value = {
|
bitbucket_client.get_pull_request.return_value = {
|
||||||
'toRef': {'latestCommit': '9569922b22fe4fd0968be6a50ed99f71efcd0504'},
|
'toRef': {'latestCommit': '9569922b22fe4fd0968be6a50ed99f71efcd0504'},
|
||||||
@ -192,6 +221,61 @@ class TestBitbucketServerProvider:
|
|||||||
]
|
]
|
||||||
|
|
||||||
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
|
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",
|
||||||
|
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
|
||||||
|
|
||||||
|
def test_get_diff_files_multi_merge_diverge_816(self):
|
||||||
|
bitbucket_client = self.get_multi_merge_diverge_mock_client(816)
|
||||||
|
|
||||||
provider = BitbucketServerProvider(
|
provider = BitbucketServerProvider(
|
||||||
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
|
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
|
||||||
|
Reference in New Issue
Block a user