From 294c4fd1940ba980f9ca543fd3bbf543623ce2d3 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 01/43] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 4 ++-- src/sentry/integrations/perforce/repository.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index ffb199398c1b63..231fb2c7b1461f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -308,7 +308,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return [] + return None def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -324,7 +324,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + return None def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..0b1170954b3b89 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + return None def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + return None def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + return None def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From 3e0ffc6a40ee5f0fae40daee938e210bf0fb85cc Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 02/43] Fix typing --- src/sentry/integrations/perforce/client.py | 13 +++++++++++-- src/sentry/integrations/perforce/repository.py | 6 +++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 231fb2c7b1461f..82298daf0b5621 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -308,7 +308,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return None + return [] def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -324,7 +324,16 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return None + return [] + + def get_file( + self, repo: Repository, path: str, ref: str | None, codeowners: bool = False + ) -> str: + """ + Get file contents from Perforce depot. + Required by abstract base class but not used (CODEOWNERS feature removed). + """ + raise NotImplementedError("get_file is not supported for Perforce") def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 0b1170954b3b89..b17bb24d42a5a7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return None + return {} def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return None + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return None + return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From 0546b28243cd5b2255a6fe02a0f36a10f8e728d5 Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 03/43] 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 | 114 ++++- .../integrations/perforce/integration.py | 115 ++++- .../integrations/perforce/repository.py | 122 ++++- .../perforce/test_code_mapping.py | 435 +++++++++++++++++ .../perforce/test_stacktrace_link.py | 450 ++++++++++++++++++ 5 files changed, 1207 insertions(+), 29 deletions(-) create mode 100644 tests/sentry/integrations/perforce/test_code_mapping.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 82298daf0b5621..46d34ffd5e9547 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,6 +12,7 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -308,7 +309,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] @@ -324,16 +349,85 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + metrics.incr("integrations.perforce.get_blame_for_files") + blames = [] + p4 = self._connect() - def get_file( - self, repo: Repository, path: str, ref: str | None, codeowners: bool = False - ) -> str: - """ - Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). - """ - raise NotImplementedError("get_file is not supported for Perforce") + try: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the revision/changelist if available + depot_path = self._build_depot_path(file.repo, file.path, file.ref) + + # Use faster p4 filelog approach to get most recent changelist + # This is much faster than p4 annotate + filelog = p4.run("filelog", "-m1", depot_path) + + changelist = None + if filelog and len(filelog) > 0: + # The 'change' field contains the changelist numbers (as a list of strings) + changelists = filelog[0].get("change", []) + if changelists and len(changelists) > 0: + # Get the first (most recent) changelist number + changelist = changelists[0] + + # If we found a changelist, get detailed commit info + if changelist: + try: + change_info = p4.run("describe", "-s", changelist) + if change_info and len(change_info) > 0: + change = change_info[0] + username = change.get("user", "unknown") + # Perforce doesn't provide email by default, so we generate a fallback + email = change.get("email") or f"{username}@perforce.local" + commit = CommitInfo( + commitId=changelist, + committedDate=datetime.fromtimestamp( + int(change.get("time", 0)), tz=timezone.utc + ), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=username, + commitAuthorEmail=email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except self.P4Exception as e: + logger.warning( + "perforce.client.get_blame_for_files.describe_error", + extra={ + **extra, + "changelist": changelist, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + except self.P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.annotate_error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames + finally: + self._disconnect(p4) def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 93e85eeac0e1cb..cfe724e1ece02c 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -210,6 +210,30 @@ def source_url_matches(self, url: str) -> bool: return False + def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: + """ + Check if a file path matches this repository's depot path. + + When SRCSRV transformers remap paths to absolute depot paths (e.g., + //depot/project/src/file.cpp), this method verifies that the depot path + matches the repository's configured depot_path. + + Args: + repo: Repository object + filepath: File path (may be absolute depot path or relative path) + + Returns: + True if the filepath matches this repository's depot + """ + depot_path = repo.config.get("depot_path", repo.name) + + # If filepath is absolute depot path, check if it starts with depot_path + if filepath.startswith("//"): + return filepath.startswith(depot_path) + + # Relative paths always match (will be prepended with depot_path) + return True + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -409,6 +433,30 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] + def test_connection(self) -> dict[str, Any]: + """ + Test the Perforce connection with current credentials. + + Returns: + Dictionary with connection status and server info + """ + try: + client = self.get_client() + info = client.get_depot_info() + + return { + "status": "success", + "message": f"Connected to Perforce server at {info.get('server_address')}", + "server_info": info, + } + except Exception as e: + logger.exception("perforce.test_connection.failed") + return { + "status": "error", + "message": f"Failed to connect to Perforce server: {str(e)}", + "error": str(e), + } + def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -418,20 +466,53 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "p4port", - "type": "string", - "label": "P4PORT (Server Address)", - "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "type": "string", + "label": "Perforce Server Host", + "placeholder": "perforce.company.com", + "help": "The hostname or IP address of your Perforce server", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "port", + "type": "number", + "label": "Perforce Server Port", + "placeholder": "1666", + "help": "The port number for your Perforce server (default: 1666)", + "required": False, + "default": "1666", + "depends_on": {"auth_mode": "password"}, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", - "required": True, + "help": "Username for authenticating with Perforce", + "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "auth_type", @@ -459,6 +540,7 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -468,12 +550,25 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, + { + "name": "web_viewer_type", + "type": "choice", + "label": "Web Viewer Type", + "choices": [ + ["p4web", "P4Web"], + ["swarm", "Helix Swarm"], + ["other", "Other"], + ], + "help": "Type of web viewer (if web URL is provided)", + "required": False, + "default": "p4web", + }, { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", - "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "label": "Web Viewer URL (Optional)", + "placeholder": "https://p4web.company.com", + "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", "required": False, }, ] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..64e223e39976cd 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,12 +4,14 @@ from collections.abc import Mapping, Sequence from typing import Any +from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider from sentry.plugins.providers.integration_repository import RepositoryConfig +from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -33,7 +35,41 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + installation = self.get_installation(config.get("installation"), organization.id) + client = installation.get_client() + + depot_path = config["identifier"] # e.g., //depot or //depot/project + + # Validate depot exists and is accessible + try: + # Create a minimal repo-like object for client + class MockRepo: + def __init__(self, depot_path): + self.config = {"depot_path": depot_path} + + mock_repo = MockRepo(depot_path) + + # Try to check depot access + result = client.check_file(mock_repo, "...", None) + + if result is None: + # Try getting depot info + depots = client.get_depots() + depot_name = depot_path.strip("/").split("/")[0] + + if not any(d["name"] == depot_name for d in depots): + raise IntegrationError( + f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" + ) + + except Exception: + # Don't fail - depot might be valid but empty + pass + + config["external_id"] = depot_path + config["integration_id"] = installation.model.id + + return config def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -48,12 +84,17 @@ def build_repository_config( Returns: Repository configuration """ + depot_path = data["identifier"] + return { - "name": "", - "external_id": "", - "url": "", - "config": {}, - "integration_id": 0, + "name": depot_path, + "external_id": data["external_id"], + "url": f"p4://{depot_path}", + "config": { + "depot_path": depot_path, + "name": depot_path, + }, + "integration_id": data["integration_id"], } def compare_commits( @@ -70,7 +111,42 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + integration_id = repo.integration_id + if integration_id is None: + raise NotImplementedError("Perforce integration requires an integration_id") + + integration = integration_service.get_integration(integration_id=integration_id) + if integration is None: + raise NotImplementedError("Integration not found") + + installation = integration.get_installation(organization_id=repo.organization_id) + client = installation.get_client() + + depot_path = repo.config.get("depot_path", repo.name) + + try: + # Get changelists in range + if start_sha is None: + # Get last N changes + changes = client.get_changes(f"{depot_path}/...", max_changes=20) + else: + # Get changes between start and end + # P4 doesn't have native compare, so get changes up to end_sha + changes = client.get_changes(f"{depot_path}/...", max_changes=100, start_cl=end_sha) + + # Filter to only changes after start_sha + if changes: + start_cl_num = int(start_sha) if start_sha.isdigit() else 0 + changes = [c for c in changes if int(c["change"]) > start_cl_num] + + return self._format_commits(changes, depot_path) + + except Exception as e: + logger.exception( + "perforce.compare_commits.error", + extra={"repo": repo.name, "start": start_sha, "end": end_sha, "error": str(e)}, + ) + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,14 +161,42 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + commits = [] + + for cl in changelists: + # Format timestamp (P4 time is Unix timestamp) + timestamp = self.format_date(int(cl["time"])) + + commits.append( + { + "id": str(cl["change"]), # Changelist number as commit ID + "repository": depot_path, + "author_email": f"{cl['user']}@perforce", # P4 doesn't store email + "author_name": cl["user"], + "message": cl["desc"], + "timestamp": timestamp, + "patch_set": [], # Could fetch with 'p4 describe' if needed + } + ) + + return commits def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. Perforce doesn't have native PRs, but might integrate with Swarm. """ - return "" + web_url = None + if repo.integration_id: + integration = integration_service.get_integration(integration_id=repo.integration_id) + if integration: + web_url = integration.metadata.get("web_url") + + if web_url: + # Swarm review URL format + return f"{web_url}/reviews/{pull_request.key}" + + return f"p4://{repo.name}@{pull_request.key}" def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/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_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 f9afe5599f13dd96422edb68b0328a3321b73b67 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 13:10:55 +0100 Subject: [PATCH 04/43] Added with frontend --- src/sentry/static/sentry/images/logos/logo-perforce.svg | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 src/sentry/static/sentry/images/logos/logo-perforce.svg diff --git a/src/sentry/static/sentry/images/logos/logo-perforce.svg b/src/sentry/static/sentry/images/logos/logo-perforce.svg deleted file mode 100644 index eb8c0c234101f5..00000000000000 --- a/src/sentry/static/sentry/images/logos/logo-perforce.svg +++ /dev/null @@ -1,5 +0,0 @@ - - - - - From 4509509530df7b144b94817bf5cd492f1301f57a Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:22:47 +0100 Subject: [PATCH 05/43] Code cleanup --- src/sentry/integrations/perforce/client.py | 2 +- .../integrations/perforce/integration.py | 55 ++----------------- 2 files changed, 5 insertions(+), 52 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 46d34ffd5e9547..45886910341210 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -366,7 +366,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 cfe724e1ece02c..04e3f760c2236e 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -204,36 +204,13 @@ def source_url_matches(self, url: str) -> bool: if url.startswith("p4://"): return True - web_url = self.model.metadata.get("web_url") - if web_url and url.startswith(web_url): - return True + if self.org_integration: + web_url = self.org_integration.config.get("web_url") + if web_url and url.startswith(web_url): + return True return False - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. - - When SRCSRV transformers remap paths to absolute depot paths (e.g., - //depot/project/src/file.cpp), this method verifies that the depot path - matches the repository's configured depot_path. - - Args: - repo: Repository object - filepath: File path (may be absolute depot path or relative path) - - Returns: - True if the filepath matches this repository's depot - """ - depot_path = repo.config.get("depot_path", repo.name) - - # If filepath is absolute depot path, check if it starts with depot_path - if filepath.startswith("//"): - return filepath.startswith(depot_path) - - # Relative paths always match (will be prepended with depot_path) - return True - def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -433,30 +410,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. From 423bef3634fe36181790dda8cbad33f66e3b4537 Mon Sep 17 00:00:00 2001 From: mujacica Date: Thu, 13 Nov 2025 15:54:48 +0100 Subject: [PATCH 06/43] Fix pr comments and tests --- src/sentry/integrations/perforce/client.py | 10 +++- .../integrations/perforce/repository.py | 54 +++++++++++++------ 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 45886910341210..5c399e879838f4 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -381,10 +381,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 From 345219e6aead3a1cd89ed1432d35164be6abd823 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:16:38 +0100 Subject: [PATCH 07/43] 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 c25296f86a408ba7475c3112f0ab9e3f7438affa Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:47:27 +0100 Subject: [PATCH 08/43] Fix PR reviews and tests --- .../integrations/perforce/test_stacktrace_link.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 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 359abac71865abb9b230e976c5096dd8e2824c4d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 13 Nov 2025 16:52:40 +0100 Subject: [PATCH 09/43] 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 5c399e879838f4..0425c8a1fbb7f1 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -390,7 +390,7 @@ def get_blame_for_files( timestamp = 0 commit = CommitInfo( - commitId=changelist, + commitId=str(changelist), # Ensure string type committedDate=datetime.fromtimestamp( timestamp, tz=timezone.utc ), From 9bb3f7493336b097999c7c45cc06fd9875d62374 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 09:41:19 +0100 Subject: [PATCH 10/43] Remove P4Web (deprecated) and fix paths for swarm --- src/sentry/integrations/perforce/client.py | 2 +- .../integrations/perforce/integration.py | 19 +----- .../perforce/test_code_mapping.py | 41 ++++++------- .../perforce/test_stacktrace_link.py | 61 ------------------- 4 files changed, 24 insertions(+), 99 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 0425c8a1fbb7f1..c2f9dc4dd39d68 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -358,7 +358,7 @@ def get_blame_for_files( try: # Build depot path for the file (includes stream if specified) # file.ref contains the revision/changelist if available - depot_path = self._build_depot_path(file.repo, file.path, file.ref) + depot_path = self.build_depot_path(file.repo, file.path) # Use faster p4 filelog approach to get most recent changelist # This is much faster than p4 annotate diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 04e3f760c2236e..7cd0e0c80e862f 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -503,25 +503,12 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, - { - "name": "web_viewer_type", - "type": "choice", - "label": "Web Viewer Type", - "choices": [ - ["p4web", "P4Web"], - ["swarm", "Helix Swarm"], - ["other", "Other"], - ], - "help": "Type of web viewer (if web URL is provided)", - "required": False, - "default": "p4web", - }, { "name": "web_url", "type": "string", - "label": "Web Viewer URL (Optional)", - "placeholder": "https://p4web.company.com", - "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", + "label": "Helix Swarm URL (Optional)", + "placeholder": "https://swarm.company.com", + "help": "Optional: URL to Helix Swarm web viewer for browsing files", "required": False, }, ] diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index dde8ce98c1a9bf..1cd499f16b58ca 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -375,43 +375,42 @@ def test_symbolic_cpp_path_full_flow(self): url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) assert url == "p4://depot/game/src/main.cpp@42" - def test_full_flow_with_web_viewer(self): - """Test full flow with P4Web viewer configuration""" - integration_with_web = self.create_integration( + def test_full_flow_with_swarm_viewer(self): + """Test full flow with Swarm viewer configuration""" + integration_with_swarm = self.create_integration( organization=self.organization, provider="perforce", name="Perforce", - external_id="perforce-test-web-flow", + external_id="perforce-test-swarm-flow", metadata={}, oi_params={ "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", + "web_url": "https://swarm.example.com", } }, ) - installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] # Create repo with web viewer integration - repo_web = Repository.objects.create( + repo_swarm = Repository.objects.create( name="//depot", organization_id=self.organization.id, - integration_id=integration_with_web.id, + integration_id=integration_with_swarm.id, config={"depot_path": "//depot"}, ) # Use a new project to avoid unique constraint on (project_id, stack_root) - project_web = self.create_project(organization=self.organization) + project_swarm = self.create_project(organization=self.organization) - org_integration_web = integration_with_web.organizationintegration_set.first() - assert org_integration_web is not None + org_integration_swarm = integration_with_swarm.organizationintegration_set.first() + assert org_integration_swarm is not None - code_mapping_web = RepositoryProjectPathConfig.objects.create( - project=project_web, + code_mapping_swarm = RepositoryProjectPathConfig.objects.create( + project=project_swarm, organization_id=self.organization.id, - repository=repo_web, - organization_integration_id=org_integration_web.id, - integration_id=integration_with_web.id, + repository=repo_swarm, + organization_integration_id=org_integration_swarm.id, + integration_id=integration_with_swarm.id, stack_root="depot/", source_root="/", default_branch=None, @@ -426,13 +425,13 @@ def test_full_flow_with_web_viewer(self): # Code mapping mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, - code_mapping=code_mapping_web, + code_mapping=code_mapping_swarm, platform="python", sdk_name="sentry.python", ) - # format_source_url with web viewer (revision extracted from filename) + # format_source_url with Swarm viewer (revision extracted from filename) assert mapped_path is not None - url = installation.format_source_url(repo=repo_web, filepath=mapped_path, branch=None) + url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) - assert url == "https://p4web.example.com//depot/app/services/processor.py?ac=64&rev1=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=42" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 1982e5c3e14ae6..3d96e45054fd36 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -159,67 +159,6 @@ def test_get_stacktrace_config_multiple_code_mappings(self, mock_check_file): assert "//myproject/app/services/handler.py" in result["source_url"] assert result["src_path"] == "app/services/handler.py" - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_with_web_viewer(self, mock_check_file): - """Test stacktrace link with P4Web viewer""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - integration_with_web = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test-web-link", - metadata={}, - oi_params={ - "config": { - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", - } - }, - ) - - # Create new code mapping with new integration - # Use different project to avoid unique constraint - project_web = self.create_project(organization=self.organization) - - repo_web = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=integration_with_web.id, - config={"depot_path": "//depot"}, - ) - - org_integration = integration_with_web.organizationintegration_set.first() - assert org_integration is not None - code_mapping_web = RepositoryProjectPathConfig.objects.create( - project=project_web, - organization_id=self.organization.id, - repository=repo_web, - organization_integration_id=org_integration.id, - integration_id=integration_with_web.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping_web], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "https://p4web.example.com//depot/app/services/processor.py" in result["source_url"] - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_config_abs_path_fallback(self, mock_check_file): """Test stacktrace link uses abs_path when filename is just basename""" From e99bb60ee7e2a1cb3fa8f458fb75a5410ded4722 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:19:48 +0100 Subject: [PATCH 11/43] Implement comments on the SSL/P4Port --- .../integrations/perforce/integration.py | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 7cd0e0c80e862f..e6ce1b763f6dfe 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -26,8 +26,6 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError -from sentry.web.frontend.base import determine_active_organization -from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) @@ -440,23 +438,20 @@ def get_organization_config(self) -> list[dict[str, Any]]: "depends_on": {"auth_mode": "ticket"}, }, { - "name": "host", + "name": "p4port", "type": "string", - "label": "Perforce Server Host", - "placeholder": "perforce.company.com", - "help": "The hostname or IP address of your Perforce server", + "label": "P4PORT (Server Address)", + "placeholder": "ssl:perforce.company.com:1666", + "help": "Perforce server address in P4PORT format. Required even with tickets for multi-server environments. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended.", "required": False, - "depends_on": {"auth_mode": "password"}, }, { - "name": "port", - "type": "number", - "label": "Perforce Server Port", - "placeholder": "1666", - "help": "The port number for your Perforce server (default: 1666)", + "name": "ssl_fingerprint", + "type": "string", + "label": "SSL Fingerprint (Required for SSL)", + "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", + "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "default": "1666", - "depends_on": {"auth_mode": "password"}, }, { "name": "user", From 6b304d9c4bec18da8f7d198152a35b5d79cefc54 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Tue, 18 Nov 2025 10:39:59 +0100 Subject: [PATCH 12/43] Fix PR comments --- src/sentry/integrations/perforce/client.py | 2 - .../integrations/perforce/integration.py | 71 ++----------------- 2 files changed, 7 insertions(+), 66 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index c2f9dc4dd39d68..b66af2a83093be 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -333,8 +333,6 @@ def get_changes( finally: self._disconnect(p4) - # CommitContextClient methods (stubbed for now) - def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] ) -> list[FileBlameInfo]: diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index e6ce1b763f6dfe..34cd743eabd0e3 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -1,31 +1,16 @@ from __future__ import annotations -import hashlib import logging -from collections.abc import Mapping, Sequence -from typing import Any, TypedDict +from typing import TypedDict from django import forms -from django.http import HttpRequest, HttpResponseBase from django.utils.translation import gettext_lazy as _ -from sentry.integrations.base import ( - FeatureDescription, - IntegrationData, - IntegrationFeatures, - IntegrationMetadata, - IntegrationProvider, -) +from sentry.integrations.base import FeatureDescription, IntegrationFeatures, IntegrationMetadata from sentry.integrations.models.integration import Integration from sentry.integrations.perforce.client import PerforceClient -from sentry.integrations.pipeline import IntegrationPipeline -from sentry.integrations.services.repository import RpcRepository from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.repository import RepositoryIntegration -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, ApiUnauthorized, IntegrationError logger = logging.getLogger(__name__) @@ -416,69 +401,28 @@ def get_organization_config(self) -> list[dict[str, Any]]: Current values are provided separately via get_config_data(). """ return [ - { - "name": "auth_mode", - "type": "choice", - "label": "Authentication Mode", - "choices": [ - ["password", "Username & Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", - "required": True, - "default": "password", - }, - { - "name": "ticket", - "type": "secret", - "label": "P4 Ticket", - "placeholder": "••••••••••••••••••••••••••••••••", - "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", - "required": False, - "depends_on": {"auth_mode": "ticket"}, - }, { "name": "p4port", "type": "string", "label": "P4PORT (Server Address)", "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Required even with tickets for multi-server environments. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended.", - "required": False, - }, - { - "name": "ssl_fingerprint", - "type": "string", - "label": "SSL Fingerprint (Required for SSL)", - "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", - "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", - "required": False, + "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "required": True, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce", - "required": False, - "depends_on": {"auth_mode": "password"}, - }, - { - "name": "auth_type", - "type": "choice", - "label": "Authentication Type", - "choices": [ - ["password", "Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Select whether you're providing a password or a P4 ticket. Tickets are obtained via 'p4 login -p' and don't require re-authentication.", + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", "required": True, }, { "name": "password", "type": "secret", - "label": "Password / Ticket", + "label": "Password or P4 Ticket", "placeholder": "••••••••", - "help": "Your Perforce password or P4 authentication ticket (depending on the authentication type selected above).", + "help": "Perforce password OR P4 authentication ticket. Tickets are obtained via 'p4 login -p' and are more secure than passwords. Both are supported in this field.", "required": True, }, { @@ -488,7 +432,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", From 8c0b98d0876d0a001e55da858d698176dfa4260f Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:05:55 +0100 Subject: [PATCH 13/43] Parse file revision properly --- .../perforce/test_code_mapping.py | 28 +++++++++---------- .../perforce/test_stacktrace_link.py | 12 ++++---- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 1cd499f16b58ca..cd9eb0bdf1572c 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -69,8 +69,8 @@ def test_code_mapping_depot_root_to_slash(self): def test_code_mapping_with_symbolic_revision_syntax(self): """ - Test code mapping with Symbolic's @revision syntax. - The @revision should be preserved in the output. + Test code mapping with Symbolic's #revision syntax. + The #revision should be preserved in the output. """ repo = Repository.objects.create( name="//depot", @@ -90,17 +90,17 @@ def test_code_mapping_with_symbolic_revision_syntax(self): default_branch=None, ) - # Test C++ path from Symbolic: depot/game/src/main.cpp@42 + # Test C++ path from Symbolic: depot/game/src/main.cpp#1 frame = EventFrame( - filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" + filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" ) result = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" ) - # Should strip "depot/" and preserve "@42" - assert result == "game/src/main.cpp@42" + # Should strip "depot/" and preserve "#1" + assert result == "game/src/main.cpp#1" def test_code_mapping_multiple_depots(self): """Test code mappings for multiple depots (depot and myproject)""" @@ -362,18 +362,18 @@ def test_symbolic_cpp_path_full_flow(self): """Test full flow: Symbolic C++ -> code mapping -> format_source_url""" # 1. Symbolic transformer sends this path frame = EventFrame( - filename="depot/game/src/main.cpp@42", abs_path="depot/game/src/main.cpp@42" + filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" ) # 2. Code mapping transforms it (use existing code_mapping from setUp) mapped_path = convert_stacktrace_frame_path_to_source_path( frame=frame, code_mapping=self.code_mapping, platform="native", sdk_name="sentry.native" ) - assert mapped_path == "game/src/main.cpp@42" + assert mapped_path == "game/src/main.cpp#1" - # 3. format_source_url creates final URL (preserves @42) + # 3. format_source_url creates final URL (preserves #1) url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/game/src/main.cpp@42" + assert url == "p4://depot/game/src/main.cpp#1" def test_full_flow_with_swarm_viewer(self): """Test full flow with Swarm viewer configuration""" @@ -416,10 +416,10 @@ def test_full_flow_with_swarm_viewer(self): default_branch=None, ) - # Python SDK path with @revision from Symbolic + # Python SDK path with #revision from Symbolic frame = EventFrame( - filename="depot/app/services/processor.py@42", - abs_path="depot/app/services/processor.py@42", + filename="depot/app/services/processor.py#1", + abs_path="depot/app/services/processor.py#1", ) # Code mapping @@ -434,4 +434,4 @@ def test_full_flow_with_swarm_viewer(self): assert mapped_path is not None url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) - assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=42" + assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=1" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 3d96e45054fd36..eab78b9289cef5 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -70,12 +70,12 @@ def test_get_stacktrace_config_python_path(self, mock_check_file): @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): - """Test stacktrace link generation for C++ path with @revision""" + """Test stacktrace link generation for C++ path with #revision""" mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} ctx: StacktraceLinkContext = { - "file": "depot/game/src/main.cpp@42", - "filename": "depot/game/src/main.cpp@42", - "abs_path": "depot/game/src/main.cpp@42", + "file": "depot/game/src/main.cpp#1", + "filename": "depot/game/src/main.cpp#1", + "abs_path": "depot/game/src/main.cpp#1", "platform": "native", "sdk_name": "sentry.native", "commit_id": None, @@ -89,10 +89,10 @@ def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): assert result["source_url"] is not None assert isinstance(result["source_url"], str) - # URL will be encoded: p4://depot/game/src/main.cpp%4042 + # URL will be encoded: p4://depot/game/src/main.cpp%231 assert "depot/game/src/main.cpp" in result["source_url"] assert result["error"] is None - assert result["src_path"] == "game/src/main.cpp@42" + assert result["src_path"] == "game/src/main.cpp#1" def test_get_stacktrace_config_no_matching_code_mapping(self): """Test stacktrace link when no code mapping matches""" From 60b36e1208f3257e8ca66471e80ef9eeae641b52 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:04:12 +0100 Subject: [PATCH 14/43] Fix PR Comments --- src/sentry/integrations/perforce/client.py | 88 +++++++++++++------ .../integrations/perforce/repository.py | 57 ++++++------ 2 files changed, 91 insertions(+), 54 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index b66af2a83093be..4b2dffc8a2ba15 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -295,12 +295,44 @@ def get_user(self, username: str) -> dict[str, Any] | None: # User not found - return None (not an error condition) return None + def get_user(self, username: str) -> dict[str, Any] | None: + """ + Get user information from Perforce. + + Uses p4 user command to fetch user details including email and full name. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_user.html + + Args: + username: Perforce username + + Returns: + User info dictionary with Email and FullName fields, or None if not found + """ + p4 = self._connect() + try: + result = p4.run("user", "-o", username) + if result and len(result) > 0: + user_info = result[0] + return { + "email": user_info.get("Email", ""), + "full_name": user_info.get("FullName", ""), + "username": user_info.get("User", username), + } + return None + except self.P4Exception: + return None + finally: + self._disconnect(p4) + def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None ) -> list[dict[str, Any]]: """ Get changelists for a depot path. + Uses p4 changes command to list changelists. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html + Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return @@ -345,12 +377,19 @@ def get_blame_for_files( Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. + API docs: + - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html + - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html + Returns a list of FileBlameInfo objects containing commit details for each file. """ metrics.incr("integrations.perforce.get_blame_for_files") blames = [] p4 = self._connect() + # Cache user info to avoid multiple lookups for the same user + user_cache: dict[str, dict[str, Any] | None] = {} + try: for file in files: try: @@ -377,8 +416,23 @@ def get_blame_for_files( if change_info and len(change_info) > 0: change = change_info[0] username = change.get("user", "unknown") - # Perforce doesn't provide email by default, so we generate a fallback - email = change.get("email") or f"{username}@perforce.local" + + # Get user information from Perforce for email and full name + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username != "unknown" and username not in user_cache: + user_cache[username] = self.get_user(username) + + user_info = user_cache.get(username) + if user_info: + # Use actual email from Perforce if available + if user_info.get("email"): + author_email = user_info["email"] + # Use full name from Perforce if available + if user_info.get("full_name"): + author_name = user_info["full_name"] # Handle potentially null/invalid time field time_value = change.get("time") or 0 @@ -393,8 +447,8 @@ def get_blame_for_files( timestamp, tz=timezone.utc ), commitMessage=change.get("desc", "").strip(), - commitAuthorName=username, - commitAuthorEmail=email, + commitAuthorName=author_name, + commitAuthorEmail=author_email, ) blame_info = FileBlameInfo( @@ -406,29 +460,9 @@ def get_blame_for_files( commit=commit, ) blames.append(blame_info) - except self.P4Exception as e: - logger.warning( - "perforce.client.get_blame_for_files.describe_error", - extra={ - **extra, - "changelist": changelist, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - }, - ) - except self.P4Exception as e: - # Log but don't fail for individual file errors - logger.warning( - "perforce.client.get_blame_for_files.annotate_error", - extra={ - **extra, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - "file_lineno": file.lineno, - }, - ) + except self.P4Exception: + pass + except self.P4Exception: continue return blames diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 79ec38d9a1bff7..61d6485234240a 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -151,17 +151,13 @@ def compare_commits( continue changes = filtered_changes - return self._format_commits(changes, depot_path) + return self._format_commits(changes, depot_path, client) - except Exception as e: - logger.exception( - "perforce.compare_commits.error", - extra={"repo": repo.name, "start": start_sha, "end": end_sha, "error": str(e)}, - ) + except Exception: return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str + self, changelists: list[dict[str, Any]], depot_path: str, client: Any ) -> Sequence[Mapping[str, Any]]: """ Format Perforce changelists into Sentry commit format. @@ -169,12 +165,16 @@ def _format_commits( Args: changelists: List of changelist dictionaries from P4 depot_path: Depot path + client: Perforce client instance Returns: List of commits in Sentry format """ commits = [] + # Cache user info to avoid multiple lookups for the same user + user_cache: dict[str, dict[str, Any] | None] = {} + for cl in changelists: try: # Handle potentially null/invalid time field @@ -187,23 +187,36 @@ def _format_commits( # Format timestamp (P4 time is Unix timestamp) timestamp = self.format_date(time_int) + # Get user information from Perforce + username = cl.get("user", "unknown") + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache + if username not in user_cache: + user_cache[username] = client.get_user(username) + + user_info = user_cache[username] + if user_info: + # Use actual email from Perforce if available + if user_info.get("email"): + author_email = user_info["email"] + # Use full name from Perforce if available + if user_info.get("full_name"): + author_name = user_info["full_name"] + commits.append( { "id": str(cl["change"]), # Changelist number as commit ID "repository": depot_path, - "author_email": f"{cl.get('user', 'unknown')}@perforce", # P4 doesn't store email - "author_name": cl.get("user", "unknown"), + "author_email": author_email, + "author_name": author_name, "message": cl.get("desc", ""), "timestamp": timestamp, "patch_set": [], # Could fetch with 'p4 describe' if needed } ) - except (KeyError, TypeError) as e: - # Skip malformed changelist data - logger.warning( - "perforce.format_commits.invalid_data", - extra={"changelist": cl, "error": str(e)}, - ) + except (KeyError, TypeError): continue return commits @@ -211,19 +224,9 @@ def _format_commits( def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. - Perforce doesn't have native PRs, but might integrate with Swarm. + Perforce doesn't have native pull requests. """ - web_url = None - if repo.integration_id: - integration = integration_service.get_integration(integration_id=repo.integration_id) - if integration: - web_url = integration.metadata.get("web_url") - - if web_url: - # Swarm review URL format - return f"{web_url}/reviews/{pull_request.key}" - - return f"p4://{repo.name}@{pull_request.key}" + raise NotImplementedError("Perforce does not have native pull requests") def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" From 8487036bb87d99d40db4d98a5c031d4d77b41c6d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:15:10 +0100 Subject: [PATCH 15/43] Fix PR comments --- src/sentry/integrations/perforce/repository.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 61d6485234240a..ab3510cfe70f75 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -192,8 +192,8 @@ def _format_commits( author_email = f"{username}@perforce" author_name = username - # Fetch user info if not in cache - if username not in user_cache: + # Fetch user info if not in cache (skip "unknown" placeholder) + if username != "unknown" and username not in user_cache: user_cache[username] = client.get_user(username) user_info = user_cache[username] From 1fd380e791e80a6126aed73d253e5cd5de1116e7 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:41:11 +0100 Subject: [PATCH 16/43] Fix the PR comment --- src/sentry/integrations/perforce/repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index ab3510cfe70f75..5ffcf4bf51e790 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -196,7 +196,7 @@ def _format_commits( if username != "unknown" and username not in user_cache: user_cache[username] = client.get_user(username) - user_info = user_cache[username] + user_info = user_cache.get(username) if user_info: # Use actual email from Perforce if available if user_info.get("email"): From a73e56729264d33ebb2be26b2cd08181a91c15d0 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:49:13 +0100 Subject: [PATCH 17/43] Simplify for installation only --- src/sentry/integrations/perforce/client.py | 118 +---- .../integrations/perforce/repository.py | 156 +------ .../perforce/test_code_mapping.py | 437 ------------------ .../perforce/test_stacktrace_link.py | 414 ----------------- 4 files changed, 13 insertions(+), 1112 deletions(-) delete mode 100644 tests/sentry/integrations/perforce/test_code_mapping.py delete mode 100644 tests/sentry/integrations/perforce/test_stacktrace_link.py diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 4b2dffc8a2ba15..e0debaf2253d9e 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,7 +12,6 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, - CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -330,9 +329,6 @@ def get_changes( """ Get changelists for a depot path. - Uses p4 changes command to list changelists. - API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html - Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return @@ -341,29 +337,7 @@ def get_changes( Returns: List of changelist dictionaries """ - p4 = self._connect() - try: - args = ["-m", str(max_changes), "-l"] - - if start_cl: - args.extend(["-e", start_cl]) - - args.append(depot_path) - - changes = p4.run("changes", *args) - - return [ - { - "change": change.get("change"), - "user": change.get("user"), - "client": change.get("client"), - "time": change.get("time"), - "desc": change.get("desc"), - } - for change in changes - ] - finally: - self._disconnect(p4) + return [] def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -377,97 +351,9 @@ def get_blame_for_files( Note: This does not provide line-specific blame. It returns the most recent changelist for the entire file, which is sufficient for suspect commit detection. - API docs: - - p4 filelog: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_filelog.html - - p4 describe: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_describe.html - Returns a list of FileBlameInfo objects containing commit details for each file. """ - metrics.incr("integrations.perforce.get_blame_for_files") - blames = [] - p4 = self._connect() - - # Cache user info to avoid multiple lookups for the same user - user_cache: dict[str, dict[str, Any] | None] = {} - - try: - for file in files: - try: - # Build depot path for the file (includes stream if specified) - # file.ref contains the revision/changelist if available - depot_path = self.build_depot_path(file.repo, file.path) - - # Use faster p4 filelog approach to get most recent changelist - # This is much faster than p4 annotate - filelog = p4.run("filelog", "-m1", depot_path) - - changelist = None - if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list) - changelists = filelog[0].get("change", []) - if changelists and len(changelists) > 0: - # Get the first (most recent) changelist number - changelist = changelists[0] - - # If we found a changelist, get detailed commit info - if changelist: - try: - change_info = p4.run("describe", "-s", changelist) - if change_info and len(change_info) > 0: - change = change_info[0] - username = change.get("user", "unknown") - - # Get user information from Perforce for email and full name - author_email = f"{username}@perforce" - author_name = username - - # Fetch user info if not in cache - if username != "unknown" and username not in user_cache: - user_cache[username] = self.get_user(username) - - user_info = user_cache.get(username) - if user_info: - # Use actual email from Perforce if available - if user_info.get("email"): - author_email = user_info["email"] - # Use full name from Perforce if available - if user_info.get("full_name"): - author_name = user_info["full_name"] - - # Handle potentially null/invalid time field - time_value = change.get("time") or 0 - try: - timestamp = int(time_value) - except (TypeError, ValueError): - timestamp = 0 - - commit = CommitInfo( - commitId=str(changelist), # Ensure string type - committedDate=datetime.fromtimestamp( - timestamp, tz=timezone.utc - ), - commitMessage=change.get("desc", "").strip(), - commitAuthorName=author_name, - commitAuthorEmail=author_email, - ) - - blame_info = FileBlameInfo( - lineno=file.lineno, - path=file.path, - ref=file.ref, - repo=file.repo, - code_mapping=file.code_mapping, - commit=commit, - ) - blames.append(blame_info) - except self.P4Exception: - pass - except self.P4Exception: - continue - - return blames - finally: - self._disconnect(p4) + return [] def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 5ffcf4bf51e790..b17bb24d42a5a7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,14 +4,12 @@ from collections.abc import Mapping, Sequence from typing import Any -from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider from sentry.plugins.providers.integration_repository import RepositoryConfig -from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -35,44 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - installation = self.get_installation(config.get("installation"), organization.id) - client = installation.get_client() - - depot_path = config["identifier"] # e.g., //depot or //depot/project - - # Validate depot exists and is accessible - try: - # Create a minimal repo-like object for client - class MockRepo: - def __init__(self, depot_path): - self.config = {"depot_path": depot_path} - - mock_repo = MockRepo(depot_path) - - # Try to check depot access - result = client.check_file(mock_repo, "...", None) - - if result is None: - # Try getting depot info - depots = client.get_depots() - depot_name = depot_path.strip("/").split("/")[0] - - if not any(d["name"] == depot_name for d in depots): - raise IntegrationError( - f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" - ) - - except IntegrationError: - # Re-raise validation errors so user sees them - raise - except Exception: - # Catch only connection/P4 errors - depot might be valid but temporarily unreachable - pass - - config["external_id"] = depot_path - config["integration_id"] = installation.model.id - - return config + return {} def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -87,17 +48,12 @@ def build_repository_config( Returns: Repository configuration """ - depot_path = data["identifier"] - return { - "name": depot_path, - "external_id": data["external_id"], - "url": f"p4://{depot_path}", - "config": { - "depot_path": depot_path, - "name": depot_path, - }, - "integration_id": data["integration_id"], + "name": "", + "external_id": "", + "url": "", + "config": {}, + "integration_id": 0, } def compare_commits( @@ -114,50 +70,10 @@ def compare_commits( Returns: List of changelist dictionaries """ - integration_id = repo.integration_id - if integration_id is None: - raise NotImplementedError("Perforce integration requires an integration_id") - - integration = integration_service.get_integration(integration_id=integration_id) - if integration is None: - raise NotImplementedError("Integration not found") - - installation = integration.get_installation(organization_id=repo.organization_id) - client = installation.get_client() - - depot_path = repo.config.get("depot_path", repo.name) - - try: - # Get changelists in range - if start_sha is None: - # Get last N changes - changes = client.get_changes(f"{depot_path}/...", max_changes=20) - else: - # Get changes between start and end - # P4 doesn't have native compare, so get changes up to end_sha - changes = client.get_changes(f"{depot_path}/...", max_changes=100, start_cl=end_sha) - - # Filter to only changes after start_sha - if changes: - start_cl_num = int(start_sha) if start_sha.isdigit() else 0 - # Filter out invalid changelist data - filtered_changes = [] - for c in changes: - try: - if int(c["change"]) > start_cl_num: - filtered_changes.append(c) - except (TypeError, ValueError, KeyError): - # Skip malformed changelist data - continue - changes = filtered_changes - - return self._format_commits(changes, depot_path, client) - - except Exception: - return [] + return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str, client: Any + self, changelists: list[dict[str, Any]], depot_path: str ) -> Sequence[Mapping[str, Any]]: """ Format Perforce changelists into Sentry commit format. @@ -165,68 +81,18 @@ def _format_commits( Args: changelists: List of changelist dictionaries from P4 depot_path: Depot path - client: Perforce client instance Returns: List of commits in Sentry format """ - commits = [] - - # Cache user info to avoid multiple lookups for the same user - user_cache: dict[str, dict[str, Any] | None] = {} - - for cl in changelists: - try: - # Handle potentially null/invalid time field - time_value = cl.get("time") or 0 - try: - time_int = int(time_value) - except (TypeError, ValueError): - time_int = 0 - - # Format timestamp (P4 time is Unix timestamp) - timestamp = self.format_date(time_int) - - # Get user information from Perforce - username = cl.get("user", "unknown") - author_email = f"{username}@perforce" - author_name = username - - # Fetch user info if not in cache (skip "unknown" placeholder) - if username != "unknown" and username not in user_cache: - user_cache[username] = client.get_user(username) - - user_info = user_cache.get(username) - if user_info: - # Use actual email from Perforce if available - if user_info.get("email"): - author_email = user_info["email"] - # Use full name from Perforce if available - if user_info.get("full_name"): - author_name = user_info["full_name"] - - commits.append( - { - "id": str(cl["change"]), # Changelist number as commit ID - "repository": depot_path, - "author_email": author_email, - "author_name": author_name, - "message": cl.get("desc", ""), - "timestamp": timestamp, - "patch_set": [], # Could fetch with 'p4 describe' if needed - } - ) - except (KeyError, TypeError): - continue - - return commits + return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. - Perforce doesn't have native pull requests. + Perforce doesn't have native PRs, but might integrate with Swarm. """ - raise NotImplementedError("Perforce does not have native pull requests") + return "" def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py deleted file mode 100644 index cd9eb0bdf1572c..00000000000000 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ /dev/null @@ -1,437 +0,0 @@ -from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig -from sentry.integrations.perforce.integration import ( - PerforceIntegration, - PerforceIntegrationProvider, -) -from sentry.issues.auto_source_code_config.code_mapping import ( - convert_stacktrace_frame_path_to_source_path, -) -from sentry.models.repository import Repository -from sentry.testutils.cases import IntegrationTestCase -from sentry.utils.event_frames import EventFrame - - -class PerforceCodeMappingTest(IntegrationTestCase): - """Tests for code mapping integration with Perforce""" - - provider = PerforceIntegrationProvider - installation: PerforceIntegration - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - self.org_integration = self.integration.organizationintegration_set.first() - assert self.org_integration is not None - - def test_code_mapping_depot_root_to_slash(self): - """ - Test code mapping: depot/ -> / - This is the correct mapping for Perforce where depot name is part of path. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Test Python SDK path: depot/app/services/processor.py - frame = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should strip "depot/" leaving "app/services/processor.py" - assert result == "app/services/processor.py" - - def test_code_mapping_with_symbolic_revision_syntax(self): - """ - Test code mapping with Symbolic's #revision syntax. - The #revision should be preserved in the output. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Test C++ path from Symbolic: depot/game/src/main.cpp#1 - frame = EventFrame( - filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" - ) - - # Should strip "depot/" and preserve "#1" - assert result == "game/src/main.cpp#1" - - def test_code_mapping_multiple_depots(self): - """Test code mappings for multiple depots (depot and myproject)""" - depot_repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - myproject_repo = Repository.objects.create( - name="//myproject", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - depot_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=depot_repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="myproject/", - source_root="/", - default_branch=None, - ) - - # Test depot path - frame1 = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - result1 = convert_stacktrace_frame_path_to_source_path( - frame=frame1, code_mapping=depot_mapping, platform="python", sdk_name="sentry.python" - ) - assert result1 == "app/services/processor.py" - - # Test myproject path - frame2 = EventFrame( - filename="myproject/app/services/handler.py", - abs_path="myproject/app/services/handler.py", - ) - - result2 = convert_stacktrace_frame_path_to_source_path( - frame=frame2, - code_mapping=myproject_mapping, - platform="python", - sdk_name="sentry.python", - ) - assert result2 == "app/services/handler.py" - - def test_code_mapping_no_match_different_depot(self): - """Test that code mapping doesn't match paths from different depots""" - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Try to match a path from different depot - frame = EventFrame( - filename="myproject/app/services/processor.py", - abs_path="myproject/app/services/processor.py", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should not match - assert result is None - - def test_code_mapping_abs_path_fallback(self): - """Test that code mapping works with abs_path when filename is just basename""" - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Some platforms only provide basename in filename - frame = EventFrame(filename="processor.py", abs_path="depot/app/services/processor.py") - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="python", sdk_name="sentry.python" - ) - - # Should use abs_path and strip "depot/" - assert result == "app/services/processor.py" - - def test_code_mapping_nested_depot_paths(self): - """Test code mapping with nested depot paths""" - repo = Repository.objects.create( - name="//depot/game/project", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot/game/project"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/game/project/", - source_root="/", - default_branch=None, - ) - - frame = EventFrame( - filename="depot/game/project/src/main.cpp", - abs_path="depot/game/project/src/main.cpp", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" - ) - - assert result == "src/main.cpp" - - def test_code_mapping_preserves_windows_backslash_conversion(self): - """ - Test that code mapping handles Windows-style paths. - - Note: The generic code_mapping system does not automatically convert - backslashes to forward slashes. SDKs should normalize paths before - sending to Sentry. This test documents current behavior. - """ - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Windows-style path with backslashes - frame = EventFrame( - filename="depot\\app\\services\\processor.cpp", - abs_path="depot\\app\\services\\processor.cpp", - ) - - result = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=code_mapping, platform="native", sdk_name="sentry.native" - ) - - # Generic code mapping doesn't normalize backslashes - returns None - # SDKs should normalize paths to forward slashes before sending - assert result is None - - -class PerforceEndToEndCodeMappingTest(IntegrationTestCase): - """End-to-end tests for code mapping + format_source_url flow""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - self.org_integration = self.integration.organizationintegration_set.first() - assert self.org_integration is not None - - self.repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - self.code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=self.repo, - organization_integration_id=self.org_integration.id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - def test_python_sdk_path_full_flow(self): - """Test full flow: Python SDK -> code mapping -> format_source_url""" - # 1. Python SDK sends this path - frame = EventFrame( - filename="depot/app/services/processor.py", - abs_path="depot/app/services/processor.py", - ) - - # 2. Code mapping transforms it - mapped_path = convert_stacktrace_frame_path_to_source_path( - frame=frame, - code_mapping=self.code_mapping, - platform="python", - sdk_name="sentry.python", - ) - assert mapped_path == "app/services/processor.py" - - # 3. format_source_url creates final URL - url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/app/services/processor.py" - - def test_symbolic_cpp_path_full_flow(self): - """Test full flow: Symbolic C++ -> code mapping -> format_source_url""" - # 1. Symbolic transformer sends this path - frame = EventFrame( - filename="depot/game/src/main.cpp#1", abs_path="depot/game/src/main.cpp#1" - ) - - # 2. Code mapping transforms it (use existing code_mapping from setUp) - mapped_path = convert_stacktrace_frame_path_to_source_path( - frame=frame, code_mapping=self.code_mapping, platform="native", sdk_name="sentry.native" - ) - assert mapped_path == "game/src/main.cpp#1" - - # 3. format_source_url creates final URL (preserves #1) - url = self.installation.format_source_url(repo=self.repo, filepath=mapped_path, branch=None) - assert url == "p4://depot/game/src/main.cpp#1" - - def test_full_flow_with_swarm_viewer(self): - """Test full flow with Swarm viewer configuration""" - integration_with_swarm = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test-swarm-flow", - metadata={}, - oi_params={ - "config": { - "web_url": "https://swarm.example.com", - } - }, - ) - installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] - - # Create repo with web viewer integration - repo_swarm = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=integration_with_swarm.id, - config={"depot_path": "//depot"}, - ) - - # Use a new project to avoid unique constraint on (project_id, stack_root) - project_swarm = self.create_project(organization=self.organization) - - org_integration_swarm = integration_with_swarm.organizationintegration_set.first() - assert org_integration_swarm is not None - - code_mapping_swarm = RepositoryProjectPathConfig.objects.create( - project=project_swarm, - organization_id=self.organization.id, - repository=repo_swarm, - organization_integration_id=org_integration_swarm.id, - integration_id=integration_with_swarm.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Python SDK path with #revision from Symbolic - frame = EventFrame( - filename="depot/app/services/processor.py#1", - abs_path="depot/app/services/processor.py#1", - ) - - # Code mapping - mapped_path = convert_stacktrace_frame_path_to_source_path( - frame=frame, - code_mapping=code_mapping_swarm, - platform="python", - sdk_name="sentry.python", - ) - - # format_source_url with Swarm viewer (revision extracted from filename) - assert mapped_path is not None - url = installation.format_source_url(repo=repo_swarm, filepath=mapped_path, branch=None) - - assert url == "https://swarm.example.com/files//depot/app/services/processor.py?v=1" diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py deleted file mode 100644 index eab78b9289cef5..00000000000000 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ /dev/null @@ -1,414 +0,0 @@ -from unittest.mock import patch - -from sentry.integrations.models.repository_project_path_config import RepositoryProjectPathConfig -from sentry.integrations.perforce.integration import PerforceIntegrationProvider -from sentry.integrations.utils.stacktrace_link import get_stacktrace_config -from sentry.issues.endpoints.project_stacktrace_link import StacktraceLinkContext -from sentry.models.repository import Repository -from sentry.testutils.cases import IntegrationTestCase - - -class PerforceStacktraceLinkTest(IntegrationTestCase): - """Tests for Perforce stacktrace link generation""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.installation = self.integration.get_installation(self.organization.id) - self.project = self.create_project(organization=self.organization) - - self.repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - self.code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=self.repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_python_path(self, mock_check_file): - """Test stacktrace link generation for Python SDK path""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - assert result["error"] is None - assert result["src_path"] == "app/services/processor.py" - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_cpp_path_with_revision(self, mock_check_file): - """Test stacktrace link generation for C++ path with #revision""" - mock_check_file.return_value = {"depotFile": "//depot/game/src/main.cpp"} - ctx: StacktraceLinkContext = { - "file": "depot/game/src/main.cpp#1", - "filename": "depot/game/src/main.cpp#1", - "abs_path": "depot/game/src/main.cpp#1", - "platform": "native", - "sdk_name": "sentry.native", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - # URL will be encoded: p4://depot/game/src/main.cpp%231 - assert "depot/game/src/main.cpp" in result["source_url"] - assert result["error"] is None - assert result["src_path"] == "game/src/main.cpp#1" - - def test_get_stacktrace_config_no_matching_code_mapping(self): - """Test stacktrace link when no code mapping matches""" - ctx: StacktraceLinkContext = { - "file": "other/app/services/processor.py", - "filename": "other/app/services/processor.py", - "abs_path": "other/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is None - assert result["error"] == "stack_root_mismatch" - assert result["src_path"] is None - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_multiple_code_mappings(self, mock_check_file): - """Test stacktrace link with multiple code mappings""" - mock_check_file.return_value = {"depotFile": "//myproject/app/services/handler.py"} - # Add another depot mapping - myproject_repo = Repository.objects.create( - name="//myproject", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="myproject/", - source_root="/", - default_branch=None, - ) - - # Test with myproject path - ctx: StacktraceLinkContext = { - "file": "myproject/app/services/handler.py", - "filename": "myproject/app/services/handler.py", - "abs_path": "myproject/app/services/handler.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//myproject/app/services/handler.py" in result["source_url"] - assert result["src_path"] == "app/services/handler.py" - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_abs_path_fallback(self, mock_check_file): - """Test stacktrace link uses abs_path when filename is just basename""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - ctx: StacktraceLinkContext = { - "file": "processor.py", - "filename": "processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - assert result["src_path"] == "app/services/processor.py" - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_iteration_count(self, mock_check_file): - """Test that iteration_count is incremented only for matching mappings""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - # Add a non-matching mapping - other_repo = Repository.objects.create( - name="//other", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//other"}, - ) - - other_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=other_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="other/", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([other_mapping, self.code_mapping], ctx) - - # iteration_count should be 1 (only depot mapping matched) - assert result["iteration_count"] == 1 - assert result["source_url"] is not None - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_get_stacktrace_config_stops_on_first_match(self, mock_check_file): - """Test that iteration stops after first successful match""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - # Add another depot mapping (shouldn't be checked if first matches) - # Use different project to avoid unique constraint - project2 = self.create_project(organization=self.organization) - - myproject_repo = Repository.objects.create( - name="//myproject", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//myproject"}, - ) - - myproject_mapping = RepositoryProjectPathConfig.objects.create( - project=project2, - organization_id=self.organization.id, - repository=myproject_repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", # Same stack_root as depot mapping (but different project) - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([self.code_mapping, myproject_mapping], ctx) - - # Should stop after first match (depot mapping) - assert result["iteration_count"] == 1 - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/app/services/processor.py" in result["source_url"] - - -class PerforceStacktraceLinkEdgeCasesTest(IntegrationTestCase): - """Edge case tests for Perforce stacktrace links""" - - provider = PerforceIntegrationProvider - - def setUp(self): - super().setUp() - self.integration = self.create_integration( - organization=self.organization, - provider="perforce", - name="Perforce", - external_id="perforce-test", - metadata={}, - ) - self.project = self.create_project(organization=self.organization) - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_stacktrace_link_empty_stack_root(self, mock_check_file): - """Test stacktrace link with empty stack_root (shouldn't match anything)""" - mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.py"} - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="", - source_root="/", - default_branch=None, - ) - - ctx: StacktraceLinkContext = { - "file": "depot/app/services/processor.py", - "filename": "depot/app/services/processor.py", - "abs_path": "depot/app/services/processor.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - # Empty stack_root should match any path - assert result["source_url"] is not None - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_stacktrace_link_with_special_characters_in_path(self, mock_check_file): - """Test stacktrace link with special characters in file path""" - mock_check_file.return_value = {"depotFile": "//depot/app/my services/processor-v2.py"} - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - # Path with spaces and special chars - ctx: StacktraceLinkContext = { - "file": "depot/app/my services/processor-v2.py", - "filename": "depot/app/my services/processor-v2.py", - "abs_path": "depot/app/my services/processor-v2.py", - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - assert result["source_url"] is not None - assert result["src_path"] == "app/my services/processor-v2.py" - - @patch("sentry.integrations.perforce.client.PerforceClient.check_file") - def test_stacktrace_link_deeply_nested_path(self, mock_check_file): - """Test stacktrace link with very deeply nested path""" - mock_check_file.return_value = {"depotFile": "//depot/file.py"} - repo = Repository.objects.create( - name="//depot", - organization_id=self.organization.id, - integration_id=self.integration.id, - config={"depot_path": "//depot"}, - ) - - code_mapping = RepositoryProjectPathConfig.objects.create( - project=self.project, - organization_id=self.organization.id, - repository=repo, - organization_integration_id=self.integration.organizationintegration_set.first().id, - integration_id=self.integration.id, - stack_root="depot/", - source_root="/", - default_branch=None, - ) - - deep_path = "depot/" + "/".join([f"level{i}" for i in range(20)]) + "/file.py" - - ctx: StacktraceLinkContext = { - "file": deep_path, - "filename": deep_path, - "abs_path": deep_path, - "platform": "python", - "sdk_name": "sentry.python", - "commit_id": None, - "group_id": None, - "line_no": None, - "module": None, - "package": None, - } - - result = get_stacktrace_config([code_mapping], ctx) - - assert result["source_url"] is not None - assert isinstance(result["source_url"], str) - assert "//depot/" in result["source_url"] From aa56f83807788bced82c4497d3d3e116cfeccc70 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 14:34:37 +0100 Subject: [PATCH 18/43] Rework based on the PR comments --- src/sentry/integrations/perforce/client.py | 33 ------------------- .../integrations/perforce/integration.py | 24 +++++++++++--- 2 files changed, 20 insertions(+), 37 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index e0debaf2253d9e..4eedcbc7f3299b 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -282,10 +282,6 @@ def get_user(self, username: str) -> dict[str, Any] | None: result = p4.run("user", "-o", username) if result and len(result) > 0: user_info = result[0] - # p4 user -o returns a template for non-existent users - # Check if user actually exists by verifying Update field is set - if not user_info.get("Update"): - return None return { "email": user_info.get("Email", ""), "full_name": user_info.get("FullName", ""), @@ -294,35 +290,6 @@ def get_user(self, username: str) -> dict[str, Any] | None: # User not found - return None (not an error condition) return None - def get_user(self, username: str) -> dict[str, Any] | None: - """ - Get user information from Perforce. - - Uses p4 user command to fetch user details including email and full name. - API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_user.html - - Args: - username: Perforce username - - Returns: - User info dictionary with Email and FullName fields, or None if not found - """ - p4 = self._connect() - try: - result = p4.run("user", "-o", username) - if result and len(result) > 0: - user_info = result[0] - return { - "email": user_info.get("Email", ""), - "full_name": user_info.get("FullName", ""), - "username": user_info.get("User", username), - } - return None - except self.P4Exception: - return None - finally: - self._disconnect(p4) - def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None ) -> list[dict[str, Any]]: diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 34cd743eabd0e3..4499294c355c4f 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -187,10 +187,9 @@ def source_url_matches(self, url: str) -> bool: if url.startswith("p4://"): return True - if self.org_integration: - web_url = self.org_integration.config.get("web_url") - if web_url and url.startswith(web_url): - return True + web_url = self.model.metadata.get("web_url") + if web_url and url.startswith(web_url): + return True return False @@ -565,6 +564,23 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: "password": installation_data.get("password", ""), } + # Add optional fields if provided + if installation_data.get("client"): + metadata["client"] = installation_data["client"] + + if installation_data.get("ssl_fingerprint"): + metadata["ssl_fingerprint"] = installation_data["ssl_fingerprint"] + + if installation_data.get("web_url"): + metadata["web_url"] = installation_data["web_url"] + + # Store credentials in Integration.metadata + metadata = { + "p4port": p4port, + "user": installation_data.get("user", ""), + "password": installation_data.get("password", ""), + } + # Add optional fields if provided if installation_data.get("client"): metadata["client"] = installation_data["client"] From 582f8de40b9fcd4b5e20fb2e8697730e740388c3 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:04:14 +0100 Subject: [PATCH 19/43] Fix PR comments from Cursor --- src/sentry/integrations/perforce/client.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 4eedcbc7f3299b..ffb199398c1b63 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -282,6 +282,10 @@ def get_user(self, username: str) -> dict[str, Any] | None: result = p4.run("user", "-o", username) if result and len(result) > 0: user_info = result[0] + # p4 user -o returns a template for non-existent users + # Check if user actually exists by verifying Update field is set + if not user_info.get("Update"): + return None return { "email": user_info.get("Email", ""), "full_name": user_info.get("FullName", ""), From 19478f891ea704d32dcef57d82a7faa6fdfedf39 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:15:28 +0100 Subject: [PATCH 20/43] More cursor comment fixes --- src/sentry/integrations/perforce/integration.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 4499294c355c4f..2cc46974e9804d 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -338,7 +338,7 @@ def get_repositories( page_number_limit: Ignored (kept for base class compatibility) Returns: - List of repository dictionaries + List of repository dictionaries (limited by page_number_limit if provided) """ try: client = self.get_client() @@ -361,6 +361,10 @@ def get_repositories( } ) + # Apply pagination limit if specified + if page_number_limit and len(repositories) >= page_number_limit: + break + return repositories except ApiError: From 4746b2d8240558d598c27ce0302eb3ef7202f43e Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 15:30:52 +0100 Subject: [PATCH 21/43] Even more cursor comments --- src/sentry/integrations/perforce/integration.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 2cc46974e9804d..e3d8a7de478630 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -11,6 +11,11 @@ from sentry.integrations.perforce.client import PerforceClient from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.repository import RepositoryIntegration +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, ApiUnauthorized, IntegrationError +from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) From e1e5cba78a241cb63b589af5547c2835724dbbbf Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:34:20 +0100 Subject: [PATCH 22/43] Fix ticket authentication --- src/sentry/integrations/perforce/integration.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index e3d8a7de478630..c06b951d601ce9 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -425,12 +425,23 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", "required": True, }, + { + "name": "auth_type", + "type": "choice", + "label": "Authentication Type", + "choices": [ + ["password", "Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Select whether you're providing a password or a P4 ticket. Tickets are obtained via 'p4 login -p' and don't require re-authentication.", + "required": True, + }, { "name": "password", "type": "secret", - "label": "Password or P4 Ticket", + "label": "Password / Ticket", "placeholder": "••••••••", - "help": "Perforce password OR P4 authentication ticket. Tickets are obtained via 'p4 login -p' and are more secure than passwords. Both are supported in this field.", + "help": "Your Perforce password or P4 authentication ticket (depending on the authentication type selected above).", "required": True, }, { @@ -587,6 +598,7 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: metadata = { "p4port": p4port, "user": installation_data.get("user", ""), + "auth_type": installation_data.get("auth_type", "password"), # Default to password "password": installation_data.get("password", ""), } From cc7b0a5a5d449c139d53ab4c50a14d9f5c135630 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:47:37 +0100 Subject: [PATCH 23/43] Fix trust order of operations --- src/sentry/integrations/perforce/client.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index ffb199398c1b63..77b857bb8fc17d 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -85,9 +85,12 @@ def _connect(self): if self.client_name: p4.client = self.client_name - p4.exception_level = 1 # Only errors raise exceptions + # Lower exception level to allow connection with SSL trust warnings + # After connection, we'll assert the fingerprint with run_trust + p4.exception_level = 0 # Warnings don't raise exceptions # Connect to Perforce server + # For SSL connections, this may succeed with warnings about trust try: p4.connect() except P4Exception as e: @@ -115,6 +118,9 @@ def _connect(self): f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) + # Restore normal exception level for subsequent commands + p4.exception_level = 1 # Only errors raise exceptions + # Authenticate based on auth_type # - password: Requires run_login() to exchange password for session ticket # - ticket: Already authenticated via p4.password, no login needed From 15ed31db444964a93a2068a3e83b89b9ed3824a9 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 16:57:39 +0100 Subject: [PATCH 24/43] Fix auth type issues --- src/sentry/integrations/perforce/client.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 77b857bb8fc17d..ffb199398c1b63 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -85,12 +85,9 @@ def _connect(self): if self.client_name: p4.client = self.client_name - # Lower exception level to allow connection with SSL trust warnings - # After connection, we'll assert the fingerprint with run_trust - p4.exception_level = 0 # Warnings don't raise exceptions + p4.exception_level = 1 # Only errors raise exceptions # Connect to Perforce server - # For SSL connections, this may succeed with warnings about trust try: p4.connect() except P4Exception as e: @@ -118,9 +115,6 @@ def _connect(self): f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" ) - # Restore normal exception level for subsequent commands - p4.exception_level = 1 # Only errors raise exceptions - # Authenticate based on auth_type # - password: Requires run_login() to exchange password for session ticket # - ticket: Already authenticated via p4.password, no login needed From bbcd0d7f31965bb7b338bfdbbf21cfa2b719fe0e Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 20 Nov 2025 17:40:37 +0100 Subject: [PATCH 25/43] Fix external id 64-char limit --- src/sentry/integrations/perforce/integration.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index c06b951d601ce9..cc3fa91d5153da 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -1,5 +1,6 @@ from __future__ import annotations +import hashlib import logging from typing import TypedDict From bac06b7c18c5c46e3a8c393d18f81b5dde54fe2a Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:21:35 +0100 Subject: [PATCH 26/43] Restore deleted logo --- src/sentry/static/sentry/images/logos/logo-perforce.svg | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 src/sentry/static/sentry/images/logos/logo-perforce.svg diff --git a/src/sentry/static/sentry/images/logos/logo-perforce.svg b/src/sentry/static/sentry/images/logos/logo-perforce.svg new file mode 100644 index 00000000000000..eb8c0c234101f5 --- /dev/null +++ b/src/sentry/static/sentry/images/logos/logo-perforce.svg @@ -0,0 +1,5 @@ + + + + + From 49ea36322c217b297b0c781eac692c681e7065dc Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:35:59 +0100 Subject: [PATCH 27/43] Fix stale config after update --- src/sentry/integrations/perforce/integration.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index cc3fa91d5153da..e697d6db0a466e 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -499,6 +499,7 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: data: Updated configuration data from the form (only changed fields) """ from sentry.integrations.services.integration import integration_service + from sentry.models.integrations.integration import Integration # Update integration metadata with new values metadata = dict(self.model.metadata) # Create a mutable copy From 921aae03a210dfddea24517aed26e04cd1c0abc4 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 24 Nov 2025 11:48:06 +0100 Subject: [PATCH 28/43] Fix cursor comment --- src/sentry/integrations/perforce/integration.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index e697d6db0a466e..cc3fa91d5153da 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -499,7 +499,6 @@ def update_organization_config(self, data: Mapping[str, Any]) -> None: data: Updated configuration data from the form (only changed fields) """ from sentry.integrations.services.integration import integration_service - from sentry.models.integrations.integration import Integration # Update integration metadata with new values metadata = dict(self.model.metadata) # Create a mutable copy From f8d03a640dbafa631a49c7d5f37d62ede99cf611 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Mon, 1 Dec 2025 10:08:10 +0100 Subject: [PATCH 29/43] Review comments fixes --- .../integrations/perforce/integration.py | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index cc3fa91d5153da..ececb74576b409 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -16,6 +16,7 @@ from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError +from sentry.web.frontend.base import determine_active_organization from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) @@ -344,7 +345,7 @@ def get_repositories( page_number_limit: Ignored (kept for base class compatibility) Returns: - List of repository dictionaries (limited by page_number_limit if provided) + List of repository dictionaries """ try: client = self.get_client() @@ -367,10 +368,6 @@ def get_repositories( } ) - # Apply pagination limit if specified - if page_number_limit and len(repositories) >= page_number_limit: - break - return repositories except ApiError: @@ -585,24 +582,6 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: "password": installation_data.get("password", ""), } - # Add optional fields if provided - if installation_data.get("client"): - metadata["client"] = installation_data["client"] - - if installation_data.get("ssl_fingerprint"): - metadata["ssl_fingerprint"] = installation_data["ssl_fingerprint"] - - if installation_data.get("web_url"): - metadata["web_url"] = installation_data["web_url"] - - # Store credentials in Integration.metadata - metadata = { - "p4port": p4port, - "user": installation_data.get("user", ""), - "auth_type": installation_data.get("auth_type", "password"), # Default to password - "password": installation_data.get("password", ""), - } - # Add optional fields if provided if installation_data.get("client"): metadata["client"] = installation_data["client"] From d515b6c6433596c06a9701153beed918624a9bd5 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 09:42:32 +0100 Subject: [PATCH 30/43] Simplify integration and enable feature-flag support --- src/sentry/integrations/perforce/client.py | 4 ++-- src/sentry/integrations/perforce/integration.py | 3 ++- src/sentry/integrations/perforce/repository.py | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index ffb199398c1b63..231fb2c7b1461f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -308,7 +308,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return [] + return None def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -324,7 +324,7 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + return None def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index ececb74576b409..26f9e5a818097e 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -2,7 +2,8 @@ import hashlib import logging -from typing import TypedDict +from collections.abc import Mapping, Sequence +from typing import Any, TypedDict from django import forms from django.utils.translation import gettext_lazy as _ diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..0b1170954b3b89 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + return None def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + return None def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + return None def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From 4bd004c3e2314f6b3ad5076ec1eccbb7adef1758 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 10:06:46 +0100 Subject: [PATCH 31/43] Fix typing --- src/sentry/integrations/perforce/client.py | 13 +++++++++++-- src/sentry/integrations/perforce/repository.py | 6 +++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 231fb2c7b1461f..82298daf0b5621 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -308,7 +308,7 @@ def get_changes( Returns: List of changelist dictionaries """ - return None + return [] def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] @@ -324,7 +324,16 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return None + return [] + + def get_file( + self, repo: Repository, path: str, ref: str | None, codeowners: bool = False + ) -> str: + """ + Get file contents from Perforce depot. + Required by abstract base class but not used (CODEOWNERS feature removed). + """ + raise NotImplementedError("get_file is not supported for Perforce") def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 0b1170954b3b89..b17bb24d42a5a7 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -33,7 +33,7 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return None + return {} def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -70,7 +70,7 @@ def compare_commits( Returns: List of changelist dictionaries """ - return None + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,7 +85,7 @@ def _format_commits( Returns: List of commits in Sentry format """ - return None + return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ From f8d53530ba3c811e698f333f9299a5c5efc2a19f Mon Sep 17 00:00:00 2001 From: mujacica Date: Tue, 11 Nov 2025 19:29:28 +0100 Subject: [PATCH 32/43] 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 | 114 ++++- .../integrations/perforce/integration.py | 115 ++++- .../integrations/perforce/repository.py | 122 ++++- .../perforce/test_code_mapping.py | 435 +++++++++++++++++ .../perforce/test_stacktrace_link.py | 450 ++++++++++++++++++ 5 files changed, 1207 insertions(+), 29 deletions(-) create mode 100644 tests/sentry/integrations/perforce/test_code_mapping.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 82298daf0b5621..46d34ffd5e9547 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,6 +12,7 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, + CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -308,7 +309,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] @@ -324,16 +349,85 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - return [] + metrics.incr("integrations.perforce.get_blame_for_files") + blames = [] + p4 = self._connect() - def get_file( - self, repo: Repository, path: str, ref: str | None, codeowners: bool = False - ) -> str: - """ - Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). - """ - raise NotImplementedError("get_file is not supported for Perforce") + try: + for file in files: + try: + # Build depot path for the file (includes stream if specified) + # file.ref contains the revision/changelist if available + depot_path = self._build_depot_path(file.repo, file.path, file.ref) + + # Use faster p4 filelog approach to get most recent changelist + # This is much faster than p4 annotate + filelog = p4.run("filelog", "-m1", depot_path) + + changelist = None + if filelog and len(filelog) > 0: + # The 'change' field contains the changelist numbers (as a list of strings) + changelists = filelog[0].get("change", []) + if changelists and len(changelists) > 0: + # Get the first (most recent) changelist number + changelist = changelists[0] + + # If we found a changelist, get detailed commit info + if changelist: + try: + change_info = p4.run("describe", "-s", changelist) + if change_info and len(change_info) > 0: + change = change_info[0] + username = change.get("user", "unknown") + # Perforce doesn't provide email by default, so we generate a fallback + email = change.get("email") or f"{username}@perforce.local" + commit = CommitInfo( + commitId=changelist, + committedDate=datetime.fromtimestamp( + int(change.get("time", 0)), tz=timezone.utc + ), + commitMessage=change.get("desc", "").strip(), + commitAuthorName=username, + commitAuthorEmail=email, + ) + + blame_info = FileBlameInfo( + lineno=file.lineno, + path=file.path, + ref=file.ref, + repo=file.repo, + code_mapping=file.code_mapping, + commit=commit, + ) + blames.append(blame_info) + except self.P4Exception as e: + logger.warning( + "perforce.client.get_blame_for_files.describe_error", + extra={ + **extra, + "changelist": changelist, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + }, + ) + except self.P4Exception as e: + # Log but don't fail for individual file errors + logger.warning( + "perforce.client.get_blame_for_files.annotate_error", + extra={ + **extra, + "error": str(e), + "repo_name": file.repo.name, + "file_path": file.path, + "file_lineno": file.lineno, + }, + ) + continue + + return blames + finally: + self._disconnect(p4) def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 26f9e5a818097e..52e0083aef596a 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -201,6 +201,30 @@ def source_url_matches(self, url: str) -> bool: return False + def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: + """ + Check if a file path matches this repository's depot path. + + When SRCSRV transformers remap paths to absolute depot paths (e.g., + //depot/project/src/file.cpp), this method verifies that the depot path + matches the repository's configured depot_path. + + Args: + repo: Repository object + filepath: File path (may be absolute depot path or relative path) + + Returns: + True if the filepath matches this repository's depot + """ + depot_path = repo.config.get("depot_path", repo.name) + + # If filepath is absolute depot path, check if it starts with depot_path + if filepath.startswith("//"): + return filepath.startswith(depot_path) + + # Relative paths always match (will be prepended with depot_path) + return True + def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -400,6 +424,30 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] + def test_connection(self) -> dict[str, Any]: + """ + Test the Perforce connection with current credentials. + + Returns: + Dictionary with connection status and server info + """ + try: + client = self.get_client() + info = client.get_depot_info() + + return { + "status": "success", + "message": f"Connected to Perforce server at {info.get('server_address')}", + "server_info": info, + } + except Exception as e: + logger.exception("perforce.test_connection.failed") + return { + "status": "error", + "message": f"Failed to connect to Perforce server: {str(e)}", + "error": str(e), + } + def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. @@ -409,20 +457,53 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "p4port", - "type": "string", - "label": "P4PORT (Server Address)", - "placeholder": "ssl:perforce.company.com:1666", - "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "name": "auth_mode", + "type": "choice", + "label": "Authentication Mode", + "choices": [ + ["password", "Username & Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", "required": True, + "default": "password", + }, + { + "name": "ticket", + "type": "secret", + "label": "P4 Ticket", + "placeholder": "••••••••••••••••••••••••••••••••", + "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", + "required": False, + "depends_on": {"auth_mode": "ticket"}, + }, + { + "name": "host", + "type": "string", + "label": "Perforce Server Host", + "placeholder": "perforce.company.com", + "help": "The hostname or IP address of your Perforce server", + "required": False, + "depends_on": {"auth_mode": "password"}, + }, + { + "name": "port", + "type": "number", + "label": "Perforce Server Port", + "placeholder": "1666", + "help": "The port number for your Perforce server (default: 1666)", + "required": False, + "default": "1666", + "depends_on": {"auth_mode": "password"}, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", - "required": True, + "help": "Username for authenticating with Perforce", + "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "auth_type", @@ -450,6 +531,7 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, + "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -459,12 +541,25 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, + { + "name": "web_viewer_type", + "type": "choice", + "label": "Web Viewer Type", + "choices": [ + ["p4web", "P4Web"], + ["swarm", "Helix Swarm"], + ["other", "Other"], + ], + "help": "Type of web viewer (if web URL is provided)", + "required": False, + "default": "p4web", + }, { "name": "web_url", "type": "string", - "label": "Helix Swarm URL (Optional)", - "placeholder": "https://swarm.company.com", - "help": "Optional: URL to Helix Swarm web viewer for browsing files", + "label": "Web Viewer URL (Optional)", + "placeholder": "https://p4web.company.com", + "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", "required": False, }, ] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index b17bb24d42a5a7..64e223e39976cd 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,12 +4,14 @@ from collections.abc import Mapping, Sequence from typing import Any +from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.plugins.providers import IntegrationRepositoryProvider from sentry.plugins.providers.integration_repository import RepositoryConfig +from sentry.shared_integrations.exceptions import IntegrationError logger = logging.getLogger(__name__) @@ -33,7 +35,41 @@ def get_repository_data( Returns: Repository configuration dictionary """ - return {} + installation = self.get_installation(config.get("installation"), organization.id) + client = installation.get_client() + + depot_path = config["identifier"] # e.g., //depot or //depot/project + + # Validate depot exists and is accessible + try: + # Create a minimal repo-like object for client + class MockRepo: + def __init__(self, depot_path): + self.config = {"depot_path": depot_path} + + mock_repo = MockRepo(depot_path) + + # Try to check depot access + result = client.check_file(mock_repo, "...", None) + + if result is None: + # Try getting depot info + depots = client.get_depots() + depot_name = depot_path.strip("/").split("/")[0] + + if not any(d["name"] == depot_name for d in depots): + raise IntegrationError( + f"Depot not found or no access: {depot_path}. Available depots: {[d['name'] for d in depots]}" + ) + + except Exception: + # Don't fail - depot might be valid but empty + pass + + config["external_id"] = depot_path + config["integration_id"] = installation.model.id + + return config def build_repository_config( self, organization: RpcOrganization, data: dict[str, Any] @@ -48,12 +84,17 @@ def build_repository_config( Returns: Repository configuration """ + depot_path = data["identifier"] + return { - "name": "", - "external_id": "", - "url": "", - "config": {}, - "integration_id": 0, + "name": depot_path, + "external_id": data["external_id"], + "url": f"p4://{depot_path}", + "config": { + "depot_path": depot_path, + "name": depot_path, + }, + "integration_id": data["integration_id"], } def compare_commits( @@ -70,7 +111,42 @@ def compare_commits( Returns: List of changelist dictionaries """ - return [] + integration_id = repo.integration_id + if integration_id is None: + raise NotImplementedError("Perforce integration requires an integration_id") + + integration = integration_service.get_integration(integration_id=integration_id) + if integration is None: + raise NotImplementedError("Integration not found") + + installation = integration.get_installation(organization_id=repo.organization_id) + client = installation.get_client() + + depot_path = repo.config.get("depot_path", repo.name) + + try: + # Get changelists in range + if start_sha is None: + # Get last N changes + changes = client.get_changes(f"{depot_path}/...", max_changes=20) + else: + # Get changes between start and end + # P4 doesn't have native compare, so get changes up to end_sha + changes = client.get_changes(f"{depot_path}/...", max_changes=100, start_cl=end_sha) + + # Filter to only changes after start_sha + if changes: + start_cl_num = int(start_sha) if start_sha.isdigit() else 0 + changes = [c for c in changes if int(c["change"]) > start_cl_num] + + return self._format_commits(changes, depot_path) + + except Exception as e: + logger.exception( + "perforce.compare_commits.error", + extra={"repo": repo.name, "start": start_sha, "end": end_sha, "error": str(e)}, + ) + return [] def _format_commits( self, changelists: list[dict[str, Any]], depot_path: str @@ -85,14 +161,42 @@ def _format_commits( Returns: List of commits in Sentry format """ - return [] + commits = [] + + for cl in changelists: + # Format timestamp (P4 time is Unix timestamp) + timestamp = self.format_date(int(cl["time"])) + + commits.append( + { + "id": str(cl["change"]), # Changelist number as commit ID + "repository": depot_path, + "author_email": f"{cl['user']}@perforce", # P4 doesn't store email + "author_name": cl["user"], + "message": cl["desc"], + "timestamp": timestamp, + "patch_set": [], # Could fetch with 'p4 describe' if needed + } + ) + + return commits def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. Perforce doesn't have native PRs, but might integrate with Swarm. """ - return "" + web_url = None + if repo.integration_id: + integration = integration_service.get_integration(integration_id=repo.integration_id) + if integration: + web_url = integration.metadata.get("web_url") + + if web_url: + # Swarm review URL format + return f"{web_url}/reviews/{pull_request.key}" + + return f"p4://{repo.name}@{pull_request.key}" def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/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_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 e1c3c3e2b8f74743b603a1331c5e2f7f804e50e6 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 13:52:01 +0100 Subject: [PATCH 33/43] feat(perforce): Implement repository/depot and code mapping logic --- src/sentry/integrations/perforce/client.py | 6 ++++++ src/sentry/integrations/perforce/repository.py | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 46d34ffd5e9547..777224249f1a0f 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -301,6 +301,9 @@ def get_changes( """ Get changelists for a depot path. + Uses p4 changes command to list changelists. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_changes.html + Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return @@ -332,8 +335,11 @@ def get_changes( ] finally: self._disconnect(p4) +<<<<<<< HEAD # CommitContextClient methods (stubbed for now) +======= +>>>>>>> 2194bee20ee (feat(perforce): Implement repository/depot and code mapping logic) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 64e223e39976cd..1fa1f485f2925c 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -149,7 +149,7 @@ def compare_commits( return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str + self, changelists: list[dict[str, Any]], depot_path: str, client: Any ) -> Sequence[Mapping[str, Any]]: """ Format Perforce changelists into Sentry commit format. @@ -157,6 +157,7 @@ def _format_commits( Args: changelists: List of changelist dictionaries from P4 depot_path: Depot path + client: Perforce client instance Returns: List of commits in Sentry format @@ -184,7 +185,7 @@ def _format_commits( def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. - Perforce doesn't have native PRs, but might integrate with Swarm. + Perforce doesn't have native pull requests. """ web_url = None if repo.integration_id: From f8e62a23486ee8d540903d28d527696334543a20 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 14:18:24 +0100 Subject: [PATCH 34/43] Fix PR comments --- src/sentry/integrations/perforce/repository.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 1fa1f485f2925c..99f33cb843b812 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -130,11 +130,12 @@ def compare_commits( # 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) + # Get changes between start and end (exclusive start, inclusive end) + # P4 -e flag returns changes >= specified CL, so we get all recent changes + # and filter to range (start_sha, end_sha] + changes = client.get_changes(f"{depot_path}/...", max_changes=100) - # Filter to only changes after start_sha + # Filter to only changes in range: start_sha < change <= end_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] From e93e88367dec2ca22552bce9e5b77248bada8aef Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 19 Nov 2025 14:48:27 +0100 Subject: [PATCH 35/43] Fix PR Comments for changelists --- src/sentry/integrations/perforce/client.py | 35 ++++++++++++--- .../integrations/perforce/repository.py | 43 +++++++++++++++++++ 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 777224249f1a0f..b2d2ebee8bb4ce 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -296,7 +296,11 @@ def get_user(self, username: str) -> dict[str, Any] | None: return None def get_changes( - self, depot_path: str, max_changes: int = 20, start_cl: str | None = None + self, + depot_path: str, + max_changes: int = 20, + start_cl: str | None = None, + end_cl: str | None = None, ) -> list[dict[str, Any]]: """ Get changelists for a depot path. @@ -306,23 +310,42 @@ def get_changes( Args: depot_path: Depot path (e.g., //depot/main/...) - max_changes: Maximum number of changes to return - start_cl: Starting changelist number + max_changes: Maximum number of changes to return when start_cl/end_cl not specified + start_cl: Starting changelist number (exclusive) - returns changes > start_cl + end_cl: Ending changelist number (inclusive) - returns changes <= end_cl Returns: - List of changelist dictionaries + List of changelist dictionaries in range (start_cl, end_cl] """ p4 = self._connect() try: - args = ["-m", str(max_changes), "-l"] + # Calculate how many changes to fetch based on range + if start_cl and end_cl: + start_cl_num = int(start_cl) if start_cl.isdigit() else 0 + end_cl_num = int(end_cl) if end_cl.isdigit() else 0 + # Fetch enough to cover the range, adding buffer for safety + range_size = abs(end_cl_num - start_cl_num) + 10 + fetch_limit = max(range_size, max_changes) + else: + fetch_limit = max_changes + args = ["-m", str(fetch_limit), "-l"] + + # P4 -e flag: return changes >= specified changelist (above and including) + # Use start_cl + 1 to get exclusive start (changes > start_cl) if start_cl: - args.extend(["-e", start_cl]) + start_cl_num = int(start_cl) if start_cl.isdigit() else 0 + args.extend(["-e", str(start_cl_num + 1)]) args.append(depot_path) changes = p4.run("changes", *args) + # Client-side filter for end_cl (inclusive upper bound) + if end_cl and end_cl.isdigit(): + end_cl_num = int(end_cl) + changes = [c for c in changes if c.get("change") and int(c["change"]) <= end_cl_num] + return [ { "change": change.get("change"), diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 99f33cb843b812..f4231477f923c8 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -183,6 +183,49 @@ def _format_commits( return commits + def compare_commits( + self, repo: Repository, start_sha: str | None, end_sha: str + ) -> Sequence[Mapping[str, Any]]: + """ + Compare commits (changelists) between two versions. + + Used for release tracking when users manually specify changelist numbers. + + Args: + repo: Repository instance + start_sha: Starting changelist number (or None for initial) + end_sha: Ending changelist number + + Returns: + List of changelist dictionaries in Sentry commit format + """ + 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 (start_sha, end_sha] + changes = client.get_changes( + f"{depot_path}/...", + max_changes=20, + start_cl=start_sha, + end_cl=end_sha, + ) + + return self._format_commits(changes, depot_path, client) + + except Exception: + return [] + def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: """ Get URL for pull request. From 15f2806e97e30fcb96bdcd40d6acdb33f2660309 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Fri, 21 Nov 2025 12:39:25 +0100 Subject: [PATCH 36/43] Rework based on PR comments --- pyproject.toml | 1 + src/sentry/integrations/perforce/client.py | 157 +++++++++++++----- .../integrations/perforce/integration.py | 6 +- .../integrations/perforce/repository.py | 113 ++++++++----- .../perforce/test_code_mapping.py | 10 ++ 5 files changed, 201 insertions(+), 86 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 8822ab3a908e56..cf615998200532 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -445,6 +445,7 @@ module = [ "sentry.integrations.msteams.handlers.*", "sentry.integrations.opsgenie.handlers.*", "sentry.integrations.pagerduty.*", + "sentry.integrations.perforce.*", "sentry.integrations.project_management.*", "sentry.integrations.repository.*", "sentry.integrations.services.*", diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index b2d2ebee8bb4ce..8bf17e6c080ee1 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -1,9 +1,9 @@ from __future__ import annotations import logging -from collections.abc import Sequence +from collections.abc import Generator, Sequence from contextlib import contextmanager -from typing import Any +from typing import Any, TypedDict from P4 import P4, P4Exception @@ -23,6 +23,69 @@ logger = logging.getLogger(__name__) +# Default buffer size when fetching changelist ranges to ensure complete coverage +DEFAULT_REVISION_RANGE = 10 + + +class P4ChangeInfo(TypedDict): + """Type definition for Perforce changelist information.""" + + change: str + user: str + client: str + time: str + desc: str + + +class P4DepotInfo(TypedDict): + """Type definition for Perforce depot information.""" + + name: str + type: str + description: str + + +class P4UserInfo(TypedDict, total=False): + """Type definition for Perforce user information.""" + + email: str + full_name: str + username: str + + +class P4CommitInfo(TypedDict): + """Type definition for Sentry commit format.""" + + id: str + repository: str + author_email: str + author_name: str + message: str + timestamp: str + patch_set: list[Any] + + +class P4DepotPath: + """Encapsulates Perforce depot path logic.""" + + def __init__(self, path: str): + """ + Initialize depot path. + + Args: + path: Depot path (e.g., //depot or //depot/project) + """ + self.path = path + + def depot_name(self) -> str: + """ + Extract depot name from path. + + Returns: + Depot name (e.g., "depot" from "//depot/project") + """ + return self.path.strip("/").split("/")[0] + class PerforceClient(RepositoryClient, CommitContextClient): """ @@ -63,7 +126,7 @@ def __init__( self.ssl_fingerprint = metadata.get("ssl_fingerprint") @contextmanager - def _connect(self): + def _connect(self) -> Generator[P4]: """ Context manager for P4 connections with automatic cleanup. @@ -242,7 +305,7 @@ def build_depot_path(self, repo: Repository, path: str, stream: str | None = Non return full_path - def get_depots(self) -> list[dict[str, Any]]: + def get_depots(self) -> Sequence[P4DepotInfo]: """ List all depots accessible to the user. @@ -250,20 +313,20 @@ def get_depots(self) -> list[dict[str, Any]]: API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_depots.html Returns: - List of depot info dictionaries + Sequence of depot info dictionaries """ with self._connect() as p4: depots = p4.run("depots") return [ - { - "name": depot.get("name"), - "type": depot.get("type"), - "description": depot.get("desc", ""), - } + P4DepotInfo( + name=str(depot.get("name", "")), + type=str(depot.get("type", "")), + description=str(depot.get("desc", "")), + ) for depot in depots ] - def get_user(self, username: str) -> dict[str, Any] | None: + def get_user(self, username: str) -> P4UserInfo | None: """ Get user information from Perforce. @@ -287,11 +350,11 @@ def get_user(self, username: str) -> dict[str, Any] | None: # Check if user actually exists by verifying Update field is set if not user_info.get("Update"): return None - return { - "email": user_info.get("Email", ""), - "full_name": user_info.get("FullName", ""), - "username": user_info.get("User", username), - } + return P4UserInfo( + email=str(user_info.get("Email", "")), + full_name=str(user_info.get("FullName", "")), + username=str(user_info.get("User", username)), + ) # User not found - return None (not an error condition) return None @@ -299,9 +362,9 @@ def get_changes( self, depot_path: str, max_changes: int = 20, - start_cl: str | None = None, - end_cl: str | None = None, - ) -> list[dict[str, Any]]: + start_cl: int | None = None, + end_cl: int | None = None, + ) -> Sequence[P4ChangeInfo]: """ Get changelists for a depot path. @@ -311,20 +374,31 @@ def get_changes( Args: depot_path: Depot path (e.g., //depot/main/...) max_changes: Maximum number of changes to return when start_cl/end_cl not specified - start_cl: Starting changelist number (exclusive) - returns changes > start_cl - end_cl: Ending changelist number (inclusive) - returns changes <= end_cl + start_cl: Starting changelist number (exclusive) - returns changes > start_cl. Must be int. + end_cl: Ending changelist number (inclusive) - returns changes <= end_cl. Must be int. Returns: - List of changelist dictionaries in range (start_cl, end_cl] + Sequence of changelist dictionaries in range (start_cl, end_cl] + + Raises: + TypeError: If start_cl or end_cl are not integers """ - p4 = self._connect() - try: + with self._connect() as p4: + # Validate types - changelists must be integers + if start_cl is not None and not isinstance(start_cl, int): + raise TypeError( + f"start_cl must be an integer or None, got {type(start_cl).__name__}" + ) + if end_cl is not None and not isinstance(end_cl, int): + raise TypeError(f"end_cl must be an integer or None, got {type(end_cl).__name__}") + + start_cl_num = start_cl + end_cl_num = end_cl + # Calculate how many changes to fetch based on range - if start_cl and end_cl: - start_cl_num = int(start_cl) if start_cl.isdigit() else 0 - end_cl_num = int(end_cl) if end_cl.isdigit() else 0 + if start_cl_num is not None and end_cl_num is not None: # Fetch enough to cover the range, adding buffer for safety - range_size = abs(end_cl_num - start_cl_num) + 10 + range_size = abs(end_cl_num - start_cl_num) + DEFAULT_REVISION_RANGE fetch_limit = max(range_size, max_changes) else: fetch_limit = max_changes @@ -333,8 +407,7 @@ def get_changes( # P4 -e flag: return changes >= specified changelist (above and including) # Use start_cl + 1 to get exclusive start (changes > start_cl) - if start_cl: - start_cl_num = int(start_cl) if start_cl.isdigit() else 0 + if start_cl_num is not None: args.extend(["-e", str(start_cl_num + 1)]) args.append(depot_path) @@ -342,27 +415,19 @@ def get_changes( changes = p4.run("changes", *args) # Client-side filter for end_cl (inclusive upper bound) - if end_cl and end_cl.isdigit(): - end_cl_num = int(end_cl) + if end_cl_num is not None: changes = [c for c in changes if c.get("change") and int(c["change"]) <= end_cl_num] return [ - { - "change": change.get("change"), - "user": change.get("user"), - "client": change.get("client"), - "time": change.get("time"), - "desc": change.get("desc"), - } + P4ChangeInfo( + change=str(change.get("change", "")), + user=str(change.get("user", "")), + client=str(change.get("client", "")), + time=str(change.get("time", "")), + desc=str(change.get("desc", "")), + ) for change in changes ] - finally: - self._disconnect(p4) -<<<<<<< HEAD - - # CommitContextClient methods (stubbed for now) -======= ->>>>>>> 2194bee20ee (feat(perforce): Implement repository/depot and code mapping logic) def get_blame_for_files( self, files: Sequence[SourceLineInfo], extra: dict[str, Any] diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 52e0083aef596a..5a6f667b532964 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -143,11 +143,11 @@ class PerforceInstallationForm(forms.Form): assume_scheme="https", ) - def clean_p4port(self): + def clean_p4port(self) -> str: """Strip off trailing / and whitespace from p4port""" return self.cleaned_data["p4port"].strip().rstrip("/") - def clean_web_url(self): + def clean_web_url(self) -> str: """Strip off trailing / from web_url""" web_url = self.cleaned_data.get("web_url", "") if web_url: @@ -627,7 +627,7 @@ class PerforceIntegrationProvider(IntegrationProvider): ) requires_feature_flag = True - def get_pipeline_views(self) -> Sequence[PipelineView]: + def get_pipeline_views(self) -> Sequence[PipelineView[IntegrationPipeline]]: """Get pipeline views for installation flow.""" return [PerforceInstallationView()] diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index f4231477f923c8..e5463229df317f 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -4,6 +4,13 @@ from collections.abc import Mapping, Sequence from typing import Any +from sentry.integrations.perforce.client import ( + P4ChangeInfo, + P4CommitInfo, + P4DepotPath, + P4UserInfo, + PerforceClient, +) from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization from sentry.models.pullrequest import PullRequest @@ -22,6 +29,30 @@ class PerforceRepositoryProvider(IntegrationRepositoryProvider): name = "Perforce" repo_provider = "perforce" + def _get_client_from_repo(self, repo: Repository) -> PerforceClient: + """ + Get Perforce client from repository. + + Args: + repo: Repository instance + + Returns: + PerforceClient instance + + Raises: + NotImplementedError: If integration not found + """ + 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) + return installation.get_client() + def get_repository_data( self, organization: Organization, config: dict[str, Any] ) -> Mapping[str, Any]: @@ -38,35 +69,22 @@ def get_repository_data( installation = self.get_installation(config.get("installation"), organization.id) client = installation.get_client() - depot_path = config["identifier"] # e.g., //depot or //depot/project + depot_path = P4DepotPath(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] + depots = client.get_depots() - 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]}" - ) + if not any(d["name"] == depot_path.depot_name() for d in depots): + raise IntegrationError( + f"Depot not found or no access: {depot_path.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["external_id"] = depot_path.path config["integration_id"] = installation.model.id return config @@ -150,8 +168,8 @@ def compare_commits( return [] def _format_commits( - self, changelists: list[dict[str, Any]], depot_path: str, client: Any - ) -> Sequence[Mapping[str, Any]]: + self, changelists: Sequence[P4ChangeInfo], depot_path: str, client: PerforceClient + ) -> list[P4CommitInfo]: """ Format Perforce changelists into Sentry commit format. @@ -163,7 +181,8 @@ def _format_commits( Returns: List of commits in Sentry format """ - commits = [] + commits: list[P4CommitInfo] = [] + user_cache: dict[str, P4UserInfo | None] = {} for cl in changelists: # Format timestamp (P4 time is Unix timestamp) @@ -199,31 +218,51 @@ def compare_commits( Returns: List of changelist dictionaries in Sentry commit format """ - 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() - + client = self._get_client_from_repo(repo) depot_path = repo.config.get("depot_path", repo.name) try: + # Convert changelist numbers from strings to integers + # In Perforce, SHAs are changelist numbers + start_cl = int(start_sha) if start_sha else None + end_cl = int(end_sha) + # Get changelists in range (start_sha, end_sha] changes = client.get_changes( f"{depot_path}/...", max_changes=20, - start_cl=start_sha, - end_cl=end_sha, + start_cl=start_cl, + end_cl=end_cl, ) return self._format_commits(changes, depot_path, client) - except Exception: + except (ValueError, TypeError) as e: + # Log conversion errors for debugging + logger.exception( + "perforce.compare_commits.invalid_changelist", + extra={ + "start_sha": start_sha, + "end_sha": end_sha, + "depot_path": depot_path, + "repo_id": repo.id, + "error": str(e), + }, + ) + return [] + except Exception as e: + logger.exception( + "perforce.compare_commits.failed", + extra={ + "start_sha": start_sha, + "end_sha": end_sha, + "depot_path": depot_path, + "repo_id": repo.id, + "integration_id": repo.integration_id, + "error": str(e), + "error_type": type(e).__name__, + }, + ) return [] def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index cd8cd25ed5981f..54baf73be176b3 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -381,10 +381,20 @@ def test_full_flow_with_web_viewer(self): organization=self.organization, provider="perforce", name="Perforce", +<<<<<<< HEAD external_id="perforce-test-web-flow", metadata={ "web_url": "https://p4web.example.com", "web_viewer_type": "p4web", +======= + external_id="perforce-test-swarm-flow", + metadata={ + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + "auth_type": "password", + "web_url": "https://swarm.example.com", +>>>>>>> 73c0776894f (Rework based on PR comments) }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] From e52130eaca7ab6cc75788dac5cce9e4984bd66d4 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Fri, 21 Nov 2025 12:58:51 +0100 Subject: [PATCH 37/43] Fix usage of -e P4 flag --- src/sentry/integrations/perforce/client.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 8bf17e6c080ee1..5de8967ab40204 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -405,18 +405,21 @@ def get_changes( args = ["-m", str(fetch_limit), "-l"] - # P4 -e flag: return changes >= specified changelist (above and including) - # Use start_cl + 1 to get exclusive start (changes > start_cl) - if start_cl_num is not None: - args.extend(["-e", str(start_cl_num + 1)]) + # P4 -e flag: return changes at or before specified changelist (upper bound) + # Use it for end_cl (inclusive upper bound) + if end_cl_num is not None: + args.extend(["-e", str(end_cl_num)]) args.append(depot_path) changes = p4.run("changes", *args) - # Client-side filter for end_cl (inclusive upper bound) - if end_cl_num is not None: - changes = [c for c in changes if c.get("change") and int(c["change"]) <= end_cl_num] + # Client-side filter for start_cl (exclusive lower bound) + # Filter out changes <= start_cl to get changes > start_cl + if start_cl_num is not None: + changes = [ + c for c in changes if c.get("change") and int(c["change"]) > start_cl_num + ] return [ P4ChangeInfo( From f7aee86636b4c2201f72742075d9c7f0eb61e6fc Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 11:37:56 +0100 Subject: [PATCH 38/43] Fixes after rebase --- src/sentry/integrations/perforce/client.py | 83 +-------- .../integrations/perforce/integration.py | 24 --- .../integrations/perforce/repository.py | 173 ++++++++++-------- .../perforce/test_code_mapping.py | 35 ++-- .../integrations/perforce/test_integration.py | 18 +- .../perforce/test_stacktrace_link.py | 30 ++- 6 files changed, 170 insertions(+), 193 deletions(-) diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 5de8967ab40204..f4961ed2aaa935 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -12,7 +12,6 @@ from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, - CommitInfo, FileBlameInfo, SourceLineInfo, ) @@ -446,92 +445,14 @@ def get_blame_for_files( Returns a list of FileBlameInfo objects containing commit details for each file. """ - metrics.incr("integrations.perforce.get_blame_for_files") - blames = [] - p4 = self._connect() - - try: - for file in files: - try: - # Build depot path for the file (includes stream if specified) - # file.ref contains the revision/changelist if available - depot_path = self._build_depot_path(file.repo, file.path, file.ref) - - # Use faster p4 filelog approach to get most recent changelist - # This is much faster than p4 annotate - filelog = p4.run("filelog", "-m1", depot_path) - - changelist = None - if filelog and len(filelog) > 0: - # The 'change' field contains the changelist numbers (as a list of strings) - changelists = filelog[0].get("change", []) - if changelists and len(changelists) > 0: - # Get the first (most recent) changelist number - changelist = changelists[0] - - # If we found a changelist, get detailed commit info - if changelist: - try: - change_info = p4.run("describe", "-s", changelist) - if change_info and len(change_info) > 0: - change = change_info[0] - username = change.get("user", "unknown") - # Perforce doesn't provide email by default, so we generate a fallback - email = change.get("email") or f"{username}@perforce.local" - commit = CommitInfo( - commitId=changelist, - committedDate=datetime.fromtimestamp( - int(change.get("time", 0)), tz=timezone.utc - ), - commitMessage=change.get("desc", "").strip(), - commitAuthorName=username, - commitAuthorEmail=email, - ) - - blame_info = FileBlameInfo( - lineno=file.lineno, - path=file.path, - ref=file.ref, - repo=file.repo, - code_mapping=file.code_mapping, - commit=commit, - ) - blames.append(blame_info) - except self.P4Exception as e: - logger.warning( - "perforce.client.get_blame_for_files.describe_error", - extra={ - **extra, - "changelist": changelist, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - }, - ) - except self.P4Exception as e: - # Log but don't fail for individual file errors - logger.warning( - "perforce.client.get_blame_for_files.annotate_error", - extra={ - **extra, - "error": str(e), - "repo_name": file.repo.name, - "file_path": file.path, - "file_lineno": file.lineno, - }, - ) - continue - - return blames - finally: - self._disconnect(p4) + return [] def get_file( self, repo: Repository, path: str, ref: str | None, codeowners: bool = False ) -> str: """ Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). + Required by abstract base class but not used (CODEOWNERS). """ raise NotImplementedError("get_file is not supported for Perforce") diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 5a6f667b532964..941f2f367fb6e5 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -424,30 +424,6 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" return [] - def test_connection(self) -> dict[str, Any]: - """ - Test the Perforce connection with current credentials. - - Returns: - Dictionary with connection status and server info - """ - try: - client = self.get_client() - info = client.get_depot_info() - - return { - "status": "success", - "message": f"Connected to Perforce server at {info.get('server_address')}", - "server_info": info, - } - except Exception as e: - logger.exception("perforce.test_connection.failed") - return { - "status": "error", - "message": f"Failed to connect to Perforce server: {str(e)}", - "error": str(e), - } - def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index e5463229df317f..d10cd23c7b510c 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -2,6 +2,7 @@ import logging from collections.abc import Mapping, Sequence +from datetime import datetime, timezone from typing import Any from sentry.integrations.perforce.client import ( @@ -80,9 +81,20 @@ def get_repository_data( f"Depot not found or no access: {depot_path.path}. Available depots: {[d['name'] for d in depots]}" ) - except Exception: - # Don't fail - depot might be valid but empty - pass + except IntegrationError: + # Re-raise validation errors so user sees them + raise + except Exception as e: + # Log and re-raise connection/P4 errors + # We cannot create a repository if we can't validate the depot exists + logger.exception( + "perforce.get_repository_data.depot_validation_failed", + extra={"depot_path": depot_path.path}, + ) + raise IntegrationError( + f"Failed to validate depot: {depot_path.path}. " + f"Please check your Perforce server connection and credentials." + ) from e config["external_id"] = depot_path.path config["integration_id"] = installation.model.id @@ -115,57 +127,84 @@ def build_repository_config( "integration_id": data["integration_id"], } - def compare_commits( - self, repo: Repository, start_sha: str | None, end_sha: str - ) -> Sequence[Mapping[str, Any]]: + def _extract_commit_info( + self, + change: P4ChangeInfo, + depot_path: str, + client: PerforceClient, + user_cache: dict[str, P4UserInfo | None], + ) -> P4CommitInfo: """ - Compare commits (changelists) between two versions. + Extract commit info from a Perforce changelist. Args: - repo: Repository instance - start_sha: Starting changelist number (or None for initial) - end_sha: Ending changelist number + change: Perforce changelist info + depot_path: Depot path for repository field + client: Perforce client for user lookups + user_cache: Cache of user info to avoid redundant lookups Returns: - List of changelist dictionaries + Commit info in Sentry format """ - 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) - + # Handle potentially null/invalid time field + time_value = change.get("time") or 0 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 (exclusive start, inclusive end) - # P4 -e flag returns changes >= specified CL, so we get all recent changes - # and filter to range (start_sha, end_sha] - changes = client.get_changes(f"{depot_path}/...", max_changes=100) - - # Filter to only changes in range: start_sha < change <= end_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)}, + time_int = int(time_value) + except (TypeError, ValueError) as e: + logger.warning( + "perforce.format_commits.invalid_time_value", + extra={ + "changelist": change.get("change"), + "time_value": time_value, + "error": str(e), + }, ) - return [] + time_int = 0 + + # Convert Unix timestamp to ISO 8601 format + timestamp = datetime.fromtimestamp(time_int, tz=timezone.utc).isoformat() + + # Get user information from Perforce + username = change.get("user", "unknown") + author_email = f"{username}@perforce" + author_name = username + + # Fetch user info if not in cache (skip "unknown" placeholder) + if username != "unknown" and username not in user_cache: + try: + user_cache[username] = client.get_user(username) + except Exception as e: + # Log user lookup failures but don't fail the entire commit processing + logger.warning( + "perforce.format_commits.user_lookup_failed", + extra={ + "changelist": change.get("change"), + "username": username, + "error": str(e), + "error_type": type(e).__name__, + }, + ) + # Cache None to avoid repeated failed lookups for the same user + user_cache[username] = None + + user_info = user_cache.get(username) + if user_info: + # Use actual email from Perforce if available + if user_info.get("email"): + author_email = user_info["email"] + # Use full name from Perforce if available + if user_info.get("full_name"): + author_name = user_info["full_name"] + + return P4CommitInfo( + id=str(change["change"]), + repository=depot_path, + author_email=author_email, + author_name=author_name, + message=change.get("desc", ""), + timestamp=timestamp, + patch_set=[], + ) def _format_commits( self, changelists: Sequence[P4ChangeInfo], depot_path: str, client: PerforceClient @@ -184,21 +223,21 @@ def _format_commits( commits: list[P4CommitInfo] = [] user_cache: dict[str, P4UserInfo | None] = {} - 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 - } - ) + for change in changelists: + try: + commit = self._extract_commit_info(change, depot_path, client, user_cache) + commits.append(commit) + except (KeyError, TypeError) as e: + logger.warning( + "perforce.format_commits.invalid_changelist_data", + extra={ + "changelist": change.get("change"), + "depot_path": depot_path, + "error": str(e), + "error_type": type(e).__name__, + }, + ) + continue return commits @@ -270,17 +309,7 @@ def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: Get URL for pull request. Perforce doesn't have native pull requests. """ - web_url = None - if repo.integration_id: - integration = integration_service.get_integration(integration_id=repo.integration_id) - if integration: - web_url = integration.metadata.get("web_url") - - if web_url: - # Swarm review URL format - return f"{web_url}/reviews/{pull_request.key}" - - return f"p4://{repo.name}@{pull_request.key}" + raise NotImplementedError("Perforce does not have native pull requests") def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index 54baf73be176b3..b00518d61edd8e 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.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 ( PerforceIntegration, @@ -31,6 +33,9 @@ def setUp(self): self.org_integration = self.integration.organizationintegration_set.first() assert self.org_integration is not None + def tearDown(self): + super().tearDown() + def test_code_mapping_depot_root_to_slash(self): """ Test code mapping: depot/ -> / @@ -337,6 +342,17 @@ def setUp(self): default_branch=None, ) + # Mock the Perforce client's check_file to avoid actual P4 connection + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/mock"}, + ) + self.check_file_patcher.start() + + def tearDown(self): + self.check_file_patcher.stop() + super().tearDown() + def test_python_sdk_path_full_flow(self): """Test full flow: Python SDK -> code mapping -> format_source_url""" # 1. Python SDK sends this path @@ -381,20 +397,14 @@ def test_full_flow_with_web_viewer(self): organization=self.organization, provider="perforce", name="Perforce", -<<<<<<< HEAD external_id="perforce-test-web-flow", - metadata={ - "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", -======= - external_id="perforce-test-swarm-flow", metadata={ "p4port": "ssl:perforce.example.com:1666", "user": "testuser", "password": "testpass", "auth_type": "password", - "web_url": "https://swarm.example.com", ->>>>>>> 73c0776894f (Rework based on PR comments) + "web_url": "https://p4web.example.com", + "web_viewer_type": "p4web", }, ) installation: PerforceIntegration = integration_with_web.get_installation(self.organization.id) # type: ignore[assignment] @@ -424,10 +434,10 @@ def test_full_flow_with_web_viewer(self): default_branch=None, ) - # Python SDK path with @revision from Symbolic + # Python SDK path with #revision from Symbolic frame = EventFrame( - filename="depot/app/services/processor.py@42", - abs_path="depot/app/services/processor.py@42", + filename="depot/app/services/processor.py#42", + abs_path="depot/app/services/processor.py#42", ) # Code mapping @@ -442,4 +452,5 @@ def test_full_flow_with_web_viewer(self): 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" + # Swarm format: /files/?v= + assert url == "https://p4web.example.com/files//depot/app/services/processor.py?v=42" diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 1a957e2c6a0c95..f0cec0bca2274f 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -501,23 +501,35 @@ def test_integration_lifecycle(self): # Test get_organization_config returns form schema org_config = installation.get_organization_config() - assert len(org_config) == 7 # 7 configuration fields (added auth_type) + assert len(org_config) == 11 # 11 configuration fields field_names = {field["name"] for field in org_config} assert field_names == { - "p4port", + "auth_mode", + "ticket", + "host", + "port", "user", "auth_type", "password", "ssl_fingerprint", "client", + "web_viewer_type", "web_url", } # Verify field types field_types = {field["name"]: field["type"] for field in org_config} assert field_types["password"] == "secret" - assert field_types["p4port"] == "string" + assert field_types["host"] == "string" + assert field_types["port"] == "number" assert field_types["user"] == "string" + assert field_types["auth_mode"] == "choice" + assert field_types["ticket"] == "secret" + assert field_types["auth_type"] == "choice" + assert field_types["ssl_fingerprint"] == "string" + assert field_types["client"] == "string" + assert field_types["web_viewer_type"] == "choice" + assert field_types["web_url"] == "string" # Test get_config_data returns current values config_data = installation.get_config_data() diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index acf9b50595a7a4..6bdc86cb60bf22 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,6 +43,17 @@ def setUp(self): default_branch=None, ) + # Mock the Perforce client's check_file to avoid actual P4 connection + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/mock"}, + ) + self.check_file_patcher.start() + + def tearDown(self): + self.check_file_patcher.stop() + super().tearDown() + def test_get_stacktrace_config_python_path(self): """Test stacktrace link generation for Python SDK path""" ctx: StacktraceLinkContext = { @@ -205,7 +218,11 @@ def test_get_stacktrace_config_with_web_viewer(self): 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"] + # Swarm format uses /files/ prefix + assert ( + "https://p4web.example.com/files//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""" @@ -331,6 +348,17 @@ def setUp(self): ) self.project = self.create_project(organization=self.organization) + # Mock the Perforce client's check_file to avoid actual P4 connection + self.check_file_patcher = patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + return_value={"depotFile": "//depot/mock"}, + ) + self.check_file_patcher.start() + + def tearDown(self): + self.check_file_patcher.stop() + super().tearDown() + def test_stacktrace_link_empty_stack_root(self): """Test stacktrace link with empty stack_root (shouldn't match anything)""" repo = Repository.objects.create( From 46ed8c56dafb7a9d35a0b04089dc1c2c69d4cc4d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Wed, 3 Dec 2025 12:38:25 +0100 Subject: [PATCH 39/43] Cleanup the implementation after rebase --- .../integrations/perforce/integration.py | 91 ++----------------- .../perforce/test_code_mapping.py | 1 - .../integrations/perforce/test_integration.py | 16 +--- .../perforce/test_stacktrace_link.py | 1 - 4 files changed, 14 insertions(+), 95 deletions(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 941f2f367fb6e5..cbb9e24e257250 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -201,30 +201,6 @@ def source_url_matches(self, url: str) -> bool: return False - def matches_repository_depot_path(self, repo: Repository, filepath: str) -> bool: - """ - Check if a file path matches this repository's depot path. - - When SRCSRV transformers remap paths to absolute depot paths (e.g., - //depot/project/src/file.cpp), this method verifies that the depot path - matches the repository's configured depot_path. - - Args: - repo: Repository object - filepath: File path (may be absolute depot path or relative path) - - Returns: - True if the filepath matches this repository's depot - """ - depot_path = repo.config.get("depot_path", repo.name) - - # If filepath is absolute depot path, check if it starts with depot_path - if filepath.startswith("//"): - return filepath.startswith(depot_path) - - # Relative paths always match (will be prepended with depot_path) - return True - def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: """ Check if a file exists in the Perforce depot and return the URL. @@ -433,53 +409,20 @@ def get_organization_config(self) -> list[dict[str, Any]]: """ return [ { - "name": "auth_mode", - "type": "choice", - "label": "Authentication Mode", - "choices": [ - ["password", "Username & Password"], - ["ticket", "P4 Ticket"], - ], - "help": "Choose how to authenticate with Perforce. P4 tickets are more secure and don't require storing passwords.", - "required": True, - "default": "password", - }, - { - "name": "ticket", - "type": "secret", - "label": "P4 Ticket", - "placeholder": "••••••••••••••••••••••••••••••••", - "help": "P4 authentication ticket (obtained via 'p4 login -p'). Tickets contain server/user info and are more secure than passwords.", - "required": False, - "depends_on": {"auth_mode": "ticket"}, - }, - { - "name": "host", + "name": "p4port", "type": "string", - "label": "Perforce Server Host", - "placeholder": "perforce.company.com", - "help": "The hostname or IP address of your Perforce server", - "required": False, - "depends_on": {"auth_mode": "password"}, - }, - { - "name": "port", - "type": "number", - "label": "Perforce Server Port", - "placeholder": "1666", - "help": "The port number for your Perforce server (default: 1666)", - "required": False, - "default": "1666", - "depends_on": {"auth_mode": "password"}, + "label": "P4PORT (Server Address)", + "placeholder": "ssl:perforce.company.com:1666", + "help": "Perforce server address in P4PORT format. Examples: 'ssl:perforce.company.com:1666' (encrypted), 'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). SSL is strongly recommended for production use.", + "required": True, }, { "name": "user", "type": "string", "label": "Perforce Username", "placeholder": "sentry-bot", - "help": "Username for authenticating with Perforce", - "required": False, - "depends_on": {"auth_mode": "password"}, + "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", + "required": True, }, { "name": "auth_type", @@ -507,7 +450,6 @@ def get_organization_config(self) -> list[dict[str, Any]]: "placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01", "help": "SSL fingerprint for secure connections. Required when using 'ssl:' protocol. Obtain with: p4 -p ssl:host:port trust -y", "required": False, - "depends_on": {"auth_mode": "password"}, }, { "name": "client", @@ -517,25 +459,12 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Optional: Specify a client workspace name", "required": False, }, - { - "name": "web_viewer_type", - "type": "choice", - "label": "Web Viewer Type", - "choices": [ - ["p4web", "P4Web"], - ["swarm", "Helix Swarm"], - ["other", "Other"], - ], - "help": "Type of web viewer (if web URL is provided)", - "required": False, - "default": "p4web", - }, { "name": "web_url", "type": "string", - "label": "Web Viewer URL (Optional)", - "placeholder": "https://p4web.company.com", - "help": "Optional: URL to P4Web, Swarm, or other web-based Perforce viewer", + "label": "Helix Swarm URL (Optional)", + "placeholder": "https://swarm.company.com", + "help": "Optional: URL to Helix Swarm web viewer for browsing files", "required": False, }, ] diff --git a/tests/sentry/integrations/perforce/test_code_mapping.py b/tests/sentry/integrations/perforce/test_code_mapping.py index b00518d61edd8e..621d2138209494 100644 --- a/tests/sentry/integrations/perforce/test_code_mapping.py +++ b/tests/sentry/integrations/perforce/test_code_mapping.py @@ -404,7 +404,6 @@ def test_full_flow_with_web_viewer(self): "password": "testpass", "auth_type": "password", "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_integration.py b/tests/sentry/integrations/perforce/test_integration.py index f0cec0bca2274f..69f1f08dd4ee3c 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -501,34 +501,26 @@ def test_integration_lifecycle(self): # Test get_organization_config returns form schema org_config = installation.get_organization_config() - assert len(org_config) == 11 # 11 configuration fields + assert len(org_config) == 7 # 7 configuration fields field_names = {field["name"] for field in org_config} assert field_names == { - "auth_mode", - "ticket", - "host", - "port", + "p4port", "user", "auth_type", "password", "ssl_fingerprint", "client", - "web_viewer_type", "web_url", } # Verify field types field_types = {field["name"]: field["type"] for field in org_config} - assert field_types["password"] == "secret" - assert field_types["host"] == "string" - assert field_types["port"] == "number" + assert field_types["p4port"] == "string" assert field_types["user"] == "string" - assert field_types["auth_mode"] == "choice" - assert field_types["ticket"] == "secret" assert field_types["auth_type"] == "choice" + assert field_types["password"] == "secret" assert field_types["ssl_fingerprint"] == "string" assert field_types["client"] == "string" - assert field_types["web_viewer_type"] == "choice" assert field_types["web_url"] == "string" # Test get_config_data returns current values diff --git a/tests/sentry/integrations/perforce/test_stacktrace_link.py b/tests/sentry/integrations/perforce/test_stacktrace_link.py index 6bdc86cb60bf22..715c19ae50b47e 100644 --- a/tests/sentry/integrations/perforce/test_stacktrace_link.py +++ b/tests/sentry/integrations/perforce/test_stacktrace_link.py @@ -173,7 +173,6 @@ def test_get_stacktrace_config_with_web_viewer(self): external_id="perforce-test-web-link", metadata={ "web_url": "https://p4web.example.com", - "web_viewer_type": "p4web", }, ) From 4066443d999cb2af8855dbcd13b79684fce4ec31 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 14:12:34 +0100 Subject: [PATCH 40/43] Fixes after rebase --- src/sentry/integrations/perforce/integration.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index cbb9e24e257250..bf034e292574bf 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -6,11 +6,20 @@ from typing import Any, TypedDict from django import forms +from django.http import HttpRequest, HttpResponseBase from django.utils.translation import gettext_lazy as _ -from sentry.integrations.base import FeatureDescription, IntegrationFeatures, IntegrationMetadata +from sentry.integrations.base import ( + FeatureDescription, + IntegrationData, + IntegrationFeatures, + IntegrationMetadata, + IntegrationProvider, +) from sentry.integrations.models.integration import Integration from sentry.integrations.perforce.client import PerforceClient +from sentry.integrations.pipeline import IntegrationPipeline +from sentry.integrations.services.repository import RpcRepository from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.repository import RepositoryIntegration from sentry.models.repository import Repository From 0d75b2ae20130e11b435a9ae158c1058dfb12ff6 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 14:42:46 +0100 Subject: [PATCH 41/43] Fix P4 path --- src/sentry/integrations/perforce/repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index d10cd23c7b510c..28198a738dd4ac 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -119,7 +119,7 @@ def build_repository_config( return { "name": depot_path, "external_id": data["external_id"], - "url": f"p4://{depot_path}", + "url": f"p4:{depot_path}", "config": { "depot_path": depot_path, "name": depot_path, From 30f57462620002c8a61508cd6fc53cd3318f6718 Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 15:10:35 +0100 Subject: [PATCH 42/43] Return None --- .../integrations/perforce/repository.py | 4 +- .../providers/integration_repository.py | 37 +++++++++---------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/sentry/integrations/perforce/repository.py b/src/sentry/integrations/perforce/repository.py index 28198a738dd4ac..f2235b4649f8b4 100644 --- a/src/sentry/integrations/perforce/repository.py +++ b/src/sentry/integrations/perforce/repository.py @@ -304,12 +304,12 @@ def compare_commits( ) return [] - def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str: + def pull_request_url(self, repo: Repository, pull_request: PullRequest) -> str | None: """ Get URL for pull request. Perforce doesn't have native pull requests. """ - raise NotImplementedError("Perforce does not have native pull requests") + return None def repository_external_slug(self, repo: Repository) -> str: """Get external slug for repository.""" diff --git a/src/sentry/plugins/providers/integration_repository.py b/src/sentry/plugins/providers/integration_repository.py index 9be762ce766856..3e25511c10799a 100644 --- a/src/sentry/plugins/providers/integration_repository.py +++ b/src/sentry/plugins/providers/integration_repository.py @@ -358,16 +358,15 @@ def build_repository_config( ) -> RepositoryConfig: """ Builds final dict containing all necessary data to create the repository - - >>> { - >>> 'name': data['name'], - >>> 'external_id': data['external_id'], - >>> 'url': data['url'], - >>> 'config': { - >>> # Any additional data - >>> }, - >>> 'integration_id': data['installation'], - >>> } + { + 'name': data['name'], + 'external_id': data['external_id'], + 'url': data['url'], + 'config': { + # Any additional data + }, + 'integration_id': data['installation'], + } """ raise NotImplementedError @@ -383,15 +382,15 @@ def compare_commits(self, repo, start_sha, end_sha): """ Generate a list of commits between the start & end sha Commits should be of the following format: - >>> { - >>> 'id': commit['id'], - >>> 'repository': repo.name, - >>> 'author_email': commit['author']['email'], - >>> 'author_name': commit['author']['name'], - >>> 'message': commit['message'], - >>> 'timestamp': self.format_date(commit['timestamp']), - >>> 'patch_set': commit['patch_set'], - >>> } + { + 'id': commit['id'], + 'repository': repo.name, + 'author_email': commit['author']['email'], + 'author_name': commit['author']['name'], + 'message': commit['message'], + 'timestamp': self.format_date(commit['timestamp']), + 'patch_set': commit['patch_set'], + } """ raise NotImplementedError From 4a56a95e4ed86be20c18916fe1a7d2e046aa662d Mon Sep 17 00:00:00 2001 From: Amir Mujacic Date: Thu, 4 Dec 2025 15:43:20 +0100 Subject: [PATCH 43/43] Revert unintended change --- .../providers/integration_repository.py | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/sentry/plugins/providers/integration_repository.py b/src/sentry/plugins/providers/integration_repository.py index 3e25511c10799a..9be762ce766856 100644 --- a/src/sentry/plugins/providers/integration_repository.py +++ b/src/sentry/plugins/providers/integration_repository.py @@ -358,15 +358,16 @@ def build_repository_config( ) -> RepositoryConfig: """ Builds final dict containing all necessary data to create the repository - { - 'name': data['name'], - 'external_id': data['external_id'], - 'url': data['url'], - 'config': { - # Any additional data - }, - 'integration_id': data['installation'], - } + + >>> { + >>> 'name': data['name'], + >>> 'external_id': data['external_id'], + >>> 'url': data['url'], + >>> 'config': { + >>> # Any additional data + >>> }, + >>> 'integration_id': data['installation'], + >>> } """ raise NotImplementedError @@ -382,15 +383,15 @@ def compare_commits(self, repo, start_sha, end_sha): """ Generate a list of commits between the start & end sha Commits should be of the following format: - { - 'id': commit['id'], - 'repository': repo.name, - 'author_email': commit['author']['email'], - 'author_name': commit['author']['name'], - 'message': commit['message'], - 'timestamp': self.format_date(commit['timestamp']), - 'patch_set': commit['patch_set'], - } + >>> { + >>> 'id': commit['id'], + >>> 'repository': repo.name, + >>> 'author_email': commit['author']['email'], + >>> 'author_name': commit['author']['name'], + >>> 'message': commit['message'], + >>> 'timestamp': self.format_date(commit['timestamp']), + >>> 'patch_set': commit['patch_set'], + >>> } """ raise NotImplementedError