From 3915d4a9638760f182c0d880d02e39dad3d1011c Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 01/31] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 6 +++--- src/sentry/integrations/perforce/integration.py | 2 +- src/sentry/integrations/perforce/repository.py | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 52058ce5ece2b9..21a4f777df8881 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -82,7 +82,7 @@ def get_depots(self) -> list[dict[str, Any]]: Returns: List of depot info dictionaries """ - return [] + return None def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None @@ -98,7 +98,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return [] + return None def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -114,7 +114,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 8313cb2ed6d963..1432c7afc1cfa5 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -162,7 +162,7 @@ def get_repositories( Returns: List of repository dictionaries """ - return [] + return None def has_repo_access(self, repo: RpcRepository) -> bool: """Check if integration can access the depot.""" diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..0b1170954b3b89 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + return None def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + return None def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + return None def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From 90562acdc4fab02102bb30c916758adde2ba61f1 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 02/31] Fix typing --- src/sentry/integrations/perforce/client.py | 15 ++++++++++++--- src/sentry/integrations/perforce/integration.py | 2 +- src/sentry/integrations/perforce/repository.py | 6 +++--- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 21a4f777df8881..ded8173be3ce18 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -82,7 +82,7 @@ def get_depots(self) -> list[dict[str, Any]]: Returns: List of depot info dictionaries """ - return None + return [] def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None @@ -98,7 +98,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return None + return [] def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -114,7 +114,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 1432c7afc1cfa5..8313cb2ed6d963 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -162,7 +162,7 @@ def get_repositories( Returns: List of repository dictionaries """ - return None + return [] def has_repo_access(self, repo: RpcRepository) -> bool: """Check if integration can access the depot.""" diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 0b1170954b3b89..b17bb24d42a5a7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return None + return {} def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return None + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return None + return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From d05cc0cf366ea7064726d8fa645124d538396784 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 03/31] 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 | 305 +++++++++++- .../integrations/perforce/integration.py | 356 ++++++++++++-- .../integrations/perforce/repository.py | 122 ++++- src/sentry/tasks/post_process.py | 1 + .../sentry/integrations/perforce/__init__.py | 1 + .../perforce/test_code_mapping.py | 435 +++++++++++++++++ .../integrations/perforce/test_integration.py | 404 ++++++++++++++++ .../perforce/test_stacktrace_link.py | 450 ++++++++++++++++++ 8 files changed, 2013 insertions(+), 61 deletions(-) create mode 100644 tests/sentry/integrations/perforce/__init__.py create mode 100644 tests/sentry/integrations/perforce/test_code_mapping.py create mode 100644 tests/sentry/integrations/perforce/test_integration.py create mode 100644 tests/sentry/integrations/perforce/test_stacktrace_link.py diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index ded8173be3ce18..46863fb16fea5e 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -2,18 +2,22 @@ import logging from collections.abc import Sequence +from datetime import datetime, timezone from typing import Any from P4 import P4, P4Exception 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 +from sentry.shared_integrations.exceptions import ApiError +from sentry.utils import metrics logger = logging.getLogger(__name__) @@ -50,16 +54,55 @@ def __init__( 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 with SSL support.""" - pass + """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.""" - pass + try: + if p4.connected(): + p4.disconnect() + except Exception: + pass def check_file(self, repo: Repository, path: str, version: str | None) -> object | None: """ @@ -73,7 +116,134 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object Returns: File info dict if exists, None otherwise """ - return None + 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) def get_depots(self) -> list[dict[str, Any]]: """ @@ -82,7 +252,19 @@ def get_depots(self) -> list[dict[str, Any]]: Returns: List of depot info dictionaries """ - return [] + 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) def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None @@ -98,7 +280,31 @@ def get_changes( Returns: List of changelist dictionaries """ - return [] + p4 = self._connect() + try: + args = ["-m", str(max_changes), "-l"] + + if start_cl: + args.extend(["-e", start_cl]) + + args.append(depot_path) + + changes = p4.run("changes", *args) + + 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) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -114,16 +320,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 8313cb2ed6d963..d66814d37ef62e 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -21,7 +21,7 @@ from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView -from sentry.shared_integrations.exceptions import ApiError +from sentry.shared_integrations.exceptions import ApiError, IntegrationError logger = logging.getLogger(__name__) @@ -84,8 +84,30 @@ def __init__( def get_client(self) -> PerforceClient: """Get the Perforce client instance.""" - if self._client is None: - self._client = PerforceClient() + if self._client is not None: + return self._client + + if not self.model: + raise IntegrationError("Integration model not found") + + metadata = self.model.metadata + auth_mode = metadata.get("auth_mode", "password") + + if auth_mode == "ticket": + # Ticket authentication mode + self._client = PerforceClient( + ticket=metadata.get("ticket"), + client=metadata.get("client"), + ) + else: + # Password authentication mode + self._client = PerforceClient( + host=metadata.get("host", "localhost"), + port=metadata.get("port", 1666), + user=metadata.get("user", ""), + password=metadata.get("password"), + client=metadata.get("client"), + ) return self._client def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: @@ -97,8 +119,39 @@ def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: s def source_url_matches(self, url: str) -> bool: """Check if URL is from this Perforce server.""" + if url.startswith("p4://"): + return True + + web_url = self.model.metadata.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. @@ -113,7 +166,15 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) Returns: Formatted URL if the file exists, None otherwise """ - return None + 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("//"): + if not filepath.startswith(depot_path): + return None + + # For matching paths, return the formatted URL + return self.format_source_url(repo, filepath, branch) def format_source_url(self, repo: Repository, filepath: str, branch: str | None) -> str: """ @@ -131,7 +192,49 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) Returns: Formatted URL (p4:// or Swarm web viewer URL) """ - return "" + # Handle absolute depot paths (from SRCSRV transformer) + if filepath.startswith("//"): + full_path = filepath + else: + # Relative path - prepend depot_path + depot_path = repo.config.get("depot_path", repo.name).rstrip("/") + + # Handle Perforce streams: if branch/stream is specified, insert after depot + # Format: //depot/stream/path/to/file + if branch: + # depot_path is like "//depot", branch is like "main" + # Result should be "//depot/main/path/to/file" + full_path = f"{depot_path}/{branch}/{filepath.lstrip('/')}" + else: + filepath = filepath.lstrip("/") + full_path = f"{depot_path}/{filepath}" + + # If web viewer is configured, use it + web_url = self.model.metadata.get("web_url") + if web_url: + viewer_type = self.model.metadata.get("web_viewer_type", "p4web") + + # Extract revision from filepath if present (e.g., "file.cpp@42") + revision = None + path_without_rev = full_path + if "@" in full_path: + path_without_rev, revision = full_path.rsplit("@", 1) + + if viewer_type == "swarm": + # Swarm format: /files/?v= + if revision: + return f"{web_url}/files{path_without_rev}?v={revision}" + return f"{web_url}/files{full_path}" + elif viewer_type == "p4web": + # P4Web format: ?ac=64&rev1= + if revision: + return f"{web_url}{path_without_rev}?ac=64&rev1={revision}" + return f"{web_url}{full_path}?ac=64" + + # Default: p4:// protocol URL + # Perforce uses @ for revisions, which is already in the filepath from Symbolic + # Strip leading // from full_path to avoid p4://// + return f"p4://{full_path.lstrip('/')}" def extract_branch_from_source_url(self, repo: Repository, url: str) -> str: """ @@ -151,7 +254,25 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str Returns just the file path without revision info. """ - return "" + depot_path = repo.config.get("depot_path", repo.name) + + # Remove p4:// prefix + if url.startswith("p4://"): + url = url[5:] + + # Remove revision specifier (#revision) + if "#" in url: + url = url.split("#")[0] + + # Remove query parameters (for web viewers) + if "?" in url: + url = url.split("?")[0] + + # Remove depot prefix to get relative path + if url.startswith(depot_path): + return url[len(depot_path) :].lstrip("/") + + return url def get_repositories( self, query: str | None = None, page_number_limit: int | None = None @@ -162,16 +283,82 @@ def get_repositories( Returns: List of repository dictionaries """ - return [] + try: + client = self.get_client() + depots = client.get_depots() + + repositories = [] + for depot in depots: + depot_name = depot["name"] + depot_path = f"//{depot_name}" + + # Filter by query if provided + if query and query.lower() not in depot_name.lower(): + continue + + repositories.append( + { + "name": depot_name, + "identifier": depot_path, + "default_branch": None, # Perforce uses depot paths, not branch refs + } + ) + + return repositories + + except ApiError: + # Re-raise authentication/connection errors so user sees them + raise + except Exception as e: + logger.exception("perforce.get_repositories_failed") + raise IntegrationError(f"Failed to fetch repositories from Perforce: {str(e)}") def has_repo_access(self, repo: RpcRepository) -> bool: """Check if integration can access the depot.""" - return False + try: + client = self.get_client() + depot_path = repo.config.get("depot_path", repo.name) + + # Try to list files to verify access + result = client.check_file( + repo=type("obj", (object,), {"config": {"depot_path": depot_path}})(), + path="...", + version=None, + ) + + return result is not None + + except Exception: + return False 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. @@ -179,36 +366,62 @@ 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": "password", "type": "secret", - "label": "Password or P4 Ticket", + "label": "Perforce Password", "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.", - "required": True, - }, - { - "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", + "help": "Password for the Perforce user", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -218,16 +431,82 @@ 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, }, ] + def update_organization_config(self, data: MutableMapping[str, Any]) -> None: + """ + Update organization config and optionally validate credentials. + Only tests connection when password or ticket is changed to avoid annoying + validations on every field blur. + """ + from sentry.integrations.services.integration import integration_service + + # Check if credentials are being updated + password_changed = "password" in data + ticket_changed = "ticket" in data + credentials_changed = password_changed or ticket_changed + + # First update the integration metadata with new credentials + if self.model: + metadata = dict(self.model.metadata or {}) + + # Update metadata with any provided fields + for key in [ + "auth_mode", + "host", + "port", + "user", + "password", + "ticket", + "client", + "web_url", + "web_viewer_type", + ]: + if key in data: + metadata[key] = data[key] + + integration_service.update_integration(integration_id=self.model.id, metadata=metadata) + + # Clear cached client when credentials change + if credentials_changed: + self._client = None + + # Only test connection if password or ticket was changed + if credentials_changed: + try: + result = self.test_connection() + if result["status"] != "success": + raise IntegrationError(f"Connection test failed: {result['message']}") + except Exception as e: + logger.exception("perforce.credentials_validation_failed") + raise IntegrationError( + f"Failed to connect to Perforce server with provided credentials: {str(e)}" + ) + + # Call parent to update org integration config + super().update_organization_config(data) + class PerforceIntegrationProvider(IntegrationProvider): """Provider for Perforce integration.""" @@ -258,21 +537,18 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: Returns: Integration data dictionary """ - # Use p4port if available, otherwise fall back to host:port for legacy - p4port = ( - state.get("p4port") or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" - ) - return { - "name": state.get("name", f"Perforce ({p4port})"), - "external_id": p4port, + "name": state.get("name", f"Perforce ({state['host']})"), + "external_id": f"{state['host']}:{state['port']}", "metadata": { - "p4port": p4port, - "user": state.get("user"), + "host": state["host"], + "port": state["port"], + "user": state["user"], "password": state.get("password"), "client": state.get("client"), "ssl_fingerprint": state.get("ssl_fingerprint"), "web_url": state.get("web_url"), + "web_viewer_type": state.get("web_viewer_type", "p4web"), }, } @@ -314,4 +590,10 @@ def dispatch(self, request, pipeline): request: HTTP request object pipeline: Installation pipeline """ + # Set some default values that users will configure later + pipeline.bind_state("host", "localhost") + pipeline.bind_state("port", "1666") + pipeline.bind_state("user", "") + pipeline.bind_state("name", "Perforce Integration") + return pipeline.next_step() diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..64e223e39976cd 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,12 +4,14 @@ from collections.abc import Mapping, Sequence from typing import Any +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 sentry.plugins.providers.integration_repository import RepositoryConfig +from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -33,7 +35,41 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + installation = self.get_installation(config.get("installation"), organization.id) + client = installation.get_client() + + depot_path = config["identifier"] # e.g., //depot or //depot/project + + # Validate depot exists and is accessible + try: + # Create a minimal repo-like object for client + class MockRepo: + def __init__(self, depot_path): + self.config = {"depot_path": depot_path} + + mock_repo = MockRepo(depot_path) + + # Try to check depot access + result = client.check_file(mock_repo, "...", None) + + if result is None: + # Try getting depot info + depots = client.get_depots() + depot_name = depot_path.strip("/").split("/")[0] + + if not any(d["name"] == depot_name for d in depots): + raise IntegrationError( + f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" + ) + + except Exception: + # Don't fail - depot might be valid but empty + pass + + config["external_id"] = depot_path + config["integration_id"] = installation.model.id + + return config def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -48,12 +84,17 @@ def build_repository_config( Returns: Repository configuration """ + depot_path = data["identifier"] + return { - "name": "", - "external_id": "", - "url": "", - "config": {}, - "integration_id": 0, + "name": depot_path, + "external_id": data["external_id"], + "url": f"p4://{depot_path}", + "config": { + "depot_path": depot_path, + "name": depot_path, + }, + "integration_id": data["integration_id"], } def compare_commits( @@ -70,7 +111,42 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + integration_id = repo.integration_id + if integration_id is None: + raise NotImplementedError("Perforce integration requires an integration_id") + + integration = integration_service.get_integration(integration_id=integration_id) + if integration is None: + raise NotImplementedError("Integration not found") + + installation = integration.get_installation(organization_id=repo.organization_id) + client = installation.get_client() + + depot_path = repo.config.get("depot_path", repo.name) + + try: + # Get changelists in range + if start_sha is None: + # Get last N changes + changes = client.get_changes(f"{depot_path}/...", max_changes=20) + else: + # Get changes between start and end + # P4 doesn't have native compare, so get changes up to end_sha + changes = client.get_changes(f"{depot_path}/...", max_changes=100, start_cl=end_sha) + + # Filter to only changes after start_sha + if changes: + start_cl_num = int(start_sha) if start_sha.isdigit() else 0 + changes = [c for c in changes if int(c["change"]) > start_cl_num] + + return self._format_commits(changes, depot_path) + + except Exception as e: + logger.exception( + "perforce.compare_commits.error", + extra={"repo": repo.name, "start": start_sha, "end": end_sha, "error": str(e)}, + ) + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,14 +161,42 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + commits = [] + + for cl in changelists: + # Format timestamp (P4 time is Unix timestamp) + timestamp = self.format_date(int(cl["time"])) + + commits.append( + { + "id": str(cl["change"]), # Changelist number as commit ID + "repository": depot_path, + "author_email": f"{cl['user']}@perforce", # P4 doesn't store email + "author_name": cl["user"], + "message": cl["desc"], + "timestamp": timestamp, + "patch_set": [], # Could fetch with 'p4 describe' if needed + } + ) + + return commits def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. Perforce doesn't have native PRs, but might integrate with Swarm. """ - return "" + web_url = None + if repo.integration_id: + integration = integration_service.get_integration(integration_id=repo.integration_id) + if integration: + web_url = integration.metadata.get("web_url") + + if web_url: + # Swarm review URL format + return f"{web_url}/reviews/{pull_request.key}" + + return f"p4://{repo.name}@{pull_request.key}" def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index ba3e48a743a35a..ad2ab5c96cda0c 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1150,6 +1150,7 @@ def process_commits(job: PostProcessJob) -> None: IntegrationProviderSlug.GITHUB.value, IntegrationProviderSlug.GITLAB.value, IntegrationProviderSlug.GITHUB_ENTERPRISE.value, + IntegrationProviderSlug.PERFORCE.value, ], ) has_integrations = len(org_integrations) > 0 diff --git a/tests/sentry/integrations/perforce/__init__.py b/tests/sentry/integrations/perforce/__init__.py new file mode 100644 index 00000000000000..b13e3ff4975bb9 --- /dev/null +++ b/tests/sentry/integrations/perforce/__init__.py @@ -0,0 +1 @@ +# Perforce integration tests diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py new file mode 100644 index 00000000000000..cd8cd25ed5981f --- /dev/null +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -0,0 +1,435 @@ +from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig +from sentry.integrations.perforce.integration import ( + PerforceIntegration, + PerforceIntegrationProvider, +) +from sentry.issues.auto_source_code_config.code_mapping import ( + convert_stacktrace_frame_path_to_source_path, +) +from sentry.models.repository import Repository +from sentry.testutils.cases import IntegrationTestCase +from sentry.utils.event_frames import EventFrame + + +class PerforceCodeMappingTest(IntegrationTestCase): + """Tests for code mapping integration with Perforce""" + + provider = PerforceIntegrationProvider + installation: PerforceIntegration + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.project = self.create_project(organization=self.organization) + self.org_integration = self.integration.organizationintegration_set.first() + assert self.org_integration is not None + + def test_code_mapping_depot_root_to_slash(self): + """ + Test code mapping: depot/ -> / + This is the correct mapping for Perforce where depot name is part of path. + """ + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Test Python SDK path: depot/app/services/processor.py + frame = EventFrame( + filename="depot/app/services/processor.py", + abs_path="depot/app/services/processor.py", + ) + + result = convert_stacktrace_frame_path_to_source_path( + frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" + ) + + # Should strip "depot/" leaving "app/services/processor.py" + assert result == "app/services/processor.py" + + 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. + """ + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Test C++ path from Symbolic: depot/game/src/main.cpp@42 + frame = EventFrame( + 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 "@42" + assert result == "game/src/main.cpp@42" + + def test_code_mapping_multiple_depots(self): + """Test code mappings for multiple depots (depot and myproject)""" + depot_repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + myproject_repo = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//myproject"}, + ) + + depot_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=depot_repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + myproject_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=myproject_repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="myproject/", + source_root="/", + default_branch=None, + ) + + # Test depot path + frame1 = EventFrame( + filename="depot/app/services/processor.py", + abs_path="depot/app/services/processor.py", + ) + + result1 = convert_stacktrace_frame_path_to_source_path( + frame=frame1, code_mapping=depot_mapping, platform="python", sdk_name="sentry.python" + ) + assert result1 == "app/services/processor.py" + + # Test myproject path + frame2 = EventFrame( + filename="myproject/app/services/handler.py", + abs_path="myproject/app/services/handler.py", + ) + + result2 = convert_stacktrace_frame_path_to_source_path( + frame=frame2, + code_mapping=myproject_mapping, + platform="python", + sdk_name="sentry.python", + ) + assert result2 == "app/services/handler.py" + + def test_code_mapping_no_match_different_depot(self): + """Test that code mapping doesn't match paths from different depots""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Try to match a path from different depot + frame = EventFrame( + filename="myproject/app/services/processor.py", + abs_path="myproject/app/services/processor.py", + ) + + result = convert_stacktrace_frame_path_to_source_path( + frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" + ) + + # Should not match + assert result is None + + def test_code_mapping_abs_path_fallback(self): + """Test that code mapping works with abs_path when filename is just basename""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Some platforms only provide basename in filename + frame = EventFrame(filename="processor.py", abs_path="depot/app/services/processor.py") + + result = convert_stacktrace_frame_path_to_source_path( + frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" + ) + + # Should use abs_path and strip "depot/" + assert result == "app/services/processor.py" + + def test_code_mapping_nested_depot_paths(self): + """Test code mapping with nested depot paths""" + repo = Repository.objects.create( + name="//depot/game/project", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot/game/project"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/game/project/", + source_root="/", + default_branch=None, + ) + + frame = EventFrame( + filename="depot/game/project/src/main.cpp", + abs_path="depot/game/project/src/main.cpp", + ) + + result = convert_stacktrace_frame_path_to_source_path( + frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" + ) + + assert result == "src/main.cpp" + + def test_code_mapping_preserves_windows_backslash_conversion(self): + """ + Test that code mapping handles Windows-style paths. + + Note: The generic code_mapping system does not automatically convert + backslashes to forward slashes. SDKs should normalize paths before + sending to Sentry. This test documents current behavior. + """ + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Windows-style path with backslashes + frame = EventFrame( + filename="depot\\app\\services\\processor.cpp", + abs_path="depot\\app\\services\\processor.cpp", + ) + + result = convert_stacktrace_frame_path_to_source_path( + frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" + ) + + # Generic code mapping doesn't normalize backslashes - returns None + # SDKs should normalize paths to forward slashes before sending + assert result is None + + +class PerforceEndToEndCodeMappingTest(IntegrationTestCase): + """End-to-end tests for code mapping + format_source_url flow""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.project = self.create_project(organization=self.organization) + self.org_integration = self.integration.organizationintegration_set.first() + assert self.org_integration is not None + + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + self.code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=self.repo, + organization_integration_id=self.org_integration.id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + def test_python_sdk_path_full_flow(self): + """Test full flow: Python SDK -> code mapping -> format_source_url""" + # 1. Python SDK sends this path + frame = EventFrame( + filename="depot/app/services/processor.py", + abs_path="depot/app/services/processor.py", + ) + + # 2. Code mapping transforms it + mapped_path = convert_stacktrace_frame_path_to_source_path( + frame=frame, + code_mapping=self.code_mapping, + platform="python", + sdk_name="sentry.python", + ) + assert mapped_path == "app/services/processor.py" + + # 3. format_source_url creates final URL + url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) + assert url == "p4://depot/app/services/processor.py" + + 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" + ) + + # 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" + + # 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@42" + + 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={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + + # Create repo with web viewer integration + repo_web = Repository.objects.create( + name="//depot", + organization_id=self.organization.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_web = self.create_project(organization=self.organization) + + org_integration_web = integration_with_web.organizationintegration_set.first() + assert org_integration_web is not None + + code_mapping_web = RepositoryProjectPathConfig.objects.create( + project=project_web, + organization_id=self.organization.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, + ) + + # Python SDK path with @revision from Symbolic + frame = EventFrame( + filename="depot/app/services/processor.py@42", + abs_path="depot/app/services/processor.py@42", + ) + + # Code mapping + mapped_path = convert_stacktrace_frame_path_to_source_path( + frame=frame, + code_mapping=code_mapping_web, + platform="python", + sdk_name="sentry.python", + ) + + # format_source_url with web viewer (revision extracted from filename) + assert mapped_path is not None + url = installation.format_source_url(repo=repo_web, filepath=mapped_path, branch=None) + + assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64&rev1=42" diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py new file mode 100644 index 00000000000000..e917ebc52a6cd3 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -0,0 +1,404 @@ +from sentry.integrations.perforce.integration import ( + PerforceIntegration, + PerforceIntegrationProvider, +) +from sentry.models.repository import Repository +from sentry.testutils.cases import IntegrationTestCase + + +class PerforceIntegrationTest(IntegrationTestCase): + provider = PerforceIntegrationProvider + installation: PerforceIntegration + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_format_source_url_absolute_path(self): + """Test formatting URL with absolute depot path""" + url = self.installation.format_source_url( + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_relative_path(self): + """Test formatting URL with relative path - should prepend depot_path""" + url = self.installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_with_revision_in_filename(self): + """ + Test formatting URL with revision in filename (from Symbolic transformer). + Perforce uses @ for revisions, which should be preserved. + """ + url = self.installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp@42" + + def test_format_source_url_p4web_viewer(self): + """Test formatting URL for P4Web viewer with revision in filename""" + integration_with_web = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-web", + metadata={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + ) + assert url == "https://p4web.example.com//depot/app/services/processor.cpp?ac=64&rev1=42" + + def test_format_source_url_p4web_viewer_no_revision(self): + """Test formatting URL for P4Web viewer without revision""" + integration_with_web = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-web2", + metadata={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp", branch=None + ) + assert url == "https://p4web.example.com//depot/app/services/processor.cpp?ac=64" + + def test_format_source_url_swarm_viewer(self): + """Test formatting URL for Swarm viewer with revision""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm", + metadata={ + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=42" + + def test_format_source_url_swarm_viewer_no_revision(self): + """Test formatting URL for Swarm viewer without revision""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm2", + metadata={ + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + + def test_format_source_url_strips_leading_slash_from_relative_path(self): + """Test that leading slash is stripped from relative paths""" + url = self.installation.format_source_url( + repo=self.repo, filepath="/app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_uses_repo_name_as_fallback_depot(self): + """Test that repo name is used as depot_path fallback""" + repo_without_config = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={}, + ) + + url = self.installation.format_source_url( + repo=repo_without_config, filepath="app/services/processor.cpp", branch=None + ) + assert url == "p4://myproject/app/services/processor.cpp" + + def test_check_file_absolute_depot_path(self): + """Test check_file with absolute depot path""" + assert self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") + + def test_check_file_relative_path(self): + """Test check_file with relative path""" + assert self.installation.check_file(self.repo, "app/services/processor.cpp") + + def test_check_file_with_revision_syntax(self): + """Test check_file with @revision syntax""" + assert self.installation.check_file(self.repo, "app/services/processor.cpp@42") + + def test_get_stacktrace_link(self): + """Test get_stacktrace_link returns format_source_url result""" + filepath = "app/services/processor.cpp@42" + default_branch = "" # Perforce doesn't require default_branch (used for streams) + version = None + + url = self.installation.get_stacktrace_link(self.repo, filepath, default_branch, version) + # URL will be encoded by get_stacktrace_link + assert url == "p4://depot/app/services/processor.cpp%4042" + assert "%40" in url # @ gets URL-encoded to %40 + + def test_integration_name(self): + """Test integration has correct name""" + assert self.installation.model.name == "Perforce" + + def test_integration_provider(self): + """Test integration has correct provider""" + assert self.installation.model.provider == "perforce" + + +class PerforceIntegrationCodeMappingTest(IntegrationTestCase): + """Tests for Perforce integration with code mappings""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_format_source_url_from_code_mapping_output(self): + """ + Test that paths coming from code mapping (depot/ -> /) work correctly. + Code mapping strips 'depot/' and leaves 'app/services/processor.cpp', + which should be prepended with depot_path. + """ + # This is what code mapping would output after stripping "depot/" + code_mapped_path = "app/services/processor.cpp" + + url = self.installation.format_source_url( + repo=self.repo, filepath=code_mapped_path, branch=None + ) + + # Should prepend depot_path to create full depot path + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_preserves_revision_in_filename(self): + """ + Test that @revision syntax in filename is preserved. + This tests the Symbolic transformer output format. + """ + # This is what Symbolic transformer outputs + symbolic_path = "app/services/processor.cpp@42" + + url = self.installation.format_source_url( + repo=self.repo, filepath=symbolic_path, branch=None + ) + + # The @42 should be preserved in the path + assert url == "p4://depot/app/services/processor.cpp@42" + + def test_format_source_url_python_path_without_revision(self): + """Test Python SDK paths without revision""" + # Python SDK typically doesn't include revisions + python_path = "app/services/processor.py" + + url = self.installation.format_source_url(repo=self.repo, filepath=python_path, branch=None) + + assert url == "p4://depot/app/services/processor.py" + + +class PerforceIntegrationDepotPathTest(IntegrationTestCase): + """Tests for depot path handling in different scenarios""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + ) + self.installation = self.integration.get_installation(self.organization.id) + + def test_multiple_depots(self): + """Test handling multiple depot configurations""" + depot_repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + myproject_repo = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//myproject"}, + ) + + # Test depot + url1 = self.installation.format_source_url( + repo=depot_repo, filepath="app/services/processor.cpp", branch=None + ) + assert url1 == "p4://depot/app/services/processor.cpp" + + # Test myproject + url2 = self.installation.format_source_url( + repo=myproject_repo, filepath="app/services/processor.cpp", branch=None + ) + assert url2 == "p4://myproject/app/services/processor.cpp" + + def test_nested_depot_paths(self): + """Test handling nested depot paths""" + repo = Repository.objects.create( + name="//depot/game/project", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot/game/project"}, + ) + + url = self.installation.format_source_url(repo=repo, filepath="src/main.cpp", branch=None) + assert url == "p4://depot/game/project/src/main.cpp" + + def test_depot_path_with_trailing_slash(self): + """Test depot_path with trailing slash is handled correctly""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot/"}, + ) + + url = self.installation.format_source_url( + repo=repo, filepath="app/services/processor.cpp", branch=None + ) + # Should not have double slashes in path portion + assert url == "p4://depot/app/services/processor.cpp" + + +class PerforceIntegrationWebViewersTest(IntegrationTestCase): + """Tests for web viewer URL formatting""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_p4web_extracts_revision_from_filename(self): + """Test P4Web viewer correctly extracts and formats revision from @revision syntax""" + integration_with_web = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-p4web", + metadata={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + + # Filename with revision + url = installation.format_source_url( + repo=self.repo, filepath="game/src/main.cpp@42", branch=None + ) + + # Should extract @42 and use it as rev1 parameter + assert url == "https://p4web.example.com//depot/game/src/main.cpp?ac=64&rev1=42" + + def test_swarm_extracts_revision_from_filename(self): + """Test Swarm viewer correctly extracts and formats revision from @revision syntax""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm3", + metadata={ + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + # Filename with revision + url = installation.format_source_url( + repo=self.repo, filepath="game/src/main.cpp@42", branch=None + ) + + # Should extract @42 and use it as v parameter + assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=42" + + def test_web_viewer_with_python_path_no_revision(self): + """Test web viewers work correctly without revision""" + integration_with_web = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-p4web2", + metadata={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + + # Python path without revision + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.py", branch=None + ) + + # Should not have revision parameter + assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64" + assert "rev1=" not in url diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py new file mode 100644 index 00000000000000..acf9b50595a7a4 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -0,0 +1,450 @@ +from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig +from sentry.integrations.perforce.integration import PerforceIntegrationProvider +from sentry.integrations.utils.stacktrace_link import get_stacktrace_config +from sentry.issues.endpoints.project_stacktrace_link import StacktraceLinkContext +from sentry.models.repository import Repository +from sentry.testutils.cases import IntegrationTestCase + + +class PerforceStacktraceLinkTest(IntegrationTestCase): + """Tests for Perforce stacktrace link generation""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.project = self.create_project(organization=self.organization) + + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + self.code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=self.repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + def test_get_stacktrace_config_python_path(self): + """Test stacktrace link generation for Python SDK path""" + ctx: StacktraceLinkContext = { + "file": "depot/app/services/processor.py", + "filename": "depot/app/services/processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + assert "//depot/app/services/processor.py" in result["source_url"] + assert result["error"] is None + assert result["src_path"] == "app/services/processor.py" + + def test_get_stacktrace_config_cpp_path_with_revision(self): + """Test stacktrace link generation for C++ path with @revision""" + ctx: StacktraceLinkContext = { + "file": "depot/game/src/main.cpp@42", + "filename": "depot/game/src/main.cpp@42", + "abs_path": "depot/game/src/main.cpp@42", + "platform": "native", + "sdk_name": "sentry.native", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + # URL will be encoded: p4://depot/game/src/main.cpp%4042 + assert "depot/game/src/main.cpp" in result["source_url"] + assert result["error"] is None + assert result["src_path"] == "game/src/main.cpp@42" + + def test_get_stacktrace_config_no_matching_code_mapping(self): + """Test stacktrace link when no code mapping matches""" + ctx: StacktraceLinkContext = { + "file": "other/app/services/processor.py", + "filename": "other/app/services/processor.py", + "abs_path": "other/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping], ctx) + + assert result["source_url"] is None + assert result["error"] == "stack_root_mismatch" + assert result["src_path"] is None + + def test_get_stacktrace_config_multiple_code_mappings(self): + """Test stacktrace link with multiple code mappings""" + # Add another depot mapping + myproject_repo = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//myproject"}, + ) + + myproject_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=myproject_repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="myproject/", + source_root="/", + default_branch=None, + ) + + # Test with myproject path + ctx: StacktraceLinkContext = { + "file": "myproject/app/services/handler.py", + "filename": "myproject/app/services/handler.py", + "abs_path": "myproject/app/services/handler.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + 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): + """Test stacktrace link with P4Web viewer""" + integration_with_web = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-web-link", + metadata={ + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + }, + ) + + # Create new code mapping with new integration + # Use different project to avoid unique constraint + project_web = self.create_project(organization=self.organization) + + repo_web = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=integration_with_web.id, + config={"depot_path": "//depot"}, + ) + + org_integration = integration_with_web.organizationintegration_set.first() + assert org_integration is not None + code_mapping_web = RepositoryProjectPathConfig.objects.create( + project=project_web, + organization_id=self.organization.id, + repository=repo_web, + organization_integration_id=org_integration.id, + integration_id=integration_with_web.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + ctx: StacktraceLinkContext = { + "file": "depot/app/services/processor.py", + "filename": "depot/app/services/processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([code_mapping_web], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + assert "https://p4web.example.com//depot/app/services/processor.py" in result["source_url"] + + def test_get_stacktrace_config_abs_path_fallback(self): + """Test stacktrace link uses abs_path when filename is just basename""" + ctx: StacktraceLinkContext = { + "file": "processor.py", + "filename": "processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + 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): + """Test that iteration_count is incremented only for matching mappings""" + # Add a non-matching mapping + other_repo = Repository.objects.create( + name="//other", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//other"}, + ) + + other_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=other_repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="other/", + source_root="/", + default_branch=None, + ) + + ctx: StacktraceLinkContext = { + "file": "depot/app/services/processor.py", + "filename": "depot/app/services/processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([other_mapping, self.code_mapping], ctx) + + # iteration_count should be 1 (only depot mapping matched) + assert result["iteration_count"] == 1 + assert result["source_url"] is not None + + def test_get_stacktrace_config_stops_on_first_match(self): + """Test that iteration stops after first successful match""" + # 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) + + myproject_repo = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//myproject"}, + ) + + myproject_mapping = RepositoryProjectPathConfig.objects.create( + project=project2, + organization_id=self.organization.id, + repository=myproject_repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="depot/", # Same stack_root as depot mapping (but different project) + source_root="/", + default_branch=None, + ) + + ctx: StacktraceLinkContext = { + "file": "depot/app/services/processor.py", + "filename": "depot/app/services/processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) + + # Should stop after first match (depot mapping) + assert result["iteration_count"] == 1 + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + assert "//depot/app/services/processor.py" in result["source_url"] + + +class PerforceStacktraceLinkEdgeCasesTest(IntegrationTestCase): + """Edge case tests for Perforce stacktrace links""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.project = self.create_project(organization=self.organization) + + def test_stacktrace_link_empty_stack_root(self): + """Test stacktrace link with empty stack_root (shouldn't match anything)""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="", + source_root="/", + default_branch=None, + ) + + ctx: StacktraceLinkContext = { + "file": "depot/app/services/processor.py", + "filename": "depot/app/services/processor.py", + "abs_path": "depot/app/services/processor.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([code_mapping], ctx) + + # Empty stack_root should match any path + assert result["source_url"] is not None + + def test_stacktrace_link_with_special_characters_in_path(self): + """Test stacktrace link with special characters in file path""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + # Path with spaces and special chars + ctx: StacktraceLinkContext = { + "file": "depot/app/my services/processor-v2.py", + "filename": "depot/app/my services/processor-v2.py", + "abs_path": "depot/app/my services/processor-v2.py", + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([code_mapping], ctx) + + 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): + """Test stacktrace link with very deeply nested path""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + code_mapping = RepositoryProjectPathConfig.objects.create( + project=self.project, + organization_id=self.organization.id, + repository=repo, + organization_integration_id=self.integration.organizationintegration_set.first().id, + integration_id=self.integration.id, + stack_root="depot/", + source_root="/", + default_branch=None, + ) + + deep_path = "depot/" + "/".join([f"level{i}" for i in range(20)]) + "/file.py" + + ctx: StacktraceLinkContext = { + "file": deep_path, + "filename": deep_path, + "abs_path": deep_path, + "platform": "python", + "sdk_name": "sentry.python", + "commit_id": None, + "group_id": None, + "line_no": None, + "module": None, + "package": None, + } + + result = get_stacktrace_config([code_mapping], ctx) + + assert result["source_url"] is not None + assert isinstance(result["source_url"], str) + assert "//depot/" in result["source_url"] From cdb12e5ee4eaf0f1399453a709cc424809edda08 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 13:10:55 +0100 Subject: [PATCH 04/31] 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 54335704b2d8a87ae2940e5345be0202a1b80bb1 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:22:47 +0100 Subject: [PATCH 05/31] Code cleanup --- src/sentry/integrations/perforce/client.py | 89 +--------- .../integrations/perforce/integration.py | 157 ++++-------------- 2 files changed, 33 insertions(+), 213 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 46863fb16fea5e..c43a10a6b53f8b 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -158,93 +158,6 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) 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) - def get_depots(self) -> list[dict[str, Any]]: """ List all depots accessible to the user. @@ -337,7 +250,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 d66814d37ef62e..a962c74807b238 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -87,26 +87,27 @@ def get_client(self) -> PerforceClient: if self._client is not None: return self._client - if not self.model: - raise IntegrationError("Integration model not found") + if not self.org_integration: + raise IntegrationError("Organization Integration not found") - metadata = self.model.metadata - auth_mode = metadata.get("auth_mode", "password") + # Credentials are stored in org_integration.config (per-organization) + config = self.org_integration.config + auth_mode = config.get("auth_mode", "password") if auth_mode == "ticket": # Ticket authentication mode self._client = PerforceClient( - ticket=metadata.get("ticket"), - client=metadata.get("client"), + ticket=config.get("ticket"), + client=config.get("client"), ) else: # Password authentication mode self._client = PerforceClient( - host=metadata.get("host", "localhost"), - port=metadata.get("port", 1666), - user=metadata.get("user", ""), - password=metadata.get("password"), - client=metadata.get("client"), + host=config.get("host", "localhost"), + port=config.get("port", 1666), + user=config.get("user", ""), + password=config.get("password"), + client=config.get("client"), ) return self._client @@ -122,36 +123,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. @@ -166,15 +144,17 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) Returns: Formatted URL if the file exists, None otherwise """ - 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("//"): - if not filepath.startswith(depot_path): + try: + client = self.get_client() + # Use client's check_file to verify file exists on P4 server + result = client.check_file(repo, filepath, branch) + if result is None: return None - - # For matching paths, return the formatted URL - return self.format_source_url(repo, filepath, branch) + # File exists, return formatted URL + return self.format_source_url(repo, filepath, branch) + except Exception: + # If any error occurs (auth, connection, etc), return None + return None def format_source_url(self, repo: Repository, filepath: str, branch: str | None) -> str: """ @@ -210,9 +190,13 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) full_path = f"{depot_path}/{filepath}" # If web viewer is configured, use it - web_url = self.model.metadata.get("web_url") + web_url = None + viewer_type = "swarm" + if self.org_integration: + web_url = self.org_integration.config.get("web_url") + viewer_type = self.org_integration.config.get("web_viewer_type", "swarm") + if web_url: - viewer_type = self.model.metadata.get("web_viewer_type", "p4web") # Extract revision from filepath if present (e.g., "file.cpp@42") revision = None @@ -335,30 +319,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. @@ -454,59 +414,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: }, ] - def update_organization_config(self, data: MutableMapping[str, Any]) -> None: - """ - Update organization config and optionally validate credentials. - Only tests connection when password or ticket is changed to avoid annoying - validations on every field blur. - """ - from sentry.integrations.services.integration import integration_service - - # Check if credentials are being updated - password_changed = "password" in data - ticket_changed = "ticket" in data - credentials_changed = password_changed or ticket_changed - - # First update the integration metadata with new credentials - if self.model: - metadata = dict(self.model.metadata or {}) - - # Update metadata with any provided fields - for key in [ - "auth_mode", - "host", - "port", - "user", - "password", - "ticket", - "client", - "web_url", - "web_viewer_type", - ]: - if key in data: - metadata[key] = data[key] - - integration_service.update_integration(integration_id=self.model.id, metadata=metadata) - - # Clear cached client when credentials change - if credentials_changed: - self._client = None - - # Only test connection if password or ticket was changed - if credentials_changed: - try: - result = self.test_connection() - if result["status"] != "success": - raise IntegrationError(f"Connection test failed: {result['message']}") - except Exception as e: - logger.exception("perforce.credentials_validation_failed") - raise IntegrationError( - f"Failed to connect to Perforce server with provided credentials: {str(e)}" - ) - - # Call parent to update org integration config - super().update_organization_config(data) - class PerforceIntegrationProvider(IntegrationProvider): """Provider for Perforce integration.""" From 561407af79268476e2f1212c27a5f0b4949303e0 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:54:48 +0100 Subject: [PATCH 06/31] Fix pr comments and tests --- src/sentry/integrations/perforce/client.py | 10 ++- .../integrations/perforce/repository.py | 54 +++++++++---- .../integrations/perforce/test_integration.py | 81 +++++++++++++------ 3 files changed, 104 insertions(+), 41 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index c43a10a6b53f8b..bb2925e1d7a0f4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -265,10 +265,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, diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 64e223e39976cd..4a855398e9daec 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -137,7 +137,16 @@ def compare_commits( # Filter to only changes after start_sha if changes: start_cl_num = int(start_sha) if start_sha.isdigit() else 0 - changes = [c for c in changes if int(c["change"]) > start_cl_num] + # Filter out invalid changelist data + filtered_changes = [] + for c in changes: + try: + if int(c["change"]) > start_cl_num: + filtered_changes.append(c) + except (TypeError, ValueError, KeyError): + # Skip malformed changelist data + continue + changes = filtered_changes return self._format_commits(changes, depot_path) @@ -164,20 +173,35 @@ def _format_commits( commits = [] for cl in changelists: - # Format timestamp (P4 time is Unix timestamp) - timestamp = self.format_date(int(cl["time"])) - - commits.append( - { - "id": str(cl["change"]), # Changelist number as commit ID - "repository": depot_path, - "author_email": f"{cl['user']}@perforce", # P4 doesn't store email - "author_name": cl["user"], - "message": cl["desc"], - "timestamp": timestamp, - "patch_set": [], # Could fetch with 'p4 describe' if needed - } - ) + try: + # Handle potentially null/invalid time field + time_value = cl.get("time") or 0 + try: + time_int = int(time_value) + except (TypeError, ValueError): + time_int = 0 + + # Format timestamp (P4 time is Unix timestamp) + timestamp = self.format_date(time_int) + + commits.append( + { + "id": str(cl["change"]), # Changelist number as commit ID + "repository": depot_path, + "author_email": f"{cl.get('user', 'unknown')}@perforce", # P4 doesn't store email + "author_name": cl.get("user", "unknown"), + "message": cl.get("desc", ""), + "timestamp": timestamp, + "patch_set": [], # Could fetch with 'p4 describe' if needed + } + ) + except (KeyError, TypeError) as e: + # Skip malformed changelist data + logger.warning( + "perforce.format_commits.invalid_data", + extra={"changelist": cl, "error": str(e)}, + ) + continue return commits diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index e917ebc52a6cd3..335b3e5dc14b90 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from sentry.integrations.perforce.integration import ( PerforceIntegration, PerforceIntegrationProvider, @@ -58,9 +60,12 @@ def test_format_source_url_p4web_viewer(self): provider="perforce", name="Perforce", external_id="perforce-test-web", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] @@ -77,9 +82,12 @@ def test_format_source_url_p4web_viewer_no_revision(self): provider="perforce", name="Perforce", external_id="perforce-test-web2", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] @@ -96,9 +104,12 @@ def test_format_source_url_swarm_viewer(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm", - metadata={ - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", + metadata={}, + oi_params={ + "config": { + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + } }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -115,9 +126,12 @@ def test_format_source_url_swarm_viewer_no_revision(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm2", - metadata={ - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", + metadata={}, + oi_params={ + "config": { + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + } }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -148,20 +162,28 @@ def test_format_source_url_uses_repo_name_as_fallback_depot(self): ) assert url == "p4://myproject/app/services/processor.cpp" - def test_check_file_absolute_depot_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_absolute_depot_path(self, mock_check_file): """Test check_file with absolute depot path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} assert self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") - def test_check_file_relative_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_relative_path(self, mock_check_file): """Test check_file with relative path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} assert self.installation.check_file(self.repo, "app/services/processor.cpp") - def test_check_file_with_revision_syntax(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax(self, mock_check_file): """Test check_file with @revision syntax""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} assert self.installation.check_file(self.repo, "app/services/processor.cpp@42") - def test_get_stacktrace_link(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_link(self, mock_check_file): """Test get_stacktrace_link returns format_source_url result""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} filepath = "app/services/processor.cpp@42" default_branch = "" # Perforce doesn't require default_branch (used for streams) version = None @@ -343,9 +365,12 @@ def test_p4web_extracts_revision_from_filename(self): provider="perforce", name="Perforce", external_id="perforce-test-p4web", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] @@ -365,9 +390,12 @@ def test_swarm_extracts_revision_from_filename(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm3", - metadata={ - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", + metadata={}, + oi_params={ + "config": { + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", + } }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -387,9 +415,12 @@ def test_web_viewer_with_python_path_no_revision(self): provider="perforce", name="Perforce", external_id="perforce-test-p4web2", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] From 7ef4dbe74e0b811ecf53cdefa2e04200e4d5da24 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:16:38 +0100 Subject: [PATCH 07/31] Fix tests --- .../integrations/perforce/repository.py | 5 ++- .../perforce/test_code_mapping.py | 9 +++-- .../perforce/test_stacktrace_link.py | 39 ++++++++++++++----- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 4a855398e9daec..79ec38d9a1bff7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -62,8 +62,11 @@ def __init__(self, depot_path): f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" ) + except IntegrationError: + # Re-raise validation errors so user sees them + raise except Exception: - # Don't fail - depot might be valid but empty + # Catch only connection/P4 errors - depot might be valid but temporarily unreachable pass config["external_id"] = depot_path diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index cd8cd25ed5981f..dde8ce98c1a9bf 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -382,9 +382,12 @@ def test_full_flow_with_web_viewer(self): provider="perforce", name="Perforce", external_id="perforce-test-web-flow", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index acf9b50595a7a4..2809b9bf67e30b 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig from sentry.integrations.perforce.integration import PerforceIntegrationProvider from sentry.integrations.utils.stacktrace_link import get_stacktrace_config @@ -41,8 +43,10 @@ def setUp(self): default_branch=None, ) - def test_get_stacktrace_config_python_path(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_config_python_path(self, mock_check_file): """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", @@ -64,8 +68,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", @@ -109,8 +115,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", @@ -151,16 +159,21 @@ 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", name="Perforce", external_id="perforce-test-web-link", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + metadata={}, + oi_params={ + "config": { + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", + } }, ) @@ -331,8 +344,10 @@ def setUp(self): ) self.project = self.create_project(organization=self.organization) - def test_stacktrace_link_empty_stack_root(self): + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_stacktrace_link_empty_stack_root(self, mock_check_file): """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, @@ -369,8 +384,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, @@ -408,8 +425,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 02317950a34c9763fef93a536ed1c04688fb7489 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:47:27 +0100 Subject: [PATCH 08/31] Fix PR reviews and tests --- src/sentry/integrations/perforce/integration.py | 10 ++++++++-- .../integrations/perforce/test_stacktrace_link.py | 12 +++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index a962c74807b238..b76831cc4ee5fc 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -252,9 +252,15 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str if "?" in url: url = url.split("?")[0] + # Normalize both paths by stripping leading slashes for comparison + # depot_path is typically "//depot" from config + # url after stripping prefix is "depot/path/file.cpp" + normalized_depot = depot_path.lstrip("/") + normalized_url = url.lstrip("/") + # Remove depot prefix to get relative path - if url.startswith(depot_path): - return url[len(depot_path) :].lstrip("/") + if normalized_url.startswith(normalized_depot): + return normalized_url[len(normalized_depot) :].lstrip("/") return url diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 2809b9bf67e30b..1982e5c3e14ae6 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -220,8 +220,10 @@ def test_get_stacktrace_config_with_web_viewer(self, mock_check_file): assert isinstance(result["source_url"], str) assert "https://p4web.example.com//depot/app/services/processor.py" 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", @@ -242,8 +244,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", @@ -282,8 +286,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 faf14d88c1c5860928130446c4a813ac2f4504a4 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:52:40 +0100 Subject: [PATCH 09/31] 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 bb2925e1d7a0f4..97a82415eb00b5 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -274,7 +274,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 9c9d465caf97efe6aceaa6a225d747e66136972f Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 09:41:19 +0100 Subject: [PATCH 10/31] Remove P4Web (deprecated) and fix paths for swarm --- src/sentry/integrations/perforce/client.py | 53 ++++-- .../integrations/perforce/integration.py | 60 ++---- .../perforce/test_code_mapping.py | 41 ++-- .../integrations/perforce/test_integration.py | 180 +++++++++--------- .../perforce/test_stacktrace_link.py | 61 ------ 5 files changed, 157 insertions(+), 238 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 97a82415eb00b5..c6a3285c51e0bc 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -118,7 +118,7 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object """ p4 = self._connect() try: - depot_path = self._build_depot_path(repo, path) + depot_path = self.build_depot_path(repo, path) result = p4.run("files", depot_path) if result and len(result) > 0: @@ -134,27 +134,54 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) """ Build full depot path from repo config and file path. + Handles both relative and absolute paths: + - Relative: "depot/app/file.py" or "app/file.py" → "//depot/app/file.py" + - Absolute: "//depot/app/file.py" → "//depot/app/file.py" (unchanged) + - With branch/stream: "app/file.py" + branch="main" → "//depot/main/app/file.py" + 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) + branch: Optional branch/stream name to insert after depot (e.g., "main", "dev") Returns: Full depot path with @revision preserved if present """ - depot_root = repo.config.get("depot_path", repo.name).rstrip("/") + # Extract revision if present + revision = None + path_without_rev = path + if "@" in path: + path_without_rev, revision = path.rsplit("@", 1) + + # If already absolute depot path, use as-is + if path_without_rev.startswith("//"): + full_path = path_without_rev + else: + depot_root = repo.config.get("depot_path", repo.name).rstrip("/") + + # Normalize depot_root to ensure it starts with // + if not depot_root.startswith("//"): + depot_root = f"//{depot_root}" + + # Strip depot name from path if it duplicates depot_root + # e.g., depot_root="//depot", path="depot/app/file.py" → "app/file.py" + depot_name = depot_root.lstrip("/") # "depot" + if path_without_rev.startswith(depot_name + "/") or path_without_rev == depot_name: + path_without_rev = path_without_rev[len(depot_name) :].lstrip("/") - # Ensure path doesn't start with / - path = path.lstrip("/") + # Remove leading slashes from relative path + path_without_rev = path_without_rev.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}" + # Handle Perforce streams: insert branch/stream after depot + # Format: //depot/stream/path/to/file + if branch: + full_path = f"{depot_root}/{branch}/{path_without_rev}" + else: + full_path = f"{depot_root}/{path_without_rev}" - # 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}" + # Add revision back if present + if revision: + full_path = f"{full_path}@{revision}" return full_path @@ -242,7 +269,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 b76831cc4ee5fc..1812404eb750c3 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -172,48 +172,26 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) Returns: Formatted URL (p4:// or Swarm web viewer URL) """ - # Handle absolute depot paths (from SRCSRV transformer) - if filepath.startswith("//"): - full_path = filepath - else: - # Relative path - prepend depot_path - depot_path = repo.config.get("depot_path", repo.name).rstrip("/") - - # Handle Perforce streams: if branch/stream is specified, insert after depot - # Format: //depot/stream/path/to/file - if branch: - # depot_path is like "//depot", branch is like "main" - # Result should be "//depot/main/path/to/file" - full_path = f"{depot_path}/{branch}/{filepath.lstrip('/')}" - else: - filepath = filepath.lstrip("/") - full_path = f"{depot_path}/{filepath}" - - # If web viewer is configured, use it + # Use client's build_depot_path to handle both relative and absolute paths correctly + client = self.get_client() + full_path = client.build_depot_path(repo, filepath, branch) + + # If Swarm web viewer is configured, use it web_url = None - viewer_type = "swarm" if self.org_integration: web_url = self.org_integration.config.get("web_url") - viewer_type = self.org_integration.config.get("web_viewer_type", "swarm") if web_url: - # Extract revision from filepath if present (e.g., "file.cpp@42") revision = None path_without_rev = full_path if "@" in full_path: path_without_rev, revision = full_path.rsplit("@", 1) - if viewer_type == "swarm": - # Swarm format: /files/?v= - if revision: - return f"{web_url}/files{path_without_rev}?v={revision}" - return f"{web_url}/files{full_path}" - elif viewer_type == "p4web": - # P4Web format: ?ac=64&rev1= - if revision: - return f"{web_url}{path_without_rev}?ac=64&rev1={revision}" - return f"{web_url}{full_path}?ac=64" + # Swarm format: /files/?v= + if revision: + return f"{web_url}/files{path_without_rev}?v={revision}" + return f"{web_url}/files{full_path}" # Default: p4:// protocol URL # Perforce uses @ for revisions, which is already in the filepath from Symbolic @@ -397,25 +375,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, }, ] @@ -461,7 +426,6 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: "client": state.get("client"), "ssl_fingerprint": state.get("ssl_fingerprint"), "web_url": state.get("web_url"), - "web_viewer_type": state.get("web_viewer_type", "p4web"), }, } diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index dde8ce98c1a9bf..1cd499f16b58ca 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -375,43 +375,42 @@ 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", - external_id="perforce-test-web-flow", + external_id="perforce-test-swarm-flow", metadata={}, oi_params={ "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + "web_url": "https://swarm.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, @@ -426,13 +425,13 @@ 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) - assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64&rev1=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=42" diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 335b3e5dc14b90..4daa7d99a4f80a 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -43,6 +43,22 @@ def test_format_source_url_relative_path(self): ) assert url == "p4://depot/app/services/processor.cpp" + def test_format_source_url_path_starting_with_depot_name(self): + """Test that paths starting with depot name don't get duplicated (depot/app -> //depot/app not //depot/depot/app)""" + url = self.installation.format_source_url( + repo=self.repo, filepath="depot/app/services/processor.cpp", branch=None + ) + # Should strip "depot/" prefix and prepend "//depot/" -> "//depot/app/services/processor.cpp" + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_absolute_path_starting_with_depot_name(self): + """Test that absolute paths with depot name are handled correctly (//depot/app stays as //depot/app)""" + url = self.installation.format_source_url( + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None + ) + # Should preserve as-is (already absolute) + assert url == "p4://depot/app/services/processor.cpp" + def test_format_source_url_with_revision_in_filename(self): """ Test formatting URL with revision in filename (from Symbolic transformer). @@ -53,93 +69,85 @@ def test_format_source_url_with_revision_in_filename(self): ) assert url == "p4://depot/app/services/processor.cpp@42" - def test_format_source_url_p4web_viewer(self): - """Test formatting URL for P4Web viewer with revision in filename""" - integration_with_web = self.create_integration( + def test_format_source_url_swarm_viewer(self): + """Test formatting URL for Swarm viewer with revision""" + integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", - external_id="perforce-test-web", + external_id="perforce-test-swarm", metadata={}, oi_params={ "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", } }, ) - 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] url = installation.format_source_url( repo=self.repo, filepath="app/services/processor.cpp@42", branch=None ) - assert url == "https://p4web.example.com//depot/app/services/processor.cpp?ac=64&rev1=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=42" - def test_format_source_url_p4web_viewer_no_revision(self): - """Test formatting URL for P4Web viewer without revision""" - integration_with_web = self.create_integration( + def test_format_source_url_swarm_viewer_no_revision(self): + """Test formatting URL for Swarm viewer without revision""" + integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", - external_id="perforce-test-web2", + external_id="perforce-test-swarm2", metadata={}, oi_params={ "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + "web_url": "https://swarm.example.com", + "web_viewer_type": "swarm", } }, ) - 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] url = installation.format_source_url( repo=self.repo, filepath="app/services/processor.cpp", branch=None ) - assert url == "https://p4web.example.com//depot/app/services/processor.cpp?ac=64" + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" - def test_format_source_url_swarm_viewer(self): - """Test formatting URL for Swarm viewer with revision""" + def test_format_source_url_swarm_viewer_absolute_path(self): + """Test Swarm viewer with absolute path (//depot/...)""" integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", - external_id="perforce-test-swarm", + external_id="perforce-test-swarm-abs", metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", - } - }, + oi_params={"config": {"web_url": "https://swarm.example.com"}}, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] url = installation.format_source_url( - repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None ) - assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + assert "depot/depot" not in url - def test_format_source_url_swarm_viewer_no_revision(self): - """Test formatting URL for Swarm viewer without revision""" + def test_format_source_url_swarm_viewer_depot_name_path(self): + """Test Swarm viewer with path starting with depot name (depot/...)""" integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", - external_id="perforce-test-swarm2", + external_id="perforce-test-swarm-depot", metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", - } - }, + oi_params={"config": {"web_url": "https://swarm.example.com"}}, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] url = installation.format_source_url( - repo=self.repo, filepath="app/services/processor.cpp", branch=None + repo=self.repo, filepath="depot/app/services/processor.cpp", branch=None ) assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + assert "depot/depot" not in url def test_format_source_url_strips_leading_slash_from_relative_path(self): """Test that leading slash is stripped from relative paths""" @@ -164,21 +172,54 @@ def test_format_source_url_uses_repo_name_as_fallback_depot(self): @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_check_file_absolute_depot_path(self, mock_check_file): - """Test check_file with absolute depot path""" + """Test check_file with absolute depot path (//depot/...)""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - assert self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") + result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") + assert result is not None + assert "//depot/app/services/processor.cpp" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_path_with_depot_name(self, mock_check_file): + """Test check_file with path starting with depot name (depot/...)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp") + assert result is not None + # Should not duplicate depot name in result + assert "//depot/app/services/processor.cpp" in result + assert "depot/depot" not in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_check_file_relative_path(self, mock_check_file): - """Test check_file with relative path""" + """Test check_file with normal relative path (app/...)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "app/services/processor.cpp") + assert result is not None + assert "//depot/app/services/processor.cpp" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax_absolute(self, mock_check_file): + """Test check_file with @revision syntax on absolute path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp@42") + assert result is not None + assert "@42" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax_depot_name(self, mock_check_file): + """Test check_file with @revision syntax on path with depot name""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - assert self.installation.check_file(self.repo, "app/services/processor.cpp") + result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp@42") + assert result is not None + assert "@42" in result + assert "depot/depot" not in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_check_file_with_revision_syntax(self, mock_check_file): - """Test check_file with @revision syntax""" + def test_check_file_with_revision_syntax_relative(self, mock_check_file): + """Test check_file with @revision syntax on relative path""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - assert self.installation.check_file(self.repo, "app/services/processor.cpp@42") + result = self.installation.check_file(self.repo, "app/services/processor.cpp@42") + assert result is not None + assert "@42" in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_link(self, mock_check_file): @@ -358,31 +399,6 @@ def setUp(self): config={"depot_path": "//depot"}, ) - def test_p4web_extracts_revision_from_filename(self): - """Test P4Web viewer correctly extracts and formats revision from @revision syntax""" - integration_with_web = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test-p4web", - metadata={}, - oi_params={ - "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", - } - }, - ) - installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] - - # Filename with revision - url = installation.format_source_url( - repo=self.repo, filepath="game/src/main.cpp@42", branch=None - ) - - # Should extract @42 and use it as rev1 parameter - assert url == "https://p4web.example.com//depot/game/src/main.cpp?ac=64&rev1=42" - def test_swarm_extracts_revision_from_filename(self): """Test Swarm viewer correctly extracts and formats revision from @revision syntax""" integration_with_swarm = self.create_integration( @@ -407,29 +423,3 @@ def test_swarm_extracts_revision_from_filename(self): # Should extract @42 and use it as v parameter assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=42" - - def test_web_viewer_with_python_path_no_revision(self): - """Test web viewers work correctly without revision""" - integration_with_web = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test-p4web2", - metadata={}, - oi_params={ - "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", - } - }, - ) - installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] - - # Python path without revision - url = installation.format_source_url( - repo=self.repo, filepath="app/services/processor.py", branch=None - ) - - # Should not have revision parameter - assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64" - assert "rev1=" not in url diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 1982e5c3e14ae6..3d96e45054fd36 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -159,67 +159,6 @@ def test_get_stacktrace_config_multiple_code_mappings(self, mock_check_file): assert "//myproject/app/services/handler.py" in result["source_url"] assert result["src_path"] == "app/services/handler.py" - @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", - name="Perforce", - external_id="perforce-test-web-link", - metadata={}, - oi_params={ - "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", - } - }, - ) - - # Create new code mapping with new integration - # Use different project to avoid unique constraint - project_web = self.create_project(organization=self.organization) - - repo_web = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=integration_with_web.id, - config={"depot_path": "//depot"}, - ) - - org_integration = integration_with_web.organizationintegration_set.first() - assert org_integration is not None - code_mapping_web = RepositoryProjectPathConfig.objects.create( - project=project_web, - organization_id=self.organization.id, - repository=repo_web, - organization_integration_id=org_integration.id, - integration_id=integration_with_web.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping_web], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "https://p4web.example.com//depot/app/services/processor.py" in result["source_url"] - @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""" From d41a61e0759604fd7b61beb4ddbc6c58a0c32ad1 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:19:48 +0100 Subject: [PATCH 11/31] Implement comments on the SSL/P4Port --- src/sentry/integrations/perforce/client.py | 36 +++++++++++---- .../integrations/perforce/integration.py | 45 ++++++++++--------- 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index c6a3285c51e0bc..4207d19209329f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -54,25 +54,26 @@ def __init__( 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.""" + """Create and connect a P4 instance with SSL support.""" is_ticket_auth = bool(self.ticket) p4 = self.P4() + # Always set P4PORT (required for multi-server environments and SSL) + p4.port = self.p4port + 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 + # Ticket authentication: In multi-server deployments with login forwarding, + # tickets are identical across servers. Must still set p4.port to connect + # to the correct server endpoint. p4.password = self.ticket else: - # Password authentication: set host/port/user explicitly - p4.port = f"{self.host}:{self.port}" + # Password authentication p4.user = self.user - if self.password: p4.password = self.password @@ -84,6 +85,18 @@ def _connect(self): try: p4.connect() + # Establish SSL trust if fingerprint is provided + # Must be done AFTER connect but BEFORE login + if self.ssl_fingerprint and self.p4port.startswith("ssl:"): + try: + p4.run_trust("-i", self.ssl_fingerprint) + except self.P4Exception as trust_error: + p4.disconnect() + raise ApiError( + f"Failed to establish SSL trust: {trust_error}. " + f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" + ) + # Authenticate with the server if password is provided (not ticket) if self.password and not is_ticket_auth: try: @@ -94,7 +107,14 @@ def _connect(self): return p4 except self.P4Exception as e: - raise ApiError(f"Failed to connect to Perforce: {e}") + error_msg = str(e) + # Provide helpful error message for SSL issues + if "SSL" in error_msg or "trust" in error_msg.lower(): + raise ApiError( + f"Failed to connect to Perforce (SSL issue): {error_msg}. " + f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" + ) + raise ApiError(f"Failed to connect to Perforce: {error_msg}") def _disconnect(self, p4): """Disconnect P4 instance.""" diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 1812404eb750c3..2d7cc8bb164112 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -96,18 +96,21 @@ def get_client(self) -> PerforceClient: if auth_mode == "ticket": # Ticket authentication mode + # P4PORT still required for multi-server environments with login forwarding self._client = PerforceClient( ticket=config.get("ticket"), + p4port=config.get("p4port", "localhost:1666"), client=config.get("client"), + ssl_fingerprint=config.get("ssl_fingerprint"), ) else: # Password authentication mode self._client = PerforceClient( - host=config.get("host", "localhost"), - port=config.get("port", 1666), + p4port=config.get("p4port", "localhost:1666"), user=config.get("user", ""), password=config.get("password"), client=config.get("client"), + ssl_fingerprint=config.get("ssl_fingerprint"), ) return self._client @@ -331,23 +334,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", @@ -415,13 +415,17 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: Returns: Integration data dictionary """ + # Use p4port if available, otherwise fall back to host:port for legacy + p4port = ( + state.get("p4port") or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" + ) + return { - "name": state.get("name", f"Perforce ({state['host']})"), - "external_id": f"{state['host']}:{state['port']}", + "name": state.get("name", f"Perforce ({p4port})"), + "external_id": p4port, "metadata": { - "host": state["host"], - "port": state["port"], - "user": state["user"], + "p4port": p4port, + "user": state.get("user"), "password": state.get("password"), "client": state.get("client"), "ssl_fingerprint": state.get("ssl_fingerprint"), @@ -468,8 +472,7 @@ def dispatch(self, request, pipeline): pipeline: Installation pipeline """ # Set some default values that users will configure later - pipeline.bind_state("host", "localhost") - pipeline.bind_state("port", "1666") + pipeline.bind_state("p4port", "localhost:1666") pipeline.bind_state("user", "") pipeline.bind_state("name", "Perforce Integration") From b330c50db54dc95717417674d88e7acc6cdabfb7 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:39:59 +0100 Subject: [PATCH 12/31] Fix PR comments --- src/sentry/integrations/perforce/client.py | 24 +----- .../integrations/perforce/integration.py | 79 ++++++------------- 2 files changed, 25 insertions(+), 78 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 4207d19209329f..9d2865576fa7e4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -59,23 +59,10 @@ def __init__( def _connect(self): """Create and connect a P4 instance with SSL support.""" - is_ticket_auth = bool(self.ticket) - p4 = self.P4() - - # Always set P4PORT (required for multi-server environments and SSL) p4.port = self.p4port - - if is_ticket_auth: - # Ticket authentication: In multi-server deployments with login forwarding, - # tickets are identical across servers. Must still set p4.port to connect - # to the correct server endpoint. - p4.password = self.ticket - else: - # Password authentication - p4.user = self.user - if self.password: - p4.password = self.password + p4.user = self.user + p4.password = self.password if self.client_name: p4.client = self.client_name @@ -85,8 +72,6 @@ def _connect(self): try: p4.connect() - # Establish SSL trust if fingerprint is provided - # Must be done AFTER connect but BEFORE login if self.ssl_fingerprint and self.p4port.startswith("ssl:"): try: p4.run_trust("-i", self.ssl_fingerprint) @@ -97,8 +82,7 @@ def _connect(self): f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) - # Authenticate with the server if password is provided (not ticket) - if self.password and not is_ticket_auth: + if self.password: try: p4.run_login() except self.P4Exception as login_error: @@ -264,8 +248,6 @@ def get_changes( finally: self._disconnect(p4) - # CommitContextClient methods (stubbed for now) - def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] ) -> list[FileBlameInfo]: diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 2d7cc8bb164112..84b6358c62e38c 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -92,26 +92,14 @@ def get_client(self) -> PerforceClient: # Credentials are stored in org_integration.config (per-organization) config = self.org_integration.config - auth_mode = config.get("auth_mode", "password") - - if auth_mode == "ticket": - # Ticket authentication mode - # P4PORT still required for multi-server environments with login forwarding - self._client = PerforceClient( - ticket=config.get("ticket"), - p4port=config.get("p4port", "localhost:1666"), - client=config.get("client"), - ssl_fingerprint=config.get("ssl_fingerprint"), - ) - else: - # Password authentication mode - self._client = PerforceClient( - p4port=config.get("p4port", "localhost:1666"), - user=config.get("user", ""), - password=config.get("password"), - client=config.get("client"), - ssl_fingerprint=config.get("ssl_fingerprint"), - ) + + self._client = PerforceClient( + p4port=config.get("p4port", "localhost:1666"), + user=config.get("user", ""), + password=config.get("password"), + client=config.get("client"), + ssl_fingerprint=config.get("ssl_fingerprint"), + ) return self._client def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: @@ -312,60 +300,37 @@ def get_organization_config(self) -> list[dict[str, Any]]: These fields will be displayed in the integration settings UI. """ 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"}, + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", + "required": True, }, { "name": "password", "type": "secret", - "label": "Perforce Password", + "label": "Password or P4 Ticket", "placeholder": "••••••••", - "help": "Password for the Perforce user", + "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, + }, + { + "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, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", From 1a1f25b2483825363cad9c07dfda2a7bd7d86f58 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:05:55 +0100 Subject: [PATCH 13/31] Parse file revision properly --- src/sentry/integrations/perforce/client.py | 15 +++--- .../integrations/perforce/integration.py | 9 ++-- .../perforce/test_code_mapping.py | 28 +++++----- .../integrations/perforce/test_integration.py | 52 +++++++++---------- .../perforce/test_stacktrace_link.py | 12 ++--- 5 files changed, 58 insertions(+), 58 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 9d2865576fa7e4..931d190b193461 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -145,17 +145,18 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) Args: repo: Repository object - path: File path (may include @revision syntax like "file.cpp@42") + path: File path (may include #revision for file revisions like "file.cpp#1") branch: Optional branch/stream name to insert after depot (e.g., "main", "dev") Returns: - Full depot path with @revision preserved if present + Full depot path with #revision preserved if present """ - # Extract revision if present + # Extract file revision if present (# syntax only) revision = None path_without_rev = path - if "@" in path: - path_without_rev, revision = path.rsplit("@", 1) + + if "#" in path: + path_without_rev, revision = path.rsplit("#", 1) # If already absolute depot path, use as-is if path_without_rev.startswith("//"): @@ -183,9 +184,9 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) else: full_path = f"{depot_root}/{path_without_rev}" - # Add revision back if present + # Add file revision back if present if revision: - full_path = f"{full_path}@{revision}" + full_path = f"{full_path}#{revision}" return full_path diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 84b6358c62e38c..6cdd751d8c125e 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -173,19 +173,18 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) web_url = self.org_integration.config.get("web_url") if web_url: - # Extract revision from filepath if present (e.g., "file.cpp@42") + # Extract file revision from filepath if present (e.g., "file.cpp#1") revision = None path_without_rev = full_path - if "@" in full_path: - path_without_rev, revision = full_path.rsplit("@", 1) + if "#" in full_path: + path_without_rev, revision = full_path.rsplit("#", 1) # Swarm format: /files/?v= if revision: return f"{web_url}/files{path_without_rev}?v={revision}" return f"{web_url}/files{full_path}" - # Default: p4:// protocol URL - # Perforce uses @ for revisions, which is already in the filepath from Symbolic + # Default: p4:// protocol URL with file revision (#) syntax # Strip leading // from full_path to avoid p4://// return f"p4://{full_path.lstrip('/')}" diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 1cd499f16b58ca..cd9eb0bdf1572c 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -69,8 +69,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", @@ -90,17 +90,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)""" @@ -362,18 +362,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""" @@ -416,10 +416,10 @@ def test_full_flow_with_swarm_viewer(self): default_branch=None, ) - # Python SDK path with @revision from Symbolic + # Python SDK path with #revision from Symbolic frame = EventFrame( - filename="depot/app/services/processor.py@42", - abs_path="depot/app/services/processor.py@42", + filename="depot/app/services/processor.py#1", + abs_path="depot/app/services/processor.py#1", ) # Code mapping @@ -434,4 +434,4 @@ 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) - assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=1" diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 4daa7d99a4f80a..1ec13fcf328d37 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -62,12 +62,12 @@ def test_format_source_url_absolute_path_starting_with_depot_name(self): def test_format_source_url_with_revision_in_filename(self): """ Test formatting URL with revision in filename (from Symbolic transformer). - Perforce uses @ for revisions, which should be preserved. + Perforce uses # for file revisions, which should be preserved. """ url = self.installation.format_source_url( - repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + repo=self.repo, filepath="app/services/processor.cpp#1", branch=None ) - assert url == "p4://depot/app/services/processor.cpp@42" + assert url == "p4://depot/app/services/processor.cpp#1" def test_format_source_url_swarm_viewer(self): """Test formatting URL for Swarm viewer with revision""" @@ -87,9 +87,9 @@ def test_format_source_url_swarm_viewer(self): installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] url = installation.format_source_url( - repo=self.repo, filepath="app/services/processor.cpp@42", branch=None + repo=self.repo, filepath="app/services/processor.cpp#1", branch=None ) - assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=1" def test_format_source_url_swarm_viewer_no_revision(self): """Test formatting URL for Swarm viewer without revision""" @@ -198,41 +198,41 @@ def test_check_file_relative_path(self, mock_check_file): @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_check_file_with_revision_syntax_absolute(self, mock_check_file): - """Test check_file with @revision syntax on absolute path""" + """Test check_file with #revision syntax on absolute path""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp@42") + result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp#1") assert result is not None - assert "@42" in result + assert "#1" in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_check_file_with_revision_syntax_depot_name(self, mock_check_file): - """Test check_file with @revision syntax on path with depot name""" + """Test check_file with #revision syntax on path with depot name""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp@42") + result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp#1") assert result is not None - assert "@42" in result + assert "#1" in result assert "depot/depot" not in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_check_file_with_revision_syntax_relative(self, mock_check_file): - """Test check_file with @revision syntax on relative path""" + """Test check_file with #revision syntax on relative path""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "app/services/processor.cpp@42") + result = self.installation.check_file(self.repo, "app/services/processor.cpp#1") assert result is not None - assert "@42" in result + assert "#1" in result @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_link(self, mock_check_file): """Test get_stacktrace_link returns format_source_url result""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - filepath = "app/services/processor.cpp@42" + filepath = "app/services/processor.cpp#1" default_branch = "" # Perforce doesn't require default_branch (used for streams) version = None url = self.installation.get_stacktrace_link(self.repo, filepath, default_branch, version) - # URL will be encoded by get_stacktrace_link - assert url == "p4://depot/app/services/processor.cpp%4042" - assert "%40" in url # @ gets URL-encoded to %40 + # The # character is preserved as-is (not URL-encoded in this method) + assert url == "p4://depot/app/services/processor.cpp#1" + assert "#1" in url def test_integration_name(self): """Test integration has correct name""" @@ -283,18 +283,18 @@ def test_format_source_url_from_code_mapping_output(self): def test_format_source_url_preserves_revision_in_filename(self): """ - Test that @revision syntax in filename is preserved. + Test that #revision syntax in filename is preserved. This tests the Symbolic transformer output format. """ # This is what Symbolic transformer outputs - symbolic_path = "app/services/processor.cpp@42" + symbolic_path = "app/services/processor.cpp#1" url = self.installation.format_source_url( repo=self.repo, filepath=symbolic_path, branch=None ) - # The @42 should be preserved in the path - assert url == "p4://depot/app/services/processor.cpp@42" + # The #1 should be preserved in the path + assert url == "p4://depot/app/services/processor.cpp#1" def test_format_source_url_python_path_without_revision(self): """Test Python SDK paths without revision""" @@ -400,7 +400,7 @@ def setUp(self): ) def test_swarm_extracts_revision_from_filename(self): - """Test Swarm viewer correctly extracts and formats revision from @revision syntax""" + """Test Swarm viewer correctly extracts and formats revision from #revision syntax""" integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", @@ -418,8 +418,8 @@ def test_swarm_extracts_revision_from_filename(self): # Filename with revision url = installation.format_source_url( - repo=self.repo, filepath="game/src/main.cpp@42", branch=None + repo=self.repo, filepath="game/src/main.cpp#1", branch=None ) - # Should extract @42 and use it as v parameter - assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=42" + # Should extract #1 and use it as v parameter + assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=1" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 3d96e45054fd36..eab78b9289cef5 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -70,12 +70,12 @@ def test_get_stacktrace_config_python_path(self, mock_check_file): @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, @@ -89,10 +89,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 302e110cd899900013ce5918e18a2bf0c85ab02f Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:12:47 +0100 Subject: [PATCH 14/31] Finalize rebase --- src/sentry/integrations/perforce/client.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 931d190b193461..8169d6a760a0bd 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -49,7 +49,7 @@ def __init__( client: Client/workspace name ssl_fingerprint: SSL trust fingerprint for secure connections """ - self.p4port = p4port + self.p4port = p4port or "localhost:1666" self.ssl_fingerprint = ssl_fingerprint self.user = user or "" self.password = password @@ -134,19 +134,19 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object finally: self._disconnect(p4) - def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) -> str: + def build_depot_path(self, repo: Repository, path: str, stream: str | None = None) -> str: """ Build full depot path from repo config and file path. Handles both relative and absolute paths: - Relative: "depot/app/file.py" or "app/file.py" → "//depot/app/file.py" - Absolute: "//depot/app/file.py" → "//depot/app/file.py" (unchanged) - - With branch/stream: "app/file.py" + branch="main" → "//depot/main/app/file.py" + - With stream: "app/file.py" + stream="main" → "//depot/main/app/file.py" Args: repo: Repository object path: File path (may include #revision for file revisions like "file.cpp#1") - branch: Optional branch/stream name to insert after depot (e.g., "main", "dev") + stream: Optional stream name to insert after depot (e.g., "main", "dev") Returns: Full depot path with #revision preserved if present @@ -177,10 +177,10 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) # Remove leading slashes from relative path path_without_rev = path_without_rev.lstrip("/") - # Handle Perforce streams: insert branch/stream after depot + # Handle Perforce streams: insert stream after depot # Format: //depot/stream/path/to/file - if branch: - full_path = f"{depot_root}/{branch}/{path_without_rev}" + if stream: + full_path = f"{depot_root}/{stream}/{path_without_rev}" else: full_path = f"{depot_root}/{path_without_rev}" From 5ab2f3298961b58e089c86321b46fc581d4344aa Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:04:12 +0100 Subject: [PATCH 15/31] Fix PR Comments --- src/sentry/integrations/perforce/client.py | 140 ++++++++---- .../integrations/perforce/integration.py | 204 +++++++++++++++--- .../integrations/perforce/repository.py | 57 ++--- .../sentry/integrations/perforce-config.html | 42 ++++ 4 files changed, 339 insertions(+), 104 deletions(-) create mode 100644 src/sentry/templates/sentry/integrations/perforce-config.html diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8169d6a760a0bd..d826fe5d987be4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -7,6 +7,9 @@ from P4 import P4, P4Exception +from sentry.integrations.models.integration import Integration +from sentry.integrations.models.organization_integration import OrganizationIntegration +from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, CommitInfo, @@ -16,7 +19,7 @@ from sentry.integrations.source_code_management.repository import RepositoryClient from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository -from sentry.shared_integrations.exceptions import ApiError +from sentry.shared_integrations.exceptions import ApiError, IntegrationError from sentry.utils import metrics logger = logging.getLogger(__name__) @@ -33,32 +36,41 @@ class PerforceClient(RepositoryClient, CommitContextClient): def __init__( self, - p4port: str | None = None, - user: str | None = None, - password: str | None = None, - client: str | None = None, - ssl_fingerprint: str | None = None, + integration: Integration | RpcIntegration, + org_integration: OrganizationIntegration | RpcOrganizationIntegration | None = None, ): """ Initialize Perforce client. Args: - p4port: P4PORT string (e.g., 'ssl:host:port', 'tcp:host:port', or 'host:port') - user: Perforce username - password: Perforce password OR P4 ticket (both are supported) - client: Client/workspace name - ssl_fingerprint: SSL trust fingerprint for secure connections + integration: Integration instance + org_integration: Organization integration instance containing per-org config """ - self.p4port = p4port or "localhost:1666" - self.ssl_fingerprint = ssl_fingerprint - self.user = user or "" - self.password = password - self.client_name = client + self.integration = integration + self.org_integration = org_integration self.P4 = P4 self.P4Exception = P4Exception + # Extract configuration from org_integration + if not org_integration: + raise IntegrationError("Organization Integration is required for Perforce") + + config = org_integration.config + self.p4port = config.get("p4port", "localhost:1666") + self.user = config.get("user", "") + self.password = config.get("password") + self.client_name = config.get("client") + self.ssl_fingerprint = config.get("ssl_fingerprint") + def _connect(self): - """Create and connect a P4 instance with SSL support.""" + """ + Create and connect a P4 instance with SSL support. + + Uses P4Python API: + - p4.connect(): https://www.perforce.com/manuals/p4python/Content/P4Python/python.programming.html#python.programming.connecting + - p4.run_trust(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_trust.html + - p4.run_login(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_login.html + """ p4 = self.P4() p4.port = self.p4port p4.user = self.user @@ -112,6 +124,9 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object """ Check if a file exists in the depot. + Uses p4 files command to list file(s) in the depot. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_files.html + Args: repo: Repository object containing depot path (includes stream if specified) path: File path relative to depot @@ -194,6 +209,9 @@ def get_depots(self) -> list[dict[str, Any]]: """ List all depots accessible to the user. + Uses p4 depots command to display a list of all depots. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_depots.html + Returns: List of depot info dictionaries """ @@ -211,12 +229,44 @@ def get_depots(self) -> list[dict[str, Any]]: finally: self._disconnect(p4) + 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, max_changes: int = 20, start_cl: str | None = None ) -> list[dict[str, Any]]: """ 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 @@ -261,12 +311,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: @@ -293,8 +350,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 @@ -309,8 +381,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( @@ -322,29 +394,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 diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 6cdd751d8c125e..3ca42fb300fe67 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -4,6 +4,8 @@ from collections.abc import Mapping, Sequence from typing import Any +from django import forms +from django.http import HttpRequest, HttpResponseBase from django.utils.translation import gettext_lazy as _ from sentry.integrations.base import ( @@ -15,6 +17,7 @@ ) from sentry.integrations.models.integration import Integration from sentry.integrations.perforce.client import PerforceClient +from sentry.integrations.pipeline import IntegrationPipeline from sentry.integrations.services.repository import RpcRepository from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.repository import RepositoryIntegration @@ -22,6 +25,7 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView from sentry.shared_integrations.exceptions import ApiError, IntegrationError +from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) @@ -66,6 +70,73 @@ ) +class PerforceInstallationForm(forms.Form): + """Form for Perforce installation configuration.""" + + p4port = forms.CharField( + label=_("P4PORT (Server Address)"), + help_text=_( + "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." + ), + widget=forms.TextInput(attrs={"placeholder": "ssl:perforce.company.com:1666"}), + ) + user = forms.CharField( + label=_("Perforce Username"), + help_text=_( + "Username for authenticating with Perforce. " + "Required for both password and ticket authentication." + ), + widget=forms.TextInput(attrs={"placeholder": "sentry-bot"}), + ) + password = forms.CharField( + label=_("Password or P4 Ticket"), + help_text=_( + "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." + ), + widget=forms.PasswordInput(attrs={"placeholder": "••••••••"}), + ) + client = forms.CharField( + label=_("Perforce Client/Workspace (Optional)"), + help_text=_("Optional: Specify a client workspace name"), + widget=forms.TextInput(attrs={"placeholder": "sentry-workspace"}), + required=False, + ) + ssl_fingerprint = forms.CharField( + label=_("SSL Fingerprint (Required for SSL)"), + help_text=_( + "SSL fingerprint for secure connections. " + "Required when using 'ssl:' protocol. " + "Obtain with: p4 -p ssl:host:port trust -y" + ), + widget=forms.TextInput( + attrs={"placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01"} + ), + required=False, + ) + web_url = forms.CharField( + label=_("Helix Swarm URL (Optional)"), + help_text=_("Optional: URL to Helix Swarm web viewer for browsing files"), + widget=forms.TextInput(attrs={"placeholder": "https://swarm.company.com"}), + required=False, + ) + + def clean_p4port(self): + """Strip off trailing / and whitespace from p4port""" + return self.cleaned_data["p4port"].strip().rstrip("/") + + def clean_web_url(self): + """Strip off trailing / from web_url""" + web_url = self.cleaned_data.get("web_url", "") + if web_url: + return web_url.rstrip("/") + return web_url + + class PerforceIntegration(RepositoryIntegration, CommitContextIntegration): """ Integration for Perforce/Helix Core version control system. @@ -90,15 +161,9 @@ def get_client(self) -> PerforceClient: if not self.org_integration: raise IntegrationError("Organization Integration not found") - # Credentials are stored in org_integration.config (per-organization) - config = self.org_integration.config - self._client = PerforceClient( - p4port=config.get("p4port", "localhost:1666"), - user=config.get("user", ""), - password=config.get("password"), - client=config.get("client"), - ssl_fingerprint=config.get("ssl_fingerprint"), + integration=self.model, + org_integration=self.org_integration, ) return self._client @@ -181,12 +246,15 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) # Swarm format: /files/?v= if revision: - return f"{web_url}/files{path_without_rev}?v={revision}" - return f"{web_url}/files{full_path}" + url = f"{web_url}/files{path_without_rev}?v={revision}" + else: + url = f"{web_url}/files{full_path}" + return url # Default: p4:// protocol URL with file revision (#) syntax # Strip leading // from full_path to avoid p4://// - return f"p4://{full_path.lstrip('/')}" + url = f"p4://{full_path.lstrip('/')}" + return url def extract_branch_from_source_url(self, repo: Repository, url: str) -> str: """ @@ -268,7 +336,6 @@ def get_repositories( # Re-raise authentication/connection errors so user sees them raise except Exception as e: - logger.exception("perforce.get_repositories_failed") raise IntegrationError(f"Failed to fetch repositories from Perforce: {str(e)}") def has_repo_access(self, repo: RpcRepository) -> bool: @@ -373,27 +440,39 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: """ Build integration data from installation state. + Each organization gets its own private Perforce integration instance, + even if multiple orgs connect to the same Perforce server. This ensures + credentials and configuration are isolated per organization. + Args: state: Installation state from pipeline Returns: Integration data dictionary + + Raises: + IntegrationError: If organization_id is not provided """ + # Validate organization_id is present + organization_id = state.get("organization_id") + if not organization_id: + raise IntegrationError("Organization ID is required for Perforce integration") + # Use p4port if available, otherwise fall back to host:port for legacy p4port = ( state.get("p4port") or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" ) + # Create unique external_id per organization to ensure private integrations + # Format: "org-{org_id}:{p4port}" e.g. "org-123:ssl:perforce.company.com:1666" + external_id = f"org-{organization_id}:{p4port}" + return { "name": state.get("name", f"Perforce ({p4port})"), - "external_id": p4port, - "metadata": { - "p4port": p4port, - "user": state.get("user"), - "password": state.get("password"), - "client": state.get("client"), - "ssl_fingerprint": state.get("ssl_fingerprint"), - "web_url": state.get("web_url"), + "external_id": external_id, + "metadata": {}, + "post_install_data": { + "installation_data": state.get("installation_data", {}), }, } @@ -404,8 +483,41 @@ def post_install( *, extra: dict[str, Any], ) -> None: - """Actions after installation.""" - pass + """ + Actions after installation. + Sets organization-level configuration from installation pipeline state. + """ + from sentry.integrations.services.integration import integration_service + + installation_data = extra.get("installation_data", {}) + + # Store per-organization configuration + org_integration_config = { + "p4port": installation_data.get("p4port", "localhost:1666"), + "user": installation_data.get("user", ""), + "password": installation_data.get("password", ""), + } + + # Add optional fields if provided + if installation_data.get("client"): + org_integration_config["client"] = installation_data.get("client") + + if installation_data.get("ssl_fingerprint"): + org_integration_config["ssl_fingerprint"] = installation_data.get("ssl_fingerprint") + + if installation_data.get("web_url"): + org_integration_config["web_url"] = installation_data.get("web_url") + + # Update organization integration with configuration + org_integration = integration_service.get_organization_integration( + integration_id=integration.id, organization_id=organization.id + ) + + if org_integration: + integration_service.update_organization_integration( + org_integration_id=org_integration.id, + config=org_integration_config, + ) def setup(self) -> None: """Setup integration provider.""" @@ -423,21 +535,47 @@ def setup(self) -> None: class PerforceInstallationView: """ Installation view for Perforce configuration. - - This is a simple pass-through view. The actual configuration - happens in the Settings tab after installation via get_organization_config(). + Collects and validates Perforce server credentials during installation. """ - def dispatch(self, request, pipeline): + def dispatch(self, request: HttpRequest, pipeline: IntegrationPipeline) -> HttpResponseBase: """ - Handle installation request. + Handle installation request with form validation. + Args: request: HTTP request object pipeline: Installation pipeline - """ - # Set some default values that users will configure later - pipeline.bind_state("p4port", "localhost:1666") - pipeline.bind_state("user", "") - pipeline.bind_state("name", "Perforce Integration") - return pipeline.next_step() + Returns: + HTTP response (form render or redirect to next step) + """ + if request.method == "POST": + form = PerforceInstallationForm(request.POST) + if form.is_valid(): + form_data = form.cleaned_data + + # Bind configuration data to pipeline state + pipeline.bind_state("installation_data", form_data) + pipeline.bind_state("p4port", form_data.get("p4port")) + pipeline.bind_state("name", f"Perforce ({form_data.get('p4port')})") + # Include organization_id to create unique external_id per org + pipeline.bind_state("organization_id", pipeline.organization.id) + + pipeline.get_logger().info( + "perforce.setup.installation-config-view.success", + extra={ + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + "has_ssl_fingerprint": bool(form_data.get("ssl_fingerprint")), + "has_web_url": bool(form_data.get("web_url")), + }, + ) + return pipeline.next_step() + else: + form = PerforceInstallationForm() + + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 79ec38d9a1bff7..61d6485234240a 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -151,17 +151,13 @@ def compare_commits( continue changes = filtered_changes - return self._format_commits(changes, depot_path) + return self._format_commits(changes, depot_path, client) - except Exception as e: - logger.exception( - "perforce.compare_commits.error", - extra={"repo": repo.name, "start": start_sha, "end": end_sha, "error": str(e)}, - ) + except Exception: return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str + self, changelists: list[dict[str, Any]], depot_path: str, client: Any ) -> Sequence[Mapping[str, Any]]: """ Format Perforce changelists into Sentry commit format. @@ -169,12 +165,16 @@ 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 """ commits = [] + # Cache user info to avoid multiple lookups for the same user + user_cache: dict[str, dict[str, Any] | None] = {} + for cl in changelists: try: # Handle potentially null/invalid time field @@ -187,23 +187,36 @@ def _format_commits( # Format timestamp (P4 time is Unix timestamp) timestamp = self.format_date(time_int) + # Get user information from Perforce + username = cl.get("user", "unknown") + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username not in user_cache: + user_cache[username] = client.get_user(username) + + user_info = user_cache[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"] + commits.append( { "id": str(cl["change"]), # Changelist number as commit ID "repository": depot_path, - "author_email": f"{cl.get('user', 'unknown')}@perforce", # P4 doesn't store email - "author_name": cl.get("user", "unknown"), + "author_email": author_email, + "author_name": author_name, "message": cl.get("desc", ""), "timestamp": timestamp, "patch_set": [], # Could fetch with 'p4 describe' if needed } ) - except (KeyError, TypeError) as e: - # Skip malformed changelist data - logger.warning( - "perforce.format_commits.invalid_data", - extra={"changelist": cl, "error": str(e)}, - ) + except (KeyError, TypeError): continue return commits @@ -211,19 +224,9 @@ def _format_commits( def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. - Perforce doesn't have native PRs, but might integrate with Swarm. + Perforce doesn't have native pull requests. """ - web_url = None - if repo.integration_id: - integration = integration_service.get_integration(integration_id=repo.integration_id) - if integration: - web_url = integration.metadata.get("web_url") - - if web_url: - # Swarm review URL format - return f"{web_url}/reviews/{pull_request.key}" - - return f"p4://{repo.name}@{pull_request.key}" + raise NotImplementedError("Perforce does not have native pull requests") def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/src/sentry/templates/sentry/integrations/perforce-config.html b/src/sentry/templates/sentry/integrations/perforce-config.html new file mode 100644 index 00000000000000..e7288bbe251c98 --- /dev/null +++ b/src/sentry/templates/sentry/integrations/perforce-config.html @@ -0,0 +1,42 @@ +{% extends "sentry/bases/modal.html" %} +{% load crispy_forms_tags %} +{% load sentry_assets %} +{% load i18n %} + +{% block css %} + +{% endblock %} + +{% block wrapperclass %} narrow auth {% endblock %} +{% block modal_header_signout %} {% endblock %} + +{% 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 "See the" %} {% trans "docs" %} {% trans "for more information." %}

+ +
+ {% csrf_token %} + + + {{ form|as_crispy_errors }} + + {% for field in form %} + {{ field|as_crispy_field }} + {% endfor %} + +
+
+ +
+
+
+{% endblock %} From 028d0e5771d14bbba3b00444882975b09b99936d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:15:10 +0100 Subject: [PATCH 16/31] Fix PR comments --- src/sentry/integrations/perforce/integration.py | 10 ++++++++++ src/sentry/integrations/perforce/repository.py | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 3ca42fb300fe67..7d91e339d69ffa 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -276,6 +276,16 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str """ depot_path = repo.config.get("depot_path", repo.name) + # Handle Swarm web viewer URLs + if self.org_integration: + web_url = self.org_integration.config.get("web_url") + if web_url and url.startswith(web_url): + # Strip Swarm base URL and /files prefix + # e.g., "https://swarm.example.com/files//depot/path/file.cpp" -> "//depot/path/file.cpp" + url = url[len(web_url) :] + if url.startswith("/files"): + url = url[6:] # Remove "/files" + # Remove p4:// prefix if url.startswith("p4://"): url = url[5:] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 61d6485234240a..ab3510cfe70f75 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -192,8 +192,8 @@ def _format_commits( author_email = f"{username}@perforce" author_name = username - # Fetch user info if not in cache - if username not in user_cache: + # Fetch user info if not in cache (skip "unknown" placeholder) + if username != "unknown" and username not in user_cache: user_cache[username] = client.get_user(username) user_info = user_cache[username] From fdbb309987c65e6e5a37b9bfa91a2b67a7409e99 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:41:11 +0100 Subject: [PATCH 17/31] Fix the PR comment --- src/sentry/integrations/perforce/repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index ab3510cfe70f75..5ffcf4bf51e790 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -196,7 +196,7 @@ def _format_commits( if username != "unknown" and username not in user_cache: user_cache[username] = client.get_user(username) - user_info = user_cache[username] + user_info = user_cache.get(username) if user_info: # Use actual email from Perforce if available if user_info.get("email"): From f0bfd8ccae01e0ea562b2cebedcfb24a87cbc12c Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:49:13 +0100 Subject: [PATCH 18/31] Simplify for installation only --- src/sentry/integrations/perforce/client.py | 120 +---- .../integrations/perforce/repository.py | 156 +------ .../perforce/test_code_mapping.py | 437 ------------------ .../perforce/test_stacktrace_link.py | 414 ----------------- 4 files changed, 13 insertions(+), 1114 deletions(-) delete mode 100644 tests/sentry/integrations/perforce/test_code_mapping.py delete mode 100644 tests/sentry/integrations/perforce/test_stacktrace_link.py diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index d826fe5d987be4..12743634ecb95f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -2,7 +2,6 @@ import logging from collections.abc import Sequence -from datetime import datetime, timezone from typing import Any from P4 import P4, P4Exception @@ -12,7 +11,6 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, - CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -20,7 +18,6 @@ from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository from sentry.shared_integrations.exceptions import ApiError, IntegrationError -from sentry.utils import metrics logger = logging.getLogger(__name__) @@ -264,9 +261,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 @@ -275,29 +269,7 @@ def get_changes( Returns: List of changelist dictionaries """ - p4 = self._connect() - try: - args = ["-m", str(max_changes), "-l"] - - if start_cl: - args.extend(["-e", start_cl]) - - args.append(depot_path) - - changes = p4.run("changes", *args) - - 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) + return [] def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -311,97 +283,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 5ffcf4bf51e790..b17bb24d42a5a7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,14 +4,12 @@ from collections.abc import Mapping, Sequence from typing import Any -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 sentry.plugins.providers.integration_repository import RepositoryConfig -from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -35,44 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - installation = self.get_installation(config.get("installation"), organization.id) - client = installation.get_client() - - depot_path = config["identifier"] # e.g., //depot or //depot/project - - # Validate depot exists and is accessible - try: - # Create a minimal repo-like object for client - class MockRepo: - def __init__(self, depot_path): - self.config = {"depot_path": depot_path} - - mock_repo = MockRepo(depot_path) - - # Try to check depot access - result = client.check_file(mock_repo, "...", None) - - if result is None: - # Try getting depot info - depots = client.get_depots() - depot_name = depot_path.strip("/").split("/")[0] - - if not any(d["name"] == depot_name for d in depots): - raise IntegrationError( - f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" - ) - - except IntegrationError: - # Re-raise validation errors so user sees them - raise - except Exception: - # Catch only connection/P4 errors - depot might be valid but temporarily unreachable - pass - - config["external_id"] = depot_path - config["integration_id"] = installation.model.id - - return config + return {} def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -87,17 +48,12 @@ def build_repository_config( Returns: Repository configuration """ - depot_path = data["identifier"] - return { - "name": depot_path, - "external_id": data["external_id"], - "url": f"p4://{depot_path}", - "config": { - "depot_path": depot_path, - "name": depot_path, - }, - "integration_id": data["integration_id"], + "name": "", + "external_id": "", + "url": "", + "config": {}, + "integration_id": 0, } def compare_commits( @@ -114,50 +70,10 @@ def compare_commits( Returns: List of changelist dictionaries """ - integration_id = repo.integration_id - if integration_id is None: - raise NotImplementedError("Perforce integration requires an integration_id") - - integration = integration_service.get_integration(integration_id=integration_id) - if integration is None: - raise NotImplementedError("Integration not found") - - installation = integration.get_installation(organization_id=repo.organization_id) - client = installation.get_client() - - depot_path = repo.config.get("depot_path", repo.name) - - try: - # Get changelists in range - if start_sha is None: - # Get last N changes - changes = client.get_changes(f"{depot_path}/...", max_changes=20) - else: - # Get changes between start and end - # P4 doesn't have native compare, so get changes up to end_sha - changes = client.get_changes(f"{depot_path}/...", max_changes=100, start_cl=end_sha) - - # Filter to only changes after start_sha - if changes: - start_cl_num = int(start_sha) if start_sha.isdigit() else 0 - # Filter out invalid changelist data - filtered_changes = [] - for c in changes: - try: - if int(c["change"]) > start_cl_num: - filtered_changes.append(c) - except (TypeError, ValueError, KeyError): - # Skip malformed changelist data - continue - changes = filtered_changes - - return self._format_commits(changes, depot_path, client) - - except Exception: - return [] + return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str, client: Any + self, changelists: list[dict[str, Any]], depot_path: str ) -> Sequence[Mapping[str, Any]]: """ Format Perforce changelists into Sentry commit format. @@ -165,68 +81,18 @@ 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 """ - commits = [] - - # Cache user info to avoid multiple lookups for the same user - user_cache: dict[str, dict[str, Any] | None] = {} - - for cl in changelists: - try: - # Handle potentially null/invalid time field - time_value = cl.get("time") or 0 - try: - time_int = int(time_value) - except (TypeError, ValueError): - time_int = 0 - - # Format timestamp (P4 time is Unix timestamp) - timestamp = self.format_date(time_int) - - # Get user information from Perforce - username = cl.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: - user_cache[username] = client.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"] - - commits.append( - { - "id": str(cl["change"]), # Changelist number as commit ID - "repository": depot_path, - "author_email": author_email, - "author_name": author_name, - "message": cl.get("desc", ""), - "timestamp": timestamp, - "patch_set": [], # Could fetch with 'p4 describe' if needed - } - ) - except (KeyError, TypeError): - continue - - return commits + return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. - Perforce doesn't have native pull requests. + Perforce doesn't have native PRs, but might integrate with Swarm. """ - raise NotImplementedError("Perforce does not have native pull requests") + return "" def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py deleted file mode 100644 index cd9eb0bdf1572c..00000000000000 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ /dev/null @@ -1,437 +0,0 @@ -from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig -from sentry.integrations.perforce.integration import ( - PerforceIntegration, - PerforceIntegrationProvider, -) -from sentry.issues.auto_source_code_config.code_mapping import ( - convert_stacktrace_frame_path_to_source_path, -) -from sentry.models.repository import Repository -from sentry.testutils.cases import IntegrationTestCase -from sentry.utils.event_frames import EventFrame - - -class PerforceCodeMappingTest(IntegrationTestCase): - """Tests for code mapping integration with Perforce""" - - provider = PerforceIntegrationProvider - installation: PerforceIntegration - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - self.org_integration = self.integration.organizationintegration_set.first() - assert self.org_integration is not None - - def test_code_mapping_depot_root_to_slash(self): - """ - Test code mapping: depot/ -> / - This is the correct mapping for Perforce where depot name is part of path. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Test Python SDK path: depot/app/services/processor.py - frame = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should strip "depot/" leaving "app/services/processor.py" - assert result == "app/services/processor.py" - - 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. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Test C++ path from Symbolic: depot/game/src/main.cpp#1 - frame = EventFrame( - 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 "#1" - assert result == "game/src/main.cpp#1" - - def test_code_mapping_multiple_depots(self): - """Test code mappings for multiple depots (depot and myproject)""" - depot_repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - myproject_repo = Repository.objects.create( - name="//myproject", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - depot_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=depot_repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="myproject/", - source_root="/", - default_branch=None, - ) - - # Test depot path - frame1 = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - result1 = convert_stacktrace_frame_path_to_source_path( - frame=frame1, code_mapping=depot_mapping, platform="python", sdk_name="sentry.python" - ) - assert result1 == "app/services/processor.py" - - # Test myproject path - frame2 = EventFrame( - filename="myproject/app/services/handler.py", - abs_path="myproject/app/services/handler.py", - ) - - result2 = convert_stacktrace_frame_path_to_source_path( - frame=frame2, - code_mapping=myproject_mapping, - platform="python", - sdk_name="sentry.python", - ) - assert result2 == "app/services/handler.py" - - def test_code_mapping_no_match_different_depot(self): - """Test that code mapping doesn't match paths from different depots""" - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Try to match a path from different depot - frame = EventFrame( - filename="myproject/app/services/processor.py", - abs_path="myproject/app/services/processor.py", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should not match - assert result is None - - def test_code_mapping_abs_path_fallback(self): - """Test that code mapping works with abs_path when filename is just basename""" - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Some platforms only provide basename in filename - frame = EventFrame(filename="processor.py", abs_path="depot/app/services/processor.py") - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should use abs_path and strip "depot/" - assert result == "app/services/processor.py" - - def test_code_mapping_nested_depot_paths(self): - """Test code mapping with nested depot paths""" - repo = Repository.objects.create( - name="//depot/game/project", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot/game/project"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/game/project/", - source_root="/", - default_branch=None, - ) - - frame = EventFrame( - filename="depot/game/project/src/main.cpp", - abs_path="depot/game/project/src/main.cpp", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" - ) - - assert result == "src/main.cpp" - - def test_code_mapping_preserves_windows_backslash_conversion(self): - """ - Test that code mapping handles Windows-style paths. - - Note: The generic code_mapping system does not automatically convert - backslashes to forward slashes. SDKs should normalize paths before - sending to Sentry. This test documents current behavior. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Windows-style path with backslashes - frame = EventFrame( - filename="depot\\app\\services\\processor.cpp", - abs_path="depot\\app\\services\\processor.cpp", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" - ) - - # Generic code mapping doesn't normalize backslashes - returns None - # SDKs should normalize paths to forward slashes before sending - assert result is None - - -class PerforceEndToEndCodeMappingTest(IntegrationTestCase): - """End-to-end tests for code mapping + format_source_url flow""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - self.org_integration = self.integration.organizationintegration_set.first() - assert self.org_integration is not None - - self.repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - self.code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=self.repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - def test_python_sdk_path_full_flow(self): - """Test full flow: Python SDK -> code mapping -> format_source_url""" - # 1. Python SDK sends this path - frame = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - # 2. Code mapping transforms it - mapped_path = convert_stacktrace_frame_path_to_source_path( - frame=frame, - code_mapping=self.code_mapping, - platform="python", - sdk_name="sentry.python", - ) - assert mapped_path == "app/services/processor.py" - - # 3. format_source_url creates final URL - url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/app/services/processor.py" - - 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" - ) - - # 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" - - # 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#1" - - 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", - external_id="perforce-test-swarm-flow", - metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - } - }, - ) - installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] - - # Create repo with web viewer integration - repo_swarm = Repository.objects.create( - name="//depot", - organization_id=self.organization.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_swarm = self.create_project(organization=self.organization) - - org_integration_swarm = integration_with_swarm.organizationintegration_set.first() - assert org_integration_swarm is not None - - code_mapping_swarm = RepositoryProjectPathConfig.objects.create( - project=project_swarm, - organization_id=self.organization.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, - ) - - # Python SDK path with #revision from Symbolic - frame = EventFrame( - filename="depot/app/services/processor.py#1", - abs_path="depot/app/services/processor.py#1", - ) - - # Code mapping - mapped_path = convert_stacktrace_frame_path_to_source_path( - frame=frame, - code_mapping=code_mapping_swarm, - platform="python", - sdk_name="sentry.python", - ) - - # format_source_url with Swarm viewer (revision extracted from filename) - assert mapped_path is not None - url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) - - assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=1" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py deleted file mode 100644 index eab78b9289cef5..00000000000000 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ /dev/null @@ -1,414 +0,0 @@ -from unittest.mock import patch - -from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig -from sentry.integrations.perforce.integration import PerforceIntegrationProvider -from sentry.integrations.utils.stacktrace_link import get_stacktrace_config -from sentry.issues.endpoints.project_stacktrace_link import StacktraceLinkContext -from sentry.models.repository import Repository -from sentry.testutils.cases import IntegrationTestCase - - -class PerforceStacktraceLinkTest(IntegrationTestCase): - """Tests for Perforce stacktrace link generation""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - - self.repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - self.code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=self.repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_python_path(self, mock_check_file): - """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", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - assert result["error"] is None - assert result["src_path"] == "app/services/processor.py" - - @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#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, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - # 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#1" - - def test_get_stacktrace_config_no_matching_code_mapping(self): - """Test stacktrace link when no code mapping matches""" - ctx: StacktraceLinkContext = { - "file": "other/app/services/processor.py", - "filename": "other/app/services/processor.py", - "abs_path": "other/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is None - assert result["error"] == "stack_root_mismatch" - assert result["src_path"] is None - - @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", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="myproject/", - source_root="/", - default_branch=None, - ) - - # Test with myproject path - ctx: StacktraceLinkContext = { - "file": "myproject/app/services/handler.py", - "filename": "myproject/app/services/handler.py", - "abs_path": "myproject/app/services/handler.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//myproject/app/services/handler.py" in result["source_url"] - assert result["src_path"] == "app/services/handler.py" - - @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", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - assert result["src_path"] == "app/services/processor.py" - - @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", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//other"}, - ) - - other_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=other_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="other/", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([other_mapping, self.code_mapping], ctx) - - # iteration_count should be 1 (only depot mapping matched) - assert result["iteration_count"] == 1 - assert result["source_url"] is not None - - @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) - - myproject_repo = Repository.objects.create( - name="//myproject", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=project2, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", # Same stack_root as depot mapping (but different project) - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) - - # Should stop after first match (depot mapping) - assert result["iteration_count"] == 1 - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - - -class PerforceStacktraceLinkEdgeCasesTest(IntegrationTestCase): - """Edge case tests for Perforce stacktrace links""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.project = self.create_project(organization=self.organization) - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_stacktrace_link_empty_stack_root(self, mock_check_file): - """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, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - # Empty stack_root should match any path - assert result["source_url"] is not None - - @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, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Path with spaces and special chars - ctx: StacktraceLinkContext = { - "file": "depot/app/my services/processor-v2.py", - "filename": "depot/app/my services/processor-v2.py", - "abs_path": "depot/app/my services/processor-v2.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - assert result["source_url"] is not None - assert result["src_path"] == "app/my services/processor-v2.py" - - @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, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - deep_path = "depot/" + "/".join([f"level{i}" for i in range(20)]) + "/file.py" - - ctx: StacktraceLinkContext = { - "file": deep_path, - "filename": deep_path, - "abs_path": deep_path, - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/" in result["source_url"] From cab2d9b0489b0daade72b61f68545e372557804b Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 14:34:37 +0100 Subject: [PATCH 19/31] Rework based on the PR comments --- src/sentry/integrations/perforce/client.py | 126 ++++++++++-------- .../integrations/perforce/integration.py | 78 +++++------ 2 files changed, 104 insertions(+), 100 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 12743634ecb95f..b38c1e1a3d2e0d 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -2,6 +2,7 @@ import logging from collections.abc import Sequence +from contextlib import contextmanager from typing import Any from P4 import P4, P4Exception @@ -17,7 +18,7 @@ from sentry.integrations.source_code_management.repository import RepositoryClient from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository -from sentry.shared_integrations.exceptions import ApiError, IntegrationError +from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError logger = logging.getLogger(__name__) @@ -40,33 +41,40 @@ def __init__( Initialize Perforce client. Args: - integration: Integration instance - org_integration: Organization integration instance containing per-org config + integration: Integration instance containing credentials in metadata + org_integration: Organization integration instance (required for API compatibility) """ self.integration = integration self.org_integration = org_integration self.P4 = P4 self.P4Exception = P4Exception - # Extract configuration from org_integration + # Extract configuration from integration.metadata if not org_integration: raise IntegrationError("Organization Integration is required for Perforce") - config = org_integration.config - self.p4port = config.get("p4port", "localhost:1666") - self.user = config.get("user", "") - self.password = config.get("password") - self.client_name = config.get("client") - self.ssl_fingerprint = config.get("ssl_fingerprint") + metadata = integration.metadata + self.p4port = metadata.get("p4port", "localhost:1666") + self.user = metadata.get("user", "") + self.password = metadata.get("password") + self.client_name = metadata.get("client") + self.ssl_fingerprint = metadata.get("ssl_fingerprint") + @contextmanager def _connect(self): """ - Create and connect a P4 instance with SSL support. + Context manager for P4 connections with automatic cleanup. + + Yields a connected P4 instance and ensures disconnection on exit. Uses P4Python API: - p4.connect(): https://www.perforce.com/manuals/p4python/Content/P4Python/python.programming.html#python.programming.connecting - p4.run_trust(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_trust.html - p4.run_login(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_login.html + + Example: + with self._connect() as p4: + result = p4.run("info") """ p4 = self.P4() p4.port = self.p4port @@ -78,27 +86,9 @@ def _connect(self): p4.exception_level = 1 # Only errors raise exceptions + # Connect to Perforce server try: p4.connect() - - if self.ssl_fingerprint and self.p4port.startswith("ssl:"): - try: - p4.run_trust("-i", self.ssl_fingerprint) - except self.P4Exception as trust_error: - p4.disconnect() - raise ApiError( - f"Failed to establish SSL trust: {trust_error}. " - f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" - ) - - if self.password: - 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: error_msg = str(e) # Provide helpful error message for SSL issues @@ -109,13 +99,41 @@ def _connect(self): ) raise ApiError(f"Failed to connect to Perforce: {error_msg}") - def _disconnect(self, p4): - """Disconnect P4 instance.""" + # Establish SSL trust if needed + if self.ssl_fingerprint and self.p4port.startswith("ssl:"): + try: + p4.run_trust("-i", self.ssl_fingerprint) + except self.P4Exception as trust_error: + try: + p4.disconnect() + except Exception: + pass + raise ApiError( + f"Failed to establish SSL trust: {trust_error}. " + f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" + ) + + # Authenticate if password provided + if self.password: + try: + p4.run_login() + except self.P4Exception as login_error: + try: + p4.disconnect() + except Exception: + pass + raise ApiUnauthorized(f"Failed to authenticate with Perforce: {login_error}") + try: - if p4.connected(): - p4.disconnect() - except Exception: - pass + yield p4 + finally: + # Ensure cleanup + try: + if p4.connected(): + p4.disconnect() + 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) def check_file(self, repo: Repository, path: str, version: str | None) -> object | None: """ @@ -132,19 +150,17 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object Returns: File info dict if exists, None otherwise """ - p4 = self._connect() - try: - depot_path = self.build_depot_path(repo, path) - result = p4.run("files", depot_path) + with self._connect() as p4: + 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 + if result and len(result) > 0: + return result[0] + return None - except self.P4Exception: - return None - finally: - self._disconnect(p4) + except self.P4Exception: + return None def build_depot_path(self, repo: Repository, path: str, stream: str | None = None) -> str: """ @@ -212,8 +228,7 @@ def get_depots(self) -> list[dict[str, Any]]: Returns: List of depot info dictionaries """ - p4 = self._connect() - try: + with self._connect() as p4: depots = p4.run("depots") return [ { @@ -223,8 +238,6 @@ def get_depots(self) -> list[dict[str, Any]]: } for depot in depots ] - finally: - self._disconnect(p4) def get_user(self, username: str) -> dict[str, Any] | None: """ @@ -238,9 +251,11 @@ def get_user(self, username: str) -> dict[str, Any] | None: Returns: User info dictionary with Email and FullName fields, or None if not found + + Raises: + P4Exception: For connection or transient errors that may be retryable """ - p4 = self._connect() - try: + with self._connect() as p4: result = p4.run("user", "-o", username) if result and len(result) > 0: user_info = result[0] @@ -249,11 +264,8 @@ def get_user(self, username: str) -> dict[str, Any] | None: "full_name": user_info.get("FullName", ""), "username": user_info.get("User", username), } + # User not found - return None (not an error condition) return None - except self.P4Exception: - return None - finally: - self._disconnect(p4) def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 7d91e339d69ffa..3b426cfcbb29f2 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -118,11 +118,12 @@ class PerforceInstallationForm(forms.Form): ), required=False, ) - web_url = forms.CharField( + web_url = forms.URLField( label=_("Helix Swarm URL (Optional)"), help_text=_("Optional: URL to Helix Swarm web viewer for browsing files"), - widget=forms.TextInput(attrs={"placeholder": "https://swarm.company.com"}), + widget=forms.URLInput(attrs={"placeholder": "https://swarm.company.com"}), required=False, + assume_scheme="https", ) def clean_p4port(self): @@ -179,10 +180,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 @@ -454,6 +454,9 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: even if multiple orgs connect to the same Perforce server. This ensures credentials and configuration are isolated per organization. + Credentials are stored in Integration.metadata since each integration + is unique per organization (external_id includes org_id). + Args: state: Installation state from pipeline @@ -469,21 +472,38 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: raise IntegrationError("Organization ID is required for Perforce integration") # Use p4port if available, otherwise fall back to host:port for legacy + installation_data = state.get("installation_data", {}) p4port = ( - state.get("p4port") or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" + installation_data.get("p4port") + or state.get("p4port") + or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" ) # Create unique external_id per organization to ensure private integrations # Format: "org-{org_id}:{p4port}" e.g. "org-123:ssl:perforce.company.com:1666" external_id = f"org-{organization_id}:{p4port}" + # 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"] + + 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"] + return { "name": state.get("name", f"Perforce ({p4port})"), "external_id": external_id, - "metadata": {}, - "post_install_data": { - "installation_data": state.get("installation_data", {}), - }, + "metadata": metadata, } def post_install( @@ -495,39 +515,11 @@ def post_install( ) -> None: """ Actions after installation. - Sets organization-level configuration from installation pipeline state. - """ - from sentry.integrations.services.integration import integration_service - - installation_data = extra.get("installation_data", {}) - - # Store per-organization configuration - org_integration_config = { - "p4port": installation_data.get("p4port", "localhost:1666"), - "user": installation_data.get("user", ""), - "password": installation_data.get("password", ""), - } - - # Add optional fields if provided - if installation_data.get("client"): - org_integration_config["client"] = installation_data.get("client") - if installation_data.get("ssl_fingerprint"): - org_integration_config["ssl_fingerprint"] = installation_data.get("ssl_fingerprint") - - if installation_data.get("web_url"): - org_integration_config["web_url"] = installation_data.get("web_url") - - # Update organization integration with configuration - org_integration = integration_service.get_organization_integration( - integration_id=integration.id, organization_id=organization.id - ) - - if org_integration: - integration_service.update_organization_integration( - org_integration_id=org_integration.id, - config=org_integration_config, - ) + Configuration is now stored in Integration.metadata (set by build_integration), + so no additional setup is needed per organization. + """ + pass def setup(self) -> None: """Setup integration provider.""" From 4ca6de40ddbb4a26ea39c9922d57a1ac0a10bbd1 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:04:14 +0100 Subject: [PATCH 20/31] Fix PR comments from Cursor --- src/sentry/integrations/perforce/client.py | 8 +++++++- src/sentry/integrations/perforce/integration.py | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index b38c1e1a3d2e0d..c241e174b9be41 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -155,7 +155,9 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object depot_path = self.build_depot_path(repo, path) result = p4.run("files", depot_path) - if result and len(result) > 0: + # Verify result contains actual file data (not just warnings) + # When exception_level=1, warnings are returned in result list + if result and len(result) > 0 and "depotFile" in result[0]: return result[0] return None @@ -259,6 +261,10 @@ def get_user(self, username: str) -> dict[str, Any] | None: result = p4.run("user", "-o", username) if result and len(result) > 0: user_info = result[0] + # p4 user -o returns a template for non-existent users + # Check if user actually exists by verifying Update field is set + if not user_info.get("Update"): + return None return { "email": user_info.get("Email", ""), "full_name": user_info.get("FullName", ""), diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 3b426cfcbb29f2..0119d6496f9037 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -305,7 +305,8 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str normalized_url = url.lstrip("/") # Remove depot prefix to get relative path - if normalized_url.startswith(normalized_depot): + # Ensure exact match by checking for separator or exact equality + if normalized_url.startswith(normalized_depot + "/") or normalized_url == normalized_depot: return normalized_url[len(normalized_depot) :].lstrip("/") return url From ef458601634ea33165b98b3c19796fa8ab11226a Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:15:28 +0100 Subject: [PATCH 21/31] More cursor comment fixes --- .../integrations/perforce/integration.py | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 0119d6496f9037..a4c20abd5a456f 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -317,8 +317,12 @@ def get_repositories( """ Get list of depots/streams from Perforce server. + Args: + query: Optional search query to filter depot names + page_number_limit: Maximum number of repositories to return + Returns: - List of repository dictionaries + List of repository dictionaries (limited by page_number_limit if provided) """ try: client = self.get_client() @@ -341,6 +345,10 @@ def get_repositories( } ) + # Apply pagination limit if specified + if page_number_limit and len(repositories) >= page_number_limit: + break + return repositories except ApiError: @@ -355,14 +363,15 @@ def has_repo_access(self, repo: RpcRepository) -> bool: client = self.get_client() depot_path = repo.config.get("depot_path", repo.name) - # Try to list files to verify access - result = client.check_file( - repo=type("obj", (object,), {"config": {"depot_path": depot_path}})(), - path="...", - version=None, - ) + # Verify depot exists by checking depot list instead of listing all files + # Using "..." wildcard would fetch metadata for all files in large repos + depots = client.get_depots() + + # Extract depot name from path (e.g., "//depot" -> "depot") + depot_name = depot_path.lstrip("/").split("/")[0] - return result is not None + # Check if depot exists in the list + return any(depot["name"] == depot_name for depot in depots) except Exception: return False From b43eb3616340bd8188f4ab546fc9316fb0c2eafd Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:30:52 +0100 Subject: [PATCH 22/31] Even more cursor comments --- .../integrations/perforce/integration.py | 35 ++++++++----- .../integrations/perforce/test_integration.py | 49 ++++++++++--------- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index a4c20abd5a456f..f56306c3d2708f 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -24,7 +24,7 @@ from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView -from sentry.shared_integrations.exceptions import ApiError, IntegrationError +from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) @@ -199,6 +199,10 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) Returns: Formatted URL if the file exists, None otherwise + + Raises: + ApiUnauthorized: For authentication failures (should be shown to user) + IntegrationError: For configuration errors (should be shown to user) """ try: client = self.get_client() @@ -208,8 +212,14 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) return None # File exists, return formatted URL return self.format_source_url(repo, filepath, branch) + except (ApiUnauthorized, IntegrationError): + # Re-raise auth/config errors so they're visible to users + raise + except ApiError: + # Re-raise API errors for visibility + raise except Exception: - # If any error occurs (auth, connection, etc), return None + # For other errors (e.g., file not found), return None return None def format_source_url(self, repo: Repository, filepath: str, branch: str | None) -> str: @@ -233,9 +243,8 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) full_path = client.build_depot_path(repo, filepath, branch) # If Swarm web viewer is configured, use it - web_url = None - if self.org_integration: - web_url = self.org_integration.config.get("web_url") + # Web URL is stored in Integration.metadata + web_url = self.model.metadata.get("web_url") if web_url: # Extract file revision from filepath if present (e.g., "file.cpp#1") @@ -277,14 +286,14 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str depot_path = repo.config.get("depot_path", repo.name) # Handle Swarm web viewer URLs - if self.org_integration: - web_url = self.org_integration.config.get("web_url") - if web_url and url.startswith(web_url): - # Strip Swarm base URL and /files prefix - # e.g., "https://swarm.example.com/files//depot/path/file.cpp" -> "//depot/path/file.cpp" - url = url[len(web_url) :] - if url.startswith("/files"): - url = url[6:] # Remove "/files" + # Web URL is stored in Integration.metadata + web_url = self.model.metadata.get("web_url") + if web_url and url.startswith(web_url): + # Strip Swarm base URL and /files prefix + # e.g., "https://swarm.example.com/files//depot/path/file.cpp" -> "//depot/path/file.cpp" + url = url[len(web_url) :] + if url.startswith("/files"): + url = url[6:] # Remove "/files" # Remove p4:// prefix if url.startswith("p4://"): diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 1ec13fcf328d37..179cc381be81f5 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -76,12 +76,11 @@ def test_format_source_url_swarm_viewer(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm", - metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", - } + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -98,12 +97,11 @@ def test_format_source_url_swarm_viewer_no_revision(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm2", - metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", - } + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -120,8 +118,12 @@ def test_format_source_url_swarm_viewer_absolute_path(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm-abs", - metadata={}, - oi_params={"config": {"web_url": "https://swarm.example.com"}}, + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -138,8 +140,12 @@ def test_format_source_url_swarm_viewer_depot_name_path(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm-depot", - metadata={}, - oi_params={"config": {"web_url": "https://swarm.example.com"}}, + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] @@ -406,12 +412,11 @@ def test_swarm_extracts_revision_from_filename(self): provider="perforce", name="Perforce", external_id="perforce-test-swarm3", - metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - "web_viewer_type": "swarm", - } + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", }, ) installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] From a98b0f500e0476fa03b9cdab1f370cc944fc35d9 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:10:35 +0100 Subject: [PATCH 23/31] Test connection during initialization workflow --- .../integrations/perforce/integration.py | 110 ++++++++++++- .../integrations/perforce/test_integration.py | 144 ++++++++++++++++++ 2 files changed, 253 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index f56306c3d2708f..d510f86158c4c8 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -392,7 +392,9 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. - These fields will be displayed in the integration settings UI. + + Returns the form schema (field definitions, labels, help text, types). + Current values are provided separately via get_config_data(). """ return [ { @@ -445,6 +447,45 @@ def get_organization_config(self) -> list[dict[str, Any]]: }, ] + def get_config_data(self) -> Mapping[str, Any]: + """ + Get current configuration values for the integration. + + This is called by the serializer to populate the form fields with existing values. + Since we store credentials in integration.metadata (not org_integration.config), + we override the base implementation to read from metadata. + + Returns: + Dictionary of current configuration values that will be used to populate + the form fields defined in get_organization_config() + """ + return self.model.metadata + + def update_organization_config(self, data: Mapping[str, Any]) -> None: + """ + Update organization configuration by saving to integration.metadata. + + Since each organization has its own private Perforce integration instance, + we store credentials in integration.metadata instead of org_integration.config. + + Only updates fields that are present in data, preserving existing values + for fields not included in the update. + + Args: + data: Updated configuration data from the form (only changed fields) + """ + from sentry.integrations.services.integration import integration_service + + # Update integration metadata with new values + metadata = dict(self.model.metadata) # Create a mutable copy + metadata.update(data) # Only update fields present in data + + # Update the integration with new metadata + integration_service.update_integration( + integration_id=self.model.id, + metadata=metadata, + ) + class PerforceIntegrationProvider(IntegrationProvider): """Provider for Perforce integration.""" @@ -575,6 +616,73 @@ def dispatch(self, request: HttpRequest, pipeline: IntegrationPipeline) -> HttpR if form.is_valid(): form_data = form.cleaned_data + # Verify connection to Perforce server before completing installation + try: + client = PerforceClient( + integration=type( + "obj", + (object,), + { + "metadata": { + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + "password": form_data.get("password"), + "client": form_data.get("client"), + "ssl_fingerprint": form_data.get("ssl_fingerprint"), + } + }, + )(), + org_integration=type("obj", (object,), {})(), + ) + # Test connection by fetching depot list + client.get_depots() + + pipeline.get_logger().info( + "perforce.setup.connection-verified", + extra={ + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + }, + ) + except ApiUnauthorized as e: + form.add_error( + None, + f"Authentication failed: {e}. Please check your username and password.", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + except ApiError as e: + form.add_error( + None, + f"Failed to connect to Perforce server: {e}. Please verify your server address and SSL fingerprint.", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + except Exception as e: + pipeline.get_logger().error( + "perforce.setup.connection-verification-failed", + extra={ + "p4port": form_data.get("p4port"), + "error": str(e), + }, + exc_info=True, + ) + form.add_error( + None, + f"Unexpected error during connection verification: {e}", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + # Bind configuration data to pipeline state pipeline.bind_state("installation_data", form_data) pipeline.bind_state("p4port", form_data.get("p4port")) diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 179cc381be81f5..f461d450b16ed4 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -428,3 +428,147 @@ def test_swarm_extracts_revision_from_filename(self): # Should extract #1 and use it as v parameter assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=1" + + +class PerforceIntegrationEndToEndTest(IntegrationTestCase): + """ + End-to-end test covering the full integration lifecycle: + - Installation with credentials + - Configuration retrieval + - Partial updates + - Full updates + """ + + provider = PerforceIntegrationProvider + + def test_integration_lifecycle(self): + """Test complete integration lifecycle from installation to updates""" + + # Step 1: Simulate installation with build_integration + provider = PerforceIntegrationProvider() + state = { + "organization_id": self.organization.id, + "installation_data": { + "p4port": "ssl:perforce.example.com:1666", + "user": "sentry-bot", + "password": "initial_password", + "client": "sentry-workspace", + "ssl_fingerprint": "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD", + "web_url": "https://swarm.example.com", + }, + "name": "Perforce (ssl:perforce.example.com:1666)", + } + + integration_data = provider.build_integration(state) + + # Verify integration data structure + assert integration_data["name"] == "Perforce (ssl:perforce.example.com:1666)" + assert ( + integration_data["external_id"] + == f"org-{self.organization.id}:ssl:perforce.example.com:1666" + ) + assert "metadata" in integration_data + + # Verify all credentials are in metadata + metadata = integration_data["metadata"] + assert metadata["p4port"] == "ssl:perforce.example.com:1666" + assert metadata["user"] == "sentry-bot" + assert metadata["password"] == "initial_password" + assert metadata["client"] == "sentry-workspace" + assert ( + metadata["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) + assert metadata["web_url"] == "https://swarm.example.com" + + # Step 2: Create integration (simulating ensure_integration) + integration = self.create_integration( + organization=self.organization, + provider="perforce", + name=integration_data["name"], + external_id=integration_data["external_id"], + metadata=integration_data["metadata"], + ) + + # Step 3: Get installation and verify configuration retrieval + installation: PerforceIntegration = integration.get_installation(self.organization.id) # type: ignore[assignment] + + # Test get_organization_config returns form schema + org_config = installation.get_organization_config() + assert len(org_config) == 6 # 6 configuration fields + field_names = {field["name"] for field in org_config} + assert field_names == {"p4port", "user", "password", "ssl_fingerprint", "client", "web_url"} + + # Verify field types + field_types = {field["name"]: field["type"] for field in org_config} + assert field_types["password"] == "secret" + assert field_types["p4port"] == "string" + assert field_types["user"] == "string" + + # Test get_config_data returns current values + config_data = installation.get_config_data() + assert config_data["p4port"] == "ssl:perforce.example.com:1666" + assert config_data["user"] == "sentry-bot" + assert config_data["password"] == "initial_password" + assert config_data["client"] == "sentry-workspace" + assert ( + config_data["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) + assert config_data["web_url"] == "https://swarm.example.com" + + # Step 4: Test partial update (only change password) + installation.update_organization_config({"password": "updated_password_123"}) + + # Refresh and verify password changed but other fields preserved + integration.refresh_from_db() + updated_config = installation.get_config_data() + assert updated_config["password"] == "updated_password_123" + assert updated_config["p4port"] == "ssl:perforce.example.com:1666" # Preserved + assert updated_config["user"] == "sentry-bot" # Preserved + assert updated_config["client"] == "sentry-workspace" # Preserved + assert ( + updated_config["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) # Preserved + assert updated_config["web_url"] == "https://swarm.example.com" # Preserved + + # Step 5: Test multiple field update + installation.update_organization_config( + { + "user": "new-user", + "client": "new-workspace", + "web_url": "https://new-swarm.example.com", + } + ) + + # Refresh and verify multiple fields changed + integration.refresh_from_db() + final_config = installation.get_config_data() + assert final_config["user"] == "new-user" + assert final_config["client"] == "new-workspace" + assert final_config["web_url"] == "https://new-swarm.example.com" + # Verify other fields still preserved + assert final_config["password"] == "updated_password_123" # From previous update + assert final_config["p4port"] == "ssl:perforce.example.com:1666" # Original value + assert ( + final_config["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) # Original value + + # Step 6: Verify empty optional fields don't break anything + installation.update_organization_config( + { + "client": "", # Clear client + "web_url": "", # Clear web_url + } + ) + + integration.refresh_from_db() + cleared_config = installation.get_config_data() + assert cleared_config["client"] == "" + assert cleared_config["web_url"] == "" + # Required fields still preserved + assert cleared_config["p4port"] == "ssl:perforce.example.com:1666" + assert cleared_config["user"] == "new-user" + assert cleared_config["password"] == "updated_password_123" From e62aaac4355601a081f0ece4635f53399ee39f3d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:34:20 +0100 Subject: [PATCH 24/31] Fix ticket authentication --- src/sentry/integrations/perforce/client.py | 28 +++++++++++++-- .../integrations/perforce/integration.py | 35 +++++++++++++++---- .../integrations/perforce/test_integration.py | 14 ++++++-- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index c241e174b9be41..0de856b922e8cb 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -57,6 +57,9 @@ def __init__( self.p4port = metadata.get("p4port", "localhost:1666") self.user = metadata.get("user", "") self.password = metadata.get("password") + self.auth_type = metadata.get( + "auth_type", "password" + ) # Default to password for backwards compat self.client_name = metadata.get("client") self.ssl_fingerprint = metadata.get("ssl_fingerprint") @@ -113,8 +116,10 @@ def _connect(self): f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) - # Authenticate if password provided - if self.password: + # Authenticate based on auth_type + # - password: Requires run_login() to exchange password for session ticket + # - ticket: Already authenticated via p4.password, no login needed + if self.password and self.auth_type == "password": try: p4.run_login() except self.P4Exception as login_error: @@ -122,7 +127,24 @@ def _connect(self): p4.disconnect() except Exception: pass - raise ApiUnauthorized(f"Failed to authenticate with Perforce: {login_error}") + raise ApiUnauthorized( + f"Failed to authenticate with Perforce: {login_error}. " + "Verify your password is correct." + ) + elif self.password and self.auth_type == "ticket": + # Ticket authentication: p4.password is already set to the ticket + # Verify ticket works by running a test command + try: + p4.run("info") + except self.P4Exception as e: + try: + p4.disconnect() + except Exception: + pass + raise ApiUnauthorized( + f"Failed to authenticate with Perforce ticket: {e}. " + "Verify your P4 ticket is valid. Obtain a new ticket with: p4 login -p" + ) try: yield p4 diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index d510f86158c4c8..34ca7b90c20da0 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -91,12 +91,23 @@ class PerforceInstallationForm(forms.Form): ), widget=forms.TextInput(attrs={"placeholder": "sentry-bot"}), ) + auth_type = forms.ChoiceField( + label=_("Authentication Type"), + choices=[ + ("password", _("Password")), + ("ticket", _("P4 Ticket")), + ], + initial="password", + help_text=_( + "Select whether you're providing a password or a P4 ticket. " + "Tickets are obtained via 'p4 login -p' and don't require re-authentication." + ), + ) password = forms.CharField( - label=_("Password or P4 Ticket"), + label=_("Password / Ticket"), help_text=_( - "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." + "Your Perforce password or P4 authentication ticket " + "(depending on the authentication type selected above)." ), widget=forms.PasswordInput(attrs={"placeholder": "••••••••"}), ) @@ -413,12 +424,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, }, { @@ -547,6 +569,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", ""), } diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index f461d450b16ed4..3e425df8aa9bd9 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -451,6 +451,7 @@ def test_integration_lifecycle(self): "installation_data": { "p4port": "ssl:perforce.example.com:1666", "user": "sentry-bot", + "auth_type": "password", "password": "initial_password", "client": "sentry-workspace", "ssl_fingerprint": "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD", @@ -473,6 +474,7 @@ def test_integration_lifecycle(self): metadata = integration_data["metadata"] assert metadata["p4port"] == "ssl:perforce.example.com:1666" assert metadata["user"] == "sentry-bot" + assert metadata["auth_type"] == "password" assert metadata["password"] == "initial_password" assert metadata["client"] == "sentry-workspace" assert ( @@ -495,9 +497,17 @@ def test_integration_lifecycle(self): # Test get_organization_config returns form schema org_config = installation.get_organization_config() - assert len(org_config) == 6 # 6 configuration fields + assert len(org_config) == 7 # 7 configuration fields (added auth_type) field_names = {field["name"] for field in org_config} - assert field_names == {"p4port", "user", "password", "ssl_fingerprint", "client", "web_url"} + assert field_names == { + "p4port", + "user", + "auth_type", + "password", + "ssl_fingerprint", + "client", + "web_url", + } # Verify field types field_types = {field["name"]: field["type"] for field in org_config} From 5f868e70052a5da7b33d767bc9295f8b03916206 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:47:37 +0100 Subject: [PATCH 25/31] Fix trust order of operations --- src/sentry/integrations/perforce/client.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 0de856b922e8cb..e7fe54170f7f24 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -87,14 +87,17 @@ def _connect(self): 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 self.P4Exception as e: error_msg = str(e) - # Provide helpful error message for SSL issues + # Provide helpful error message for connection failures if "SSL" in error_msg or "trust" in error_msg.lower(): raise ApiError( f"Failed to connect to Perforce (SSL issue): {error_msg}. " @@ -102,7 +105,8 @@ def _connect(self): ) raise ApiError(f"Failed to connect to Perforce: {error_msg}") - # Establish SSL trust if needed + # 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:"): try: p4.run_trust("-i", self.ssl_fingerprint) @@ -116,6 +120,9 @@ def _connect(self): 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 d8a05533fc57a7c777b1116b81ff03c9c0376e52 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:57:39 +0100 Subject: [PATCH 26/31] Fix auth type issues --- src/sentry/integrations/perforce/client.py | 8 +------- src/sentry/integrations/perforce/integration.py | 1 + 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index e7fe54170f7f24..71c4e3181413f6 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -87,12 +87,9 @@ def _connect(self): 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 self.P4Exception as e: @@ -120,9 +117,6 @@ def _connect(self): 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 diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 34ca7b90c20da0..11de51eea7faf9 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -650,6 +650,7 @@ def dispatch(self, request: HttpRequest, pipeline: IntegrationPipeline) -> HttpR "p4port": form_data.get("p4port"), "user": form_data.get("user"), "password": form_data.get("password"), + "auth_type": form_data.get("auth_type", "password"), "client": form_data.get("client"), "ssl_fingerprint": form_data.get("ssl_fingerprint"), } From eea34fbd2815542fbbce68d98d8f1235d5c813ab Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 17:40:37 +0100 Subject: [PATCH 27/31] Fix external id 64-char limit --- src/sentry/integrations/perforce/integration.py | 7 +++++-- .../sentry/integrations/perforce/test_integration.py | 12 ++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 11de51eea7faf9..d4c013f6b384c4 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -1,5 +1,6 @@ from __future__ import annotations +import hashlib import logging from collections.abc import Mapping, Sequence from typing import Any @@ -562,8 +563,10 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: ) # Create unique external_id per organization to ensure private integrations - # Format: "org-{org_id}:{p4port}" e.g. "org-123:ssl:perforce.company.com:1666" - external_id = f"org-{organization_id}:{p4port}" + # Use hash to avoid exceeding 64-character external_id limit with long hostnames + # Format: "perforce-org-{org_id}-{hash}" where hash is first 8 chars of SHA256(p4port) + p4port_hash = hashlib.sha256(p4port.encode("utf-8")).hexdigest()[:8] + external_id = f"perforce-org-{organization_id}-{p4port_hash}" # Store credentials in Integration.metadata metadata = { diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 3e425df8aa9bd9..1a957e2c6a0c95 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -1,3 +1,4 @@ +import hashlib from unittest.mock import patch from sentry.integrations.perforce.integration import ( @@ -464,10 +465,13 @@ def test_integration_lifecycle(self): # Verify integration data structure assert integration_data["name"] == "Perforce (ssl:perforce.example.com:1666)" - assert ( - integration_data["external_id"] - == f"org-{self.organization.id}:ssl:perforce.example.com:1666" - ) + + # Verify external_id format: perforce-org-{org_id}-{hash} + # Hash is first 8 chars of SHA256(p4port) + p4port_hash = hashlib.sha256(b"ssl:perforce.example.com:1666").hexdigest()[:8] + expected_external_id = f"perforce-org-{self.organization.id}-{p4port_hash}" + assert integration_data["external_id"] == expected_external_id + assert "metadata" in integration_data # Verify all credentials are in metadata From a02da77628cc0fe9ec6e2a9e3311805a0877846f Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:21:35 +0100 Subject: [PATCH 28/31] 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 ca37e790856f85d6800073ffc2b8ef7aae0ee9c8 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:35:59 +0100 Subject: [PATCH 29/31] Fix stale config after update --- src/sentry/integrations/perforce/integration.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index d4c013f6b384c4..c5299bbb2f5a51 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -498,6 +498,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 @@ -509,6 +510,12 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: metadata=metadata, ) + # Refresh self.model from database to get updated metadata + self.model = Integration.objects.get(id=self.model.id) + + # Invalidate cached client so it gets recreated with new credentials + self._client = None + class PerforceIntegrationProvider(IntegrationProvider): """Provider for Perforce integration.""" From 44a9f77d707fe312599630abcea651a3f400d647 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:48:06 +0100 Subject: [PATCH 30/31] Fix cursor comment --- src/sentry/integrations/perforce/integration.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index c5299bbb2f5a51..ee6828b4458557 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -171,9 +171,7 @@ def get_client(self) -> PerforceClient: if self._client is not None: return self._client - if not self.org_integration: - raise IntegrationError("Organization Integration not found") - + # Accessing self.org_integration will raise OrganizationIntegrationNotFound if it doesn't exist self._client = PerforceClient( integration=self.model, org_integration=self.org_integration, @@ -498,7 +496,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 @@ -511,7 +508,9 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: ) # Refresh self.model from database to get updated metadata - self.model = Integration.objects.get(id=self.model.id) + refreshed = integration_service.get_integration(integration_id=self.model.id) + if refreshed: + self.model = refreshed # Invalidate cached client so it gets recreated with new credentials self._client = None From 494bf1d4b7c6756e860162ddabb7e263e2413c7e Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 1 Dec 2025 10:08:10 +0100 Subject: [PATCH 31/31] Review comments fixes --- src/sentry/integrations/perforce/client.py | 14 ++++---- .../integrations/perforce/integration.py | 34 ++++++++++++------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 71c4e3181413f6..ffb199398c1b63 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -46,8 +46,6 @@ def __init__( """ self.integration = integration self.org_integration = org_integration - self.P4 = P4 - self.P4Exception = P4Exception # Extract configuration from integration.metadata if not org_integration: @@ -79,7 +77,7 @@ def _connect(self): with self._connect() as p4: result = p4.run("info") """ - p4 = self.P4() + p4 = P4() p4.port = self.p4port p4.user = self.user p4.password = self.password @@ -92,7 +90,7 @@ def _connect(self): # Connect to Perforce server try: p4.connect() - except self.P4Exception as e: + except P4Exception as e: error_msg = str(e) # Provide helpful error message for connection failures if "SSL" in error_msg or "trust" in error_msg.lower(): @@ -107,7 +105,7 @@ def _connect(self): if self.ssl_fingerprint and self.p4port.startswith("ssl:"): try: p4.run_trust("-i", self.ssl_fingerprint) - except self.P4Exception as trust_error: + except P4Exception as trust_error: try: p4.disconnect() except Exception: @@ -123,7 +121,7 @@ def _connect(self): if self.password and self.auth_type == "password": try: p4.run_login() - except self.P4Exception as login_error: + except P4Exception as login_error: try: p4.disconnect() except Exception: @@ -137,7 +135,7 @@ def _connect(self): # Verify ticket works by running a test command try: p4.run("info") - except self.P4Exception as e: + except P4Exception as e: try: p4.disconnect() except Exception: @@ -184,7 +182,7 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object return result[0] return None - except self.P4Exception: + except P4Exception: return None def build_depot_path(self, repo: Repository, path: str, stream: str | None = None) -> str: diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index ee6828b4458557..93e85eeac0e1cb 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -3,7 +3,7 @@ import hashlib import logging from collections.abc import Mapping, Sequence -from typing import Any +from typing import Any, TypedDict from django import forms from django.http import HttpRequest, HttpResponseBase @@ -26,10 +26,24 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError +from sentry.web.frontend.base import determine_active_organization from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) + +class PerforceMetadata(TypedDict, total=False): + """Type definition for Perforce integration metadata stored in Integration.metadata.""" + + p4port: str + user: str + password: str + auth_type: str + client: str + ssl_fingerprint: str + web_url: str + + DESCRIPTION = """ Connect your Sentry organization to your Perforce/Helix Core server to enable stacktrace linking, commit tracking, suspect commit detection, and code ownership. @@ -338,10 +352,10 @@ def get_repositories( Args: query: Optional search query to filter depot names - page_number_limit: Maximum number of repositories to return + 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() @@ -364,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: @@ -575,7 +585,7 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: external_id = f"perforce-org-{organization_id}-{p4port_hash}" # Store credentials in Integration.metadata - metadata = { + metadata: PerforceMetadata = { "p4port": p4port, "user": installation_data.get("user", ""), "auth_type": installation_data.get("auth_type", "password"), # Default to password @@ -595,7 +605,7 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: return { "name": state.get("name", f"Perforce ({p4port})"), "external_id": external_id, - "metadata": metadata, + "metadata": dict(metadata), # Cast TypedDict to dict for compatibility } def post_install( @@ -718,10 +728,10 @@ def dispatch(self, request: HttpRequest, pipeline: IntegrationPipeline) -> HttpR # Bind configuration data to pipeline state pipeline.bind_state("installation_data", form_data) - pipeline.bind_state("p4port", form_data.get("p4port")) - pipeline.bind_state("name", f"Perforce ({form_data.get('p4port')})") # Include organization_id to create unique external_id per org - pipeline.bind_state("organization_id", pipeline.organization.id) + active_org = determine_active_organization(request) + if active_org: + pipeline.bind_state("organization_id", active_org.organization.id) pipeline.get_logger().info( "perforce.setup.installation-config-view.success",