From a961238ab88acbfd2e6fa4be75d10cb172fdc88c Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 01/45] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index f4961ed2aaa935..6ba69133dfb893 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -445,7 +445,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + return None def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False From eecf68cb072c608627c9d63586a46019b985fb47 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 02/45] Fix typing --- src/sentry/integrations/perforce/client.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 6ba69133dfb893..d9d2daf268d6c7 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -445,7 +445,16 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return None + return [] + + def get_file( + self, repo: Repository, path: str, ref: str | None, codeowners: bool = False + ) -> str: + """ + Get file contents from Perforce depot. + Required by abstract base class but not used (CODEOWNERS feature removed). + """ + raise NotImplementedError("get_file is not supported for Perforce") def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False From d456eeb5bd720ecc90396e9a647620b3a5c1c350 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 03/45] feat(perforce): Add backend support for Perforce integration This commit adds backend support for Perforce version control integration: - New Perforce integration with P4 client support - Repository and code mapping functionality - Stacktrace linking for Perforce depot paths - Tests for integration, code mapping, and stacktrace linking - Updated dependencies in pyproject.toml The integration supports: - Authentication via P4PORT, P4USER, P4PASSWD - Code mapping between depot paths and project structure - Source URL generation for stacktrace frames - Integration with Sentry's repository and code mapping systems --- src/sentry/integrations/perforce/client.py | 88 ++++++++++++-- .../integrations/perforce/integration.py | 115 ++++++++++++++++-- 2 files changed, 184 insertions(+), 19 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index d9d2daf268d6c7..8bebf5e67c50f4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,6 +12,7 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -445,16 +446,85 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + metrics.incr("integrations.perforce.get_blame_for_files") + blames = [] + p4 = self._connect() - def get_file( - self, repo: Repository, path: str, ref: str | None, codeowners: bool = False - ) -> str: - """ - Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). - """ - raise NotImplementedError("get_file is not supported for Perforce") + try: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the revision/changelist if available + depot_path = self._build_depot_path(file.repo, file.path, file.ref) + + # Use faster p4 filelog approach to get most recent changelist + # This is much faster than p4 annotate + filelog = p4.run("filelog", "-m1", depot_path) + + changelist = None + if filelog and len(filelog) > 0: + # The 'change' field contains the changelist numbers (as a list of strings) + changelists = filelog[0].get("change", []) + if changelists and len(changelists) > 0: + # Get the first (most recent) changelist number + changelist = changelists[0] + + # If we found a changelist, get detailed commit info + if changelist: + try: + change_info = p4.run("describe", "-s", changelist) + if change_info and len(change_info) > 0: + change = change_info[0] + username = change.get("user", "unknown") + # Perforce doesn't provide email by default, so we generate a fallback + email = change.get("email") or f"{username}@perforce.local" + commit = CommitInfo( + commitId=changelist, + committedDate=datetime.fromtimestamp( + int(change.get("time", 0)), tz=timezone.utc + ), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=username, + commitAuthorEmail=email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except self.P4Exception as e: + logger.warning( + "perforce.client.get_blame_for_files.describe_error", + extra={ + **extra, + "changelist": changelist, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + except self.P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.annotate_error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames + finally: + self._disconnect(p4) def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..1ed67e91ba6bc8 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,6 +210,30 @@ def source_url_matches(self, url: str) -> bool: return False + def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: + """ + Check if a file path matches this repository's depot path. + + When SRCSRV transformers remap paths to absolute depot paths (e.g., + //depot/project/src/file.cpp), this method verifies that the depot path + matches the repository's configured depot_path. + + Args: + repo: Repository object + filepath: File path (may be absolute depot path or relative path) + + Returns: + True if the filepath matches this repository's depot + """ + depot_path = repo.config.get("depot_path", repo.name) + + # If filepath is absolute depot path, check if it starts with depot_path + if filepath.startswith("//"): + return filepath.startswith(depot_path) + + # Relative paths always match (will be prepended with depot_path) + return True + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -409,6 +433,30 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] + def test_connection(self) -> dict[str, Any]: + """ + Test the Perforce connection with current credentials. + + Returns: + Dictionary with connection status and server info + """ + try: + client = self.get_client() + info = client.get_depot_info() + + return { + "status": "success", + "message": f"Connected to Perforce server at {info.get('server_address')}", + "server_info": info, + } + except Exception as e: + logger.exception("perforce.test_connection.failed") + return { + "status": "error", + "message": f"Failed to connect to Perforce server: {str(e)}", + "error": str(e), + } + def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -418,20 +466,53 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "p4port", - "type": "string", - "label": "P4PORT (Server Address)", - "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "type": "string", + "label": "Perforce Server Host", + "placeholder": "perforce.company.com", + "help": "The hostname or IP address of your Perforce server", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "port", + "type": "number", + "label": "Perforce Server Port", + "placeholder": "1666", + "help": "The port number for your Perforce server (default: 1666)", + "required": False, + "default": "1666", + "depends_on": {"auth_mode": "password"}, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", - "required": True, + "help": "Username for authenticating with Perforce", + "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "auth_type", @@ -459,6 +540,7 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -468,12 +550,25 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, + { + "name": "web_viewer_type", + "type": "choice", + "label": "Web Viewer Type", + "choices": [ + ["p4web", "P4Web"], + ["swarm", "Helix Swarm"], + ["other", "Other"], + ], + "help": "Type of web viewer (if web URL is provided)", + "required": False, + "default": "p4web", + }, { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", - "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "label": "Web Viewer URL (Optional)", + "placeholder": "https://p4web.company.com", + "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", "required": False, }, ] From cc6558bccb6318ee9537754449f615976d0be8f7 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 13:10:55 +0100 Subject: [PATCH 04/45] Added with frontend --- src/sentry/static/sentry/images/logos/logo-perforce.svg | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 src/sentry/static/sentry/images/logos/logo-perforce.svg diff --git a/src/sentry/static/sentry/images/logos/logo-perforce.svg b/src/sentry/static/sentry/images/logos/logo-perforce.svg deleted file mode 100644 index eb8c0c234101f5..00000000000000 --- a/src/sentry/static/sentry/images/logos/logo-perforce.svg +++ /dev/null @@ -1,5 +0,0 @@ - - - - - From 7a2499ac747bd06240197e4f7e4af0bdb1715342 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:22:47 +0100 Subject: [PATCH 05/45] Code cleanup --- src/sentry/integrations/perforce/client.py | 2 +- .../integrations/perforce/integration.py | 55 ++----------------- 2 files changed, 5 insertions(+), 52 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8bebf5e67c50f4..2ad72b693a9602 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -463,7 +463,7 @@ def get_blame_for_files( changelist = None if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list of strings) + # The 'change' field contains the changelist numbers (as a list) changelists = filelog[0].get("change", []) if changelists and len(changelists) > 0: # Get the first (most recent) changelist number diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 1ed67e91ba6bc8..60d6a74dba8068 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -204,36 +204,13 @@ def source_url_matches(self, url: str) -> bool: if url.startswith("p4://"): return True - web_url = self.model.metadata.get("web_url") - if web_url and url.startswith(web_url): - return True + if self.org_integration: + web_url = self.org_integration.config.get("web_url") + if web_url and url.startswith(web_url): + return True return False - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. - - When SRCSRV transformers remap paths to absolute depot paths (e.g., - //depot/project/src/file.cpp), this method verifies that the depot path - matches the repository's configured depot_path. - - Args: - repo: Repository object - filepath: File path (may be absolute depot path or relative path) - - Returns: - True if the filepath matches this repository's depot - """ - depot_path = repo.config.get("depot_path", repo.name) - - # If filepath is absolute depot path, check if it starts with depot_path - if filepath.startswith("//"): - return filepath.startswith(depot_path) - - # Relative paths always match (will be prepended with depot_path) - return True - def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -433,30 +410,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. From eaac40610910a80ebebde0aba17e24a511df257f Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:54:48 +0100 Subject: [PATCH 06/45] Fix pr comments and tests --- src/sentry/integrations/perforce/client.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 2ad72b693a9602..f081498190e466 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -478,10 +478,18 @@ def get_blame_for_files( username = change.get("user", "unknown") # Perforce doesn't provide email by default, so we generate a fallback email = change.get("email") or f"{username}@perforce.local" + + # Handle potentially null/invalid time field + time_value = change.get("time") or 0 + try: + timestamp = int(time_value) + except (TypeError, ValueError): + timestamp = 0 + commit = CommitInfo( commitId=changelist, committedDate=datetime.fromtimestamp( - int(change.get("time", 0)), tz=timezone.utc + timestamp, tz=timezone.utc ), commitMessage=change.get("desc", "").strip(), commitAuthorName=username, From 54f4d1a352569b19623dae864896e39e7a807a43 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:16:38 +0100 Subject: [PATCH 07/45] Fix tests --- .../perforce/test_stacktrace_link.py | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 715c19ae50b47e..614046fdb95288 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -56,6 +56,7 @@ def tearDown(self): def test_get_stacktrace_config_python_path(self): """Test stacktrace link generation for Python SDK path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} ctx: StacktraceLinkContext = { "file": "depot/app/services/processor.py", "filename": "depot/app/services/processor.py", @@ -77,8 +78,10 @@ def test_get_stacktrace_config_python_path(self): assert result["error"] is None assert result["src_path"] == "app/services/processor.py" - def test_get_stacktrace_config_cpp_path_with_revision(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): """Test stacktrace link generation for C++ path with @revision""" + mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} ctx: StacktraceLinkContext = { "file": "depot/game/src/main.cpp@42", "filename": "depot/game/src/main.cpp@42", @@ -122,8 +125,10 @@ def test_get_stacktrace_config_no_matching_code_mapping(self): assert result["error"] == "stack_root_mismatch" assert result["src_path"] is None - def test_get_stacktrace_config_multiple_code_mappings(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_multiple_code_mappings(self, mock_check_file): """Test stacktrace link with multiple code mappings""" + mock_check_file.return_value = {"depotFile": "//myproject/app/services/handler.py"} # Add another depot mapping myproject_repo = Repository.objects.create( name="//myproject", @@ -164,8 +169,10 @@ def test_get_stacktrace_config_multiple_code_mappings(self): assert "//myproject/app/services/handler.py" in result["source_url"] assert result["src_path"] == "app/services/handler.py" - def test_get_stacktrace_config_with_web_viewer(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_with_web_viewer(self, mock_check_file): """Test stacktrace link with P4Web viewer""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} integration_with_web = self.create_integration( organization=self.organization, provider="perforce", @@ -360,6 +367,7 @@ def tearDown(self): def test_stacktrace_link_empty_stack_root(self): """Test stacktrace link with empty stack_root (shouldn't match anything)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} repo = Repository.objects.create( name="//depot", organization_id=self.organization.id, @@ -396,8 +404,10 @@ def test_stacktrace_link_empty_stack_root(self): # Empty stack_root should match any path assert result["source_url"] is not None - def test_stacktrace_link_with_special_characters_in_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_stacktrace_link_with_special_characters_in_path(self, mock_check_file): """Test stacktrace link with special characters in file path""" + mock_check_file.return_value = {"depotFile": "//depot/app/my services/processor-v2.py"} repo = Repository.objects.create( name="//depot", organization_id=self.organization.id, @@ -435,8 +445,10 @@ def test_stacktrace_link_with_special_characters_in_path(self): assert result["source_url"] is not None assert result["src_path"] == "app/my services/processor-v2.py" - def test_stacktrace_link_deeply_nested_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_stacktrace_link_deeply_nested_path(self, mock_check_file): """Test stacktrace link with very deeply nested path""" + mock_check_file.return_value = {"depotFile": "//depot/file.py"} repo = Repository.objects.create( name="//depot", organization_id=self.organization.id, From 4b42f4343aff229ce2d999ec80c7f21b7696e990 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:47:27 +0100 Subject: [PATCH 08/45] Fix PR reviews and tests --- .../integrations/perforce/test_stacktrace_link.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 614046fdb95288..8199ff82d691f2 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -230,8 +230,10 @@ def test_get_stacktrace_config_with_web_viewer(self, mock_check_file): in result["source_url"] ) - def test_get_stacktrace_config_abs_path_fallback(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_abs_path_fallback(self, mock_check_file): """Test stacktrace link uses abs_path when filename is just basename""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} ctx: StacktraceLinkContext = { "file": "processor.py", "filename": "processor.py", @@ -252,8 +254,10 @@ def test_get_stacktrace_config_abs_path_fallback(self): assert "//depot/app/services/processor.py" in result["source_url"] assert result["src_path"] == "app/services/processor.py" - def test_get_stacktrace_config_iteration_count(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_iteration_count(self, mock_check_file): """Test that iteration_count is incremented only for matching mappings""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} # Add a non-matching mapping other_repo = Repository.objects.create( name="//other", @@ -292,8 +296,10 @@ def test_get_stacktrace_config_iteration_count(self): assert result["iteration_count"] == 1 assert result["source_url"] is not None - def test_get_stacktrace_config_stops_on_first_match(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_stops_on_first_match(self, mock_check_file): """Test that iteration stops after first successful match""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} # Add another depot mapping (shouldn't be checked if first matches) # Use different project to avoid unique constraint project2 = self.create_project(organization=self.organization) From b042b645462c1c2204dee1329ca9b759b4f32b09 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:52:40 +0100 Subject: [PATCH 09/45] Fix commit info --- src/sentry/integrations/perforce/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index f081498190e466..58a6281fae0d90 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -487,7 +487,7 @@ def get_blame_for_files( timestamp = 0 commit = CommitInfo( - commitId=changelist, + commitId=str(changelist), # Ensure string type committedDate=datetime.fromtimestamp( timestamp, tz=timezone.utc ), From 8c70da06afc73da4b6f712294fa6a818c238cdeb Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 09:41:19 +0100 Subject: [PATCH 10/45] Remove P4Web (deprecated) and fix paths for swarm --- src/sentry/integrations/perforce/client.py | 2 +- .../integrations/perforce/integration.py | 19 ++--------- .../perforce/test_code_mapping.py | 34 +++++++++---------- 3 files changed, 21 insertions(+), 34 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 58a6281fae0d90..3338278d1fc206 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -455,7 +455,7 @@ def get_blame_for_files( try: # Build depot path for the file (includes stream if specified) # file.ref contains the revision/changelist if available - depot_path = self._build_depot_path(file.repo, file.path, file.ref) + depot_path = self.build_depot_path(file.repo, file.path) # Use faster p4 filelog approach to get most recent changelist # This is much faster than p4 annotate diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 60d6a74dba8068..f53b911177d05a 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -503,25 +503,12 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, - { - "name": "web_viewer_type", - "type": "choice", - "label": "Web Viewer Type", - "choices": [ - ["p4web", "P4Web"], - ["swarm", "Helix Swarm"], - ["other", "Other"], - ], - "help": "Type of web viewer (if web URL is provided)", - "required": False, - "default": "p4web", - }, { "name": "web_url", "type": "string", - "label": "Web Viewer URL (Optional)", - "placeholder": "https://p4web.company.com", - "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", + "label": "Helix Swarm URL (Optional)", + "placeholder": "https://swarm.company.com", + "help": "Optional: URL to Helix Swarm web viewer for browsing files", "required": False, }, ] diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 621d2138209494..03b0b772a07131 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -391,9 +391,9 @@ def test_symbolic_cpp_path_full_flow(self): url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) assert url == "p4://depot/game/src/main.cpp@42" - def test_full_flow_with_web_viewer(self): - """Test full flow with P4Web viewer configuration""" - integration_with_web = self.create_integration( + def test_full_flow_with_swarm_viewer(self): + """Test full flow with Swarm viewer configuration""" + integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", @@ -406,28 +406,28 @@ def test_full_flow_with_web_viewer(self): "web_url": "https://p4web.example.com", }, ) - installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] # Create repo with web viewer integration - repo_web = Repository.objects.create( + repo_swarm = Repository.objects.create( name="//depot", organization_id=self.organization.id, - integration_id=integration_with_web.id, + integration_id=integration_with_swarm.id, config={"depot_path": "//depot"}, ) # Use a new project to avoid unique constraint on (project_id, stack_root) - project_web = self.create_project(organization=self.organization) + project_swarm = self.create_project(organization=self.organization) - org_integration_web = integration_with_web.organizationintegration_set.first() - assert org_integration_web is not None + org_integration_swarm = integration_with_swarm.organizationintegration_set.first() + assert org_integration_swarm is not None - code_mapping_web = RepositoryProjectPathConfig.objects.create( - project=project_web, + code_mapping_swarm = RepositoryProjectPathConfig.objects.create( + project=project_swarm, organization_id=self.organization.id, - repository=repo_web, - organization_integration_id=org_integration_web.id, - integration_id=integration_with_web.id, + repository=repo_swarm, + organization_integration_id=org_integration_swarm.id, + integration_id=integration_with_swarm.id, stack_root="depot/", source_root="/", default_branch=None, @@ -442,14 +442,14 @@ def test_full_flow_with_web_viewer(self): # Code mapping mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, - code_mapping=code_mapping_web, + code_mapping=code_mapping_swarm, platform="python", sdk_name="sentry.python", ) - # format_source_url with web viewer (revision extracted from filename) + # format_source_url with Swarm viewer (revision extracted from filename) assert mapped_path is not None - url = installation.format_source_url(repo=repo_web, filepath=mapped_path, branch=None) + url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) # Swarm format: /files/?v= assert url == "https://p4web.example.com/files//depot/app/services/processor.py?v=42" From 860917b8706671ecd6533ad3e616ec312d9480e7 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:19:48 +0100 Subject: [PATCH 11/45] Implement comments on the SSL/P4Port --- .../integrations/perforce/integration.py | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index f53b911177d05a..fe3904f4cf759b 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -440,23 +440,20 @@ def get_organization_config(self) -> list[dict[str, Any]]: "depends_on": {"auth_mode": "ticket"}, }, { - "name": "host", + "name": "p4port", "type": "string", - "label": "Perforce Server Host", - "placeholder": "perforce.company.com", - "help": "The hostname or IP address of your Perforce server", + "label": "P4PORT (Server Address)", + "placeholder": "ssl:perforce.company.com:1666", + "help": "Perforce server address in P4PORT format. Required even with tickets for multi-server environments. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended.", "required": False, - "depends_on": {"auth_mode": "password"}, }, { - "name": "port", - "type": "number", - "label": "Perforce Server Port", - "placeholder": "1666", - "help": "The port number for your Perforce server (default: 1666)", + "name": "ssl_fingerprint", + "type": "string", + "label": "SSL Fingerprint (Required for SSL)", + "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", + "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "default": "1666", - "depends_on": {"auth_mode": "password"}, }, { "name": "user", From 0300c1d18261aeead1f5af81caf9b8a4a23698f2 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:39:59 +0100 Subject: [PATCH 12/45] Fix PR comments --- .../integrations/perforce/integration.py | 52 ++----------------- 1 file changed, 5 insertions(+), 47 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index fe3904f4cf759b..9c153e0c0732dc 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -418,69 +418,28 @@ def get_organization_config(self) -> list[dict[str, Any]]: Current values are provided separately via get_config_data(). """ return [ - { - "name": "auth_mode", - "type": "choice", - "label": "Authentication Mode", - "choices": [ - ["password", "Username & Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", - "required": True, - "default": "password", - }, - { - "name": "ticket", - "type": "secret", - "label": "P4 Ticket", - "placeholder": "••••••••••••••••••••••••••••••••", - "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", - "required": False, - "depends_on": {"auth_mode": "ticket"}, - }, { "name": "p4port", "type": "string", "label": "P4PORT (Server Address)", "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Required even with tickets for multi-server environments. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended.", - "required": False, - }, - { - "name": "ssl_fingerprint", - "type": "string", - "label": "SSL Fingerprint (Required for SSL)", - "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", - "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", - "required": False, + "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "required": True, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce", - "required": False, - "depends_on": {"auth_mode": "password"}, - }, - { - "name": "auth_type", - "type": "choice", - "label": "Authentication Type", - "choices": [ - ["password", "Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Select whether you're providing a password or a P4 ticket. Tickets are obtained via 'p4 login -p' and don't require re-authentication.", + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", "required": True, }, { "name": "password", "type": "secret", - "label": "Password / Ticket", + "label": "Password or P4 Ticket", "placeholder": "••••••••", - "help": "Your Perforce password or P4 authentication ticket (depending on the authentication type selected above).", + "help": "Perforce password OR P4 authentication ticket. Tickets are obtained via 'p4 login -p' and are more secure than passwords. Both are supported in this field.", "required": True, }, { @@ -490,7 +449,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", From 25643135735874693be084245fafa4a556c7c6ce Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:05:55 +0100 Subject: [PATCH 13/45] Parse file revision properly --- .../perforce/test_code_mapping.py | 20 +++++++++---------- .../perforce/test_stacktrace_link.py | 12 +++++------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 03b0b772a07131..6e35f5a3457dc8 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -74,8 +74,8 @@ def test_code_mapping_depot_root_to_slash(self): def test_code_mapping_with_symbolic_revision_syntax(self): """ - Test code mapping with Symbolic's @revision syntax. - The @revision should be preserved in the output. + Test code mapping with Symbolic's #revision syntax. + The #revision should be preserved in the output. """ repo = Repository.objects.create( name="//depot", @@ -95,17 +95,17 @@ def test_code_mapping_with_symbolic_revision_syntax(self): default_branch=None, ) - # Test C++ path from Symbolic: depot/game/src/main.cpp@42 + # Test C++ path from Symbolic: depot/game/src/main.cpp#1 frame = EventFrame( - filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" + filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" ) result = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" ) - # Should strip "depot/" and preserve "@42" - assert result == "game/src/main.cpp@42" + # Should strip "depot/" and preserve "#1" + assert result == "game/src/main.cpp#1" def test_code_mapping_multiple_depots(self): """Test code mappings for multiple depots (depot and myproject)""" @@ -378,18 +378,18 @@ def test_symbolic_cpp_path_full_flow(self): """Test full flow: Symbolic C++ -> code mapping -> format_source_url""" # 1. Symbolic transformer sends this path frame = EventFrame( - filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" + filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" ) # 2. Code mapping transforms it (use existing code_mapping from setUp) mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=self.code_mapping, platform="native", sdk_name="sentry.native" ) - assert mapped_path == "game/src/main.cpp@42" + assert mapped_path == "game/src/main.cpp#1" - # 3. format_source_url creates final URL (preserves @42) + # 3. format_source_url creates final URL (preserves #1) url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/game/src/main.cpp@42" + assert url == "p4://depot/game/src/main.cpp#1" def test_full_flow_with_swarm_viewer(self): """Test full flow with Swarm viewer configuration""" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 8199ff82d691f2..8dfcfc97f21e7c 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -80,12 +80,12 @@ def test_get_stacktrace_config_python_path(self): @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): - """Test stacktrace link generation for C++ path with @revision""" + """Test stacktrace link generation for C++ path with #revision""" mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} ctx: StacktraceLinkContext = { - "file": "depot/game/src/main.cpp@42", - "filename": "depot/game/src/main.cpp@42", - "abs_path": "depot/game/src/main.cpp@42", + "file": "depot/game/src/main.cpp#1", + "filename": "depot/game/src/main.cpp#1", + "abs_path": "depot/game/src/main.cpp#1", "platform": "native", "sdk_name": "sentry.native", "commit_id": None, @@ -99,10 +99,10 @@ def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): assert result["source_url"] is not None assert isinstance(result["source_url"], str) - # URL will be encoded: p4://depot/game/src/main.cpp%4042 + # URL will be encoded: p4://depot/game/src/main.cpp%231 assert "depot/game/src/main.cpp" in result["source_url"] assert result["error"] is None - assert result["src_path"] == "game/src/main.cpp@42" + assert result["src_path"] == "game/src/main.cpp#1" def test_get_stacktrace_config_no_matching_code_mapping(self): """Test stacktrace link when no code mapping matches""" From 655269df74e3700d90219109181a58714fa34e8a Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:04:12 +0100 Subject: [PATCH 14/45] Fix PR Comments --- src/sentry/integrations/perforce/client.py | 85 +++++++++++++++------- 1 file changed, 58 insertions(+), 27 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 3338278d1fc206..34c72d564fcebb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -358,6 +358,35 @@ def get_user(self, username: str) -> P4UserInfo | None: # User not found - return None (not an error condition) return None + def get_user(self, username: str) -> dict[str, Any] | None: + """ + Get user information from Perforce. + + Uses p4 user command to fetch user details including email and full name. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_user.html + + Args: + username: Perforce username + + Returns: + User info dictionary with Email and FullName fields, or None if not found + """ + p4 = self._connect() + try: + result = p4.run("user", "-o", username) + if result and len(result) > 0: + user_info = result[0] + return { + "email": user_info.get("Email", ""), + "full_name": user_info.get("FullName", ""), + "username": user_info.get("User", username), + } + return None + except self.P4Exception: + return None + finally: + self._disconnect(p4) + def get_changes( self, depot_path: str, @@ -444,12 +473,19 @@ def get_blame_for_files( Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. + API docs: + - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html + - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html + Returns a list of FileBlameInfo objects containing commit details for each file. """ metrics.incr("integrations.perforce.get_blame_for_files") blames = [] p4 = self._connect() + # Cache user info to avoid multiple lookups for the same user + user_cache: dict[str, dict[str, Any] | None] = {} + try: for file in files: try: @@ -476,8 +512,23 @@ def get_blame_for_files( if change_info and len(change_info) > 0: change = change_info[0] username = change.get("user", "unknown") - # Perforce doesn't provide email by default, so we generate a fallback - email = change.get("email") or f"{username}@perforce.local" + + # Get user information from Perforce for email and full name + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username != "unknown" and username not in user_cache: + user_cache[username] = self.get_user(username) + + user_info = user_cache.get(username) + if user_info: + # Use actual email from Perforce if available + if user_info.get("email"): + author_email = user_info["email"] + # Use full name from Perforce if available + if user_info.get("full_name"): + author_name = user_info["full_name"] # Handle potentially null/invalid time field time_value = change.get("time") or 0 @@ -492,8 +543,8 @@ def get_blame_for_files( timestamp, tz=timezone.utc ), commitMessage=change.get("desc", "").strip(), - commitAuthorName=username, - commitAuthorEmail=email, + commitAuthorName=author_name, + commitAuthorEmail=author_email, ) blame_info = FileBlameInfo( @@ -505,29 +556,9 @@ def get_blame_for_files( commit=commit, ) blames.append(blame_info) - except self.P4Exception as e: - logger.warning( - "perforce.client.get_blame_for_files.describe_error", - extra={ - **extra, - "changelist": changelist, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - }, - ) - except self.P4Exception as e: - # Log but don't fail for individual file errors - logger.warning( - "perforce.client.get_blame_for_files.annotate_error", - extra={ - **extra, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - "file_lineno": file.lineno, - }, - ) + except self.P4Exception: + pass + except self.P4Exception: continue return blames From 46d9f3b7296ba84b5d0b90a5b3b4d495c7848a51 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:49:13 +0100 Subject: [PATCH 15/45] Simplify for installation only --- src/sentry/integrations/perforce/client.py | 94 +------------------ .../integrations/perforce/repository.py | 6 +- 2 files changed, 2 insertions(+), 98 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 34c72d564fcebb..79c3a0e9b2a90f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,7 +12,6 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, - CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -397,9 +396,6 @@ def get_changes( """ Get changelists for a depot path. - Uses p4 changes command to list changelists. - API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html - Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return when start_cl/end_cl not specified @@ -473,97 +469,9 @@ def get_blame_for_files( Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. - API docs: - - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html - - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html - Returns a list of FileBlameInfo objects containing commit details for each file. """ - metrics.incr("integrations.perforce.get_blame_for_files") - blames = [] - p4 = self._connect() - - # Cache user info to avoid multiple lookups for the same user - user_cache: dict[str, dict[str, Any] | None] = {} - - try: - for file in files: - try: - # Build depot path for the file (includes stream if specified) - # file.ref contains the revision/changelist if available - depot_path = self.build_depot_path(file.repo, file.path) - - # Use faster p4 filelog approach to get most recent changelist - # This is much faster than p4 annotate - filelog = p4.run("filelog", "-m1", depot_path) - - changelist = None - if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list) - changelists = filelog[0].get("change", []) - if changelists and len(changelists) > 0: - # Get the first (most recent) changelist number - changelist = changelists[0] - - # If we found a changelist, get detailed commit info - if changelist: - try: - change_info = p4.run("describe", "-s", changelist) - if change_info and len(change_info) > 0: - change = change_info[0] - username = change.get("user", "unknown") - - # Get user information from Perforce for email and full name - author_email = f"{username}@perforce" - author_name = username - - # Fetch user info if not in cache - if username != "unknown" and username not in user_cache: - user_cache[username] = self.get_user(username) - - user_info = user_cache.get(username) - if user_info: - # Use actual email from Perforce if available - if user_info.get("email"): - author_email = user_info["email"] - # Use full name from Perforce if available - if user_info.get("full_name"): - author_name = user_info["full_name"] - - # Handle potentially null/invalid time field - time_value = change.get("time") or 0 - try: - timestamp = int(time_value) - except (TypeError, ValueError): - timestamp = 0 - - commit = CommitInfo( - commitId=str(changelist), # Ensure string type - committedDate=datetime.fromtimestamp( - timestamp, tz=timezone.utc - ), - commitMessage=change.get("desc", "").strip(), - commitAuthorName=author_name, - commitAuthorEmail=author_email, - ) - - blame_info = FileBlameInfo( - lineno=file.lineno, - path=file.path, - ref=file.ref, - repo=file.repo, - code_mapping=file.code_mapping, - commit=commit, - ) - blames.append(blame_info) - except self.P4Exception: - pass - except self.P4Exception: - continue - - return blames - finally: - self._disconnect(p4) + return [] def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index f2235b4649f8b4..bf65237ac490a0 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -19,7 +19,6 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider from sentry.plugins.providers.integration_repository import RepositoryConfig -from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -114,8 +113,6 @@ def build_repository_config( Returns: Repository configuration """ - depot_path = data["identifier"] - return { "name": depot_path, "external_id": data["external_id"], @@ -215,7 +212,6 @@ def _format_commits( Args: changelists: List of changelist dictionaries from P4 depot_path: Depot path - client: Perforce client instance Returns: List of commits in Sentry format @@ -307,7 +303,7 @@ def compare_commits( def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str | None: """ Get URL for pull request. - Perforce doesn't have native pull requests. + Perforce doesn't have native PRs, but might integrate with Swarm. """ return None From 36dda1a9e36ba91f8a7126d9c7e64d7887b3c5e9 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 14:34:37 +0100 Subject: [PATCH 16/45] Rework based on the PR comments --- src/sentry/integrations/perforce/client.py | 29 ------------------- .../integrations/perforce/integration.py | 24 ++++++++++++--- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 79c3a0e9b2a90f..5cf51d936f6cdb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -357,35 +357,6 @@ def get_user(self, username: str) -> P4UserInfo | None: # User not found - return None (not an error condition) return None - def get_user(self, username: str) -> dict[str, Any] | None: - """ - Get user information from Perforce. - - Uses p4 user command to fetch user details including email and full name. - API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_user.html - - Args: - username: Perforce username - - Returns: - User info dictionary with Email and FullName fields, or None if not found - """ - p4 = self._connect() - try: - result = p4.run("user", "-o", username) - if result and len(result) > 0: - user_info = result[0] - return { - "email": user_info.get("Email", ""), - "full_name": user_info.get("FullName", ""), - "username": user_info.get("User", username), - } - return None - except self.P4Exception: - return None - finally: - self._disconnect(p4) - def get_changes( self, depot_path: str, diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 9c153e0c0732dc..67ee83d2b3ae63 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -204,10 +204,9 @@ def source_url_matches(self, url: str) -> bool: if url.startswith("p4://"): return True - if self.org_integration: - web_url = self.org_integration.config.get("web_url") - if web_url and url.startswith(web_url): - return True + web_url = self.model.metadata.get("web_url") + if web_url and url.startswith(web_url): + return True return False @@ -582,6 +581,23 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: "password": installation_data.get("password", ""), } + # Add optional fields if provided + if installation_data.get("client"): + metadata["client"] = installation_data["client"] + + if installation_data.get("ssl_fingerprint"): + metadata["ssl_fingerprint"] = installation_data["ssl_fingerprint"] + + if installation_data.get("web_url"): + metadata["web_url"] = installation_data["web_url"] + + # Store credentials in Integration.metadata + metadata = { + "p4port": p4port, + "user": installation_data.get("user", ""), + "password": installation_data.get("password", ""), + } + # Add optional fields if provided if installation_data.get("client"): metadata["client"] = installation_data["client"] From 936db40898fadb8360011aae7c2e8fc3ac37eb4b Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:15:28 +0100 Subject: [PATCH 17/45] More cursor comment fixes --- src/sentry/integrations/perforce/integration.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 67ee83d2b3ae63..19f39781e5d0cc 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -355,7 +355,7 @@ def get_repositories( page_number_limit: Ignored (kept for base class compatibility) Returns: - List of repository dictionaries + List of repository dictionaries (limited by page_number_limit if provided) """ try: client = self.get_client() @@ -378,6 +378,10 @@ def get_repositories( } ) + # Apply pagination limit if specified + if page_number_limit and len(repositories) >= page_number_limit: + break + return repositories except ApiError: From c231f85b3efebbcf60e3376553baf05bed5f13ed Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:34:20 +0100 Subject: [PATCH 18/45] Fix ticket authentication --- src/sentry/integrations/perforce/integration.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 19f39781e5d0cc..6699130ff46825 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -437,12 +437,23 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", "required": True, }, + { + "name": "auth_type", + "type": "choice", + "label": "Authentication Type", + "choices": [ + ["password", "Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Select whether you're providing a password or a P4 ticket. Tickets are obtained via 'p4 login -p' and don't require re-authentication.", + "required": True, + }, { "name": "password", "type": "secret", - "label": "Password or P4 Ticket", + "label": "Password / Ticket", "placeholder": "••••••••", - "help": "Perforce password OR P4 authentication ticket. Tickets are obtained via 'p4 login -p' and are more secure than passwords. Both are supported in this field.", + "help": "Your Perforce password or P4 authentication ticket (depending on the authentication type selected above).", "required": True, }, { @@ -599,6 +610,7 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: metadata = { "p4port": p4port, "user": installation_data.get("user", ""), + "auth_type": installation_data.get("auth_type", "password"), # Default to password "password": installation_data.get("password", ""), } From d7438bfe859b86b041618b363d632189f3e8b9fe Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:47:37 +0100 Subject: [PATCH 19/45] Fix trust order of operations --- src/sentry/integrations/perforce/client.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5cf51d936f6cdb..625fad480ceebb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -148,9 +148,12 @@ def _connect(self) -> Generator[P4]: if self.client_name: p4.client = self.client_name - p4.exception_level = 1 # Only errors raise exceptions + # Lower exception level to allow connection with SSL trust warnings + # After connection, we'll assert the fingerprint with run_trust + p4.exception_level = 0 # Warnings don't raise exceptions # Connect to Perforce server + # For SSL connections, this may succeed with warnings about trust try: p4.connect() except P4Exception as e: @@ -178,6 +181,9 @@ def _connect(self) -> Generator[P4]: f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) + # Restore normal exception level for subsequent commands + p4.exception_level = 1 # Only errors raise exceptions + # Authenticate based on auth_type # - password: Requires run_login() to exchange password for session ticket # - ticket: Already authenticated via p4.password, no login needed From 6a372edafb24b3810df3e68e8b44637f54daebc5 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:57:39 +0100 Subject: [PATCH 20/45] Fix auth type issues --- src/sentry/integrations/perforce/client.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 625fad480ceebb..5cf51d936f6cdb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -148,12 +148,9 @@ def _connect(self) -> Generator[P4]: if self.client_name: p4.client = self.client_name - # Lower exception level to allow connection with SSL trust warnings - # After connection, we'll assert the fingerprint with run_trust - p4.exception_level = 0 # Warnings don't raise exceptions + p4.exception_level = 1 # Only errors raise exceptions # Connect to Perforce server - # For SSL connections, this may succeed with warnings about trust try: p4.connect() except P4Exception as e: @@ -181,9 +178,6 @@ def _connect(self) -> Generator[P4]: f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) - # Restore normal exception level for subsequent commands - p4.exception_level = 1 # Only errors raise exceptions - # Authenticate based on auth_type # - password: Requires run_login() to exchange password for session ticket # - ticket: Already authenticated via p4.password, no login needed From 979d75d2e3d400380b41d2ec12208b1afe7e6f08 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:21:35 +0100 Subject: [PATCH 21/45] Restore deleted logo --- src/sentry/static/sentry/images/logos/logo-perforce.svg | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 src/sentry/static/sentry/images/logos/logo-perforce.svg diff --git a/src/sentry/static/sentry/images/logos/logo-perforce.svg b/src/sentry/static/sentry/images/logos/logo-perforce.svg new file mode 100644 index 00000000000000..eb8c0c234101f5 --- /dev/null +++ b/src/sentry/static/sentry/images/logos/logo-perforce.svg @@ -0,0 +1,5 @@ + + + + + From b8659e3843faabbde0a368a236976fa4acd811a2 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:35:59 +0100 Subject: [PATCH 22/45] Fix stale config after update --- src/sentry/integrations/perforce/integration.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 6699130ff46825..8b1b6354badf03 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -510,6 +510,7 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: data: Updated configuration data from the form (only changed fields) """ from sentry.integrations.services.integration import integration_service + from sentry.models.integrations.integration import Integration # Update integration metadata with new values metadata = dict(self.model.metadata) # Create a mutable copy From d034c9955fbda362ead100019b04f394c7f923db Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:48:06 +0100 Subject: [PATCH 23/45] Fix cursor comment --- src/sentry/integrations/perforce/integration.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 8b1b6354badf03..6699130ff46825 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -510,7 +510,6 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: data: Updated configuration data from the form (only changed fields) """ from sentry.integrations.services.integration import integration_service - from sentry.models.integrations.integration import Integration # Update integration metadata with new values metadata = dict(self.model.metadata) # Create a mutable copy From 990396a5042d9ed9ed41017a3166d153c3ba5c33 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 1 Dec 2025 10:08:10 +0100 Subject: [PATCH 24/45] Review comments fixes --- .../integrations/perforce/integration.py | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 6699130ff46825..bf034e292574bf 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -355,7 +355,7 @@ def get_repositories( page_number_limit: Ignored (kept for base class compatibility) Returns: - List of repository dictionaries (limited by page_number_limit if provided) + List of repository dictionaries """ try: client = self.get_client() @@ -378,10 +378,6 @@ def get_repositories( } ) - # Apply pagination limit if specified - if page_number_limit and len(repositories) >= page_number_limit: - break - return repositories except ApiError: @@ -596,24 +592,6 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: "password": installation_data.get("password", ""), } - # Add optional fields if provided - if installation_data.get("client"): - metadata["client"] = installation_data["client"] - - if installation_data.get("ssl_fingerprint"): - metadata["ssl_fingerprint"] = installation_data["ssl_fingerprint"] - - if installation_data.get("web_url"): - metadata["web_url"] = installation_data["web_url"] - - # Store credentials in Integration.metadata - metadata = { - "p4port": p4port, - "user": installation_data.get("user", ""), - "auth_type": installation_data.get("auth_type", "password"), # Default to password - "password": installation_data.get("password", ""), - } - # Add optional fields if provided if installation_data.get("client"): metadata["client"] = installation_data["client"] From f3a0a7545cfce55009392a866af4c160e680ead7 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 25/45] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 2 +- src/sentry/integrations/perforce/integration.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5cf51d936f6cdb..dae358ddc19baa 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -442,7 +442,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + return None def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..df3ca53501acd0 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -3,7 +3,11 @@ import hashlib import logging from collections.abc import Mapping, Sequence +<<<<<<< HEAD from typing import Any, TypedDict +======= +from typing import Any +>>>>>>> 7432be05bba (Simplify integration and enable feature-flag support) from django import forms from django.http import HttpRequest, HttpResponseBase From 8889198f791c4242fbe898ef82426c1296f15c19 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 26/45] Fix typing --- src/sentry/integrations/perforce/client.py | 11 ++++++++++- src/sentry/integrations/perforce/integration.py | 4 ---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index dae358ddc19baa..22508b9a1d6c8c 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -442,7 +442,16 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return None + return [] + + def get_file( + self, repo: Repository, path: str, ref: str | None, codeowners: bool = False + ) -> str: + """ + Get file contents from Perforce depot. + Required by abstract base class but not used (CODEOWNERS feature removed). + """ + raise NotImplementedError("get_file is not supported for Perforce") def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index df3ca53501acd0..bf034e292574bf 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -3,11 +3,7 @@ import hashlib import logging from collections.abc import Mapping, Sequence -<<<<<<< HEAD from typing import Any, TypedDict -======= -from typing import Any ->>>>>>> 7432be05bba (Simplify integration and enable feature-flag support) from django import forms from django.http import HttpRequest, HttpResponseBase From 4aa759b13efcb31f562258daab162e8322c74249 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 27/45] feat(perforce): Add backend support for Perforce integration This commit adds backend support for Perforce version control integration: - New Perforce integration with P4 client support - Repository and code mapping functionality - Stacktrace linking for Perforce depot paths - Tests for integration, code mapping, and stacktrace linking - Updated dependencies in pyproject.toml The integration supports: - Authentication via P4PORT, P4USER, P4PASSWD - Code mapping between depot paths and project structure - Source URL generation for stacktrace frames - Integration with Sentry's repository and code mapping systems --- src/sentry/integrations/perforce/client.py | 88 ++++++++++++-- .../integrations/perforce/integration.py | 115 ++++++++++++++++-- .../integrations/perforce/repository.py | 3 + 3 files changed, 187 insertions(+), 19 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 22508b9a1d6c8c..641125fc46f6d0 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,6 +12,7 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -442,16 +443,85 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + metrics.incr("integrations.perforce.get_blame_for_files") + blames = [] + p4 = self._connect() - def get_file( - self, repo: Repository, path: str, ref: str | None, codeowners: bool = False - ) -> str: - """ - Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). - """ - raise NotImplementedError("get_file is not supported for Perforce") + try: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the revision/changelist if available + depot_path = self._build_depot_path(file.repo, file.path, file.ref) + + # Use faster p4 filelog approach to get most recent changelist + # This is much faster than p4 annotate + filelog = p4.run("filelog", "-m1", depot_path) + + changelist = None + if filelog and len(filelog) > 0: + # The 'change' field contains the changelist numbers (as a list of strings) + changelists = filelog[0].get("change", []) + if changelists and len(changelists) > 0: + # Get the first (most recent) changelist number + changelist = changelists[0] + + # If we found a changelist, get detailed commit info + if changelist: + try: + change_info = p4.run("describe", "-s", changelist) + if change_info and len(change_info) > 0: + change = change_info[0] + username = change.get("user", "unknown") + # Perforce doesn't provide email by default, so we generate a fallback + email = change.get("email") or f"{username}@perforce.local" + commit = CommitInfo( + commitId=changelist, + committedDate=datetime.fromtimestamp( + int(change.get("time", 0)), tz=timezone.utc + ), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=username, + commitAuthorEmail=email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except self.P4Exception as e: + logger.warning( + "perforce.client.get_blame_for_files.describe_error", + extra={ + **extra, + "changelist": changelist, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + except self.P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.annotate_error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames + finally: + self._disconnect(p4) def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..1ed67e91ba6bc8 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,6 +210,30 @@ def source_url_matches(self, url: str) -> bool: return False + def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: + """ + Check if a file path matches this repository's depot path. + + When SRCSRV transformers remap paths to absolute depot paths (e.g., + //depot/project/src/file.cpp), this method verifies that the depot path + matches the repository's configured depot_path. + + Args: + repo: Repository object + filepath: File path (may be absolute depot path or relative path) + + Returns: + True if the filepath matches this repository's depot + """ + depot_path = repo.config.get("depot_path", repo.name) + + # If filepath is absolute depot path, check if it starts with depot_path + if filepath.startswith("//"): + return filepath.startswith(depot_path) + + # Relative paths always match (will be prepended with depot_path) + return True + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -409,6 +433,30 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] + def test_connection(self) -> dict[str, Any]: + """ + Test the Perforce connection with current credentials. + + Returns: + Dictionary with connection status and server info + """ + try: + client = self.get_client() + info = client.get_depot_info() + + return { + "status": "success", + "message": f"Connected to Perforce server at {info.get('server_address')}", + "server_info": info, + } + except Exception as e: + logger.exception("perforce.test_connection.failed") + return { + "status": "error", + "message": f"Failed to connect to Perforce server: {str(e)}", + "error": str(e), + } + def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -418,20 +466,53 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "p4port", - "type": "string", - "label": "P4PORT (Server Address)", - "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "type": "string", + "label": "Perforce Server Host", + "placeholder": "perforce.company.com", + "help": "The hostname or IP address of your Perforce server", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "port", + "type": "number", + "label": "Perforce Server Port", + "placeholder": "1666", + "help": "The port number for your Perforce server (default: 1666)", + "required": False, + "default": "1666", + "depends_on": {"auth_mode": "password"}, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", - "required": True, + "help": "Username for authenticating with Perforce", + "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "auth_type", @@ -459,6 +540,7 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -468,12 +550,25 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, + { + "name": "web_viewer_type", + "type": "choice", + "label": "Web Viewer Type", + "choices": [ + ["p4web", "P4Web"], + ["swarm", "Helix Swarm"], + ["other", "Other"], + ], + "help": "Type of web viewer (if web URL is provided)", + "required": False, + "default": "p4web", + }, { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", - "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "label": "Web Viewer URL (Optional)", + "placeholder": "https://p4web.company.com", + "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", "required": False, }, ] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index bf65237ac490a0..3cf937d4c28f66 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -19,6 +19,7 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider from sentry.plugins.providers.integration_repository import RepositoryConfig +from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -113,6 +114,8 @@ def build_repository_config( Returns: Repository configuration """ + depot_path = data["identifier"] + return { "name": depot_path, "external_id": data["external_id"], From 374a83d46128e08a0189ebd39528e53b25dbd62a Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:52:01 +0100 Subject: [PATCH 28/45] feat(perforce): Implement repository/depot and code mapping logic --- src/sentry/integrations/perforce/client.py | 3 +++ src/sentry/integrations/perforce/repository.py | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 641125fc46f6d0..8bebf5e67c50f4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -368,6 +368,9 @@ def get_changes( """ Get changelists for a depot path. + Uses p4 changes command to list changelists. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html + Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return when start_cl/end_cl not specified diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 3cf937d4c28f66..f2235b4649f8b4 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -215,6 +215,7 @@ def _format_commits( Args: changelists: List of changelist dictionaries from P4 depot_path: Depot path + client: Perforce client instance Returns: List of commits in Sentry format @@ -306,7 +307,7 @@ def compare_commits( def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str | None: """ Get URL for pull request. - Perforce doesn't have native PRs, but might integrate with Swarm. + Perforce doesn't have native pull requests. """ return None From 2b6ca4c971a7c598b13342a594e3fdda4652b1d1 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 11:37:56 +0100 Subject: [PATCH 29/45] Fixes after rebase --- src/sentry/integrations/perforce/client.py | 81 +------------------ .../integrations/perforce/integration.py | 24 ------ .../integrations/perforce/test_integration.py | 6 +- 3 files changed, 6 insertions(+), 105 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8bebf5e67c50f4..f4961ed2aaa935 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,7 +12,6 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, - CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -446,85 +445,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - metrics.incr("integrations.perforce.get_blame_for_files") - blames = [] - p4 = self._connect() - - try: - for file in files: - try: - # Build depot path for the file (includes stream if specified) - # file.ref contains the revision/changelist if available - depot_path = self._build_depot_path(file.repo, file.path, file.ref) - - # Use faster p4 filelog approach to get most recent changelist - # This is much faster than p4 annotate - filelog = p4.run("filelog", "-m1", depot_path) - - changelist = None - if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list of strings) - changelists = filelog[0].get("change", []) - if changelists and len(changelists) > 0: - # Get the first (most recent) changelist number - changelist = changelists[0] - - # If we found a changelist, get detailed commit info - if changelist: - try: - change_info = p4.run("describe", "-s", changelist) - if change_info and len(change_info) > 0: - change = change_info[0] - username = change.get("user", "unknown") - # Perforce doesn't provide email by default, so we generate a fallback - email = change.get("email") or f"{username}@perforce.local" - commit = CommitInfo( - commitId=changelist, - committedDate=datetime.fromtimestamp( - int(change.get("time", 0)), tz=timezone.utc - ), - commitMessage=change.get("desc", "").strip(), - commitAuthorName=username, - commitAuthorEmail=email, - ) - - blame_info = FileBlameInfo( - lineno=file.lineno, - path=file.path, - ref=file.ref, - repo=file.repo, - code_mapping=file.code_mapping, - commit=commit, - ) - blames.append(blame_info) - except self.P4Exception as e: - logger.warning( - "perforce.client.get_blame_for_files.describe_error", - extra={ - **extra, - "changelist": changelist, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - }, - ) - except self.P4Exception as e: - # Log but don't fail for individual file errors - logger.warning( - "perforce.client.get_blame_for_files.annotate_error", - extra={ - **extra, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - "file_lineno": file.lineno, - }, - ) - continue - - return blames - finally: - self._disconnect(p4) + return [] def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 1ed67e91ba6bc8..6994a56ce5f4e2 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -433,30 +433,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 69f1f08dd4ee3c..055180260eba6c 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -504,12 +504,16 @@ def test_integration_lifecycle(self): assert len(org_config) == 7 # 7 configuration fields field_names = {field["name"] for field in org_config} assert field_names == { - "p4port", + "auth_mode", + "ticket", + "host", + "port", "user", "auth_type", "password", "ssl_fingerprint", "client", + "web_viewer_type", "web_url", } From 53bcbc3e3c17d4c59c8688753b7daf87cee8b4be Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 12:38:25 +0100 Subject: [PATCH 30/45] Cleanup the implementation after rebase --- .../integrations/perforce/integration.py | 91 ++----------------- .../integrations/perforce/test_integration.py | 6 +- 2 files changed, 11 insertions(+), 86 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 6994a56ce5f4e2..bf034e292574bf 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,30 +210,6 @@ def source_url_matches(self, url: str) -> bool: return False - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. - - When SRCSRV transformers remap paths to absolute depot paths (e.g., - //depot/project/src/file.cpp), this method verifies that the depot path - matches the repository's configured depot_path. - - Args: - repo: Repository object - filepath: File path (may be absolute depot path or relative path) - - Returns: - True if the filepath matches this repository's depot - """ - depot_path = repo.config.get("depot_path", repo.name) - - # If filepath is absolute depot path, check if it starts with depot_path - if filepath.startswith("//"): - return filepath.startswith(depot_path) - - # Relative paths always match (will be prepended with depot_path) - return True - def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -442,53 +418,20 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "auth_mode", - "type": "choice", - "label": "Authentication Mode", - "choices": [ - ["password", "Username & Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", - "required": True, - "default": "password", - }, - { - "name": "ticket", - "type": "secret", - "label": "P4 Ticket", - "placeholder": "••••••••••••••••••••••••••••••••", - "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", - "required": False, - "depends_on": {"auth_mode": "ticket"}, - }, - { - "name": "host", + "name": "p4port", "type": "string", - "label": "Perforce Server Host", - "placeholder": "perforce.company.com", - "help": "The hostname or IP address of your Perforce server", - "required": False, - "depends_on": {"auth_mode": "password"}, - }, - { - "name": "port", - "type": "number", - "label": "Perforce Server Port", - "placeholder": "1666", - "help": "The port number for your Perforce server (default: 1666)", - "required": False, - "default": "1666", - "depends_on": {"auth_mode": "password"}, + "label": "P4PORT (Server Address)", + "placeholder": "ssl:perforce.company.com:1666", + "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "required": True, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce", - "required": False, - "depends_on": {"auth_mode": "password"}, + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", + "required": True, }, { "name": "auth_type", @@ -516,7 +459,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -526,25 +468,12 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, - { - "name": "web_viewer_type", - "type": "choice", - "label": "Web Viewer Type", - "choices": [ - ["p4web", "P4Web"], - ["swarm", "Helix Swarm"], - ["other", "Other"], - ], - "help": "Type of web viewer (if web URL is provided)", - "required": False, - "default": "p4web", - }, { "name": "web_url", "type": "string", - "label": "Web Viewer URL (Optional)", - "placeholder": "https://p4web.company.com", - "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", + "label": "Helix Swarm URL (Optional)", + "placeholder": "https://swarm.company.com", + "help": "Optional: URL to Helix Swarm web viewer for browsing files", "required": False, }, ] diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 055180260eba6c..69f1f08dd4ee3c 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -504,16 +504,12 @@ def test_integration_lifecycle(self): assert len(org_config) == 7 # 7 configuration fields field_names = {field["name"] for field in org_config} assert field_names == { - "auth_mode", - "ticket", - "host", - "port", + "p4port", "user", "auth_type", "password", "ssl_fingerprint", "client", - "web_viewer_type", "web_url", } From 7aa1b4a578115762e26e25346c694587742b9217 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 31/45] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index f4961ed2aaa935..6ba69133dfb893 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -445,7 +445,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + return None def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False From 5ea3f3e77ed63b05bfc7d71251be7d0718c2b932 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 32/45] Fix typing --- src/sentry/integrations/perforce/client.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 6ba69133dfb893..5cbcd192c35b0d 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -314,6 +314,7 @@ def get_depots(self) -> Sequence[P4DepotInfo]: Returns: Sequence of depot info dictionaries """ +<<<<<<< HEAD with self._connect() as p4: depots = p4.run("depots") return [ @@ -356,6 +357,9 @@ def get_user(self, username: str) -> P4UserInfo | None: ) # User not found - return None (not an error condition) return None +======= + return [] +>>>>>>> 6b799feb551 (Fix typing) def get_changes( self, @@ -382,6 +386,7 @@ def get_changes( Raises: TypeError: If start_cl or end_cl are not integers """ +<<<<<<< HEAD with self._connect() as p4: # Validate types - changelists must be integers if start_cl is not None and not isinstance(start_cl, int): @@ -430,6 +435,9 @@ def get_changes( ) for change in changes ] +======= + return [] +>>>>>>> 6b799feb551 (Fix typing) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -445,7 +453,16 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return None + return [] + + def get_file( + self, repo: Repository, path: str, ref: str | None, codeowners: bool = False + ) -> str: + """ + Get file contents from Perforce depot. + Required by abstract base class but not used (CODEOWNERS feature removed). + """ + raise NotImplementedError("get_file is not supported for Perforce") def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False From ebe9c300bc9e59db39cbe13a6ecf4e668b000008 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 33/45] feat(perforce): Add backend support for Perforce integration This commit adds backend support for Perforce version control integration: - New Perforce integration with P4 client support - Repository and code mapping functionality - Stacktrace linking for Perforce depot paths - Tests for integration, code mapping, and stacktrace linking - Updated dependencies in pyproject.toml The integration supports: - Authentication via P4PORT, P4USER, P4PASSWD - Code mapping between depot paths and project structure - Source URL generation for stacktrace frames - Integration with Sentry's repository and code mapping systems --- src/sentry/integrations/perforce/client.py | 329 +++++++++++++++++- .../integrations/perforce/integration.py | 115 +++++- .../perforce/test_code_mapping.py | 25 ++ 3 files changed, 450 insertions(+), 19 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5cbcd192c35b0d..5590771ead96de 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -1,9 +1,15 @@ from __future__ import annotations import logging +<<<<<<< HEAD from collections.abc import Generator, Sequence from contextlib import contextmanager from typing import Any, TypedDict +======= +from collections.abc import Sequence +from datetime import datetime, timezone +from typing import Any +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) from P4 import P4, P4Exception @@ -12,13 +18,19 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) from sentry.integrations.source_code_management.repository import RepositoryClient from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository +<<<<<<< HEAD from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError +======= +from sentry.shared_integrations.exceptions import ApiError +from sentry.utils import metrics +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) logger = logging.getLogger(__name__) @@ -107,6 +119,7 @@ def __init__( integration: Integration instance containing credentials in metadata org_integration: Organization integration instance (required for API compatibility) """ +<<<<<<< HEAD self.integration = integration self.org_integration = org_integration @@ -218,6 +231,62 @@ def _connect(self) -> Generator[P4]: except Exception as e: # Log disconnect failures as they may indicate connection leaks logger.warning("Failed to disconnect from Perforce: %s", e, exc_info=True) +======= + self.p4port = p4port + self.ssl_fingerprint = ssl_fingerprint + self.user = user or "" + self.password = password + self.client_name = client + self.base_url = f"p4://{self.host}:{self.port}" + self.P4 = P4 + self.P4Exception = P4Exception + + def _connect(self): + """Create and connect a P4 instance.""" + is_ticket_auth = bool(self.ticket) + + p4 = self.P4() + + if is_ticket_auth: + # Ticket authentication: P4Python auto-extracts host/port/user from ticket + # Just set the ticket as password and P4 will handle the rest + p4.password = self.ticket + else: + # Password authentication: set host/port/user explicitly + p4.port = f"{self.host}:{self.port}" + p4.user = self.user + + if self.password: + p4.password = self.password + + if self.client_name: + p4.client = self.client_name + + p4.exception_level = 1 # Only errors raise exceptions + + try: + p4.connect() + + # Authenticate with the server if password is provided (not ticket) + if self.password and not is_ticket_auth: + try: + p4.run_login() + except self.P4Exception as login_error: + p4.disconnect() + raise ApiError(f"Failed to authenticate with Perforce: {login_error}") + + return p4 + except self.P4Exception as e: + raise ApiError(f"Failed to connect to Perforce: {e}") + + def _disconnect(self, p4): + """Disconnect P4 instance.""" + try: + if p4.connected(): + p4.disconnect() + except Exception: + pass +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def check_file(self, repo: Repository, path: str, version: str | None) -> object | None: """ @@ -234,10 +303,141 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object Returns: File info dict if exists, None otherwise """ +<<<<<<< HEAD with self._connect() as p4: try: depot_path = self.build_depot_path(repo, path) result = p4.run("files", depot_path) +======= + p4 = self._connect() + try: + depot_path = self._build_depot_path(repo, path) + result = p4.run("files", depot_path) + + if result and len(result) > 0: + return result[0] + return None + + except self.P4Exception: + return None + finally: + self._disconnect(p4) + + def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) -> str: + """ + Build full depot path from repo config and file path. + + Args: + repo: Repository object + path: File path (may include @revision syntax like "file.cpp@42") + ref: Optional ref/revision (for compatibility, but Perforce uses @revision in path) + + Returns: + Full depot path with @revision preserved if present + """ + depot_root = repo.config.get("depot_path", repo.name).rstrip("/") + + # Ensure path doesn't start with / + path = path.lstrip("/") + + # If path contains @revision, preserve it (e.g., "file.cpp@42") + # If ref is provided and path doesn't have @revision, append it + full_path = f"{depot_root}/{path}" + + # If ref is provided and path doesn't already have @revision, append it + # Skip "master" as it's a Git concept and not valid in Perforce + if ref and "@" not in path and ref != "master": + full_path = f"{full_path}@{ref}" + + return full_path + + def get_blame( + self, repo: Repository, path: str, ref: str | None = None, lineno: int | None = None + ) -> list[dict[str, Any]]: + """ + Get blame/annotate information for a file (like git blame). + + Uses 'p4 filelog' + 'p4 describe' which is much faster than 'p4 annotate'. + Returns the most recent changelist that modified the file and its author. + This is used for CODEOWNERS-style ownership detection. + + Args: + repo: Repository object (depot_path includes stream if specified) + path: File path relative to depot (may include @revision like "file.cpp@42") + ref: Optional revision/changelist number (appended as @ref if not in path) + lineno: Specific line number to blame (optional, currently ignored) + + Returns: + List with a single entry containing: + - changelist: changelist number + - user: username who made the change + - date: date of change + - description: changelist description + """ + p4 = self._connect() + try: + depot_path = self._build_depot_path(repo, path, ref) + + # Use 'p4 filelog' to get the most recent changelist for this file + # This is much faster than 'p4 annotate' + # If depot_path includes @revision, filelog will show history up to that revision + filelog = p4.run("filelog", "-m1", depot_path) + + if not filelog or len(filelog) == 0: + return [] + + # Get the most recent changelist number + # filelog returns a list of revisions, we want the first one + revisions = filelog[0].get("rev", []) + if not revisions or len(revisions) == 0: + return [] + + # Get the changelist number from the first revision + changelist = revisions[0].get("change") + if not changelist: + return [] + + # Get detailed changelist information using 'p4 describe' + # -s flag means "short" - don't include diffs, just metadata + change_info = p4.run("describe", "-s", changelist) + + if not change_info: + return [] + + change_data = change_info[0] + return [ + { + "changelist": str(changelist), + "user": change_data.get("user", "unknown"), + "date": change_data.get("time", ""), + "description": change_data.get("desc", ""), + } + ] + + except self.P4Exception: + return [] + finally: + self._disconnect(p4) + + def get_depot_info(self) -> dict[str, Any]: + """ + Get server info for testing connection. + + Returns: + Server info dictionary + """ + p4 = self._connect() + try: + info = p4.run("info")[0] + return { + "server_address": info.get("serverAddress"), + "server_version": info.get("serverVersion"), + "user": info.get("userName"), + "client": info.get("clientName"), + } + finally: + self._disconnect(p4) +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) # Verify result contains actual file data (not just warnings) # When exception_level=1, warnings are returned in result list @@ -314,6 +514,7 @@ def get_depots(self) -> Sequence[P4DepotInfo]: Returns: Sequence of depot info dictionaries """ +<<<<<<< HEAD <<<<<<< HEAD with self._connect() as p4: depots = p4.run("depots") @@ -360,6 +561,21 @@ def get_user(self, username: str) -> P4UserInfo | None: ======= return [] >>>>>>> 6b799feb551 (Fix typing) +======= + p4 = self._connect() + try: + depots = p4.run("depots") + return [ + { + "name": depot.get("name"), + "type": depot.get("type"), + "description": depot.get("desc", ""), + } + for depot in depots + ] + finally: + self._disconnect(p4) +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def get_changes( self, @@ -386,6 +602,7 @@ def get_changes( Raises: TypeError: If start_cl or end_cl are not integers """ +<<<<<<< HEAD <<<<<<< HEAD with self._connect() as p4: # Validate types - changelists must be integers @@ -413,11 +630,20 @@ def get_changes( # Use it for end_cl (inclusive upper bound) if end_cl_num is not None: args.extend(["-e", str(end_cl_num)]) +======= + p4 = self._connect() + try: + args = ["-m", str(max_changes), "-l"] + + if start_cl: + args.extend(["-e", start_cl]) +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) args.append(depot_path) changes = p4.run("changes", *args) +<<<<<<< HEAD # Client-side filter for start_cl (exclusive lower bound) # Filter out changes <= start_cl to get changes > start_cl if start_cl_num is not None: @@ -438,6 +664,22 @@ def get_changes( ======= return [] >>>>>>> 6b799feb551 (Fix typing) +======= + return [ + { + "change": change.get("change"), + "user": change.get("user"), + "client": change.get("client"), + "time": change.get("time"), + "desc": change.get("desc"), + } + for change in changes + ] + finally: + self._disconnect(p4) + + # CommitContextClient methods (stubbed for now) +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -453,16 +695,85 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + metrics.incr("integrations.perforce.get_blame_for_files") + blames = [] + p4 = self._connect() - def get_file( - self, repo: Repository, path: str, ref: str | None, codeowners: bool = False - ) -> str: - """ - Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). - """ - raise NotImplementedError("get_file is not supported for Perforce") + try: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the revision/changelist if available + depot_path = self._build_depot_path(file.repo, file.path, file.ref) + + # Use faster p4 filelog approach to get most recent changelist + # This is much faster than p4 annotate + filelog = p4.run("filelog", "-m1", depot_path) + + changelist = None + if filelog and len(filelog) > 0: + # The 'change' field contains the changelist numbers (as a list of strings) + changelists = filelog[0].get("change", []) + if changelists and len(changelists) > 0: + # Get the first (most recent) changelist number + changelist = changelists[0] + + # If we found a changelist, get detailed commit info + if changelist: + try: + change_info = p4.run("describe", "-s", changelist) + if change_info and len(change_info) > 0: + change = change_info[0] + username = change.get("user", "unknown") + # Perforce doesn't provide email by default, so we generate a fallback + email = change.get("email") or f"{username}@perforce.local" + commit = CommitInfo( + commitId=changelist, + committedDate=datetime.fromtimestamp( + int(change.get("time", 0)), tz=timezone.utc + ), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=username, + commitAuthorEmail=email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except self.P4Exception as e: + logger.warning( + "perforce.client.get_blame_for_files.describe_error", + extra={ + **extra, + "changelist": changelist, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + except self.P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.annotate_error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames + finally: + self._disconnect(p4) def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..1ed67e91ba6bc8 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,6 +210,30 @@ def source_url_matches(self, url: str) -> bool: return False + def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: + """ + Check if a file path matches this repository's depot path. + + When SRCSRV transformers remap paths to absolute depot paths (e.g., + //depot/project/src/file.cpp), this method verifies that the depot path + matches the repository's configured depot_path. + + Args: + repo: Repository object + filepath: File path (may be absolute depot path or relative path) + + Returns: + True if the filepath matches this repository's depot + """ + depot_path = repo.config.get("depot_path", repo.name) + + # If filepath is absolute depot path, check if it starts with depot_path + if filepath.startswith("//"): + return filepath.startswith(depot_path) + + # Relative paths always match (will be prepended with depot_path) + return True + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -409,6 +433,30 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] + def test_connection(self) -> dict[str, Any]: + """ + Test the Perforce connection with current credentials. + + Returns: + Dictionary with connection status and server info + """ + try: + client = self.get_client() + info = client.get_depot_info() + + return { + "status": "success", + "message": f"Connected to Perforce server at {info.get('server_address')}", + "server_info": info, + } + except Exception as e: + logger.exception("perforce.test_connection.failed") + return { + "status": "error", + "message": f"Failed to connect to Perforce server: {str(e)}", + "error": str(e), + } + def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -418,20 +466,53 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "p4port", - "type": "string", - "label": "P4PORT (Server Address)", - "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "type": "string", + "label": "Perforce Server Host", + "placeholder": "perforce.company.com", + "help": "The hostname or IP address of your Perforce server", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "port", + "type": "number", + "label": "Perforce Server Port", + "placeholder": "1666", + "help": "The port number for your Perforce server (default: 1666)", + "required": False, + "default": "1666", + "depends_on": {"auth_mode": "password"}, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", - "required": True, + "help": "Username for authenticating with Perforce", + "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "auth_type", @@ -459,6 +540,7 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -468,12 +550,25 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, + { + "name": "web_viewer_type", + "type": "choice", + "label": "Web Viewer Type", + "choices": [ + ["p4web", "P4Web"], + ["swarm", "Helix Swarm"], + ["other", "Other"], + ], + "help": "Type of web viewer (if web URL is provided)", + "required": False, + "default": "p4web", + }, { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", - "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "label": "Web Viewer URL (Optional)", + "placeholder": "https://p4web.company.com", + "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", "required": False, }, ] diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 6e35f5a3457dc8..48761c0c667d15 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -1,5 +1,8 @@ +<<<<<<< HEAD from unittest.mock import patch +======= +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig from sentry.integrations.perforce.integration import ( PerforceIntegration, @@ -33,9 +36,12 @@ def setUp(self): self.org_integration = self.integration.organizationintegration_set.first() assert self.org_integration is not None +<<<<<<< HEAD def tearDown(self): super().tearDown() +======= +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def test_code_mapping_depot_root_to_slash(self): """ Test code mapping: depot/ -> / @@ -342,6 +348,7 @@ def setUp(self): default_branch=None, ) +<<<<<<< HEAD # Mock the Perforce client's check_file to avoid actual P4 connection self.check_file_patcher = patch( "sentry.integrations.perforce.client.PerforceClient.check_file", @@ -353,6 +360,8 @@ def tearDown(self): self.check_file_patcher.stop() super().tearDown() +======= +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def test_python_sdk_path_full_flow(self): """Test full flow: Python SDK -> code mapping -> format_source_url""" # 1. Python SDK sends this path @@ -399,11 +408,16 @@ def test_full_flow_with_swarm_viewer(self): name="Perforce", external_id="perforce-test-web-flow", metadata={ +<<<<<<< HEAD "p4port": "ssl:perforce.example.com:1666", "user": "testuser", "password": "testpass", "auth_type": "password", "web_url": "https://p4web.example.com", +======= + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -433,10 +447,17 @@ def test_full_flow_with_swarm_viewer(self): default_branch=None, ) +<<<<<<< HEAD # Python SDK path with #revision from Symbolic frame = EventFrame( filename="depot/app/services/processor.py#42", abs_path="depot/app/services/processor.py#42", +======= + # Python SDK path with @revision from Symbolic + frame = EventFrame( + filename="depot/app/services/processor.py@42", + abs_path="depot/app/services/processor.py@42", +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) ) # Code mapping @@ -451,5 +472,9 @@ def test_full_flow_with_swarm_viewer(self): assert mapped_path is not None url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) +<<<<<<< HEAD # Swarm format: /files/?v= assert url == "https://p4web.example.com/files//depot/app/services/processor.py?v=42" +======= + assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64&rev1=42" +>>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) From 57eab647359c7447574f91035712732a89b9cee8 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:12:47 +0100 Subject: [PATCH 34/45] Finalize rebase --- src/sentry/integrations/perforce/client.py | 249 --------------------- 1 file changed, 249 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5590771ead96de..8bebf5e67c50f4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -1,15 +1,9 @@ from __future__ import annotations import logging -<<<<<<< HEAD from collections.abc import Generator, Sequence from contextlib import contextmanager from typing import Any, TypedDict -======= -from collections.abc import Sequence -from datetime import datetime, timezone -from typing import Any ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) from P4 import P4, P4Exception @@ -25,12 +19,7 @@ from sentry.integrations.source_code_management.repository import RepositoryClient from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository -<<<<<<< HEAD from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError -======= -from sentry.shared_integrations.exceptions import ApiError -from sentry.utils import metrics ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) logger = logging.getLogger(__name__) @@ -119,7 +108,6 @@ def __init__( integration: Integration instance containing credentials in metadata org_integration: Organization integration instance (required for API compatibility) """ -<<<<<<< HEAD self.integration = integration self.org_integration = org_integration @@ -231,62 +219,6 @@ def _connect(self) -> Generator[P4]: except Exception as e: # Log disconnect failures as they may indicate connection leaks logger.warning("Failed to disconnect from Perforce: %s", e, exc_info=True) -======= - self.p4port = p4port - self.ssl_fingerprint = ssl_fingerprint - self.user = user or "" - self.password = password - self.client_name = client - self.base_url = f"p4://{self.host}:{self.port}" - self.P4 = P4 - self.P4Exception = P4Exception - - def _connect(self): - """Create and connect a P4 instance.""" - is_ticket_auth = bool(self.ticket) - - p4 = self.P4() - - if is_ticket_auth: - # Ticket authentication: P4Python auto-extracts host/port/user from ticket - # Just set the ticket as password and P4 will handle the rest - p4.password = self.ticket - else: - # Password authentication: set host/port/user explicitly - p4.port = f"{self.host}:{self.port}" - p4.user = self.user - - if self.password: - p4.password = self.password - - if self.client_name: - p4.client = self.client_name - - p4.exception_level = 1 # Only errors raise exceptions - - try: - p4.connect() - - # Authenticate with the server if password is provided (not ticket) - if self.password and not is_ticket_auth: - try: - p4.run_login() - except self.P4Exception as login_error: - p4.disconnect() - raise ApiError(f"Failed to authenticate with Perforce: {login_error}") - - return p4 - except self.P4Exception as e: - raise ApiError(f"Failed to connect to Perforce: {e}") - - def _disconnect(self, p4): - """Disconnect P4 instance.""" - try: - if p4.connected(): - p4.disconnect() - except Exception: - pass ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def check_file(self, repo: Repository, path: str, version: str | None) -> object | None: """ @@ -303,141 +235,10 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object Returns: File info dict if exists, None otherwise """ -<<<<<<< HEAD with self._connect() as p4: try: depot_path = self.build_depot_path(repo, path) result = p4.run("files", depot_path) -======= - p4 = self._connect() - try: - depot_path = self._build_depot_path(repo, path) - result = p4.run("files", depot_path) - - if result and len(result) > 0: - return result[0] - return None - - except self.P4Exception: - return None - finally: - self._disconnect(p4) - - def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) -> str: - """ - Build full depot path from repo config and file path. - - Args: - repo: Repository object - path: File path (may include @revision syntax like "file.cpp@42") - ref: Optional ref/revision (for compatibility, but Perforce uses @revision in path) - - Returns: - Full depot path with @revision preserved if present - """ - depot_root = repo.config.get("depot_path", repo.name).rstrip("/") - - # Ensure path doesn't start with / - path = path.lstrip("/") - - # If path contains @revision, preserve it (e.g., "file.cpp@42") - # If ref is provided and path doesn't have @revision, append it - full_path = f"{depot_root}/{path}" - - # If ref is provided and path doesn't already have @revision, append it - # Skip "master" as it's a Git concept and not valid in Perforce - if ref and "@" not in path and ref != "master": - full_path = f"{full_path}@{ref}" - - return full_path - - def get_blame( - self, repo: Repository, path: str, ref: str | None = None, lineno: int | None = None - ) -> list[dict[str, Any]]: - """ - Get blame/annotate information for a file (like git blame). - - Uses 'p4 filelog' + 'p4 describe' which is much faster than 'p4 annotate'. - Returns the most recent changelist that modified the file and its author. - This is used for CODEOWNERS-style ownership detection. - - Args: - repo: Repository object (depot_path includes stream if specified) - path: File path relative to depot (may include @revision like "file.cpp@42") - ref: Optional revision/changelist number (appended as @ref if not in path) - lineno: Specific line number to blame (optional, currently ignored) - - Returns: - List with a single entry containing: - - changelist: changelist number - - user: username who made the change - - date: date of change - - description: changelist description - """ - p4 = self._connect() - try: - depot_path = self._build_depot_path(repo, path, ref) - - # Use 'p4 filelog' to get the most recent changelist for this file - # This is much faster than 'p4 annotate' - # If depot_path includes @revision, filelog will show history up to that revision - filelog = p4.run("filelog", "-m1", depot_path) - - if not filelog or len(filelog) == 0: - return [] - - # Get the most recent changelist number - # filelog returns a list of revisions, we want the first one - revisions = filelog[0].get("rev", []) - if not revisions or len(revisions) == 0: - return [] - - # Get the changelist number from the first revision - changelist = revisions[0].get("change") - if not changelist: - return [] - - # Get detailed changelist information using 'p4 describe' - # -s flag means "short" - don't include diffs, just metadata - change_info = p4.run("describe", "-s", changelist) - - if not change_info: - return [] - - change_data = change_info[0] - return [ - { - "changelist": str(changelist), - "user": change_data.get("user", "unknown"), - "date": change_data.get("time", ""), - "description": change_data.get("desc", ""), - } - ] - - except self.P4Exception: - return [] - finally: - self._disconnect(p4) - - def get_depot_info(self) -> dict[str, Any]: - """ - Get server info for testing connection. - - Returns: - Server info dictionary - """ - p4 = self._connect() - try: - info = p4.run("info")[0] - return { - "server_address": info.get("serverAddress"), - "server_version": info.get("serverVersion"), - "user": info.get("userName"), - "client": info.get("clientName"), - } - finally: - self._disconnect(p4) ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) # Verify result contains actual file data (not just warnings) # When exception_level=1, warnings are returned in result list @@ -514,8 +315,6 @@ def get_depots(self) -> Sequence[P4DepotInfo]: Returns: Sequence of depot info dictionaries """ -<<<<<<< HEAD -<<<<<<< HEAD with self._connect() as p4: depots = p4.run("depots") return [ @@ -558,24 +357,6 @@ def get_user(self, username: str) -> P4UserInfo | None: ) # User not found - return None (not an error condition) return None -======= - return [] ->>>>>>> 6b799feb551 (Fix typing) -======= - p4 = self._connect() - try: - depots = p4.run("depots") - return [ - { - "name": depot.get("name"), - "type": depot.get("type"), - "description": depot.get("desc", ""), - } - for depot in depots - ] - finally: - self._disconnect(p4) ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def get_changes( self, @@ -602,8 +383,6 @@ def get_changes( Raises: TypeError: If start_cl or end_cl are not integers """ -<<<<<<< HEAD -<<<<<<< HEAD with self._connect() as p4: # Validate types - changelists must be integers if start_cl is not None and not isinstance(start_cl, int): @@ -630,20 +409,11 @@ def get_changes( # Use it for end_cl (inclusive upper bound) if end_cl_num is not None: args.extend(["-e", str(end_cl_num)]) -======= - p4 = self._connect() - try: - args = ["-m", str(max_changes), "-l"] - - if start_cl: - args.extend(["-e", start_cl]) ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) args.append(depot_path) changes = p4.run("changes", *args) -<<<<<<< HEAD # Client-side filter for start_cl (exclusive lower bound) # Filter out changes <= start_cl to get changes > start_cl if start_cl_num is not None: @@ -661,25 +431,6 @@ def get_changes( ) for change in changes ] -======= - return [] ->>>>>>> 6b799feb551 (Fix typing) -======= - return [ - { - "change": change.get("change"), - "user": change.get("user"), - "client": change.get("client"), - "time": change.get("time"), - "desc": change.get("desc"), - } - for change in changes - ] - finally: - self._disconnect(p4) - - # CommitContextClient methods (stubbed for now) ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] From 9dde7f89cc08b117335c1e422e18558e949bf7d9 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:52:01 +0100 Subject: [PATCH 35/45] feat(perforce): Implement repository/depot and code mapping logic --- .../perforce/test_code_mapping.py | 79 +++++++------------ 1 file changed, 27 insertions(+), 52 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 48761c0c667d15..621d2138209494 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -1,8 +1,5 @@ -<<<<<<< HEAD from unittest.mock import patch -======= ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig from sentry.integrations.perforce.integration import ( PerforceIntegration, @@ -36,12 +33,9 @@ def setUp(self): self.org_integration = self.integration.organizationintegration_set.first() assert self.org_integration is not None -<<<<<<< HEAD def tearDown(self): super().tearDown() -======= ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def test_code_mapping_depot_root_to_slash(self): """ Test code mapping: depot/ -> / @@ -80,8 +74,8 @@ def test_code_mapping_depot_root_to_slash(self): def test_code_mapping_with_symbolic_revision_syntax(self): """ - Test code mapping with Symbolic's #revision syntax. - The #revision should be preserved in the output. + Test code mapping with Symbolic's @revision syntax. + The @revision should be preserved in the output. """ repo = Repository.objects.create( name="//depot", @@ -101,17 +95,17 @@ def test_code_mapping_with_symbolic_revision_syntax(self): default_branch=None, ) - # Test C++ path from Symbolic: depot/game/src/main.cpp#1 + # Test C++ path from Symbolic: depot/game/src/main.cpp@42 frame = EventFrame( - filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" + filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" ) result = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" ) - # Should strip "depot/" and preserve "#1" - assert result == "game/src/main.cpp#1" + # Should strip "depot/" and preserve "@42" + assert result == "game/src/main.cpp@42" def test_code_mapping_multiple_depots(self): """Test code mappings for multiple depots (depot and myproject)""" @@ -348,7 +342,6 @@ def setUp(self): default_branch=None, ) -<<<<<<< HEAD # Mock the Perforce client's check_file to avoid actual P4 connection self.check_file_patcher = patch( "sentry.integrations.perforce.client.PerforceClient.check_file", @@ -360,8 +353,6 @@ def tearDown(self): self.check_file_patcher.stop() super().tearDown() -======= ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) def test_python_sdk_path_full_flow(self): """Test full flow: Python SDK -> code mapping -> format_source_url""" # 1. Python SDK sends this path @@ -387,94 +378,78 @@ def test_symbolic_cpp_path_full_flow(self): """Test full flow: Symbolic C++ -> code mapping -> format_source_url""" # 1. Symbolic transformer sends this path frame = EventFrame( - filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" + filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" ) # 2. Code mapping transforms it (use existing code_mapping from setUp) mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=self.code_mapping, platform="native", sdk_name="sentry.native" ) - assert mapped_path == "game/src/main.cpp#1" + assert mapped_path == "game/src/main.cpp@42" - # 3. format_source_url creates final URL (preserves #1) + # 3. format_source_url creates final URL (preserves @42) url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/game/src/main.cpp#1" + assert url == "p4://depot/game/src/main.cpp@42" - def test_full_flow_with_swarm_viewer(self): - """Test full flow with Swarm viewer configuration""" - integration_with_swarm = self.create_integration( + def test_full_flow_with_web_viewer(self): + """Test full flow with P4Web viewer configuration""" + integration_with_web = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", external_id="perforce-test-web-flow", metadata={ -<<<<<<< HEAD "p4port": "ssl:perforce.example.com:1666", "user": "testuser", "password": "testpass", "auth_type": "password", "web_url": "https://p4web.example.com", -======= - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) }, ) - installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] # Create repo with web viewer integration - repo_swarm = Repository.objects.create( + repo_web = Repository.objects.create( name="//depot", organization_id=self.organization.id, - integration_id=integration_with_swarm.id, + integration_id=integration_with_web.id, config={"depot_path": "//depot"}, ) # Use a new project to avoid unique constraint on (project_id, stack_root) - project_swarm = self.create_project(organization=self.organization) + project_web = self.create_project(organization=self.organization) - org_integration_swarm = integration_with_swarm.organizationintegration_set.first() - assert org_integration_swarm is not None + org_integration_web = integration_with_web.organizationintegration_set.first() + assert org_integration_web is not None - code_mapping_swarm = RepositoryProjectPathConfig.objects.create( - project=project_swarm, + code_mapping_web = RepositoryProjectPathConfig.objects.create( + project=project_web, organization_id=self.organization.id, - repository=repo_swarm, - organization_integration_id=org_integration_swarm.id, - integration_id=integration_with_swarm.id, + repository=repo_web, + organization_integration_id=org_integration_web.id, + integration_id=integration_with_web.id, stack_root="depot/", source_root="/", default_branch=None, ) -<<<<<<< HEAD # Python SDK path with #revision from Symbolic frame = EventFrame( filename="depot/app/services/processor.py#42", abs_path="depot/app/services/processor.py#42", -======= - # Python SDK path with @revision from Symbolic - frame = EventFrame( - filename="depot/app/services/processor.py@42", - abs_path="depot/app/services/processor.py@42", ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) ) # Code mapping mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, - code_mapping=code_mapping_swarm, + code_mapping=code_mapping_web, platform="python", sdk_name="sentry.python", ) - # format_source_url with Swarm viewer (revision extracted from filename) + # format_source_url with web viewer (revision extracted from filename) assert mapped_path is not None - url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) + url = installation.format_source_url(repo=repo_web, filepath=mapped_path, branch=None) -<<<<<<< HEAD # Swarm format: /files/?v= assert url == "https://p4web.example.com/files//depot/app/services/processor.py?v=42" -======= - assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64&rev1=42" ->>>>>>> 6541b7c05ef (feat(perforce): Add backend support for Perforce integration) From c51bb146bb4e9292c6d50a69c863bfc74bc58982 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 14:18:24 +0100 Subject: [PATCH 36/45] Fix PR comments --- src/sentry/integrations/perforce/repository.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index f2235b4649f8b4..79555977ef0c64 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -14,7 +14,6 @@ ) from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization -from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider From 7b5d9933e85a36481c7cf2729fe285de4b2ece93 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:55:30 +0100 Subject: [PATCH 37/45] feat(perforce): Implement stacktrace linking and file blame (annotate) logic --- src/sentry/integrations/perforce/client.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8bebf5e67c50f4..7c6dca79128eec 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -444,6 +444,10 @@ def get_blame_for_files( Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. + API docs: + - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html + - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html + Returns a list of FileBlameInfo objects containing commit details for each file. """ metrics.incr("integrations.perforce.get_blame_for_files") From 5bdbe84e5254f7210c714956bc18e7ce830c4b99 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 14:29:26 +0100 Subject: [PATCH 38/45] Fix PR comments, deduplicate logic --- src/sentry/integrations/perforce/client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 7c6dca79128eec..8bcd02d60197ac 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -335,6 +335,7 @@ def get_user(self, username: str) -> P4UserInfo | None: Args: username: Perforce username + p4: Optional P4 connection to reuse (avoids creating new connection) Returns: User info dictionary with Email and FullName fields, or None if not found From 9c202a134c118923c1b8c73a17425477264dc2f4 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Fri, 21 Nov 2025 13:40:06 +0100 Subject: [PATCH 39/45] Adaptations after PR comments --- src/sentry/integrations/perforce/client.py | 55 +++++++++++++++++-- .../integrations/perforce/repository.py | 32 +---------- 2 files changed, 51 insertions(+), 36 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8bcd02d60197ac..d433d1072281ba 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -3,6 +3,7 @@ import logging from collections.abc import Generator, Sequence from contextlib import contextmanager +from datetime import datetime, timezone from typing import Any, TypedDict from P4 import P4, P4Exception @@ -359,6 +360,46 @@ def get_user(self, username: str) -> P4UserInfo | None: # User not found - return None (not an error condition) return None + def get_author_info_from_cache( + self, username: str, user_cache: dict[str, P4UserInfo | None] + ) -> tuple[str, str]: + """ + Get author email and name from username with caching. + + Args: + username: Perforce username + user_cache: Cache dictionary for user lookups + + Returns: + Tuple of (author_email, author_name) + """ + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username not in user_cache: + try: + user_cache[username] = self.get_user(username) + except Exception as e: + logger.warning( + "perforce.get_author_info.user_lookup_failed", + extra={ + "username": username, + "error": str(e), + "error_type": type(e).__name__, + }, + ) + user_cache[username] = None + + user_info = user_cache.get(username) + if user_info: + if user_info.get("email"): + author_email = user_info["email"] + if user_info.get("full_name"): + author_name = user_info["full_name"] + + return author_email, author_name + def get_changes( self, depot_path: str, @@ -450,10 +491,14 @@ def get_blame_for_files( - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html Returns a list of FileBlameInfo objects containing commit details for each file. + + Performance notes: + - Makes ~3 P4 API calls per file: filelog, describe, user (cached) + - User lookups are cached within the request to minimize redundant calls + - Perforce doesn't have explicit rate limiting like GitHub + - Individual file failures are caught and logged without failing entire batch """ - metrics.incr("integrations.perforce.get_blame_for_files") - blames = [] - p4 = self._connect() + blames: list[FileBlameInfo] = [] try: for file in files: @@ -527,9 +572,7 @@ def get_blame_for_files( ) continue - return blames - finally: - self._disconnect(p4) + return blames def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 79555977ef0c64..cd72ac00352419 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -163,37 +163,9 @@ def _extract_commit_info( # Convert Unix timestamp to ISO 8601 format timestamp = datetime.fromtimestamp(time_int, tz=timezone.utc).isoformat() - # Get user information from Perforce + # Get user information from Perforce using shared helper username = change.get("user", "unknown") - author_email = f"{username}@perforce" - author_name = username - - # Fetch user info if not in cache (skip "unknown" placeholder) - if username != "unknown" and username not in user_cache: - try: - user_cache[username] = client.get_user(username) - except Exception as e: - # Log user lookup failures but don't fail the entire commit processing - logger.warning( - "perforce.format_commits.user_lookup_failed", - extra={ - "changelist": change.get("change"), - "username": username, - "error": str(e), - "error_type": type(e).__name__, - }, - ) - # Cache None to avoid repeated failed lookups for the same user - user_cache[username] = None - - user_info = user_cache.get(username) - if user_info: - # Use actual email from Perforce if available - if user_info.get("email"): - author_email = user_info["email"] - # Use full name from Perforce if available - if user_info.get("full_name"): - author_name = user_info["full_name"] + author_email, author_name = client.get_author_info_from_cache(username, user_cache) return P4CommitInfo( id=str(change["change"]), From 75aaa745b2befdaa3efd5e4d35fdac395b0a10f6 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Fri, 21 Nov 2025 13:52:49 +0100 Subject: [PATCH 40/45] Fix handling of file revisions for suspect commits --- src/sentry/integrations/perforce/client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index d433d1072281ba..5fd4e59862a22d 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -336,7 +336,6 @@ def get_user(self, username: str) -> P4UserInfo | None: Args: username: Perforce username - p4: Optional P4 connection to reuse (avoids creating new connection) Returns: User info dictionary with Email and FullName fields, or None if not found @@ -505,9 +504,9 @@ def get_blame_for_files( try: # Build depot path for the file (includes stream if specified) # file.ref contains the revision/changelist if available - depot_path = self._build_depot_path(file.repo, file.path, file.ref) + depot_path = self.build_depot_path(file.repo, file.path, file.ref) - # Use faster p4 filelog approach to get most recent changelist + # Use faster p4 filelog approach to get changelist for specific file revision # This is much faster than p4 annotate filelog = p4.run("filelog", "-m1", depot_path) From b6716608d4bbf9d4e88ca9235bfbcd2153f18b64 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 14:09:04 +0100 Subject: [PATCH 41/45] Fixes after rebase --- src/sentry/integrations/perforce/client.py | 6 +- .../integrations/perforce/integration.py | 115 +--- .../integrations/perforce/repository.py | 1 + .../integrations/perforce/test_client.py | 592 ++++++++++++++++++ 4 files changed, 606 insertions(+), 108 deletions(-) create mode 100644 tests/sentry/integrations/perforce/test_client.py diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5fd4e59862a22d..2c7d2e7f474b73 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -499,7 +499,7 @@ def get_blame_for_files( """ blames: list[FileBlameInfo] = [] - try: + with self._connect() as p4: for file in files: try: # Build depot path for the file (includes stream if specified) @@ -546,7 +546,7 @@ def get_blame_for_files( commit=commit, ) blames.append(blame_info) - except self.P4Exception as e: + except P4Exception as e: logger.warning( "perforce.client.get_blame_for_files.describe_error", extra={ @@ -557,7 +557,7 @@ def get_blame_for_files( "file_path": file.path, }, ) - except self.P4Exception as e: + except P4Exception as e: # Log but don't fail for individual file errors logger.warning( "perforce.client.get_blame_for_files.annotate_error", diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 1ed67e91ba6bc8..bf034e292574bf 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,30 +210,6 @@ def source_url_matches(self, url: str) -> bool: return False - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. - - When SRCSRV transformers remap paths to absolute depot paths (e.g., - //depot/project/src/file.cpp), this method verifies that the depot path - matches the repository's configured depot_path. - - Args: - repo: Repository object - filepath: File path (may be absolute depot path or relative path) - - Returns: - True if the filepath matches this repository's depot - """ - depot_path = repo.config.get("depot_path", repo.name) - - # If filepath is absolute depot path, check if it starts with depot_path - if filepath.startswith("//"): - return filepath.startswith(depot_path) - - # Relative paths always match (will be prepended with depot_path) - return True - def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -433,30 +409,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -466,53 +418,20 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "auth_mode", - "type": "choice", - "label": "Authentication Mode", - "choices": [ - ["password", "Username & Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", - "required": True, - "default": "password", - }, - { - "name": "ticket", - "type": "secret", - "label": "P4 Ticket", - "placeholder": "••••••••••••••••••••••••••••••••", - "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", - "required": False, - "depends_on": {"auth_mode": "ticket"}, - }, - { - "name": "host", + "name": "p4port", "type": "string", - "label": "Perforce Server Host", - "placeholder": "perforce.company.com", - "help": "The hostname or IP address of your Perforce server", - "required": False, - "depends_on": {"auth_mode": "password"}, - }, - { - "name": "port", - "type": "number", - "label": "Perforce Server Port", - "placeholder": "1666", - "help": "The port number for your Perforce server (default: 1666)", - "required": False, - "default": "1666", - "depends_on": {"auth_mode": "password"}, + "label": "P4PORT (Server Address)", + "placeholder": "ssl:perforce.company.com:1666", + "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "required": True, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce", - "required": False, - "depends_on": {"auth_mode": "password"}, + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", + "required": True, }, { "name": "auth_type", @@ -540,7 +459,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -550,25 +468,12 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, - { - "name": "web_viewer_type", - "type": "choice", - "label": "Web Viewer Type", - "choices": [ - ["p4web", "P4Web"], - ["swarm", "Helix Swarm"], - ["other", "Other"], - ], - "help": "Type of web viewer (if web URL is provided)", - "required": False, - "default": "p4web", - }, { "name": "web_url", "type": "string", - "label": "Web Viewer URL (Optional)", - "placeholder": "https://p4web.company.com", - "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", + "label": "Helix Swarm URL (Optional)", + "placeholder": "https://swarm.company.com", + "help": "Optional: URL to Helix Swarm web viewer for browsing files", "required": False, }, ] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index cd72ac00352419..1e27ac59d7a75a 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -14,6 +14,7 @@ ) from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization +from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py new file mode 100644 index 00000000000000..b05edf394708d1 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_client.py @@ -0,0 +1,592 @@ +from datetime import datetime, timezone +from unittest import mock + +import pytest + +from sentry.integrations.perforce.client import PerforceClient +from sentry.integrations.source_code_management.commit_context import SourceLineInfo +from sentry.models.repository import Repository +from sentry.testutils.cases import TestCase + + +class PerforceClientTest(TestCase): + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce Test", + external_id="perforce-test", + metadata={ + "p4port": "perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + "auth_type": "password", + }, + ) + self.org_integration = self.integration.organizationintegration_set.first() + + self.repo = Repository.objects.create( + organization_id=self.organization.id, + name="//depot", + provider="integrations:perforce", + external_id="//depot", + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + self.p4_client = PerforceClient( + integration=self.integration, org_integration=self.org_integration + ) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_success(self, mock_p4_class): + """Test get_blame_for_files returns commit info for files""" + # Mock P4 instance + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock filelog response + mock_p4.run.side_effect = [ + # First call: filelog for file 1 + [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], + # Second call: describe for changelist 12345 + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", # 2021-01-01 00:00:00 UTC + "desc": "Fix bug in main module", + } + ], + # Third call: filelog for file 2 + [{"change": ["12346"], "depotFile": "//depot/src/utils.py"}], + # Fourth call: describe for changelist 12346 + [ + { + "change": "12346", + "user": "janedoe", + "time": "1609545600", # 2021-01-02 00:00:00 UTC + "desc": "Add utility functions", + } + ], + ] + + file1 = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + file2 = SourceLineInfo( + path="src/utils.py", + lineno=25, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) + + assert len(blames) == 2 + + # Check first blame + assert blames[0].path == "src/main.py" + assert blames[0].lineno == 10 + assert blames[0].commit.commitId == "12345" + assert blames[0].commit.commitAuthorName == "johndoe" + assert blames[0].commit.commitAuthorEmail == "johndoe@perforce.local" + assert blames[0].commit.commitMessage == "Fix bug in main module" + assert blames[0].commit.committedDate == datetime(2021, 1, 1, 0, 0, 0, tzinfo=timezone.utc) + + # Check second blame + assert blames[1].path == "src/utils.py" + assert blames[1].lineno == 25 + assert blames[1].commit.commitId == "12346" + assert blames[1].commit.commitAuthorName == "janedoe" + assert blames[1].commit.commitAuthorEmail == "janedoe@perforce.local" + assert blames[1].commit.commitMessage == "Add utility functions" + assert blames[1].commit.committedDate == datetime(2021, 1, 2, 0, 0, 0, tzinfo=timezone.utc) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_no_changelist(self, mock_p4_class): + """Test get_blame_for_files handles files with no changelist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock filelog response with no changelist + mock_p4.run.return_value = [{"depotFile": "//depot/src/new_file.py"}] + + file = SourceLineInfo( + path="src/new_file.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + # Should return empty list when no changelist found + assert len(blames) == 0 + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_blame_for_files_with_stream(self, mock_p4_class): + """Test get_blame_for_files handles files with stream (ref)""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.side_effect = [ + # filelog + [{"change": ["12345"], "depotFile": "//depot/main/src/main.py"}], + # describe + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", + "desc": "Initial commit", + } + ], + ] + + file = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="main", # Stream name + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + assert len(blames) == 1 + # Verify filelog was called with the stream path + assert mock_p4.run.call_args_list[0][0] == ("filelog", "-m1", "//depot/main/src/main.py") + + @mock.patch("sentry.integrations.perforce.client.P4") + @mock.patch("sentry.integrations.perforce.client.logger") + def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): + """Test get_blame_for_files handles P4 exceptions gracefully""" + from P4 import P4Exception + + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # First file succeeds, second file throws exception + mock_p4.run.side_effect = [ + # File 1 filelog + [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], + # File 1 describe + [ + { + "change": "12345", + "user": "johndoe", + "time": "1609459200", + "desc": "Initial commit", + } + ], + # File 2 filelog - throws exception + P4Exception("File not found"), + ] + + file1 = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + file2 = SourceLineInfo( + path="src/missing.py", + lineno=20, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file1, file2], extra={}) + + # Should return blame for file1 only + assert len(blames) == 1 + assert blames[0].path == "src/main.py" + + # Should log warning for file2 + assert mock_logger.warning.called + + @mock.patch("sentry.integrations.perforce.client.P4") + @mock.patch("sentry.integrations.perforce.client.logger") + def test_get_blame_for_files_describe_exception(self, mock_logger, mock_p4_class): + """Test get_blame_for_files handles describe exceptions""" + from P4 import P4Exception + + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.side_effect = [ + # filelog succeeds + [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], + # describe throws exception + P4Exception("Changelist not found"), + ] + + file = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + # Should return empty list when describe fails + assert len(blames) == 0 + # Should log warning + assert mock_logger.warning.called + assert "describe_error" in str(mock_logger.warning.call_args) + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_depots(self, mock_p4_class): + """Test get_depots returns depot list""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock depots response + mock_p4.run.return_value = [ + {"name": "depot", "type": "local", "desc": "Main depot"}, + {"name": "myproject", "type": "stream", "desc": "Project stream depot"}, + ] + + depots = self.p4_client.get_depots() + + # Verify p4 depots command was called + mock_p4.run.assert_called_once_with("depots") + + # Check results + assert len(depots) == 2 + assert depots[0]["name"] == "depot" + assert depots[0]["type"] == "local" + assert depots[0]["description"] == "Main depot" + + assert depots[1]["name"] == "myproject" + assert depots[1]["type"] == "stream" + assert depots[1]["description"] == "Project stream depot" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_user_existing(self, mock_p4_class): + """Test get_user returns user info for existing user""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock user response for existing user + mock_p4.run.return_value = [ + { + "User": "johndoe", + "Email": "john.doe@example.com", + "FullName": "John Doe", + "Update": "2021/01/01 12:00:00", # Indicates user exists + } + ] + + user_info = self.p4_client.get_user("johndoe") + + # Verify p4 user command was called + mock_p4.run.assert_called_once_with("user", "-o", "johndoe") + + # Check results + assert user_info is not None + assert user_info["username"] == "johndoe" + assert user_info["email"] == "john.doe@example.com" + assert user_info["full_name"] == "John Doe" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_user_nonexistent(self, mock_p4_class): + """Test get_user returns None for non-existent user""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock user response for non-existent user (template with no Update field) + mock_p4.run.return_value = [ + { + "User": "newuser", + "Email": "", + "FullName": "", + # No Update field - indicates user doesn't exist + } + ] + + user_info = self.p4_client.get_user("newuser") + + # Should return None for non-existent user + assert user_info is None + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_without_range(self, mock_p4_class): + """Test get_changes returns changelists without start/end range""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock changes response + mock_p4.run.return_value = [ + { + "change": "12346", + "user": "janedoe", + "client": "jane-workspace", + "time": "1609545600", + "desc": "Add utility functions", + }, + { + "change": "12345", + "user": "johndoe", + "client": "john-workspace", + "time": "1609459200", + "desc": "Fix bug in main module", + }, + ] + + changes = self.p4_client.get_changes("//depot/...", max_changes=20) + + # Verify p4 changes command was called with correct args + mock_p4.run.assert_called_once_with("changes", "-m", "20", "-l", "//depot/...") + + # Check results + assert len(changes) == 2 + assert changes[0]["change"] == "12346" + assert changes[0]["user"] == "janedoe" + assert changes[1]["change"] == "12345" + assert changes[1]["user"] == "johndoe" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_end_cl(self, mock_p4_class): + """Test get_changes filters by end changelist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.return_value = [ + { + "change": "12346", + "user": "janedoe", + "client": "jane-ws", + "time": "1609545600", + "desc": "Add utils", + }, + { + "change": "12345", + "user": "johndoe", + "client": "john-ws", + "time": "1609459200", + "desc": "Fix bug", + }, + ] + + changes = self.p4_client.get_changes("//depot/...", max_changes=20, end_cl=12346) + + # Verify -e flag was used for end_cl + mock_p4.run.assert_called_once_with( + "changes", "-m", "20", "-l", "-e", "12346", "//depot/..." + ) + + assert len(changes) == 2 + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_start_cl(self, mock_p4_class): + """Test get_changes filters by start changelist (exclusive)""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock returns changes 12345, 12346, 12347 + mock_p4.run.return_value = [ + { + "change": "12347", + "user": "user3", + "client": "ws3", + "time": "1609632000", + "desc": "Change 3", + }, + { + "change": "12346", + "user": "user2", + "client": "ws2", + "time": "1609545600", + "desc": "Change 2", + }, + { + "change": "12345", + "user": "user1", + "client": "ws1", + "time": "1609459200", + "desc": "Change 1", + }, + ] + + # Filter out changes <= 12345 (only want changes > 12345) + changes = self.p4_client.get_changes("//depot/...", max_changes=20, start_cl=12345) + + # Verify no -s flag (Perforce doesn't have one) + # Client-side filtering is done for start_cl + assert len(changes) == 2 + assert changes[0]["change"] == "12347" + assert changes[1]["change"] == "12346" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_with_range(self, mock_p4_class): + """Test get_changes filters by start and end changelist range""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + mock_p4.run.return_value = [ + { + "change": "12347", + "user": "user3", + "client": "ws3", + "time": "1609632000", + "desc": "Change 3", + }, + { + "change": "12346", + "user": "user2", + "client": "ws2", + "time": "1609545600", + "desc": "Change 2", + }, + { + "change": "12345", + "user": "user1", + "client": "ws1", + "time": "1609459200", + "desc": "Change 1", + }, + ] + + # Get changes in range (12345, 12348] = 12346, 12347 + changes = self.p4_client.get_changes( + "//depot/...", max_changes=20, start_cl=12345, end_cl=12348 + ) + + # Should filter out 12345 (exclusive lower bound) + assert len(changes) == 2 + assert changes[0]["change"] == "12347" + assert changes[1]["change"] == "12346" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_get_changes_type_validation(self, mock_p4_class): + """Test get_changes validates that start_cl and end_cl are integers""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Test with string start_cl + with pytest.raises(TypeError, match="start_cl must be an integer"): + self.p4_client.get_changes("//depot/...", start_cl="12345") # type: ignore[arg-type] + + # Test with string end_cl + with pytest.raises(TypeError, match="end_cl must be an integer"): + self.p4_client.get_changes("//depot/...", end_cl="12346") # type: ignore[arg-type] + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_exists(self, mock_p4_class): + """Test check_file returns file info when file exists""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock files response + mock_p4.run.return_value = [ + { + "depotFile": "//depot/src/main.py", + "rev": "5", + "change": "12345", + "action": "edit", + } + ] + + result = self.p4_client.check_file(self.repo, "src/main.py", None) + + # Verify p4 files command was called + mock_p4.run.assert_called_once_with("files", "//depot/src/main.py") + + # Check result + assert result is not None + assert isinstance(result, dict) + assert result["depotFile"] == "//depot/src/main.py" + assert result["rev"] == "5" + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_not_exists(self, mock_p4_class): + """Test check_file returns None when file doesn't exist""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock P4Exception for non-existent file + from P4 import P4Exception + + mock_p4.run.side_effect = P4Exception("File not found") + + result = self.p4_client.check_file(self.repo, "src/missing.py", None) + + # Should return None for missing file + assert result is None + + @mock.patch("sentry.integrations.perforce.client.P4") + def test_check_file_empty_result(self, mock_p4_class): + """Test check_file returns None when result has no depotFile""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock empty result (just warnings) + mock_p4.run.return_value = [{"warning": "File is not in depot"}] + + result = self.p4_client.check_file(self.repo, "src/file.py", None) + + # Should return None when no depotFile in result + assert result is None + + def test_build_depot_path_relative(self): + """Test build_depot_path with relative path""" + # Test with simple relative path + path = self.p4_client.build_depot_path(self.repo, "app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + # Test with depot name in path (should strip it) + path = self.p4_client.build_depot_path(self.repo, "depot/app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + def test_build_depot_path_absolute(self): + """Test build_depot_path with absolute path""" + path = self.p4_client.build_depot_path(self.repo, "//depot/app/services/processor.py") + assert path == "//depot/app/services/processor.py" + + def test_build_depot_path_with_stream(self): + """Test build_depot_path with stream parameter""" + path = self.p4_client.build_depot_path( + self.repo, "app/services/processor.py", stream="main" + ) + assert path == "//depot/main/app/services/processor.py" + + def test_build_depot_path_with_revision(self): + """Test build_depot_path preserves #revision syntax""" + # Test with relative path + path = self.p4_client.build_depot_path(self.repo, "app/services/processor.py#42") + assert path == "//depot/app/services/processor.py#42" + + # Test with absolute path + path = self.p4_client.build_depot_path(self.repo, "//depot/app/main.cpp#1") + assert path == "//depot/app/main.cpp#1" + + # Test with stream and revision + path = self.p4_client.build_depot_path(self.repo, "app/main.cpp#5", stream="dev") + assert path == "//depot/dev/app/main.cpp#5" + + def test_build_depot_path_nested_depot(self): + """Test build_depot_path with nested depot paths""" + nested_repo = Repository.objects.create( + organization_id=self.organization.id, + name="//depot/game/project", + provider="integrations:perforce", + external_id="//depot/game/project", + integration_id=self.integration.id, + config={"depot_path": "//depot/game/project"}, + ) + + path = self.p4_client.build_depot_path(nested_repo, "src/main.cpp") + assert path == "//depot/game/project/src/main.cpp" From cdc3cf4e18f8e34cfc624b921188b0505f6f5f60 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 14:22:58 +0100 Subject: [PATCH 42/45] Fix PR comments --- src/sentry/integrations/perforce/client.py | 35 ++++++++--- .../integrations/perforce/test_client.py | 59 +++++++++++++++++-- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 2c7d2e7f474b73..195448dd9fa202 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -498,6 +498,7 @@ def get_blame_for_files( - Individual file failures are caught and logged without failing entire batch """ blames: list[FileBlameInfo] = [] + user_cache: dict[str, P4UserInfo | None] = {} with self._connect() as p4: for file in files: @@ -525,16 +526,36 @@ def get_blame_for_files( if change_info and len(change_info) > 0: change = change_info[0] username = change.get("user", "unknown") - # Perforce doesn't provide email by default, so we generate a fallback - email = change.get("email") or f"{username}@perforce.local" + + # Get author email and name with caching + author_email, author_name = self.get_author_info_from_cache( + username, user_cache + ) + + # Handle potentially null/invalid time field + time_value = change.get("time") or 0 + try: + time_int = int(time_value) + except (TypeError, ValueError) as e: + logger.warning( + "perforce.client.get_blame_for_files.invalid_time_value", + extra={ + **extra, + "changelist": changelist, + "time_value": time_value, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + time_int = 0 + commit = CommitInfo( commitId=changelist, - committedDate=datetime.fromtimestamp( - int(change.get("time", 0)), tz=timezone.utc - ), + committedDate=datetime.fromtimestamp(time_int, tz=timezone.utc), commitMessage=change.get("desc", "").strip(), - commitAuthorName=username, - commitAuthorEmail=email, + commitAuthorName=author_name, + commitAuthorEmail=author_email, ) blame_info = FileBlameInfo( diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py index b05edf394708d1..019228b2e4c7ff 100644 --- a/tests/sentry/integrations/perforce/test_client.py +++ b/tests/sentry/integrations/perforce/test_client.py @@ -59,9 +59,11 @@ def test_get_blame_for_files_success(self, mock_p4_class): "desc": "Fix bug in main module", } ], - # Third call: filelog for file 2 + # Third call: user lookup for johndoe (returns no Update field = doesn't exist) + [{"User": "johndoe", "Email": "", "FullName": ""}], + # Fourth call: filelog for file 2 [{"change": ["12346"], "depotFile": "//depot/src/utils.py"}], - # Fourth call: describe for changelist 12346 + # Fifth call: describe for changelist 12346 [ { "change": "12346", @@ -70,6 +72,8 @@ def test_get_blame_for_files_success(self, mock_p4_class): "desc": "Add utility functions", } ], + # Sixth call: user lookup for janedoe (returns no Update field = doesn't exist) + [{"User": "janedoe", "Email": "", "FullName": ""}], ] file1 = SourceLineInfo( @@ -96,7 +100,7 @@ def test_get_blame_for_files_success(self, mock_p4_class): assert blames[0].lineno == 10 assert blames[0].commit.commitId == "12345" assert blames[0].commit.commitAuthorName == "johndoe" - assert blames[0].commit.commitAuthorEmail == "johndoe@perforce.local" + assert blames[0].commit.commitAuthorEmail == "johndoe@perforce" assert blames[0].commit.commitMessage == "Fix bug in main module" assert blames[0].commit.committedDate == datetime(2021, 1, 1, 0, 0, 0, tzinfo=timezone.utc) @@ -105,7 +109,7 @@ def test_get_blame_for_files_success(self, mock_p4_class): assert blames[1].lineno == 25 assert blames[1].commit.commitId == "12346" assert blames[1].commit.commitAuthorName == "janedoe" - assert blames[1].commit.commitAuthorEmail == "janedoe@perforce.local" + assert blames[1].commit.commitAuthorEmail == "janedoe@perforce" assert blames[1].commit.commitMessage == "Add utility functions" assert blames[1].commit.committedDate == datetime(2021, 1, 2, 0, 0, 0, tzinfo=timezone.utc) @@ -149,6 +153,8 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): "desc": "Initial commit", } ], + # user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], ] file = SourceLineInfo( @@ -187,6 +193,8 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): "desc": "Initial commit", } ], + # File 1 user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], # File 2 filelog - throws exception P4Exception("File not found"), ] @@ -247,6 +255,49 @@ def test_get_blame_for_files_describe_exception(self, mock_logger, mock_p4_class assert mock_logger.warning.called assert "describe_error" in str(mock_logger.warning.call_args) + @mock.patch("sentry.integrations.perforce.client.P4") + @mock.patch("sentry.integrations.perforce.client.logger") + def test_get_blame_for_files_invalid_time(self, mock_logger, mock_p4_class): + """Test get_blame_for_files handles invalid time values gracefully""" + mock_p4 = mock.Mock() + mock_p4_class.return_value = mock_p4 + + # Mock with invalid time value (non-numeric string) + mock_p4.run.side_effect = [ + # filelog + [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], + # describe with invalid time value + [ + { + "change": "12345", + "user": "johndoe", + "time": "invalid", # Invalid time value that will raise ValueError + "desc": "Initial commit", + } + ], + # user lookup for johndoe + [{"User": "johndoe", "Email": "", "FullName": ""}], + ] + + file = SourceLineInfo( + path="src/main.py", + lineno=10, + ref="", + repo=self.repo, + code_mapping=None, # type: ignore[arg-type] + ) + + blames = self.p4_client.get_blame_for_files([file], extra={}) + + # Should still return blame with epoch time (0) + assert len(blames) == 1 + assert blames[0].commit.committedDate == datetime(1970, 1, 1, 0, 0, 0, tzinfo=timezone.utc) + + # Should log warning about invalid time + assert mock_logger.warning.called + warning_calls = [str(call) for call in mock_logger.warning.call_args_list] + assert any("invalid_time_value" in call for call in warning_calls) + @mock.patch("sentry.integrations.perforce.client.P4") def test_get_depots(self, mock_p4_class): """Test get_depots returns depot list""" From 023e6eb9cc492c68d1b67041d17188707a485d12 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 11:41:24 +0100 Subject: [PATCH 43/45] Reworks after meeting with Perforce --- src/sentry/integrations/perforce/client.py | 113 +++++++----------- .../integrations/perforce/integration.py | 14 +-- .../sentry/integrations/perforce-config.html | 4 +- .../integrations/perforce/test_client.py | 74 +++--------- 4 files changed, 74 insertions(+), 131 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 195448dd9fa202..acb5af34f3d794 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -167,7 +167,7 @@ def _connect(self) -> Generator[P4]: # Assert SSL trust after connection (if needed) # This must be done after p4.connect() but before p4.run_login() - if self.ssl_fingerprint and self.p4port.startswith("ssl:"): + if self.ssl_fingerprint and self.p4port.startswith("ssl"): try: p4.run_trust("-i", self.ssl_fingerprint) except P4Exception as trust_error: @@ -477,22 +477,21 @@ def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] ) -> list[FileBlameInfo]: """ - Get blame information for multiple files using p4 filelog. + Get blame information for multiple files using p4 changes. - Uses 'p4 filelog' + 'p4 describe' which is much faster than 'p4 annotate'. - Returns the most recent changelist that modified each file. + Uses 'p4 changes -m 1 -l' to get the most recent changelist that modified each file. + This is simpler and faster than using p4 filelog + p4 describe. Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. API docs: - - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html - - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html + - p4 changes: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html Returns a list of FileBlameInfo objects containing commit details for each file. Performance notes: - - Makes ~3 P4 API calls per file: filelog, describe, user (cached) + - Makes ~2 P4 API calls per file: changes (with -l for description), user (cached) - User lookups are cached within the request to minimize redundant calls - Perforce doesn't have explicit rate limiting like GitHub - Individual file failures are caught and logged without failing entire batch @@ -507,81 +506,61 @@ def get_blame_for_files( # file.ref contains the revision/changelist if available depot_path = self.build_depot_path(file.repo, file.path, file.ref) - # Use faster p4 filelog approach to get changelist for specific file revision - # This is much faster than p4 annotate - filelog = p4.run("filelog", "-m1", depot_path) + # Use p4 changes -m 1 -l to get most recent change for this file + # -m 1: limit to 1 result (most recent) + # -l: include full changelist description + changes = p4.run("changes", "-m", "1", "-l", depot_path) - changelist = None - if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list of strings) - changelists = filelog[0].get("change", []) - if changelists and len(changelists) > 0: - # Get the first (most recent) changelist number - changelist = changelists[0] + if changes and len(changes) > 0: + change = changes[0] + changelist = change.get("change", "") + username = change.get("user", "unknown") - # If we found a changelist, get detailed commit info - if changelist: + # Get author email and name with caching + author_email, author_name = self.get_author_info_from_cache( + username, user_cache + ) + + # Handle potentially null/invalid time field + time_value = change.get("time") or 0 try: - change_info = p4.run("describe", "-s", changelist) - if change_info and len(change_info) > 0: - change = change_info[0] - username = change.get("user", "unknown") - - # Get author email and name with caching - author_email, author_name = self.get_author_info_from_cache( - username, user_cache - ) - - # Handle potentially null/invalid time field - time_value = change.get("time") or 0 - try: - time_int = int(time_value) - except (TypeError, ValueError) as e: - logger.warning( - "perforce.client.get_blame_for_files.invalid_time_value", - extra={ - **extra, - "changelist": changelist, - "time_value": time_value, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - }, - ) - time_int = 0 - - commit = CommitInfo( - commitId=changelist, - committedDate=datetime.fromtimestamp(time_int, tz=timezone.utc), - commitMessage=change.get("desc", "").strip(), - commitAuthorName=author_name, - commitAuthorEmail=author_email, - ) - - blame_info = FileBlameInfo( - lineno=file.lineno, - path=file.path, - ref=file.ref, - repo=file.repo, - code_mapping=file.code_mapping, - commit=commit, - ) - blames.append(blame_info) - except P4Exception as e: + time_int = int(time_value) + except (TypeError, ValueError) as e: logger.warning( - "perforce.client.get_blame_for_files.describe_error", + "perforce.client.get_blame_for_files.invalid_time_value", extra={ **extra, "changelist": changelist, + "time_value": time_value, "error": str(e), "repo_name": file.repo.name, "file_path": file.path, }, ) + time_int = 0 + + commit = CommitInfo( + commitId=str(changelist), + committedDate=datetime.fromtimestamp(time_int, tz=timezone.utc), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=author_name, + commitAuthorEmail=author_email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except P4Exception as e: # Log but don't fail for individual file errors logger.warning( - "perforce.client.get_blame_for_files.annotate_error", + "perforce.client.get_blame_for_files.error", extra={ **extra, "error": str(e), diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index bf034e292574bf..3262275bf5e5d2 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -45,7 +45,7 @@ class PerforceMetadata(TypedDict, total=False): DESCRIPTION = """ -Connect your Sentry organization to your Perforce/Helix Core server to enable +Connect your Sentry organization to your P4 Core server to enable stacktrace linking, commit tracking, suspect commit detection, and code ownership. View source code directly from error stack traces and identify suspect commits that may have introduced issues. @@ -55,7 +55,7 @@ class PerforceMetadata(TypedDict, total=False): FeatureDescription( """ Link your Sentry stack traces back to your Perforce depot files with support - for Helix Swarm web viewer. Automatically maps error locations to + for P4 Code Review viewer. Automatically maps error locations to source code using configurable code mappings. """, IntegrationFeatures.STACKTRACE_LINK, @@ -145,8 +145,8 @@ class PerforceInstallationForm(forms.Form): required=False, ) web_url = forms.URLField( - label=_("Helix Swarm URL (Optional)"), - help_text=_("Optional: URL to Helix Swarm web viewer for browsing files"), + label=_("P4 Code Review URL (Optional)"), + help_text=_("Optional: URL to P4 Code Review web viewer for browsing files"), widget=forms.URLInput(attrs={"placeholder": "https://swarm.company.com"}), required=False, assume_scheme="https", @@ -166,7 +166,7 @@ def clean_web_url(self) -> str: class PerforceIntegration(RepositoryIntegration, CommitContextIntegration): """ - Integration for Perforce/Helix Core version control system. + Integration for P4 Core version control system. Provides stacktrace linking to depot files and suspect commit detection. """ @@ -471,9 +471,9 @@ def get_organization_config(self) -> list[dict[str, Any]]: { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", + "label": "P4 Core URL (Optional)", "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "help": "Optional: URL to P4 Core web viewer for browsing files", "required": False, }, ] diff --git a/src/sentry/templates/sentry/integrations/perforce-config.html b/src/sentry/templates/sentry/integrations/perforce-config.html index e7288bbe251c98..df2a500937a193 100644 --- a/src/sentry/templates/sentry/integrations/perforce-config.html +++ b/src/sentry/templates/sentry/integrations/perforce-config.html @@ -19,8 +19,8 @@ {% block title %} {% trans "Perforce Setup" %} | {{ block.super }} {% endblock %} {% block main %} -

{% trans "Configure Perforce/Helix Core Connection" %}

-

{% trans "Enter your Perforce server credentials to connect Sentry with your Perforce/Helix Core server." %}

+

{% trans "Configure P4 Core Connection" %}

+

{% trans "Enter your Perforce server credentials to connect Sentry with your P4 Core server." %}

{% trans "See the" %} {% trans "docs" %} {% trans "for more information." %}

diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py index 019228b2e4c7ff..ca490b3f07c1db 100644 --- a/tests/sentry/integrations/perforce/test_client.py +++ b/tests/sentry/integrations/perforce/test_client.py @@ -46,11 +46,9 @@ def test_get_blame_for_files_success(self, mock_p4_class): mock_p4 = mock.Mock() mock_p4_class.return_value = mock_p4 - # Mock filelog response + # Mock p4 changes responses (simpler now - one call per file) mock_p4.run.side_effect = [ - # First call: filelog for file 1 - [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], - # Second call: describe for changelist 12345 + # First call: changes for file 1 [ { "change": "12345", @@ -59,11 +57,9 @@ def test_get_blame_for_files_success(self, mock_p4_class): "desc": "Fix bug in main module", } ], - # Third call: user lookup for johndoe (returns no Update field = doesn't exist) + # Second call: user lookup for johndoe [{"User": "johndoe", "Email": "", "FullName": ""}], - # Fourth call: filelog for file 2 - [{"change": ["12346"], "depotFile": "//depot/src/utils.py"}], - # Fifth call: describe for changelist 12346 + # Third call: changes for file 2 [ { "change": "12346", @@ -72,7 +68,7 @@ def test_get_blame_for_files_success(self, mock_p4_class): "desc": "Add utility functions", } ], - # Sixth call: user lookup for janedoe (returns no Update field = doesn't exist) + # Fourth call: user lookup for janedoe [{"User": "janedoe", "Email": "", "FullName": ""}], ] @@ -119,8 +115,8 @@ def test_get_blame_for_files_no_changelist(self, mock_p4_class): mock_p4 = mock.Mock() mock_p4_class.return_value = mock_p4 - # Mock filelog response with no changelist - mock_p4.run.return_value = [{"depotFile": "//depot/src/new_file.py"}] + # Mock p4 changes response with empty result (no changes for this file) + mock_p4.run.return_value = [] file = SourceLineInfo( path="src/new_file.py", @@ -142,9 +138,7 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): mock_p4_class.return_value = mock_p4 mock_p4.run.side_effect = [ - # filelog - [{"change": ["12345"], "depotFile": "//depot/main/src/main.py"}], - # describe + # changes for file with stream [ { "change": "12345", @@ -168,8 +162,14 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): blames = self.p4_client.get_blame_for_files([file], extra={}) assert len(blames) == 1 - # Verify filelog was called with the stream path - assert mock_p4.run.call_args_list[0][0] == ("filelog", "-m1", "//depot/main/src/main.py") + # Verify changes was called with the stream path + assert mock_p4.run.call_args_list[0][0] == ( + "changes", + "-m", + "1", + "-l", + "//depot/main/src/main.py", + ) @mock.patch("sentry.integrations.perforce.client.P4") @mock.patch("sentry.integrations.perforce.client.logger") @@ -182,9 +182,7 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): # First file succeeds, second file throws exception mock_p4.run.side_effect = [ - # File 1 filelog - [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], - # File 1 describe + # File 1 changes [ { "change": "12345", @@ -195,7 +193,7 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): ], # File 1 user lookup for johndoe [{"User": "johndoe", "Email": "", "FullName": ""}], - # File 2 filelog - throws exception + # File 2 changes - throws exception P4Exception("File not found"), ] @@ -223,38 +221,6 @@ def test_get_blame_for_files_p4_exception(self, mock_logger, mock_p4_class): # Should log warning for file2 assert mock_logger.warning.called - @mock.patch("sentry.integrations.perforce.client.P4") - @mock.patch("sentry.integrations.perforce.client.logger") - def test_get_blame_for_files_describe_exception(self, mock_logger, mock_p4_class): - """Test get_blame_for_files handles describe exceptions""" - from P4 import P4Exception - - mock_p4 = mock.Mock() - mock_p4_class.return_value = mock_p4 - - mock_p4.run.side_effect = [ - # filelog succeeds - [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], - # describe throws exception - P4Exception("Changelist not found"), - ] - - file = SourceLineInfo( - path="src/main.py", - lineno=10, - ref="", - repo=self.repo, - code_mapping=None, # type: ignore[arg-type] - ) - - blames = self.p4_client.get_blame_for_files([file], extra={}) - - # Should return empty list when describe fails - assert len(blames) == 0 - # Should log warning - assert mock_logger.warning.called - assert "describe_error" in str(mock_logger.warning.call_args) - @mock.patch("sentry.integrations.perforce.client.P4") @mock.patch("sentry.integrations.perforce.client.logger") def test_get_blame_for_files_invalid_time(self, mock_logger, mock_p4_class): @@ -264,9 +230,7 @@ def test_get_blame_for_files_invalid_time(self, mock_logger, mock_p4_class): # Mock with invalid time value (non-numeric string) mock_p4.run.side_effect = [ - # filelog - [{"change": ["12345"], "depotFile": "//depot/src/main.py"}], - # describe with invalid time value + # changes with invalid time value [ { "change": "12345", From d8a307950efd0ce6039e88f5a0cb109d856316ba Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 13:47:12 +0100 Subject: [PATCH 44/45] Suspect commit fixes --- src/sentry/integrations/perforce/client.py | 5 +++-- tests/sentry/integrations/perforce/test_client.py | 12 ++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index acb5af34f3d794..2af04bd3bb4eeb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -503,8 +503,9 @@ def get_blame_for_files( for file in files: try: # Build depot path for the file (includes stream if specified) - # file.ref contains the revision/changelist if available - depot_path = self.build_depot_path(file.repo, file.path, file.ref) + # file.ref contains the stream but we are ignoring it since it's + # already part of the depot path we get from stacktrace (SourceLineInfo) + depot_path = self.build_depot_path(file.repo, file.path, None) # Use p4 changes -m 1 -l to get most recent change for this file # -m 1: limit to 1 result (most recent) diff --git a/tests/sentry/integrations/perforce/test_client.py b/tests/sentry/integrations/perforce/test_client.py index ca490b3f07c1db..a2152984bb25e1 100644 --- a/tests/sentry/integrations/perforce/test_client.py +++ b/tests/sentry/integrations/perforce/test_client.py @@ -133,7 +133,11 @@ def test_get_blame_for_files_no_changelist(self, mock_p4_class): @mock.patch("sentry.integrations.perforce.client.P4") def test_get_blame_for_files_with_stream(self, mock_p4_class): - """Test get_blame_for_files handles files with stream (ref)""" + """Test get_blame_for_files handles files with stream (ref). + + Note: The stream name in ref is ignored because the path from SourceLineInfo + already contains the full depot path including any stream/branch information. + """ mock_p4 = mock.Mock() mock_p4_class.return_value = mock_p4 @@ -154,7 +158,7 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): file = SourceLineInfo( path="src/main.py", lineno=10, - ref="main", # Stream name + ref="main", # Stream name (ignored - path already contains full depot path) repo=self.repo, code_mapping=None, # type: ignore[arg-type] ) @@ -162,13 +166,13 @@ def test_get_blame_for_files_with_stream(self, mock_p4_class): blames = self.p4_client.get_blame_for_files([file], extra={}) assert len(blames) == 1 - # Verify changes was called with the stream path + # Verify changes was called with the depot path (stream from ref is not used) assert mock_p4.run.call_args_list[0][0] == ( "changes", "-m", "1", "-l", - "//depot/main/src/main.py", + "//depot/src/main.py", ) @mock.patch("sentry.integrations.perforce.client.P4") From 91415ca2fcf0ebc77da8a1ddbb4470006f6b54f8 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 17:12:33 +0100 Subject: [PATCH 45/45] Finalize rebase --- .../integrations/perforce/test_stacktrace_link.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 8dfcfc97f21e7c..11ecfb5155bee4 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -56,7 +56,12 @@ def tearDown(self): def test_get_stacktrace_config_python_path(self): """Test stacktrace link generation for Python SDK path""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} + self.check_file_patcher.stop() + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/app/services/processor.py"}, + ) + self.check_file_patcher.start() ctx: StacktraceLinkContext = { "file": "depot/app/services/processor.py", "filename": "depot/app/services/processor.py", @@ -373,7 +378,12 @@ def tearDown(self): def test_stacktrace_link_empty_stack_root(self): """Test stacktrace link with empty stack_root (shouldn't match anything)""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} + self.check_file_patcher.stop() + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/app/services/processor.py"}, + ) + self.check_file_patcher.start() repo = Repository.objects.create( name="//depot", organization_id=self.organization.id,