Skip to content

Commit d4d9a7f

Browse files
mrT23mrT23
authored
Merge pull request #1172 from MaxHoecker/bugfix/bb-2-way-diff-fix
Bugfix/bb 2 way diff fix
2 parents c14c497 + cf14e45 commit d4d9a7f

File tree

2 files changed

+333
-7
lines changed

2 files changed

+333
-7
lines changed

‎pr_agent/git_providers/bitbucket_server_provider.py‎

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1+
from distutils.version import LooseVersion
2+
from requests.exceptions import HTTPError
13
from typing import Optional, Tuple
24
from urllib.parse import quote_plus, urlparse
35

4-
from requests.exceptions import HTTPError
56
from atlassian.bitbucket import Bitbucket
6-
from starlette_context import context
77

88
from .git_provider import GitProvider
99
from ..algo.types import EDIT_TYPE, FilePatchInfo
@@ -15,7 +15,8 @@
1515

1616
class BitbucketServerProvider(GitProvider):
1717
def __init__(
18-
self, pr_url: Optional[str] = None, incremental: Optional[bool] = False
18+
self, pr_url: Optional[str] = None, incremental: Optional[bool] = False,
19+
bitbucket_client: Optional[Bitbucket] = None,
1920
):
2021
self.bitbucket_server_url = None
2122
self.workspace_slug = None
@@ -30,8 +31,13 @@ def __init__(
3031
self.bitbucket_pull_request_api_url = pr_url
3132

3233
self.bitbucket_server_url = self._parse_bitbucket_server(url=pr_url)
33-
self.bitbucket_client = Bitbucket(url=self.bitbucket_server_url,
34-
token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN", None))
34+
self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url,
35+
token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN",
36+
None))
37+
try:
38+
self.bitbucket_api_version = LooseVersion(self.bitbucket_client.get("rest/api/1.0/application-properties").get('version'))
39+
except Exception:
40+
self.bitbucket_api_version = None
3541

3642
if pr_url:
3743
self.set_pr(pr_url)
@@ -134,13 +140,51 @@ def get_files(self):
134140
diffstat = [change["path"]['toString'] for change in changes]
135141
return diffstat
136142

143+
#gets the best common ancestor: https://git-scm.com/docs/git-merge-base
144+
@staticmethod
145+
def get_best_common_ancestor(source_commits_list, destination_commits_list, guaranteed_common_ancestor) -> str:
146+
destination_commit_hashes = {commit['id'] for commit in destination_commits_list} | {guaranteed_common_ancestor}
147+
148+
for commit in source_commits_list:
149+
for parent_commit in commit['parents']:
150+
if parent_commit['id'] in destination_commit_hashes:
151+
return parent_commit['id']
152+
153+
return guaranteed_common_ancestor
154+
137155
def get_diff_files(self) -> list[FilePatchInfo]:
138156
if self.diff_files:
139157
return self.diff_files
140158

141-
base_sha = self.pr.toRef['latestCommit']
142159
head_sha = self.pr.fromRef['latestCommit']
143160

161+
# if Bitbucket api version is >= 8.16 then use the merge-base api for 2-way diff calculation
162+
if self.bitbucket_api_version is not None and self.bitbucket_api_version >= LooseVersion("8.16"):
163+
try:
164+
base_sha = self.bitbucket_client.get(self._get_merge_base())['id']
165+
except Exception as e:
166+
get_logger().error(f"Failed to get the best common ancestor for PR: {self.pr_url}, \nerror: {e}")
167+
raise e
168+
else:
169+
source_commits_list = list(self.bitbucket_client.get_pull_requests_commits(
170+
self.workspace_slug,
171+
self.repo_slug,
172+
self.pr_num
173+
))
174+
# if Bitbucket api version is None or < 7.0 then do a simple diff with a guaranteed common ancestor
175+
base_sha = source_commits_list[-1]['parents'][0]['id']
176+
# if Bitbucket api version is 7.0-8.15 then use 2-way diff functionality for the base_sha
177+
if self.bitbucket_api_version is not None and self.bitbucket_api_version >= LooseVersion("7.0"):
178+
try:
179+
destination_commits = list(
180+
self.bitbucket_client.get_commits(self.workspace_slug, self.repo_slug, base_sha,
181+
self.pr.toRef['latestCommit']))
182+
base_sha = self.get_best_common_ancestor(source_commits_list, destination_commits, base_sha)
183+
except Exception as e:
184+
get_logger().error(
185+
f"Failed to get the commit list for calculating best common ancestor for PR: {self.pr_url}, \nerror: {e}")
186+
raise e
187+
144188
diff_files = []
145189
original_file_content_str = ""
146190
new_file_content_str = ""
@@ -407,3 +451,6 @@ def get_pr_labels(self, update=False):
407451

