diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 84b03121121642..59edd972fcde2b 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -2,16 +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__) @@ -38,14 +44,54 @@ def __init__( 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.""" - pass + 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: """ @@ -59,7 +105,19 @@ 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 get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False @@ -76,7 +134,21 @@ def get_file( Returns: File contents as string """ - return "" + p4 = self._connect() + try: + depot_path = self._build_depot_path(repo, path) + result = p4.run("print", "-q", depot_path) + + if result and len(result) > 1: + # First element is file info, second is content + return result[1] + + raise ApiError(f"File not found: {depot_path}") + + except self.P4Exception as e: + raise ApiError(f"Failed to get file: {e}") + finally: + self._disconnect(p4) def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) -> str: """ @@ -90,41 +162,21 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) Returns: Full depot path with @revision preserved if present """ - return "" - - 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. + depot_root = repo.config.get("depot_path", repo.name).rstrip("/") - 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) + # Ensure path doesn't start with / + path = path.lstrip("/") - Returns: - List with a single entry containing: - - changelist: changelist number - - user: username who made the change - - date: date of change - - description: changelist description - """ - return [] + # 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}" - def get_depot_info(self) -> dict[str, Any]: - """ - Get server info for testing connection. + # 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}" - Returns: - Server info dictionary - """ - return {} + return full_path def get_depots(self) -> list[dict[str, Any]]: """ @@ -133,7 +185,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 @@ -149,7 +213,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] @@ -165,7 +253,93 @@ 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() + + 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) + 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" + + # 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=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 create_comment(self, repo: str, issue_id: str, data: dict[str, Any]) -> Any: """Create comment. Not applicable for Perforce.""" diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 26af17f2be357e..9c8bd573105b59 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from collections.abc import Mapping, MutableMapping, Sequence +from collections.abc import Mapping, Sequence from typing import Any from django.utils.translation import gettext_lazy as _ @@ -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__) @@ -94,7 +94,32 @@ def __init__( def get_client(self) -> PerforceClient: """Get the Perforce client instance.""" - raise NotImplementedError + if self._client is not None: + return self._client + + 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 + auth_mode = config.get("auth_mode", "password") + + if auth_mode == "ticket": + # Ticket authentication mode + self._client = PerforceClient( + ticket=config.get("ticket"), + client=config.get("client"), + ) + else: + # Password authentication mode + self._client = PerforceClient( + 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 def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: """ @@ -105,32 +130,21 @@ 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.""" - return False - - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. + if url.startswith("p4://"): + return True - 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) + if self.org_integration: + web_url = self.org_integration.config.get("web_url") + if web_url and url.startswith(web_url): + return True - Returns: - True if the filepath matches this repository's depot - """ return False def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ - Check if a filepath belongs to this Perforce repository and return the URL. + Check if a file exists in the Perforce depot and return the URL. - Perforce doesn't have a REST API to check file existence, so we just - verify the filepath matches the depot_path configuration and return - the formatted URL. + Uses the client's check_file method to verify file existence on the P4 server. Args: repo: Repository object @@ -138,9 +152,19 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) branch: Branch/stream name (optional) Returns: - Formatted URL if the filepath matches this repository, None otherwise + Formatted URL if the file exists, None otherwise """ - return None + 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 + # 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: """ @@ -158,7 +182,53 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) Returns: Formatted URL (p4:// or 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 = 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" + + # 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: """ @@ -178,7 +248,31 @@ 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] + + # 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 normalized_url.startswith(normalized_depot): + return normalized_url[len(normalized_depot) :].lstrip("/") + + return url def get_repositories( self, query: str | None = None, page_number_limit: int | None = None @@ -189,39 +283,152 @@ 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 - """ - return {} - 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. """ - return [] - - 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. - """ - pass + return [ + { + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", + "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "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": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "password", + "type": "secret", + "label": "Perforce Password", + "placeholder": "••••••••", + "help": "Password for the Perforce user", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "client", + "type": "string", + "label": "Perforce Client/Workspace (Optional)", + "placeholder": "sentry-workspace", + "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", + "required": False, + }, + ] class PerforceIntegrationProvider(IntegrationProvider): @@ -253,7 +460,19 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: Returns: Integration data dictionary """ - return {"external_id": ""} + return { + "name": state.get("name", f"Perforce ({state['host']})"), + "external_id": f"{state['host']}:{state['port']}", + "metadata": { + "host": state["host"], + "port": state["port"], + "user": state["user"], + "password": state.get("password"), + "client": state.get("client"), + "web_url": state.get("web_url"), + "web_viewer_type": state.get("web_viewer_type", "p4web"), + }, + } def post_install( self, @@ -267,7 +486,15 @@ def post_install( def setup(self) -> None: """Setup integration provider.""" - pass + from sentry.plugins.base import bindings + + from .repository import PerforceRepositoryProvider + + bindings.add( + "integration-repository.provider", + PerforceRepositoryProvider, + id="integrations:perforce", + ) class PerforceInstallationView: @@ -286,4 +513,10 @@ def dispatch(self, request, pipeline): the Settings form, we just pass through to create the integration. Users will configure P4 server details in the Settings tab. """ - pass + # 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 b084ca707a0c61..79ec38d9a1bff7 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,44 @@ 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 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 def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -48,7 +87,18 @@ def build_repository_config( Returns: Repository configuration """ - raise NotImplementedError + 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"], + } def compare_commits( self, repo: Repository, start_sha: str | None, end_sha: str @@ -64,7 +114,51 @@ 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 + # 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) + + 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 @@ -79,15 +173,58 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + commits = [] + + 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) + + 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 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.""" - return "" + return repo.config.get("depot_path", repo.name) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index cc64c249c51892..35d601a64bf262 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1176,6 +1176,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..dde8ce98c1a9bf --- /dev/null +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -0,0 +1,438 @@ +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={}, + 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] + + # 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..335b3e5dc14b90 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -0,0 +1,435 @@ +from unittest.mock import patch + +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={}, + 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] + + 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={}, + 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] + + 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={}, + 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] + + 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={}, + 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] + + 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" + + @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") + + @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") + + @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") + + @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 + + 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={}, + 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( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm3", + 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] + + # 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={}, + 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 new file mode 100644 index 00000000000000..1982e5c3e14ae6 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -0,0 +1,475 @@ +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@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 + + @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_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""" + 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"]