Skip to content

fix(API): Allow range in commit parameter of org releases endpoint as per docs #13618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 12, 2019
Merged

fix(API): Allow range in commit parameter of org releases endpoint as per docs #13618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98db33a
add commit parameter validation and tests
lobsterkatie Jun 10, 2019
6ecbfb8
go back to hard-coded commit range delimiter
lobsterkatie Jun 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/sentry/api/serializers/rest_framework/release.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from sentry.api.serializers.rest_framework import CommitSerializer, ListField
from sentry.api.fields.user import UserField
from sentry.constants import MAX_COMMIT_LENGTH, MAX_VERSION_LENGTH
from sentry.constants import COMMIT_RANGE_DELIMITER, MAX_COMMIT_LENGTH, MAX_VERSION_LENGTH
from sentry.models import Release


Expand All @@ -15,10 +15,38 @@ class ReleaseHeadCommitSerializerDeprecated(serializers.Serializer):


class ReleaseHeadCommitSerializer(serializers.Serializer):
commit = serializers.CharField(max_length=MAX_COMMIT_LENGTH)
commit = serializers.CharField()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the downstream model code knows how to split a commit string containing .. into its prev/next parts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, right here:

sentry/src/sentry/models/release.py

Lines 275 to 288 in 75ab9e5

def handle_commit_ranges(self, refs):
"""
Takes commit refs of the form:
[
{
'previousCommit': None,
'commit': 'previous_commit..commit',
}
]
Note: Overwrites 'previousCommit' and 'commit'
"""
for ref in refs:
if COMMIT_RANGE_DELIMITER in ref['commit']:
ref['previousCommit'], ref['commit'] = ref['commit'].split(COMMIT_RANGE_DELIMITER)

repository = serializers.CharField(max_length=200)
previousCommit = serializers.CharField(max_length=MAX_COMMIT_LENGTH, required=False)

def validate_commit(self, attrs, source):
"""
Value can be either a single commit or a commit range (1a2b3c..6f5e4d)
"""
value = attrs[source]

if COMMIT_RANGE_DELIMITER in value:
startCommit, endCommit = value.split(COMMIT_RANGE_DELIMITER)

if not startCommit or not endCommit:
raise serializers.ValidationError(
'Commit cannot begin or end with %s' %
COMMIT_RANGE_DELIMITER)

if len(startCommit) > MAX_COMMIT_LENGTH or len(endCommit) > MAX_COMMIT_LENGTH:
raise serializers.ValidationError(
'Start or end commit too long - max is %s chars each' %
MAX_COMMIT_LENGTH)

return attrs

if len(value) > MAX_COMMIT_LENGTH:
raise serializers.ValidationError(
'Commit too long - max is %s chars' %
MAX_COMMIT_LENGTH)

return attrs


class ReleaseSerializer(serializers.Serializer):
ref = serializers.CharField(max_length=MAX_VERSION_LENGTH, required=False)
Expand Down
122 changes: 120 additions & 2 deletions tests/sentry/api/endpoints/test_organization_releases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,15 @@
from django.core.urlresolvers import reverse
from exam import fixture

from sentry.api.endpoints.organization_releases import ReleaseSerializerWithProjects
from sentry.constants import BAD_RELEASE_CHARS, MAX_VERSION_LENGTH
from sentry.api.endpoints.organization_releases import (
ReleaseHeadCommitSerializer,
ReleaseSerializerWithProjects,
)
from sentry.constants import (
BAD_RELEASE_CHARS,
MAX_COMMIT_LENGTH,
MAX_VERSION_LENGTH,
)
from sentry.models import (
Activity,
ApiKey,
Expand Down Expand Up @@ -1506,3 +1513,114 @@ def test_version_cannot_be_latest(self):
'projects': self.projects,
})
assert not serializer.is_valid()


class ReleaseHeadCommitSerializerTest(TestCase):
def setUp(self):
super(ReleaseHeadCommitSerializerTest, self).setUp()
self.repo_name = 'repo/name'
self.commit = 'b' * 40
self.commit_range = '%s..%s' % ('a' * 40, 'b' * 40)
self.prev_commit = 'a' * 40

def test_simple(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': self.prev_commit,
'repository': self.repo_name
})

assert serializer.is_valid()
assert sorted(serializer.fields.keys()) == sorted(
['commit', 'previousCommit', 'repository'])
result = serializer.object
assert result['commit'] == self.commit
assert result['previousCommit'] == self.prev_commit
assert result['repository'] == self.repo_name

def test_prev_commit_not_required(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': None,
'repository': self.repo_name
})
assert serializer.is_valid()

def test_do_not_allow_null_or_empty_commit_or_repo(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': None,
'previousCommit': self.prev_commit,
'repository': self.repo_name
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': '',
'previousCommit': self.prev_commit,
'repository': self.repo_name
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': self.prev_commit,
'repository': None
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': self.prev_commit,
'repository': ''
})
assert not serializer.is_valid()

def test_single_commit_limited_by_max_commit_length(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': 'b' * MAX_COMMIT_LENGTH,
'repository': self.repo_name,
})
assert serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': 'a' * MAX_COMMIT_LENGTH,
'repository': self.repo_name,
})
assert serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': 'b' * (MAX_COMMIT_LENGTH + 1),
'repository': self.repo_name,
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': self.commit,
'previousCommit': 'a' * (MAX_COMMIT_LENGTH + 1),
'repository': self.repo_name,
})
assert not serializer.is_valid()

def test_commit_range_does_not_allow_empty_commits(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': '%s..%s' % ('', 'b' * MAX_COMMIT_LENGTH),
'repository': self.repo_name,
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': '%s..%s' % ('a' * MAX_COMMIT_LENGTH, ''),
'repository': self.repo_name,
})
assert not serializer.is_valid()

def test_commit_range_limited_by_max_commit_length(self):
serializer = ReleaseHeadCommitSerializer(data={
'commit': '%s..%s' % ('a' * MAX_COMMIT_LENGTH, 'b' * MAX_COMMIT_LENGTH),
'repository': self.repo_name,
})
assert serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': '%s..%s' % ('a' * (MAX_COMMIT_LENGTH + 1), 'b' * MAX_COMMIT_LENGTH),
'repository': self.repo_name,
})
assert not serializer.is_valid()
serializer = ReleaseHeadCommitSerializer(data={
'commit': '%s..%s' % ('a' * MAX_COMMIT_LENGTH, 'b' * (MAX_COMMIT_LENGTH + 1)),
'repository': self.repo_name,
})
assert not serializer.is_valid()