Repository: getsentry/sentry
Pull request: Use GroupResolution to query for groups resolved in a release
Fixes #5786 

Problem statement:
Issues resolved via UI do not appear in releases overview
To reproduce:

* Navigate to an issue
* Click "Resolve in current release (abc123)"
* Navigate to that release (abc123)
* Issue does not appear under "New Issues in this Release"

Hints from issue discussion:

Commit message: Add tests

Patch to review:
diff --git a/src/sentry/api/endpoints/issues_resolved_in_release.py b/src/sentry/api/endpoints/issues_resolved_in_release.py
index 3673b6c99916bd..eee0efd4dbbbf5 100644
--- a/src/sentry/api/endpoints/issues_resolved_in_release.py
+++ b/src/sentry/api/endpoints/issues_resolved_in_release.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import
 
+from itertools import chain
 from rest_framework.response import Response
 
 from sentry.api.base import DocSection
@@ -10,6 +11,7 @@
 from sentry.models import (
     Group,
     GroupCommitResolution,
+    GroupResolution,
     Release,
     ReleaseCommit,
 )
@@ -37,13 +39,26 @@ def get(self, request, project, version):
         except Release.DoesNotExist:
             raise ResourceDoesNotExist
 
+        ids_from_GroupResolution = GroupResolution.objects.filter(
+            release=release,
+        ).values_list(
+            'group_id', flat=True
+        )
+
+        ids_from_GroupCommitResolution = GroupCommitResolution.objects.filter(
+            commit_id__in=ReleaseCommit.objects.filter(
+                release=release,
+            ).values_list(
+                'commit_id',
+                flat=True,
+            )
+        ).values_list(
+            'group_id',
+            flat=True,
+        )
+
         groups = Group.objects.filter(
-            project=project,
-            id__in=GroupCommitResolution.objects.filter(
-                commit_id__in=ReleaseCommit.objects.filter(
-                    release=release,
-                ).values_list('commit_id', flat=True),
-            ).values_list('group_id', flat=True),
+            project=project, id__in=chain(ids_from_GroupResolution, ids_from_GroupCommitResolution)
         )
 
         context = serialize(list(groups), request.user, StreamGroupSerializer(stats_period=None))
diff --git a/tests/sentry/api/endpoints/test_issues_resolved_in_release.py b/tests/sentry/api/endpoints/test_issues_resolved_in_release.py
new file mode 100644
index 00000000000000..33e2824edae944
--- /dev/null
+++ b/tests/sentry/api/endpoints/test_issues_resolved_in_release.py
@@ -0,0 +1,238 @@
+from __future__ import absolute_import
+
+from django.core.urlresolvers import reverse
+import six
+
+from sentry.models import (
+    Commit,
+    GroupCommitResolution,
+    GroupResolution,
+    Release,
+    ReleaseCommit,
+    Repository,
+)
+
+from sentry.testutils import APITestCase
+
+
+class IssuesResolvedInReleaseEndpointTest(APITestCase):
+    def test_shows_issues_from_groupresolution(self):
+        """
+        tests that the endpoint will correctly retrieve issues resolved
+        in a release from the GroupResolution model
+        """
+        user = self.create_user('foo@example.com', is_superuser=True)
+        project = self.create_project(
+            name='foo',
+        )
+        self.login_as(user)
+        release = Release.objects.create(
+            organization_id=project.organization_id,
+            version='1',
+        )
+        release.add_project(project)
+        group = self.create_group(project=project)
+        GroupResolution.objects.create(
+            group=group,
+            release=release,
+            type=GroupResolution.Type.in_release,
+        )
+        url = reverse(
+            'sentry-api-0-release-resolved',
+            kwargs={
+                'organization_slug': project.organization.slug,
+                'project_slug': project.slug,
+                'version': release.version,
+            }
+        )
+
+        response = self.client.get(url)
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        assert response.data[0]['id'] == six.text_type(group.id)
+
+    def test_shows_issues_from_groupcommitresolution(self):
+        """
+        tests that the endpoint will correctly retrieve issues resolved
+        in a release from the GroupCommitResolution model
+        """
+        user = self.create_user('foo@example.com', is_superuser=True)
+        project = self.create_project(
+            name='foo',
+        )
+        self.login_as(user)
+        release = Release.objects.create(
+            organization_id=project.organization_id,
+            version='1',
+        )
+        release.add_project(project)
+        group = self.create_group(project=project)
+        repo = Repository.objects.create(
+            organization_id=project.organization_id,
+            name=project.name,
+        )
+        commit = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='a' * 40,
+        )
+        commit2 = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='b' * 40,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit,
+            order=1,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit2,
+            order=0,
+        )
+        GroupCommitResolution.objects.create(group_id=group.id, commit_id=commit.id)
+        url = reverse(
+            'sentry-api-0-release-resolved',
+            kwargs={
+                'organization_slug': project.organization.slug,
+                'project_slug': project.slug,
+                'version': release.version,
+            }
+        )
+
+        response = self.client.get(url)
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        assert response.data[0]['id'] == six.text_type(group.id)
+
+    def test_does_not_return_duplicate_groups(self):
+        """
+        tests that the endpoint will correctly retrieve issues resolved
+        in a release from the GroupCommitResolution and GroupResolution model
+        but will not return the groups twice if they appear in both
+        """
+        user = self.create_user('foo@example.com', is_superuser=True)
+        project = self.create_project(
+            name='foo',
+        )
+        self.login_as(user)
+        release = Release.objects.create(
+            organization_id=project.organization_id,
+            version='1',
+        )
+        release.add_project(project)
+        group = self.create_group(project=project)
+        repo = Repository.objects.create(
+            organization_id=project.organization_id,
+            name=project.name,
+        )
+        commit = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='a' * 40,
+        )
+        commit2 = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='b' * 40,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit,
+            order=1,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit2,
+            order=0,
+        )
+        GroupCommitResolution.objects.create(group_id=group.id, commit_id=commit.id)
+        GroupResolution.objects.create(
+            group=group,
+            release=release,
+            type=GroupResolution.Type.in_release,
+        )
+        url = reverse(
+            'sentry-api-0-release-resolved',
+            kwargs={
+                'organization_slug': project.organization.slug,
+                'project_slug': project.slug,
+                'version': release.version,
+            }
+        )
+
+        response = self.client.get(url)
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        assert response.data[0]['id'] == six.text_type(group.id)
+
+    def test_return_groups_from_both_types(self):
+        """
+        tests that the endpoint will correctly retrieve issues resolved
+        in a release from both the GroupCommitResolution and GroupResolution model
+        """
+        user = self.create_user('foo@example.com', is_superuser=True)
+        project = self.create_project(
+            name='foo',
+        )
+        self.login_as(user)
+        release = Release.objects.create(
+            organization_id=project.organization_id,
+            version='1',
+        )
+        release.add_project(project)
+        group = self.create_group(project=project)
+        group2 = self.create_group(project=project)
+        repo = Repository.objects.create(
+            organization_id=project.organization_id,
+            name=project.name,
+        )
+        commit = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='a' * 40,
+        )
+        commit2 = Commit.objects.create(
+            organization_id=project.organization_id,
+            repository_id=repo.id,
+            key='b' * 40,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit,
+            order=1,
+        )
+        ReleaseCommit.objects.create(
+            organization_id=project.organization_id,
+            release=release,
+            commit=commit2,
+            order=0,
+        )
+        GroupCommitResolution.objects.create(group_id=group.id, commit_id=commit.id)
+        GroupResolution.objects.create(
+            group=group2,
+            release=release,
+            type=GroupResolution.Type.in_release,
+        )
+        url = reverse(
+            'sentry-api-0-release-resolved',
+            kwargs={
+                'organization_slug': project.organization.slug,
+                'project_slug': project.slug,
+                'version': release.version,
+            }
+        )
+
+        response = self.client.get(url)
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 2


