From 4fe3da498ad51308550509a461cb1b1249978ba0 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 3 Mar 2026 12:19:05 -0500 Subject: [PATCH] Fix GitLab integration type errors for mypy compliance (#13172) Co-authored-by: openhands --- .../integrations/gitlab/gitlab_manager.py | 39 ++++++++++--------- .../integrations/gitlab/gitlab_service.py | 25 ++++++++++++ enterprise/integrations/gitlab/gitlab_view.py | 2 +- .../gitlab/webhook_installation.py | 24 ++++++------ enterprise/server/middleware.py | 6 +-- .../server/routes/integration/gitlab.py | 6 +++ enterprise/sync/install_gitlab_webhooks.py | 31 ++++++++------- .../unit/sync/test_install_gitlab_webhooks.py | 4 +- 8 files changed, 87 insertions(+), 50 deletions(-) diff --git a/enterprise/integrations/gitlab/gitlab_manager.py b/enterprise/integrations/gitlab/gitlab_manager.py index 9574e7ff44..914823f8f5 100644 --- a/enterprise/integrations/gitlab/gitlab_manager.py +++ b/enterprise/integrations/gitlab/gitlab_manager.py @@ -1,4 +1,7 @@ +from __future__ import annotations + from types import MappingProxyType +from typing import cast from integrations.gitlab.gitlab_view import ( GitlabFactory, @@ -67,11 +70,11 @@ class GitlabManager(Manager): logger.warning(f'Got invalid keyloak user id for GitLab User {user_id}') return False - # Importing here prevents circular import + # GitLabServiceImpl returns SaaSGitLabService in enterprise context from integrations.gitlab.gitlab_service import SaaSGitLabService - gitlab_service: SaaSGitLabService = GitLabServiceImpl( - external_auth_id=keycloak_user_id + gitlab_service = cast( + SaaSGitLabService, GitLabServiceImpl(external_auth_id=keycloak_user_id) ) return await gitlab_service.user_has_write_access(project_id) @@ -130,36 +133,36 @@ class GitlabManager(Manager): """ keycloak_user_id = gitlab_view.user_info.keycloak_user_id - # Importing here prevents circular import + # GitLabServiceImpl returns SaaSGitLabService in enterprise context from integrations.gitlab.gitlab_service import SaaSGitLabService - gitlab_service: SaaSGitLabService = GitLabServiceImpl( - external_auth_id=keycloak_user_id + gitlab_service = cast( + SaaSGitLabService, GitLabServiceImpl(external_auth_id=keycloak_user_id) ) if isinstance(gitlab_view, GitlabInlineMRComment) or isinstance( gitlab_view, GitlabMRComment ): await gitlab_service.reply_to_mr( - gitlab_view.project_id, - gitlab_view.issue_number, - gitlab_view.discussion_id, - message, + project_id=str(gitlab_view.project_id), + merge_request_iid=str(gitlab_view.issue_number), + discussion_id=gitlab_view.discussion_id, + body=message, ) elif isinstance(gitlab_view, GitlabIssueComment): await gitlab_service.reply_to_issue( - gitlab_view.project_id, - gitlab_view.issue_number, - gitlab_view.discussion_id, - message, + project_id=str(gitlab_view.project_id), + issue_number=str(gitlab_view.issue_number), + discussion_id=gitlab_view.discussion_id, + body=message, ) elif isinstance(gitlab_view, GitlabIssue): await gitlab_service.reply_to_issue( - gitlab_view.project_id, - gitlab_view.issue_number, - None, # no discussion id, issue is tagged - message, + project_id=str(gitlab_view.project_id), + issue_number=str(gitlab_view.issue_number), + discussion_id=None, # no discussion id, issue is tagged + body=message, ) else: logger.warning( diff --git a/enterprise/integrations/gitlab/gitlab_service.py b/enterprise/integrations/gitlab/gitlab_service.py index e5c6ce5272..558cc15058 100644 --- a/enterprise/integrations/gitlab/gitlab_service.py +++ b/enterprise/integrations/gitlab/gitlab_service.py @@ -185,6 +185,31 @@ class SaaSGitLabService(GitLabService): users_personal_projects: List of personal projects owned by the user repositories: List of Repository objects to store """ + # If external_auth_id is not set, try to determine it from the Keycloak token + if not self.external_auth_id and self.external_auth_token: + try: + user_info = await self.token_manager.get_user_info( + self.external_auth_token.get_secret_value() + ) + keycloak_user_id = user_info.get('sub') + if keycloak_user_id: + self.external_auth_id = keycloak_user_id + logger.info( + f'Determined external_auth_id from Keycloak token: {self.external_auth_id}' + ) + except Exception: + logger.warning( + 'Cannot store repository data: external_auth_id is not set and could not be determined from token', + exc_info=True, + ) + return + + if not self.external_auth_id: + logger.warning( + 'Cannot store repository data: external_auth_id could not be determined' + ) + return + try: # First, add owned projects and groups to the database await self.add_owned_projects_and_groups_to_db(users_personal_projects) diff --git a/enterprise/integrations/gitlab/gitlab_view.py b/enterprise/integrations/gitlab/gitlab_view.py index bad7e7b451..924d18dcd6 100644 --- a/enterprise/integrations/gitlab/gitlab_view.py +++ b/enterprise/integrations/gitlab/gitlab_view.py @@ -303,7 +303,7 @@ class GitlabFactory: @staticmethod async def create_gitlab_view_from_payload( message: Message, token_manager: TokenManager - ) -> ResolverViewInterface: + ) -> GitlabViewType: payload = message.message['payload'] installation_id = message.message['installation_id'] user = payload['user'] diff --git a/enterprise/integrations/gitlab/webhook_installation.py b/enterprise/integrations/gitlab/webhook_installation.py index 123ec21079..71cb427ae7 100644 --- a/enterprise/integrations/gitlab/webhook_installation.py +++ b/enterprise/integrations/gitlab/webhook_installation.py @@ -4,7 +4,9 @@ This module contains reusable functions and classes for installing GitLab webhoo that can be used by both the cron job and API routes. """ -from typing import cast +from __future__ import annotations + +from typing import TYPE_CHECKING from uuid import uuid4 from integrations.types import GitLabResourceType @@ -13,7 +15,9 @@ from storage.gitlab_webhook import GitlabWebhook, WebhookStatus from storage.gitlab_webhook_store import GitlabWebhookStore from openhands.core.logger import openhands_logger as logger -from openhands.integrations.service_types import GitService + +if TYPE_CHECKING: + from integrations.gitlab.gitlab_service import SaaSGitLabService # Webhook configuration constants WEBHOOK_NAME = 'OpenHands Resolver' @@ -35,7 +39,7 @@ class BreakLoopException(Exception): async def verify_webhook_conditions( - gitlab_service: type[GitService], + gitlab_service: SaaSGitLabService, resource_type: GitLabResourceType, resource_id: str, webhook_store: GitlabWebhookStore, @@ -52,10 +56,6 @@ async def verify_webhook_conditions( webhook_store: Webhook store instance webhook: Webhook object to verify """ - from integrations.gitlab.gitlab_service import SaaSGitLabService - - gitlab_service = cast(type[SaaSGitLabService], gitlab_service) - # Check if resource exists does_resource_exist, status = await gitlab_service.check_resource_exists( resource_type, resource_id @@ -106,7 +106,9 @@ async def verify_webhook_conditions( does_webhook_exist_on_resource, status, ) = await gitlab_service.check_webhook_exists_on_resource( - resource_type, resource_id, GITLAB_WEBHOOK_URL + resource_type=resource_type, + resource_id=resource_id, + webhook_url=GITLAB_WEBHOOK_URL, ) logger.info( @@ -131,7 +133,7 @@ async def verify_webhook_conditions( async def install_webhook_on_resource( - gitlab_service: type[GitService], + gitlab_service: SaaSGitLabService, resource_type: GitLabResourceType, resource_id: str, webhook_store: GitlabWebhookStore, @@ -150,10 +152,6 @@ async def install_webhook_on_resource( Returns: Tuple of (webhook_id, status) """ - from integrations.gitlab.gitlab_service import SaaSGitLabService - - gitlab_service = cast(type[SaaSGitLabService], gitlab_service) - webhook_secret = f'{webhook.user_id}-{str(uuid4())}' webhook_uuid = f'{str(uuid4())}' diff --git a/enterprise/server/middleware.py b/enterprise/server/middleware.py index c1c11d6f49..726c552d3b 100644 --- a/enterprise/server/middleware.py +++ b/enterprise/server/middleware.py @@ -53,9 +53,9 @@ class SetAuthCookieMiddleware: ) # On re-authentication (token refresh), kick off background sync for GitLab repos - schedule_gitlab_repo_sync( - await user_auth.get_user_id(), - ) + user_id = await user_auth.get_user_id() + if user_id: + schedule_gitlab_repo_sync(user_id) if ( self._should_attach(request) diff --git a/enterprise/server/routes/integration/gitlab.py b/enterprise/server/routes/integration/gitlab.py index a67c0c9742..acdb93c6fa 100644 --- a/enterprise/server/routes/integration/gitlab.py +++ b/enterprise/server/routes/integration/gitlab.py @@ -329,6 +329,12 @@ async def reinstall_gitlab_webhook( resource_type, resource_id ) + if not webhook: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail='Failed to create or fetch webhook record', + ) + # Verify conditions and install webhook try: await verify_webhook_conditions( diff --git a/enterprise/sync/install_gitlab_webhooks.py b/enterprise/sync/install_gitlab_webhooks.py index e11085e300..bf1f9f7f42 100644 --- a/enterprise/sync/install_gitlab_webhooks.py +++ b/enterprise/sync/install_gitlab_webhooks.py @@ -1,5 +1,7 @@ +from __future__ import annotations + import asyncio -from typing import cast +from typing import TYPE_CHECKING, cast from integrations.gitlab.webhook_installation import ( BreakLoopException, @@ -15,7 +17,9 @@ from storage.gitlab_webhook_store import GitlabWebhookStore from openhands.core.logger import openhands_logger as logger from openhands.integrations.gitlab.gitlab_service import GitLabServiceImpl -from openhands.integrations.service_types import GitService + +if TYPE_CHECKING: + from integrations.gitlab.gitlab_service import SaaSGitLabService CHUNK_SIZE = 100 @@ -35,7 +39,7 @@ class VerifyWebhookStatus: async def check_if_webhook_already_exists_on_resource( self, - gitlab_service: type[GitService], + gitlab_service: SaaSGitLabService, resource_type: GitLabResourceType, resource_id: str, webhook_store: GitlabWebhookStore, @@ -44,14 +48,13 @@ class VerifyWebhookStatus: """ Check whether webhook already exists on resource """ - from integrations.gitlab.gitlab_service import SaaSGitLabService - - gitlab_service = cast(type[SaaSGitLabService], gitlab_service) ( does_webhook_exist_on_resource, status, ) = await gitlab_service.check_webhook_exists_on_resource( - resource_type, resource_id, GITLAB_WEBHOOK_URL + resource_type=resource_type, + resource_id=resource_id, + webhook_url=GITLAB_WEBHOOK_URL, ) logger.info( @@ -75,7 +78,7 @@ class VerifyWebhookStatus: async def verify_conditions_are_met( self, - gitlab_service: type[GitService], + gitlab_service: SaaSGitLabService, resource_type: GitLabResourceType, resource_id: str, webhook_store: GitlabWebhookStore, @@ -92,7 +95,7 @@ class VerifyWebhookStatus: async def create_new_webhook( self, - gitlab_service: type[GitService], + gitlab_service: SaaSGitLabService, resource_type: GitLabResourceType, resource_id: str, webhook_store: GitlabWebhookStore, @@ -165,12 +168,12 @@ class VerifyWebhookStatus: webhook ) - gitlab_service_impl = GitLabServiceImpl(external_auth_id=user_id) + # GitLabServiceImpl returns SaaSGitLabService in enterprise context + from integrations.gitlab.gitlab_service import SaaSGitLabService - if not isinstance(gitlab_service_impl, SaaSGitLabService): - raise Exception('Only SaaSGitLabService is supported') - # Cast needed when mypy can see OpenHands - gitlab_service = cast(type[SaaSGitLabService], gitlab_service_impl) + gitlab_service = cast( + SaaSGitLabService, GitLabServiceImpl(external_auth_id=user_id) + ) await self.verify_conditions_are_met( gitlab_service=gitlab_service, diff --git a/enterprise/tests/unit/sync/test_install_gitlab_webhooks.py b/enterprise/tests/unit/sync/test_install_gitlab_webhooks.py index 3d8d91a965..b1027c842b 100644 --- a/enterprise/tests/unit/sync/test_install_gitlab_webhooks.py +++ b/enterprise/tests/unit/sync/test_install_gitlab_webhooks.py @@ -75,7 +75,9 @@ class TestVerifyWebhookConditions: resource_type, resource_id ) mock_gitlab_service.check_webhook_exists_on_resource.assert_called_once_with( - resource_type, resource_id, GITLAB_WEBHOOK_URL + resource_type=resource_type, + resource_id=resource_id, + webhook_url=GITLAB_WEBHOOK_URL, ) mock_webhook_store.delete_webhook.assert_not_called()