From 4ed24e4498e3556c9a31ef09ed484844111917e4 Mon Sep 17 00:00:00 2001 From: Michael Sun <55160142+MichaelSun48@users.noreply.github.com> Date: Tue, 16 Apr 2024 17:18:44 -0400 Subject: [PATCH] feat(code-mappings): Add support for stacktrace frames with backslashes for automatic code mappings (#68845) This PR updates the existing code mapping logic to support stack traces whose filepaths contained backslashes instead of forward slashes. This effectively adds automatic code mapping support for issues derived from Windows machines (for the existing supported languages). I've made the choice to save stack roots in the code mappings using whichever slash delimiter that the original stack path used. For example, if we have the following stack path and source: * Stack Path = `"D:\Users\code\sentry\models\release.py"` * Source path = `"src/sentry/models/release.py"` Then the roots generated will be: * Stack Root = `"D:\Users\code\"` (note that this source root still uses backslashes instead of forward slashes) * Source Root = `"src/"` This should be less confusing to the end user since we explicitly say we are going to replace the `stack_root` in the `stack_path` with the `source_root` to get the `source_path`. It might be confusing if the `stack_root`'s slash-delimiter was not the same as that in the `stack_path`. This also means that the previous logic to apply code mappings to a given frame to generate a stacktrace link needed to be updated so that the source path generated would only include slashes (since this path is used in a github URL, which only uses forward slashes). --- src/sentry/integrations/utils/code_mapping.py | 38 ++++++--- .../integrations/utils/test_code_mapping.py | 1 - .../sentry/tasks/test_derive_code_mappings.py | 77 +++++++++++++++++++ 3 files changed, 106 insertions(+), 10 deletions(-) diff --git a/src/sentry/integrations/utils/code_mapping.py b/src/sentry/integrations/utils/code_mapping.py index 4eabab1ab05d89..12922c1e407e3c 100644 --- a/src/sentry/integrations/utils/code_mapping.py +++ b/src/sentry/integrations/utils/code_mapping.py @@ -61,7 +61,12 @@ class UnsupportedFrameFilename(Exception): class FrameFilename: def __init__(self, frame_file_path: str) -> None: self.raw_path = frame_file_path - if frame_file_path[0] == "/": + is_windows_path = False + if "\\" in frame_file_path: + is_windows_path = True + frame_file_path = frame_file_path.replace("\\", "/") + + if frame_file_path[0] == "/" or frame_file_path[0] == "\\": frame_file_path = frame_file_path[1:] # Using regexes would be better but this is easier to understand @@ -69,7 +74,6 @@ def __init__(self, frame_file_path: str) -> None: not frame_file_path or frame_file_path[0] in ["[", "<"] or frame_file_path.find(" ") > -1 - or frame_file_path.find("\\") > -1 # Windows support or frame_file_path.find("/") == -1 ): raise UnsupportedFrameFilename("This path is not supported.") @@ -79,8 +83,20 @@ def __init__(self, frame_file_path: str) -> None: if not self.extension: raise UnsupportedFrameFilename("It needs an extension.") + # Remove drive letter if it exists + if is_windows_path and frame_file_path[1] == ":": + frame_file_path = frame_file_path[2:] + # windows drive letters can be like C:\ or C: + # so we need to remove the slash if it exists + if frame_file_path[0] == "/": + frame_file_path = frame_file_path[1:] + start_at_index = get_straight_path_prefix_end_index(frame_file_path) self.straight_path_prefix = frame_file_path[:start_at_index] + + # We normalize the path to be as close to what the path would + # look like in the source code repository, hence why we remove + # the straight path prefix and drive letter self.normalized_path = frame_file_path[start_at_index:] if start_at_index == 0: self.root = frame_file_path.split("/")[0] @@ -145,7 +161,7 @@ def list_file_matches(self, frame_filename: FrameFilename) -> list[dict[str, str ) continue - if stack_path.replace(stack_root, source_root, 1) != source_path: + if stack_path.replace(stack_root, source_root, 1).replace("\\", "/") != source_path: logger.info( "Unexpected stack_path/source_path found. A code mapping was not generated.", extra={ @@ -259,7 +275,7 @@ def _generate_code_mapping_from_tree( ) return [] - if stack_path.replace(stack_root, source_root, 1) != source_path: + if stack_path.replace(stack_root, source_root, 1).replace("\\", "/") != source_path: logger.info( "Unexpected stack_path/source_path found. A code mapping was not generated.", extra={ @@ -370,6 +386,10 @@ def convert_stacktrace_frame_path_to_source_path( If the code mapping does not apply to the frame, returns None. """ + stack_root = code_mapping.stack_root + if "\\" in code_mapping.stack_root: + stack_root = code_mapping.stack_root.replace("\\", "/") + # In most cases, code mappings get applied to frame.filename, but some platforms such as Java # contain folder info in other parts of the frame (e.g. frame.module="com.example.app.MainActivity" # gets transformed to "com/example/app/MainActivity.java"), so in those cases we use the @@ -379,13 +399,13 @@ def convert_stacktrace_frame_path_to_source_path( ) if stacktrace_path and stacktrace_path.startswith(code_mapping.stack_root): - return stacktrace_path.replace(code_mapping.stack_root, code_mapping.source_root, 1) + return stacktrace_path.replace(stack_root, code_mapping.source_root, 1) # Some platforms only provide the file's name without folder paths, so we # need to use the absolute path instead. If the code mapping has a non-empty # stack_root value and it matches the absolute path, we do the mapping on it. if frame.abs_path and frame.abs_path.startswith(code_mapping.stack_root): - return frame.abs_path.replace(code_mapping.stack_root, code_mapping.source_root, 1) + return frame.abs_path.replace(stack_root, code_mapping.source_root, 1) return None @@ -505,8 +525,8 @@ def find_roots(stack_path: str, source_path: str) -> tuple[str, str]: If there is no overlap, raise an exception since this should not happen """ stack_root = "" - if stack_path[0] == "/": - stack_root += "/" + if stack_path[0] == "/" or stack_path[0] == "\\": + stack_root += stack_path[0] stack_path = stack_path[1:] if stack_path == source_path: @@ -534,7 +554,7 @@ def find_roots(stack_path: str, source_path: str) -> tuple[str, str]: source_root = source_path.rpartition(overlap)[0] stack_root += stack_path_delim.join(stack_root_items) - if stack_root and stack_root[-1] != SLASH: # append trailing slash + if stack_root and stack_root[-1] != stack_path_delim: # append trailing slash stack_root = f"{stack_root}{stack_path_delim}" if source_root and source_root[-1] != SLASH: source_root = f"{source_root}{SLASH}" diff --git a/tests/sentry/integrations/utils/test_code_mapping.py b/tests/sentry/integrations/utils/test_code_mapping.py index 2623053400191e..c9863feb76e317 100644 --- a/tests/sentry/integrations/utils/test_code_mapping.py +++ b/tests/sentry/integrations/utils/test_code_mapping.py @@ -48,7 +48,6 @@ "README", # no extension "ssl.py", # XXX: The following will need to be supported - "C:\\Users\\Donia\\AppData\\Roaming\\Adobe\\UXP\\Plugins\\External\\452f92d2_0.13.0\\main.js", "initialization.dart", "backburner.js", ] diff --git a/tests/sentry/tasks/test_derive_code_mappings.py b/tests/sentry/tasks/test_derive_code_mappings.py index 11f0067f6f2a55..887735544d41cc 100644 --- a/tests/sentry/tasks/test_derive_code_mappings.py +++ b/tests/sentry/tasks/test_derive_code_mappings.py @@ -97,6 +97,83 @@ def test_raises_generic_errors(self, mock_logger): ) +class TestBackSlashDeriveCodeMappings(BaseDeriveCodeMappings): + def setUp(self): + super().setUp() + self.platform = "python" + # The lack of a \ after the drive letter in the third frame signals that + # this is a relative path. This may be unlikely to occur in practice, + # but worth testing nonetheless. + self.event_data = self.generate_data( + [ + {"in_app": True, "filename": "\\sentry\\mouse.py"}, + {"in_app": True, "filename": "\\sentry\\dog\\cat\\parrot.py"}, + {"in_app": True, "filename": "C:sentry\\tasks.py"}, + {"in_app": True, "filename": "D:\\Users\\code\\sentry\\models\\release.py"}, + ] + ) + + @responses.activate + def test_backslash_filename_simple(self): + repo_name = "foo/bar" + with patch( + "sentry.integrations.github.client.GitHubClientMixin.get_trees_for_org" + ) as mock_get_trees_for_org: + mock_get_trees_for_org.return_value = { + repo_name: RepoTree(Repo(repo_name, "master"), ["sentry/mouse.py"]) + } + derive_code_mappings(self.project.id, self.event_data) + code_mapping = RepositoryProjectPathConfig.objects.all()[0] + assert code_mapping.stack_root == "\\" + assert code_mapping.source_root == "" + assert code_mapping.repository.name == repo_name + + @responses.activate + def test_backslash_drive_letter_filename_simple(self): + repo_name = "foo/bar" + with patch( + "sentry.integrations.github.client.GitHubClientMixin.get_trees_for_org" + ) as mock_get_trees_for_org: + mock_get_trees_for_org.return_value = { + repo_name: RepoTree(Repo(repo_name, "master"), ["sentry/tasks.py"]) + } + derive_code_mappings(self.project.id, self.event_data) + code_mapping = RepositoryProjectPathConfig.objects.all()[0] + assert code_mapping.stack_root == "C:sentry\\" + assert code_mapping.source_root == "sentry/" + assert code_mapping.repository.name == repo_name + + @responses.activate + def test_backslash_drive_letter_filename_monorepo(self): + repo_name = "foo/bar" + with patch( + "sentry.integrations.github.client.GitHubClientMixin.get_trees_for_org" + ) as mock_get_trees_for_org: + mock_get_trees_for_org.return_value = { + repo_name: RepoTree(Repo(repo_name, "master"), ["src/sentry/tasks.py"]) + } + derive_code_mappings(self.project.id, self.event_data) + code_mapping = RepositoryProjectPathConfig.objects.all()[0] + assert code_mapping.stack_root == "C:sentry\\" + assert code_mapping.source_root == "src/sentry/" + assert code_mapping.repository.name == repo_name + + @responses.activate + def test_backslash_drive_letter_filename_abs_path(self): + repo_name = "foo/bar" + with patch( + "sentry.integrations.github.client.GitHubClientMixin.get_trees_for_org" + ) as mock_get_trees_for_org: + mock_get_trees_for_org.return_value = { + repo_name: RepoTree(Repo(repo_name, "master"), ["sentry/models/release.py"]) + } + derive_code_mappings(self.project.id, self.event_data) + code_mapping = RepositoryProjectPathConfig.objects.all()[0] + assert code_mapping.stack_root == "D:\\Users\\code\\" + assert code_mapping.source_root == "" + assert code_mapping.repository.name == repo_name + + class TestJavascriptDeriveCodeMappings(BaseDeriveCodeMappings): def setUp(self): super().setUp()