Skip to content

Commit 5e70962

Browse files
author
NisanthanNanthakumar
authored
feat(commit-context): Do not create if older than 1 year old (#54866)
## Objective Update on the reverted PR: #54624 The original PR had errors from GitLab date strings not being parsed correctly bc they are formatted differently from GitHub. So this PR returns the datetime object for internal usage and will make date comparisons easier without needing to know which integration it came from.
1 parent ad908a0 commit 5e70962

File tree

6 files changed

+70
-25
lines changed

6 files changed

+70
-25
lines changed

src/sentry/integrations/github/integration.py

+8-5
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22

33
import logging
44
import re
5-
from datetime import datetime
5+
from datetime import timezone
66
from typing import Any, Collection, Dict, Mapping, Sequence
77

88
from django.http import HttpResponse
99
from django.utils.text import slugify
1010
from django.utils.translation import gettext_lazy as _
11+
from isodate import parse_datetime
1112
from rest_framework.request import Request
1213

1314
from sentry import features, options
@@ -249,9 +250,7 @@ def get_commit_context(
249250
for blame in blame_range
250251
if blame.get("startingLine", 0) <= lineno <= blame.get("endingLine", 0)
251252
),
252-
key=lambda blame: datetime.strptime(
253-
blame.get("commit", {}).get("committedDate"), "%Y-%m-%dT%H:%M:%SZ"
254-
),
253+
key=lambda blame: parse_datetime(blame.get("commit", {}).get("committedDate")),
255254
default={},
256255
)
257256
if not commit:
@@ -263,9 +262,13 @@ def get_commit_context(
263262
if not commitInfo:
264263
return None
265264
else:
265+
committed_date = parse_datetime(commitInfo.get("committed_date")).astimezone(
266+
timezone.utc
267+
)
268+
266269
return {
267270
"commitId": commitInfo.get("oid"),
268-
"committedDate": commitInfo.get("committedDate"),
271+
"committedDate": committed_date,
269272
"commitMessage": commitInfo.get("message"),
270273
"commitAuthorName": commitInfo.get("author", {}).get("name"),
271274
"commitAuthorEmail": commitInfo.get("author", {}).get("email"),

src/sentry/integrations/gitlab/integration.py

+7-9
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
from __future__ import annotations
22

3-
from datetime import datetime, timezone
3+
from datetime import timezone
44
from typing import Any, Mapping, Sequence
55
from urllib.parse import urlparse
66

77
from django import forms
88
from django.http import HttpResponse
99
from django.utils.translation import gettext_lazy as _
10+
from isodate import parse_datetime
1011
from rest_framework.request import Request
1112

1213
from sentry.identity.gitlab import get_oauth_data, get_user_info
@@ -166,13 +167,10 @@ def get_commit_context(
166167
except ApiError as e:
167168
raise e
168169

169-
date_format_expected = "%Y-%m-%dT%H:%M:%S.%f%z"
170170
try:
171171
commit = max(
172172
blame_range,
173-
key=lambda blame: datetime.strptime(
174-
blame.get("commit", {}).get("committed_date"), date_format_expected
175-
),
173+
key=lambda blame: parse_datetime(blame.get("commit", {}).get("committed_date")),
176174
)
177175
except (ValueError, IndexError):
178176
return None
@@ -181,11 +179,11 @@ def get_commit_context(
181179
if not commitInfo:
182180
return None
183181
else:
184-
committed_date = "{}Z".format(
185-
datetime.strptime(commitInfo.get("committed_date"), date_format_expected)
186-
.astimezone(timezone.utc)
187-
.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3]
182+
# TODO(nisanthan): Use dateutil.parser.isoparse once on python 3.11
183+
committed_date = parse_datetime(commitInfo.get("committed_date")).astimezone(
184+
timezone.utc
188185
)
186+
189187
return {
190188
"commitId": commitInfo.get("id"),
191189
"committedDate": committed_date,

src/sentry/integrations/utils/commit_context.py

+7-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from __future__ import annotations
22

33
import logging
4+
from datetime import datetime, timedelta, timezone
45
from typing import Any, List, Mapping, Sequence, Tuple
56

67
import sentry_sdk
@@ -108,7 +109,12 @@ def find_commit_context_for_event(
108109
},
109110
)
110111

111-
if commit_context:
112+
# Only return suspect commits that are less than a year old
113+
if commit_context and is_date_less_than_year(commit_context["committedDate"]):
112114
result.append((commit_context, code_mapping))
113115

114116
return result, installation
117+
118+
119+
def is_date_less_than_year(date: datetime):
120+
return date > datetime.now(tz=timezone.utc) - timedelta(days=365)

tests/sentry/integrations/gitlab/test_integration.py

+2-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import responses
66
from django.core.cache import cache
77
from django.test import override_settings
8+
from isodate import parse_datetime
89

910
from fixtures.gitlab import GET_COMMIT_RESPONSE, GitLabTestCase
1011
from sentry.integrations.gitlab import GitlabIntegrationProvider
@@ -425,7 +426,7 @@ def test_get_commit_context(self):
425426

426427
commit_context_expected = {
427428
"commitId": "d42409d56517157c48bf3bd97d3f75974dde19fb",
428-
"committedDate": "2015-12-18T08:12:22.000Z",
429+
"committedDate": parse_datetime("2015-12-18T08:12:22.000Z"),
429430
"commitMessage": "Add installation instructions",
430431
"commitAuthorName": "Nisanthan Nanthakumar",
431432
"commitAuthorEmail": "[email protected]",

tests/sentry/tasks/test_commit_context.py

+45-8
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
from datetime import timedelta
1+
from datetime import datetime, timedelta
2+
from datetime import timezone as datetime_timezone
23
from unittest.mock import Mock, patch
34

45
import pytest
@@ -83,7 +84,7 @@ class TestCommitContext(TestCommitContextMixin):
8384
"sentry.integrations.github.GitHubIntegration.get_commit_context",
8485
return_value={
8586
"commitId": "asdfwreqr",
86-
"committedDate": "2023-02-14T11:11Z",
87+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
8788
"commitMessage": "placeholder commit message",
8889
"commitAuthorName": "",
8990
"commitAuthorEmail": "admin@localhost",
@@ -141,11 +142,47 @@ def test_failed_to_fetch_commit_context_record(self, mock_get_commit_context, mo
141142
error_message="integration_failed",
142143
)
143144

145+
@patch("sentry.tasks.commit_context.logger")
144146
@patch(
145147
"sentry.integrations.github.GitHubIntegration.get_commit_context",
146148
return_value={
147149
"commitId": "asdfasdf",
148-
"committedDate": "2023-02-14T11:11Z",
150+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=370)),
151+
"commitMessage": "placeholder commit message",
152+
"commitAuthorName": "",
153+
"commitAuthorEmail": "admin@localhost",
154+
},
155+
)
156+
def test_found_commit_is_too_old(self, mock_get_commit_context, mock_logger):
157+
with self.tasks():
158+
assert not GroupOwner.objects.filter(group=self.event.group).exists()
159+
event_frames = get_frame_paths(self.event)
160+
process_commit_context(
161+
event_id=self.event.event_id,
162+
event_platform=self.event.platform,
163+
event_frames=event_frames,
164+
group_id=self.event.group_id,
165+
project_id=self.event.project_id,
166+
)
167+
168+
assert mock_logger.info.call_count == 1
169+
mock_logger.info.assert_called_with(
170+
"process_commit_context.find_commit_context",
171+
extra={
172+
"event": self.event.event_id,
173+
"group": self.event.group_id,
174+
"organization": self.event.group.project.organization_id,
175+
"reason": "could_not_fetch_commit_context",
176+
"code_mappings_count": 1,
177+
"fallback": True,
178+
},
179+
)
180+
181+
@patch(
182+
"sentry.integrations.github.GitHubIntegration.get_commit_context",
183+
return_value={
184+
"commitId": "asdfasdf",
185+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
149186
"commitMessage": "placeholder commit message",
150187
"commitAuthorName": "",
151188
"commitAuthorEmail": "admin@localhost",
@@ -170,7 +207,7 @@ def test_no_matching_commit_in_db(self, mock_get_commit_context):
170207
"sentry.integrations.github.GitHubIntegration.get_commit_context",
171208
return_value={
172209
"commitId": "asdfwreqr",
173-
"committedDate": "2023-02-14T11:11Z",
210+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
174211
"commitMessage": "placeholder commit message",
175212
"commitAuthorName": "",
176213
"commitAuthorEmail": "admin@localhost",
@@ -255,7 +292,7 @@ def test_no_inapp_frame_in_stacktrace(self, mock_process_suspect_commits):
255292
"sentry.integrations.github.GitHubIntegration.get_commit_context",
256293
return_value={
257294
"commitId": "somekey",
258-
"committedDate": "2023-02-14T11:11Z",
295+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
259296
"commitMessage": "placeholder commit message",
260297
"commitAuthorName": "",
261298
"commitAuthorEmail": "[email protected]",
@@ -296,7 +333,7 @@ def test_commit_author_not_in_sentry(self, mock_get_commit_context):
296333
"sentry.integrations.github.GitHubIntegration.get_commit_context",
297334
return_value={
298335
"commitId": "somekey",
299-
"committedDate": "2023-02-14T11:11Z",
336+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
300337
"commitMessage": "placeholder commit message",
301338
"commitAuthorName": "",
302339
"commitAuthorEmail": "[email protected]",
@@ -337,7 +374,7 @@ def test_commit_author_no_user(self, mock_get_commit_context, mock_get_users_for
337374
"sentry.integrations.github.GitHubIntegration.get_commit_context",
338375
return_value={
339376
"commitId": "somekey",
340-
"committedDate": "2023-02-14T11:11Z",
377+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
341378
"commitMessage": "placeholder commit message",
342379
"commitAuthorName": "",
343380
"commitAuthorEmail": "[email protected]",
@@ -423,7 +460,7 @@ def after_return(self, status, retval, task_id, args, kwargs, einfo):
423460
Mock(
424461
return_value={
425462
"commitId": "asdfwreqr",
426-
"committedDate": "2023-02-14T11:11Z",
463+
"committedDate": (datetime.now(tz=datetime_timezone.utc) - timedelta(days=7)),
427464
"commitMessage": "placeholder commit message",
428465
"commitAuthorName": "",
429466
"commitAuthorEmail": "admin@localhost",

tests/sentry/tasks/test_post_process.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -1303,7 +1303,7 @@ def test_issue_owners_should_ratelimit(self, logger):
13031303
class ProcessCommitsTestMixin(BasePostProgressGroupMixin):
13041304
github_blame_return_value = {
13051305
"commitId": "asdfwreqr",
1306-
"committedDate": "",
1306+
"committedDate": (datetime.now(timezone.utc) - timedelta(days=2)),
13071307
"commitMessage": "placeholder commit message",
13081308
"commitAuthorName": "",
13091309
"commitAuthorEmail": "admin@localhost",

0 commit comments

Comments
 (0)