Merge pull request #1172 from MaxHoecker/bugfix/bb-2-way-diff-fix

Bugfix/bb 2 way diff fix

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
@@ -15,7 +15,8 @@ from ..log import get_logger
 
 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,13 @@ class BitbucketServerProvider(GitProvider):
         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,
+        self.bitbucket_client = bitbucket_client or Bitbucket(url=self.bitbucket_server_url,
-                                          token=get_settings().get("BITBUCKET_SERVER.BEARER_TOKEN", None))
+                                                              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)
@@ -134,13 +140,51 @@ class BitbucketServerProvider(GitProvider):
         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} | {guaranteed_common_ancestor}
+
+        for commit in source_commits_list:
+            for parent_commit in commit['parents']:
+                if 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']
         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}")
+                    raise e
+
         diff_files = []
         original_file_content_str = ""
         new_file_content_str = ""
@@ -407,3 +451,6 @@ class BitbucketServerProvider(GitProvider):
 
     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_merge_base(self):
+        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

@@ -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,285 @@ class TestBitbucketProvider:
         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):
+        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',
+            '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
+
+          - o  branch b
+         /
+    o - o - o  main
+        ^ node c
+    '''
+    def test_get_diff_files_simple_diverge_70(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.side_effect = self.mock_get_from_bitbucket_70
+        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_70(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.side_effect = self.mock_get_from_bitbucket_70
+        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 get_multi_merge_diverge_mock_client(self, api_version):
+        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
+        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(
+            "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