From 774fb0d6214f79534dd06b1a60006b244559e619 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 1/7] 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 | 274 ++++++++++- .../integrations/perforce/integration.py | 350 +++++++++++++- .../integrations/perforce/repository.py | 122 ++++- .../sentry/images/logos/logo-perforce.svg | 6 + 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 ++++++++++++++++++ 9 files changed, 2013 insertions(+), 30 deletions(-) create mode 100644 src/sentry/static/sentry/images/logos/logo-perforce.svg 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 84b03121121642..d74afc3e669a20 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,22 @@ 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: + logger.exception("perforce.get_file_failed", extra={"path": path}) + 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,7 +163,21 @@ def _build_depot_path(self, repo: Repository, path: str, ref: str | None = None) Returns: Full depot path with @revision preserved if present """ - return "" + 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 @@ -115,7 +202,50 @@ def get_blame( - date: date of change - description: changelist description """ - return [] + 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]: """ @@ -124,7 +254,17 @@ def get_depot_info(self) -> dict[str, Any]: Returns: Server info dictionary """ - return {} + 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]]: """ @@ -133,7 +273,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 +301,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 +341,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() + + 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 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..9c9a08fd3e3d96 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__) @@ -94,7 +94,31 @@ 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.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: """ @@ -105,6 +129,13 @@ 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: @@ -122,7 +153,14 @@ def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool Returns: True if the filepath matches this repository's depot """ - return False + 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: """ @@ -140,7 +178,15 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) Returns: Formatted URL if the filepath matches this repository, 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: """ @@ -158,7 +204,49 @@ 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 = 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: """ @@ -178,7 +266,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 @@ -189,11 +295,53 @@ 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.""" @@ -206,14 +354,117 @@ def test_connection(self) -> dict[str, Any]: Returns: Dictionary with connection status and server info """ - return {} + 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. These fields will be displayed in the integration settings UI. """ - return [] + 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, + }, + ] def update_organization_config(self, data: MutableMapping[str, Any]) -> None: """ @@ -221,7 +472,52 @@ def update_organization_config(self, data: MutableMapping[str, Any]) -> None: Only tests connection when password or ticket is changed to avoid annoying validations on every field blur. """ - pass + 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): @@ -253,7 +549,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 +575,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 +602,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..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,7 +84,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 +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 @@ -79,15 +161,43 @@ 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.""" - return "" + return repo.config.get("depot_path", repo.name) 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..e640a6e7469cb0 --- /dev/null +++ b/src/sentry/static/sentry/images/logos/logo-perforce.svg @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file 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..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 ecfa2ce81a4b049ec82bdc1e2edbfa2e1d8f477d Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 13:10:55 +0100 Subject: [PATCH 2/7] Added with frontend --- src/sentry/static/sentry/images/logos/logo-perforce.svg | 6 ------ 1 file changed, 6 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 e640a6e7469cb0..00000000000000 --- a/src/sentry/static/sentry/images/logos/logo-perforce.svg +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - \ No newline at end of file From 8cc81457dbeaf8aec3919908fa779c403bf4cb08 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:22:47 +0100 Subject: [PATCH 3/7] Code cleanup --- src/sentry/integrations/perforce/client.py | 90 +--------- .../integrations/perforce/integration.py | 167 ++++-------------- 2 files changed, 37 insertions(+), 220 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index d74afc3e669a20..e995be141c1e78 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -146,7 +146,6 @@ def get_file( raise ApiError(f"File not found: {depot_path}") except self.P4Exception as e: - logger.exception("perforce.get_file_failed", extra={"path": path}) raise ApiError(f"Failed to get file: {e}") finally: self._disconnect(p4) @@ -179,93 +178,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. @@ -358,7 +270,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 9c9a08fd3e3d96..ee13bb2fbd0df5 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 _ @@ -97,26 +97,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 @@ -132,43 +133,18 @@ 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 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 @@ -176,17 +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 """ - 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: """ @@ -222,9 +200,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 @@ -347,30 +329,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -466,59 +424,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 690bb14ec5ce5ea7290e20c72005dd14ce165a31 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:54:48 +0100 Subject: [PATCH 4/7] 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 e995be141c1e78..c6ec5636592bfc 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -285,10 +285,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 c89204c8fa46e243f36776166388681e9b024f52 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:16:38 +0100 Subject: [PATCH 5/7] 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 e0e3a682ebd1fdf374a88d0ee7ca740b94a0a1c7 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:47:27 +0100 Subject: [PATCH 6/7] 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 ee13bb2fbd0df5..9c8bd573105b59 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -262,9 +262,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 dc45b1c99b3b407ea58d6e3bc5a32e6cb46df199 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:52:40 +0100 Subject: [PATCH 7/7] 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 c6ec5636592bfc..59edd972fcde2b 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -294,7 +294,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 ),