Browse Source

fix(commits) Improve suspect commit matching for java (#12046)

Java stacktrace frames do not have `abs_path` or full paths in the
`filename` key as that information is not easily available in Java's
stdlib. We need a full file path to match frames with commits. We can
leverage the general practice of java module paths ~= file paths to
improve suspect commit matching for java projects.

Ideally in the longer term the java-sdk would mangle stackframes before
submitting them but we're not there yet.

Fixes APP-1099
Mark Story 6 years ago
parent
commit
a4295b2fe4
2 changed files with 212 additions and 2 deletions
  1. 10 0
      src/sentry/utils/committers.py
  2. 202 2
      tests/sentry/utils/test_committers.py

+ 10 - 0
src/sentry/utils/committers.py

@@ -191,6 +191,16 @@ def get_event_file_committers(project, event, frame_limit=25):
     if not app_frames:
         app_frames = [frame for frame in frames][-frame_limit:]
 
+    # Java stackframes don't have an absolute path in the filename key.
+    # That property is usually just the basename of the file. In the future
+    # the Java SDK might generate better file paths, but for now we use the module
+    # path to approximate the file path so that we can intersect it with commit
+    # file paths.
+    if event.platform == 'java':
+        for frame in frames:
+            if '/' not in frame.get('filename') and frame.get('module'):
+                frame['filename'] = frame['module'].replace('.', '/') + '/' + frame['filename']
+
     # TODO(maxbittker) return this set instead of annotated frames
     # XXX(dcramer): frames may not define a filepath. For example, in Java its common
     # to only have a module name/path

+ 202 - 2
tests/sentry/utils/test_committers.py

@@ -7,9 +7,17 @@ from uuid import uuid4
 
 from sentry.models import Commit, CommitAuthor, CommitFileChange, Release, Repository
 from sentry.testutils import TestCase
-from sentry.utils.committers import _get_commit_file_changes, _get_frame_paths, get_previous_releases, _match_commits_path, score_path_match_length, tokenize_path
+from sentry.utils.committers import (
+    _get_commit_file_changes,
+    _get_frame_paths,
+    _match_commits_path,
+    get_event_file_committers,
+    get_previous_releases,
+    score_path_match_length,
+    tokenize_path
+)
 
-# TODO(lb): Tests are still needed for _get_committers and _get_vent_file_commiters
+# TODO(lb): Tests are still needed for _get_committers and _get_event_file_commiters
 
 
 class CommitTestCase(TestCase):
@@ -207,3 +215,195 @@ class GetPreviousReleasesTestCase(TestCase):
         assert len(releases) == 2
         assert releases[0] == release2
         assert releases[1] == release1
+
+
+class GetEventFileCommitters(CommitTestCase):
+    def setUp(self):
+        super(GetEventFileCommitters, self).setUp()
+        self.release = self.create_release(
+            project=self.project,
+            version='v12'
+        )
+        self.group = self.create_group(
+            project=self.project,
+            message='Kaboom!',
+            first_release=self.release,
+        )
+
+    def test_java_sdk_path_mangling(self):
+        event = self.create_event(
+            group=self.group,
+            message='Kaboom!',
+            platform='java',
+            stacktrace={
+                'frames': [
+                    {
+                        "function": "invoke0",
+                        "abs_path": "NativeMethodAccessorImpl.java",
+                        "in_app": False,
+                        "module": "jdk.internal.reflect.NativeMethodAccessorImpl",
+                        "filename": "NativeMethodAccessorImpl.java",
+                    },
+                    {
+                        "function": "home",
+                        "abs_path": "Application.java",
+                        "module": "io.sentry.example.Application",
+                        "in_app": True,
+                        "lineno": 30,
+                        "filename": "Application.java",
+                    },
+                    {
+                        "function": "handledError",
+                        "abs_path": "Application.java",
+                        "module": "io.sentry.example.Application",
+                        "in_app": True,
+                        "lineno": 39,
+                        "filename": "Application.java",
+                    }
+                ]
+            }
+        )
+        self.release.set_commits([
+            {
+                'id': 'a' * 40,
+                'repository': self.repo.name,
+                'author_email': 'bob@example.com',
+                'author_name': 'Bob',
+                'message': 'i fixed a bug',
+                'patch_set': [
+                    {
+                        'path': 'sentry/example/Application/Application.java',
+                        'type': 'M',
+                    },
+                ]
+            }
+        ])
+
+        result = get_event_file_committers(self.project, event)
+        assert len(result) == 1
+        assert 'commits' in result[0]
+        assert len(result[0]['commits']) == 1
+        assert result[0]['commits'][0]['id'] == 'a' * 40
+
+    def test_matching(self):
+        event = self.create_event(
+            group=self.group,
+            message='Kaboom!',
+            platform='python',
+            stacktrace={
+                'frames': [
+                    {
+                        "function": "handle_set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/tasks.py",
+                        "module": "sentry.tasks",
+                        "in_app": True,
+                        "lineno": 30,
+                        "filename": "sentry/tasks.py",
+                    },
+                    {
+                        "function": "set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/models/release.py",
+                        "module": "sentry.models.release",
+                        "in_app": True,
+                        "lineno": 39,
+                        "filename": "sentry/models/release.py",
+                    }
+                ]
+            }
+        )
+        self.release.set_commits([
+            {
+                'id': 'a' * 40,
+                'repository': self.repo.name,
+                'author_email': 'bob@example.com',
+                'author_name': 'Bob',
+                'message': 'i fixed a bug',
+                'patch_set': [
+                    {
+                        'path': 'src/sentry/models/release.py',
+                        'type': 'M',
+                    },
+                ]
+            }
+        ])
+
+        result = get_event_file_committers(self.project, event)
+        assert len(result) == 1
+        assert 'commits' in result[0]
+        assert len(result[0]['commits']) == 1
+        assert result[0]['commits'][0]['id'] == 'a' * 40
+
+    def test_not_matching(self):
+        event = self.create_event(
+            group=self.group,
+            message='Kaboom!',
+            platform='python',
+            stacktrace={
+                'frames': [
+                    {
+                        "function": "handle_set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/tasks.py",
+                        "module": "sentry.tasks",
+                        "in_app": True,
+                        "lineno": 30,
+                        "filename": "sentry/tasks.py",
+                    },
+                    {
+                        "function": "set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/models/release.py",
+                        "module": "sentry.models.release",
+                        "in_app": True,
+                        "lineno": 39,
+                        "filename": "sentry/models/release.py",
+                    }
+                ]
+            }
+        )
+        self.release.set_commits([
+            {
+                'id': 'a' * 40,
+                'repository': self.repo.name,
+                'author_email': 'bob@example.com',
+                'author_name': 'Bob',
+                'message': 'i fixed a bug',
+                'patch_set': [
+                    {
+                        'path': 'some/other/path.py',
+                        'type': 'M',
+                    },
+                ]
+            }
+        ])
+
+        result = get_event_file_committers(self.project, event)
+        assert len(result) == 0
+
+    def test_no_commits(self):
+        event = self.create_event(
+            group=self.group,
+            message='Kaboom!',
+            platform='python',
+            stacktrace={
+                'frames': [
+                    {
+                        "function": "handle_set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/tasks.py",
+                        "module": "sentry.tasks",
+                        "in_app": True,
+                        "lineno": 30,
+                        "filename": "sentry/tasks.py",
+                    },
+                    {
+                        "function": "set_commits",
+                        "abs_path": "/usr/src/sentry/src/sentry/models/release.py",
+                        "module": "sentry.models.release",
+                        "in_app": True,
+                        "lineno": 39,
+                        "filename": "sentry/models/release.py",
+                    }
+                ]
+            }
+        )
+
+        with self.assertRaises(Commit.DoesNotExist):
+            get_event_file_committers(self.project, event)