408452
def _get_pr_comments_path(self):
409453
return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/comments"
454+
455+
def _get_merge_base(self):
456+
return f"rest/api/latest/projects/{self.workspace_slug}/repos/{self.repo_slug}/pull-requests/{self.pr_num}/merge-base"

‎tests/unittest/test_bitbucket_provider.py‎

Lines changed: 280 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
from pr_agent.git_providers import BitbucketServerProvider
22
from pr_agent.git_providers.bitbucket_provider import BitbucketProvider
3+
from unittest.mock import MagicMock
4+
from atlassian.bitbucket import Bitbucket
5+
from pr_agent.algo.types import EDIT_TYPE, FilePatchInfo
36

47

58
class TestBitbucketProvider:
@@ -10,9 +13,285 @@ def test_parse_pr_url(self):
1013
assert repo_slug == "MY_TEST_REPO"
1114
assert pr_number == 321
1215

13-
def test_bitbucket_server_pr_url(self):
16+
17+
class TestBitbucketServerProvider:
18+
def test_parse_pr_url(self):
1419
url = "https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1"
1520
workspace_slug, repo_slug, pr_number = BitbucketServerProvider._parse_pr_url(url)
1621
assert workspace_slug == "AAA"
1722
assert repo_slug == "my-repo"
1823
assert pr_number == 1
24+
25+
def mock_get_content_of_file(self, project_key, repository_slug, filename, at=None, markup=None):
26+
content_map = {
27+
'9c1cffdd9f276074bfb6fb3b70fbee62d298b058': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n',
28+
'2a1165446bdf991caf114d01f7c88d84ae7399cf': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n',
29+
'f617708826cdd0b40abb5245eda71630192a17e3': 'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n',
30+
'cb68a3027d6dda065a7692ebf2c90bed1bcdec28': 'file\nwith\nsome\nchanges\nto\nemulate\na\nreal\nfile\n',
31+
'1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b': 'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
32+
'ae4eca7f222c96d396927d48ab7538e2ee13ca63': 'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
33+
'548f8ba15abc30875a082156314426806c3f4d97': 'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile',
34+
'0e898cb355a5170d8c8771b25d43fcaa1d2d9489': 'file\nwith\nmultiple\nlines\nto\nemulate\na\nreal\nfile'
35+
}
36+
return content_map.get(at, '')
37+
38+
def mock_get_from_bitbucket_60(self, url):
39+
response_map = {
40+
"rest/api/1.0/application-properties": {
41+
"version": "6.0"
42+
}
43+
}
44+
return response_map.get(url, '')
45+
46+
def mock_get_from_bitbucket_70(self, url):
47+
response_map = {
48+
"rest/api/1.0/application-properties": {
49+
"version": "7.0"
50+
}
51+
}
52+
return response_map.get(url, '')
53+
54+
def mock_get_from_bitbucket_816(self, url):
55+
response_map = {
56+
"rest/api/1.0/application-properties": {
57+
"version": "8.16"
58+
},
59+
"rest/api/latest/projects/AAA/repos/my-repo/pull-requests/1/merge-base": {
60+
'id': '548f8ba15abc30875a082156314426806c3f4d97'
61+
}
62+
}
63+
return response_map.get(url, '')
64+
65+
66+
'''
67+
tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c
68+
NOT between the HEAD of main and the HEAD of branch b
69+
70+
- o branch b
71+
/
72+
o - o - o main
73+
^ node c
74+
'''
75+
def test_get_diff_files_simple_diverge_70(self):
76+
bitbucket_client = MagicMock(Bitbucket)
77+
bitbucket_client.get_pull_request.return_value = {
78+
'toRef': {'latestCommit': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
79+
'fromRef': {'latestCommit': '2a1165446bdf991caf114d01f7c88d84ae7399cf'}
80+
}
81+
bitbucket_client.get_pull_requests_commits.return_value = [
82+
{'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf',
83+
'parents': [{'id': 'f617708826cdd0b40abb5245eda71630192a17e3'}]}
84+
]
85+
bitbucket_client.get_commits.return_value = [
86+
{'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
87+
{'id': 'dbca09554567d2e4bee7f07993390153280ee450'}
88+
]
89+
bitbucket_client.get_pull_requests_changes.return_value = [
90+
{
91+
'path': {'toString': 'Readme.md'},
92+
'type': 'MODIFY',
93+
}
94+
]
95+
96+
bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
97+
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
98+
99+
provider = BitbucketServerProvider(
100+
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
101+
bitbucket_client=bitbucket_client
102+
)
103+
104+
expected = [
105+
FilePatchInfo(
106+
'file\nwith\nmultiple \nlines\nto\nemulate\na\nreal\nfile\n',
107+
'file\nwith\nmultiple \nlines\nto\nemulate\na\nfake\nfile\n',
108+
'--- \n+++ \n@@ -5,5 +5,5 @@\n to\n emulate\n a\n-real\n+fake\n file\n',
109+
'Readme.md',
110+
edit_type=EDIT_TYPE.MODIFIED,
111+
)
112+
]
113+
114+
actual = provider.get_diff_files()
115+
116+
assert actual == expected
117+
118+
119+
'''
120+
tests the 2-way diff functionality where the diff should be between the HEAD of branch b and node c
121+
NOT between the HEAD of main and the HEAD of branch b
122+
123+
- o - o - o branch b
124+
/ /
125+
o - o -- o - o main
126+
^ node c
127+
'''
128+
def test_get_diff_files_diverge_with_merge_commit_70(self):
129+
bitbucket_client = MagicMock(Bitbucket)
130+
bitbucket_client.get_pull_request.return_value = {
131+
'toRef': {'latestCommit': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'},
132+
'fromRef': {'latestCommit': '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b'}
133+
}
134+
bitbucket_client.get_pull_requests_commits.return_value = [
135+
{'id': '1905dcf16c0aac6ac24f7ab617ad09c73dc1d23b',
136+
'parents': [{'id': '692772f456c3db77a90b11ce39ea516f8c2bad93'}]},
137+
{'id': '692772f456c3db77a90b11ce39ea516f8c2bad93', 'parents': [
138+
{'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf'},
139+
{'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
140+
]},
141+
{'id': '2a1165446bdf991caf114d01f7c88d84ae7399cf',
142+
'parents': [{'id': 'f617708826cdd0b40abb5245eda71630192a17e3'}]}
143+
]
144+
bitbucket_client.get_commits.return_value = [
145+
{'id': 'cb68a3027d6dda065a7692ebf2c90bed1bcdec28'},
146+
{'id': '9c1cffdd9f276074bfb6fb3b70fbee62d298b058'},
147+
{'id': 'dbca09554567d2e4bee7f07993390153280ee450'}
148+
]
149+
bitbucket_client.get_pull_requests_changes.return_value = [
150+
{
151+
'path': {'toString': 'Readme.md'},
152+
'type': 'MODIFY',
153+
}
154+
]
155+
156+
bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
157+
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
158+
159+
provider = BitbucketServerProvider(
160+
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
161+
bitbucket_client=bitbucket_client
162+
)
163+
164+
expected = [
165+
FilePatchInfo(
166+
'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile\n',
167+
'file\nwith\nsome\nlines\nto\nemulate\na\nfake\ntest\n',
168+
'--- \n+++ \n@@ -5,5 +5,5 @@\n to\n emulate\n a\n-real\n-file\n+fake\n+test\n',
169+
'Readme.md',
170+
edit_type=EDIT_TYPE.MODIFIED,
171+
)
172+
]
173+
174+
actual = provider.get_diff_files()
175+
176+
assert actual == expected
177+
178+
179+
'''
180+
tests the 2-way diff functionality where the diff should be between the HEAD of branch c and node d
181+
NOT between the HEAD of main and the HEAD of branch c
182+
183+
---- o - o branch c
184+
/ /
185+
---- o branch b
186+
/ /
187+
o - o - o main
188+
^ node d
189+
'''
190+
def get_multi_merge_diverge_mock_client(self, api_version):
191+
bitbucket_client = MagicMock(Bitbucket)
192+
bitbucket_client.get_pull_request.return_value = {
193+
'toRef': {'latestCommit': '9569922b22fe4fd0968be6a50ed99f71efcd0504'},
194+
'fromRef': {'latestCommit': 'ae4eca7f222c96d396927d48ab7538e2ee13ca63'}
195+
}
196+
bitbucket_client.get_pull_requests_commits.return_value = [
197+
{'id': 'ae4eca7f222c96d396927d48ab7538e2ee13ca63',
198+
'parents': [{'id': 'bbf300fb3af5129af8c44659f8cc7a526a6a6f31'}]},
199+
{'id': 'bbf300fb3af5129af8c44659f8cc7a526a6a6f31', 'parents': [
200+
{'id': '10b7b8e41cb370b48ceda8da4e7e6ad033182213'},
201+
{'id': 'd1bb183c706a3ebe4c2b1158c25878201a27ad8c'},
202+
]},
203+
{'id': 'd1bb183c706a3ebe4c2b1158c25878201a27ad8c', 'parents': [
204+
{'id': '5bd76251866cb415fc5ff232f63a581e89223bda'},
205+
{'id': '548f8ba15abc30875a082156314426806c3f4d97'}
206+
]},
207+
{'id': '5bd76251866cb415fc5ff232f63a581e89223bda',
208+
'parents': [{'id': '0e898cb355a5170d8c8771b25d43fcaa1d2d9489'}]},
209+
{'id': '10b7b8e41cb370b48ceda8da4e7e6ad033182213',
210+
'parents': [{'id': '0e898cb355a5170d8c8771b25d43fcaa1d2d9489'}]}
211+
]
212+
bitbucket_client.get_commits.return_value = [
213+
{'id': '9569922b22fe4fd0968be6a50ed99f71efcd0504'},
214+
{'id': '548f8ba15abc30875a082156314426806c3f4d97'}
215+
]
216+
bitbucket_client.get_pull_requests_changes.return_value = [
217+
{
218+
'path': {'toString': 'Readme.md'},
219+
'type': 'MODIFY',
220+
}
221+
]
222+
223+
bitbucket_client.get_content_of_file.side_effect = self.mock_get_content_of_file
224+
if api_version == 60:
225+
bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_60
226+
elif api_version == 70:
227+
bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_70
228+
elif api_version == 816:
229+
bitbucket_client.get.side_effect = self.mock_get_from_bitbucket_816
230+
231+
return bitbucket_client
232+
233+
def test_get_diff_files_multi_merge_diverge_60(self):
234+
bitbucket_client = self.get_multi_merge_diverge_mock_client(60)
235+
236+
provider = BitbucketServerProvider(
237+
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
238+
bitbucket_client=bitbucket_client
239+
)
240+
241+
expected = [
242+
FilePatchInfo(
243+
'file\nwith\nmultiple\nlines\nto\nemulate\na\nreal\nfile',
244+
'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
245+
'--- \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',
246+
'Readme.md',
247+
edit_type=EDIT_TYPE.MODIFIED,
248+
)
249+
]
250+
251+
actual = provider.get_diff_files()
252+
253+
assert actual == expected
254+
255+
def test_get_diff_files_multi_merge_diverge_70(self):
256+
bitbucket_client = self.get_multi_merge_diverge_mock_client(70)
257+
258+
provider = BitbucketServerProvider(
259+
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
260+
bitbucket_client=bitbucket_client
261+
)
262+
263+
expected = [
264+
FilePatchInfo(
265+
'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile',
266+
'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
267+
'--- \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',
268+
'Readme.md',
269+
edit_type=EDIT_TYPE.MODIFIED,
270+
)
271+
]
272+
273+
actual = provider.get_diff_files()
274+
275+
assert actual == expected
276+
277+
def test_get_diff_files_multi_merge_diverge_816(self):
278+
bitbucket_client = self.get_multi_merge_diverge_mock_client(816)
279+
280+
provider = BitbucketServerProvider(
281+
"https://git.onpreminstance.com/projects/AAA/repos/my-repo/pull-requests/1",
282+
bitbucket_client=bitbucket_client
283+
)
284+
285+
expected = [
286+
FilePatchInfo(
287+
'file\nwith\nsome\nlines\nto\nemulate\na\nreal\nfile',
288+
'readme\nwithout\nsome\nlines\nto\nsimulate\na\nreal\nfile',
289+
'--- \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',
290+
'Readme.md',
291+
edit_type=EDIT_TYPE.MODIFIED,
292+
)
293+
]
294+
295+
actual = provider.get_diff_files()
296+
297+
assert actual == expected

0 commit comments

Comments
 (0)