From 3432bbbb8889fd042e1d439163db8b1b53df92e3 Mon Sep 17 00:00:00 2001 From: Joe Laverty Date: Tue, 10 Mar 2026 12:08:30 -0400 Subject: [PATCH] fix: Remove N+1 request from Bitbucket Data Center integration (#13281) --- .../bitbucket_data_center/service/base.py | 26 ++++--- .../test_bitbucket_dc_repos.py | 67 +++++++------------ 2 files changed, 42 insertions(+), 51 deletions(-) diff --git a/openhands/integrations/bitbucket_data_center/service/base.py b/openhands/integrations/bitbucket_data_center/service/base.py index bb53d9102f..849b2df076 100644 --- a/openhands/integrations/bitbucket_data_center/service/base.py +++ b/openhands/integrations/bitbucket_data_center/service/base.py @@ -216,13 +216,18 @@ class BitbucketDCMixinBase(BaseGitService, HTTPClient): ) async def _parse_repository( - self, repo: dict, link_header: str | None = None + self, + repo: dict, + link_header: str | None = None, + fetch_default_branch: bool = False, ) -> Repository: """Parse a Bitbucket data center API repository response into a Repository object. Args: repo: Repository data from Bitbucket data center API link_header: Optional link header for pagination + fetch_default_branch: Whether to make an additional API call to fetch the + default branch. Set to False for listing endpoints to avoid N+1 queries. Returns: Repository object @@ -240,14 +245,15 @@ class BitbucketDCMixinBase(BaseGitService, HTTPClient): is_public = repo.get('public', False) main_branch: str | None = None - try: - default_branch_url = ( - f'{self._repo_api_base(project_key, repo_slug)}/default-branch' - ) - default_branch_data, _ = await self._make_request(default_branch_url) - main_branch = default_branch_data.get('displayId') or None - except Exception as e: - logger.debug(f'Could not fetch default branch for {full_name}: {e}') + if fetch_default_branch: + try: + default_branch_url = ( + f'{self._repo_api_base(project_key, repo_slug)}/default-branch' + ) + default_branch_data, _ = await self._make_request(default_branch_url) + main_branch = default_branch_data.get('displayId') or None + except Exception as e: + logger.debug(f'Could not fetch default branch for {full_name}: {e}') return Repository( id=str(repo.get('id', '')), @@ -275,7 +281,7 @@ class BitbucketDCMixinBase(BaseGitService, HTTPClient): owner, repo = self._extract_owner_and_repo(repository) url = self._repo_api_base(owner, repo) data, _ = await self._make_request(url) - return await self._parse_repository(data) + return await self._parse_repository(data, fetch_default_branch=True) async def _get_cursorrules_url(self, repository: str) -> str: """Get the URL for checking .cursorrules file.""" diff --git a/tests/unit/integrations/bitbucket_data_center/test_bitbucket_dc_repos.py b/tests/unit/integrations/bitbucket_data_center/test_bitbucket_dc_repos.py index 521dd61455..1196fad5aa 100644 --- a/tests/unit/integrations/bitbucket_data_center/test_bitbucket_dc_repos.py +++ b/tests/unit/integrations/bitbucket_data_center/test_bitbucket_dc_repos.py @@ -112,21 +112,15 @@ async def test_search_repositories_slash_query(): query = 'PROJ/myrepo' mock_repo = _repo_dict('PROJ', slug='myrepo', name='My Repository') - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_fetch_paginated_data', new=AsyncMock(return_value=[mock_repo]), ) as mock_fetch: - with patch.object( - svc, - '_make_request', - new=AsyncMock(return_value=(mock_default_branch, {})), - ): - repos = await svc.search_repositories( - query, 25, 'name', 'asc', False, AppMode.SAAS - ) + repos = await svc.search_repositories( + query, 25, 'name', 'asc', False, AppMode.SAAS + ) mock_fetch.assert_called_once_with( 'https://host.example.com/rest/api/1.0/projects/PROJ/repos', @@ -135,6 +129,7 @@ async def test_search_repositories_slash_query(): ) assert len(repos) == 1 assert repos[0].full_name == 'PROJ/myrepo' + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -143,24 +138,19 @@ async def test_search_repositories_slash_query_filters_by_name(): svc = make_service() matching = _repo_dict('PROJ', slug='proj-alpha', name='My Repository') non_matching = _repo_dict('PROJ', slug='proj-beta', name='Other Repo') - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_fetch_paginated_data', new=AsyncMock(return_value=[matching, non_matching]), ): - with patch.object( - svc, - '_make_request', - new=AsyncMock(return_value=(mock_default_branch, {})), - ): - repos = await svc.search_repositories( - 'PROJ/my repository', 25, 'name', 'asc', False, AppMode.SAAS - ) + repos = await svc.search_repositories( + 'PROJ/my repository', 25, 'name', 'asc', False, AppMode.SAAS + ) assert len(repos) == 1 assert repos[0].full_name == 'PROJ/proj-alpha' + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -169,24 +159,19 @@ async def test_search_repositories_slash_query_filters_by_slug(): svc = make_service() matching = _repo_dict('PROJ', slug='my-repo', name='My Repository') non_matching = _repo_dict('PROJ', slug='other-repo', name='Other Repository') - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_fetch_paginated_data', new=AsyncMock(return_value=[matching, non_matching]), ): - with patch.object( - svc, - '_make_request', - new=AsyncMock(return_value=(mock_default_branch, {})), - ): - repos = await svc.search_repositories( - 'PROJ/my-repo', 25, 'name', 'asc', False, AppMode.SAAS - ) + repos = await svc.search_repositories( + 'PROJ/my-repo', 25, 'name', 'asc', False, AppMode.SAAS + ) assert len(repos) == 1 assert repos[0].full_name == 'PROJ/my-repo' + assert repos[0].main_branch is None # ── get_paginated_repos ─────────────────────────────────────────────────────── @@ -199,18 +184,18 @@ async def test_get_paginated_repos_parses_values(): 'values': [_repo_dict()], 'isLastPage': True, } - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_make_request', - side_effect=[(mock_response, {}), (mock_default_branch, {})], + return_value=(mock_response, {}), ): repos = await svc.get_paginated_repos(1, 25, 'name', 'PROJ') assert len(repos) == 1 assert repos[0].full_name == 'PROJ/myrepo' assert repos[0].link_header == '' + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -221,17 +206,17 @@ async def test_get_paginated_repos_has_next_page(): 'isLastPage': False, 'nextPageStart': 25, } - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_make_request', - side_effect=[(mock_response, {}), (mock_default_branch, {})], + return_value=(mock_response, {}), ): repos = await svc.get_paginated_repos(1, 25, 'name', 'PROJ') assert len(repos) == 1 assert 'rel="next"' in repos[0].link_header + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -241,17 +226,17 @@ async def test_get_paginated_repos_last_page(): 'values': [_repo_dict()], 'isLastPage': True, } - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_make_request', - side_effect=[(mock_response, {}), (mock_default_branch, {})], + return_value=(mock_response, {}), ): repos = await svc.get_paginated_repos(1, 25, 'name', 'PROJ') assert len(repos) == 1 assert repos[0].link_header == '' + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -265,17 +250,17 @@ async def test_get_paginated_repos_filters_by_slug(): ], 'isLastPage': True, } - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_make_request', - side_effect=[(mock_response, {}), (mock_default_branch, {})], + return_value=(mock_response, {}), ): repos = await svc.get_paginated_repos(1, 25, 'name', 'PROJ', query='my-repo') assert len(repos) == 1 assert repos[0].full_name == 'PROJ/my-repo' + assert repos[0].main_branch is None @pytest.mark.asyncio @@ -289,12 +274,11 @@ async def test_get_paginated_repos_filters_by_name(): ], 'isLastPage': True, } - mock_default_branch = {'displayId': 'main'} with patch.object( svc, '_make_request', - side_effect=[(mock_response, {}), (mock_default_branch, {})], + return_value=(mock_response, {}), ): repos = await svc.get_paginated_repos( 1, 25, 'name', 'PROJ', query='my repository' @@ -302,6 +286,7 @@ async def test_get_paginated_repos_filters_by_name(): assert len(repos) == 1 assert repos[0].full_name == 'PROJ/proj-alpha' + assert repos[0].main_branch is None # ── get_all_repositories ────────────────────────────────────────────────────── @@ -320,14 +305,14 @@ async def test_get_all_repositories_iterates_projects(): return [_repo_dict('PROJ2', 'repo2')] return [] - mock_default_branch = {'displayId': 'main'} with patch.object(svc, '_fetch_paginated_data', side_effect=fake_fetch): - with patch.object(svc, '_make_request', return_value=(mock_default_branch, {})): - repos = await svc.get_all_repositories('name', AppMode.SAAS) + repos = await svc.get_all_repositories('name', AppMode.SAAS) full_names = {r.full_name for r in repos} assert 'PROJ1/repo1' in full_names assert 'PROJ2/repo2' in full_names + for repo in repos: + assert repo.main_branch is None # ── get_installations ───────────────────────────────────────────────────────── @@ -352,4 +337,4 @@ async def test_get_installations_returns_project_keys(): async def _make_parsed_repo(svc, repo_dict): """Helper to create a parsed Repository from a repo dict (with mocked default branch).""" with patch.object(svc, '_make_request', return_value=({'displayId': 'main'}, {})): - return await svc._parse_repository(repo_dict) + return await svc._parse_repository(repo_dict, fetch_default_branch=True)