Code review comments:
diff_hunk: @@ -37,13 +39,26 @@ def get(self, request, project, version):
         except Release.DoesNotExist:
             raise ResourceDoesNotExist
 
+        ids_from_GroupResolution = GroupResolution.objects.filter(
+            release=release,
+        ).values_list(
+            'group_id', flat=True
+        )
+
+        ids_from_GroupCommitResolution = GroupCommitResolution.objects.filter(
+            commit_id__in=ReleaseCommit.objects.filter(
path: src/sentry/api/endpoints/issues_resolved_in_release.py
text: @user1:
We can't make this query without a full sequence scan. There is no index on `commit_id`. We only have a unique on `(group_id, commit_id)`.

@author:
I think we were already making this query, right? What's the better solution - just use `GroupResolution` and not worry too much about loss of history?

@user1:
If we make this query somewhere else, mind pointing it out for me? Becuase it's bad and we should fix it. :)

The correct answer is to add an index to this column if we need to. Especially if you're trying to say "give me the groups for this commit id". We just don't currently have an index for this.

@user1:
Oh, I see, in the previous version of the code.

Then let's do this:

* Move forward with this as-is
* Follow up with adding an index to this column so we can stop full sequence scanning this table. Fortunately it's a really small table, so it's not a problem today, but as this grows, it'll become more and more of a problem, so might as well address it sooner than later.

@author:
I meant that this endpoint is already executing that query as is: https://github.com/getsentry/sentry/blob/96bc1c63df5ec73fe12c136ada11561bf52f1ec9/src/sentry/api/endpoints/issues_resolved_in_release.py#L42

diff_hunk: @@ -37,13 +39,26 @@ def get(self, request, project, version):
         except Release.DoesNotExist:
             raise ResourceDoesNotExist
 
+        ids_from_GroupResolution = GroupResolution.objects.filter(
path: src/sentry/api/endpoints/issues_resolved_in_release.py
text: @user1:
nitpick: these names are mixing snake case and camelcase, which is a bit weird. I'd just do this to simplify everthing:

```python
group_ids = set()
group_ids |= set(GroupResolution.objects.filter(...))
group_ids |= set(GroupCommitResolution.objects.filter(...))
```

Then you can simply pass this set along to the following queryset and avoid `chain()` as well, since that doesn't help you iwht much here. set() would also dedupe group ids.

@author:
Fixed!