diff --git a/docs/manual/webapi/2.0/resources/index.rst b/docs/manual/webapi/2.0/resources/index.rst index 7f15ac2cad8a69bffc276ed1f1fba925fb05613f..f8156e3b6f893ad25116c2cf65d002393503e93d 100644 --- a/docs/manual/webapi/2.0/resources/index.rst +++ b/docs/manual/webapi/2.0/resources/index.rst @@ -234,6 +234,7 @@ Validation validation validate-diff + validate-diff-commit WebHooks diff --git a/docs/manual/webapi/2.0/resources/validate-diff-commit.rst b/docs/manual/webapi/2.0/resources/validate-diff-commit.rst new file mode 100644 index 0000000000000000000000000000000000000000..838478c3a6b005a657bff70b3946bbce51e71b5d --- /dev/null +++ b/docs/manual/webapi/2.0/resources/validate-diff-commit.rst @@ -0,0 +1,3 @@ +.. webapi-resource:: + :classname: reviewboard.webapi.resources.validate_diffcommit.ValidateDiffCommitResource + :is-list: diff --git a/reviewboard/diffviewer/commit_utils.py b/reviewboard/diffviewer/commit_utils.py new file mode 100644 index 0000000000000000000000000000000000000000..7b5c26bd3b0ac4d5241dce3081634919a8e663ad --- /dev/null +++ b/reviewboard/diffviewer/commit_utils.py @@ -0,0 +1,63 @@ +"""Utilities for dealing with DiffCommits.""" + +from __future__ import unicode_literals + +from itertools import chain + +from reviewboard.scmtools.core import UNKNOWN + + +def get_file_exists_in_history(validation_info, repository, parent_id, path, + revision, base_commit_id=None, request=None): + """Return whether or not the file exists, given the validation information. + + Args: + validation_info (dict): + Validation metadata generated by the + :py:class:`~reviewboard.webapi.resources.validate_diffcommit. + ValidateDiffCommitResource`. + + repository (reviewboard.scmtools.models.Repository): + The repository. + + parent_id (unicode): + The parent commit ID of the commit currently being processed. + + path (unicode): + The file path. + + revision (unicode): + The revision of the file to retrieve. + + base_commit_id (unicode, optional): + The base commit ID of the commit series. + + request (django.http.HttpRequest): + The HTTP request from the client. + + Returns: + bool: + Whether or not the file exists. + """ + while parent_id in validation_info: + entry = validation_info[parent_id] + tree = entry['tree'] + + if revision == UNKNOWN: + for removed_info in tree['removed']: + if removed_info['filename'] == path: + return False + + for added_info in chain(tree['added'], tree['modified']): + if (added_info['filename'] == path and + (revision == UNKNOWN or + added_info['revision'] == revision)): + return True + + parent_id = entry['parent_id'] + + # We did not find an entry in our validation info, so we need to fall back + # to checking the repository. + return repository.get_file_exists(path, revision, + base_commit_id=base_commit_id, + request=request) diff --git a/reviewboard/diffviewer/diffutils.py b/reviewboard/diffviewer/diffutils.py index 37720d129a3119757e177e4f9d4d0fdae55b950c..d72d0992a205db508a2471a7afc7af4eddf11b1f 100644 --- a/reviewboard/diffviewer/diffutils.py +++ b/reviewboard/diffviewer/diffutils.py @@ -16,7 +16,7 @@ from djblets.log import log_timed from djblets.siteconfig.models import SiteConfiguration from djblets.util.contextmanagers import controlled_subprocess -from reviewboard.diffviewer.errors import PatchError +from reviewboard.diffviewer.errors import DiffTooBigError, PatchError from reviewboard.scmtools.core import PRE_CREATION, HEAD @@ -1550,3 +1550,35 @@ def get_diff_data_chunks_info(diff): _finalize_result() return results + + +def check_diff_size(diff_file, parent_diff_file=None): + """Check the size of the given diffs against the maximum allowed size. + + If either of the provided diffs are too large, an exception will be raised. + + Args: + diff_file (django.core.files.uploadedfile.UploadedFile): + The diff file. + + parent_diff_file (django.core.files.uploadedfile.UploadedFile, + optional): + The parent diff file, if any. + + Raises: + reviewboard.diffviewer.errors.DiffTooBigError: + The supplied files are too big. + """ + siteconfig = SiteConfiguration.objects.get_current() + max_diff_size = siteconfig.get('diffviewer_max_diff_size') + + if max_diff_size > 0: + if diff_file.size > max_diff_size: + raise DiffTooBigError( + _('The supplied diff file is too large.'), + max_diff_size=max_diff_size) + + if parent_diff_file and parent_diff_file.size > max_diff_size: + raise DiffTooBigError( + _('The supplied parent diff file is too large.'), + max_diff_size=max_diff_size) diff --git a/reviewboard/diffviewer/filediff_creator.py b/reviewboard/diffviewer/filediff_creator.py index 93fe0a29b027569a15c1afe2dd64256c8aa0a8a9..6599536baf9824eb04c1116e90f6fe034b5bfbc7 100644 --- a/reviewboard/diffviewer/filediff_creator.py +++ b/reviewboard/diffviewer/filediff_creator.py @@ -152,9 +152,9 @@ def create_filediffs(diff_file_contents, parent_diff_file_contents, filediff.set_line_counts(raw_insert_count=f.insert_count, raw_delete_count=f.delete_count) - filediffs.append(filediff) + filediffs.append(filediff) - if filediffs: + if not validate_only: FileDiff.objects.bulk_create(filediffs) num_filediffs = len(filediffs) diff --git a/reviewboard/diffviewer/forms.py b/reviewboard/diffviewer/forms.py index 67c27115710a84cfa2dc16cfb81e02c8af9377d1..effb096efd6cb302ac00dbc0df0caf791fc18fcb 100644 --- a/reviewboard/diffviewer/forms.py +++ b/reviewboard/diffviewer/forms.py @@ -2,18 +2,68 @@ from __future__ import unicode_literals +import base64 +import json +from functools import partial + from dateutil.parser import isoparse from django import forms from django.core.exceptions import ValidationError from django.utils.encoding import smart_unicode from django.utils.translation import ugettext, ugettext_lazy as _ +from reviewboard.diffviewer.commit_utils import get_file_exists_in_history +from reviewboard.diffviewer.differ import DiffCompatVersion +from reviewboard.diffviewer.diffutils import check_diff_size +from reviewboard.diffviewer.filediff_creator import create_filediffs from reviewboard.diffviewer.models import DiffCommit, DiffSet from reviewboard.diffviewer.validators import (COMMIT_ID_LENGTH, validate_commit_id) -class UploadCommitForm(forms.Form): +class BaseCommitValidationForm(forms.Form): + """A form mixin for handling validation metadata for commits.""" + + validation_info = forms.CharField( + label=_('Validation metadata'), + help_text=_('Validation metadata generated by the diff commit ' + 'validation resource.'), + widget=forms.HiddenInput, + required=False) + + def clean_validation_info(self): + """Clean the validation_info field. + + This method ensures that if the field is supplied that it parses as + base64-encoded JSON. + + Returns: + dict: + The parsed validation information. + + Raises: + django.core.exceptions.ValidationError: + The value could not be parsed. + """ + validation_info = self.cleaned_data.get('validation_info', '').strip() + + if not validation_info: + return {} + + try: + return json.loads(base64.b64decode(validation_info)) + except (TypeError, ValueError) as e: + raise ValidationError( + ugettext( + 'Could not parse validation info "%(validation_info)s": ' + '%(exc)s' + ) % { + 'exc': e, + 'validation_info': validation_info, + }) + + +class UploadCommitForm(BaseCommitValidationForm): """The form for uploading a diff and creating a DiffCommit.""" diff = forms.FileField( @@ -108,6 +158,7 @@ class UploadCommitForm(forms.Form): return DiffCommit.objects.create_from_upload( request=self.request, + validation_info=self.cleaned_data['validation_info'], diffset=self.diffset, repository=self.diffset.repository, diff_file=self.cleaned_data['diff'], @@ -141,10 +192,20 @@ class UploadCommitForm(forms.Form): raise ValidationError(ugettext( 'Cannot upload commits to a published diff.')) + if (self.diffset.commit_count and + 'validation_info' not in self.cleaned_data and + 'validation_info' not in self.errors): + # If validation_info is present in `errors`, it will not be in + # self.cleaned_data. We do not want to report it missing if it + # failed validation for another reason. + self._errors['validation_info'] = self.error_class([ + self.fields['validation_info'].error_messages['required'], + ]) + return self.cleaned_data def clean_author_date(self): - """Parse the date and time in the ``author_date`` field. + """Parse the date and time in the author_date field. Returns: datetime.datetime: @@ -157,7 +218,7 @@ class UploadCommitForm(forms.Form): 'This date must be in ISO 8601 format.')) def clean_committer_date(self): - """Parse the date and time in the ``committer_date`` field. + """Parse the date and time in the committer_date field. Returns: datetime.datetime: @@ -265,3 +326,160 @@ class UploadDiffForm(forms.Form): basedir=self.cleaned_data.get('basedir', ''), base_commit_id=self.cleaned_data['base_commit_id'], request=self.request) + + +class ValidateCommitForm(BaseCommitValidationForm): + """A form for validating of DiffCommits.""" + + diff = forms.FileField( + label=_('Diff'), + help_text=_('The new diff to upload.')) + + parent_diff = forms.FileField( + label=_('Parent diff'), + help_text=_('An optional diff that the main diff is based on. ' + 'This is usually used for distributed revision control ' + 'systems (Git, Mercurial, etc.).'), + required=False) + + commit_id = forms.CharField( + label=_('Commit ID'), + help_text=_('The ID of this commit.'), + max_length=COMMIT_ID_LENGTH, + validators=[validate_commit_id]) + + parent_id = forms.CharField( + label=_('Parent commit ID'), + help_text=_('The ID of the parent commit.'), + max_length=COMMIT_ID_LENGTH, + validators=[validate_commit_id]) + + base_commit_id = forms.CharField( + label=_('Base commit ID'), + help_text=_('The base commit ID that the commits are based off of.'), + required=False) + + def __init__(self, repository, request=None, *args, **kwargs): + """Initialize the form. + + Args: + repository (reviewboard.scmtools.models.Repository): + The repository against which the diff is being validated. + + request (django.http.HttpRequest, optional): + The HTTP request from the client. + + *args (tuple): + Additional positional arguments to pass to the base + class initializer. + + **kwargs (dict): + Additional keyword arguments to pass to the base class + initializer. + """ + super(ValidateCommitForm, self).__init__(*args, **kwargs) + + self.repository = repository + self.request = request + + def clean(self): + """Clean the form. + + Returns: + dict: + The cleaned form data. + + Raises: + django.core.exceptions.ValidationError: + The form data was not valid. + """ + super(ValidateCommitForm, self).clean() + + validation_info = self.cleaned_data.get('validation_info') + + if validation_info: + errors = [] + + parent_id = self.cleaned_data.get('parent_id') + commit_id = self.cleaned_data.get('commit_id') + + if commit_id and commit_id in validation_info: + errors.append(ugettext('This commit was already validated.')) + elif parent_id and parent_id not in validation_info: + errors.append(ugettext('The parent commit was not validated.')) + + if errors: + self._errors['validation_info'] = self.error_class(errors) + self.cleaned_data.pop('validation_info') + + return self.cleaned_data + + def validate_diff(self): + """Validate the DiffCommit. + + This will attempt to parse the given diff (and optionally parent + diff) into :py:class:`FileDiffs + `. This will not + result in anything being committed to the database. + + Returns: + tuple: + A 2-tuple containing the following: + + * A list of the created FileDiffs. + * A list of the parent FileDiffs, or ``None``. + + Raises: + reviewboard.diffviewer.errors.DiffParserError: + The diff could not be parsed. + + reviewboard.diffviewer.errors.DiffTooBigError: + The diff was too big. + + reviewboard.diffviewer.errors.EmptyDiffError: + The diff did not contain any changes. + + reviewboard.scmtools.errors.FileNotFoundError: + A file was not found in the repository. + + reviewboard.scmtools.errors.SCMError: + An error occurred within the SCMTool. + """ + assert self.is_valid() + + diff_file = self.cleaned_data['diff'] + parent_diff_file = self.cleaned_data.get('parent_diff') + validation_info = self.cleaned_data.get('validation_info') + + check_diff_size(diff_file, parent_diff_file) + + if parent_diff_file: + parent_diff_file_contents = parent_diff_file.read() + else: + parent_diff_file_contents = None + + base_commit_id = self.cleaned_data['base_commit_id'] + + diffset = DiffSet(name='diff', + revision=0, + basedir='', + repository=self.repository, + diffcompat=DiffCompatVersion.DEFAULT, + base_commit_id=base_commit_id) + + get_file_exists = partial(get_file_exists_in_history, + validation_info or {}, + self.repository, + self.cleaned_data['parent_id']) + + return create_filediffs( + diff_file_contents=diff_file.read(), + parent_diff_file_contents=parent_diff_file_contents, + repository=self.repository, + basedir='', + base_commit_id=base_commit_id, + get_file_exists=get_file_exists, + diffset=diffset, + request=self.request, + diffcommit=None, + validate_only=True) diff --git a/reviewboard/diffviewer/managers.py b/reviewboard/diffviewer/managers.py index 56f91d69c9d5a928ee48be2e2d415203d23a2a7a..7c3f53932d797beed8da1771b2ae239363eea4a1 100644 --- a/reviewboard/diffviewer/managers.py +++ b/reviewboard/diffviewer/managers.py @@ -6,17 +6,17 @@ import bz2 import gc import hashlib import warnings +from functools import partial from django.conf import settings from django.db import models, reset_queries, connection from django.db.models import Count, Q from django.db.utils import IntegrityError from django.utils.six.moves import range -from django.utils.translation import ugettext as _ -from djblets.siteconfig.models import SiteConfiguration +from reviewboard.diffviewer.commit_utils import get_file_exists_in_history from reviewboard.diffviewer.differ import DiffCompatVersion -from reviewboard.diffviewer.errors import DiffTooBigError +from reviewboard.diffviewer.diffutils import check_diff_size from reviewboard.diffviewer.filediff_creator import create_filediffs @@ -485,19 +485,7 @@ class BaseDiffManager(models.Manager): DeprecationWarning) validate_only = not kwargs.pop('save') - siteconfig = SiteConfiguration.objects.get_current() - max_diff_size = siteconfig.get('diffviewer_max_diff_size') - - if max_diff_size > 0: - if diff_file.size > max_diff_size: - raise DiffTooBigError( - _('The supplied diff file is too large.'), - max_diff_size=max_diff_size) - - if parent_diff_file and parent_diff_file.size > max_diff_size: - raise DiffTooBigError( - _('The supplied parent diff file is too large.'), - max_diff_size=max_diff_size) + check_diff_size(diff_file, parent_diff_file) if parent_diff_file: parent_diff_file_name = parent_diff_file.name @@ -537,6 +525,7 @@ class DiffCommitManager(BaseDiffManager): author_name, author_email, author_date, + validation_info=None, request=None, committer_name=None, committer_email=None, @@ -586,6 +575,11 @@ class DiffCommitManager(BaseDiffManager): request (django.http.HttpRequest, optional): The HTTP request from the client. + validation_info (dict, optional): + A dictionary of parsed validation information from the + :py:class:`~reviewboard.webapi.resources.validate_diffcommit. + ValidateDiffCommitResource`. + committer_name (unicode, optional): The committer's name. @@ -630,8 +624,13 @@ class DiffCommitManager(BaseDiffManager): if not validate_only: diffcommit.save() + get_file_exists = partial(get_file_exists_in_history, + validation_info or {}, + repository, + parent_id) + create_filediffs( - get_file_exists=repository.get_file_exists, + get_file_exists=get_file_exists, diff_file_contents=diff_file_contents, parent_diff_file_contents=parent_diff_file_contents, repository=repository, diff --git a/reviewboard/diffviewer/tests/test_commit_utils.py b/reviewboard/diffviewer/tests/test_commit_utils.py new file mode 100644 index 0000000000000000000000000000000000000000..1dc64ddf0ac7f4c7650b71db926a11f397827569 --- /dev/null +++ b/reviewboard/diffviewer/tests/test_commit_utils.py @@ -0,0 +1,384 @@ +"""Unit tests for reviewboard.diffviewer.commit_utils.""" + +from __future__ import unicode_literals + +from kgb import SpyAgency + +from reviewboard.diffviewer.commit_utils import get_file_exists_in_history +from reviewboard.scmtools.core import UNKNOWN +from reviewboard.testing.testcase import TestCase + + +class GetFileExistsInHistoryTests(SpyAgency, TestCase): + """Unit tests for get_file_exists_in_history.""" + + fixtures = ['test_scmtools'] + + def test_added_in_parent_with_revision(self): + """Testing get_file_exists_in_history for a file added in the parent + commit for an SCM that uses file revisions + """ + repository = self.create_repository() + validation_info = { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': 'a' * 40, + }], + 'modified': [], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision='a' * 40)) + + def test_added_in_parent_without_revision(self): + """Testing get_file_exists_in_history for a file added in the parent + commit for an SCM that doesn't use file revisions + """ + repository = self.create_repository(tool_name='Mercurial') + validation_info = { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + 'modified': [], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision=UNKNOWN)) + + def test_added_in_grandparent_with_revision(self): + """Testing get_file_exists_in_history for a file added in a + grandparent commit for an SCM that uses file revisions + """ + repository = self.create_repository() + validation_info = { + 'r2': { + 'parent_id': 'r1', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [], + }, + }, + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': 'a' * 40, + }], + 'modified': [], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r2', + path='foo', + revision='a' * 40)) + + def test_added_in_grandparent_without_revision(self): + """Testing get_file_exists_in_history for a file added in a + grandparent commit for an SCM that doesn't use file revisions + """ + repository = self.create_repository(tool_name='Mercurial') + validation_info = { + 'r2': { + 'parent_id': 'r1', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [], + }, + }, + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + 'modified': [], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r2', + path='foo', + revision=UNKNOWN)) + + def test_removed_in_parent_without_revision(self): + """Tesing get_file_exists_in_history for a file removed in a parent + revision for an SCM that uses file revisions + """ + repository = self.create_repository() + target_path = 'foo' + target_revision = 'a' * 40 + + self.spy_on( + repository.get_file_exists, + call_fake=self._make_get_file_exists_in_history(target_path, + target_revision)) + + validation_info = { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [{ + 'filename': target_path, + 'revision': target_revision, + }], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path=target_path, + revision=target_revision)) + + self.assertTrue(repository.get_file_exists.spy.called_with( + target_path, target_revision)) + + def test_removed_in_parent_unknown_revision(self): + """Testing get_file_exists_in_history for a file removed in a parent + commit for an SCM that does not use file revisions + """ + repository = self.create_repository(tool_name='Mercurial') + + validation_info = { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + }, + }, + } + + self.assertFalse(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision=UNKNOWN)) + + def test_removed_readded_in_parent_unknown_revision(self): + """Testing get_file_exists_in_history for a file removed and re-added + in parent commits for an SCM that does not use file revisions + """ + repository = self.create_repository(tool_name='Mercurial') + + validation_info = { + 'r2': { + 'parent_id': 'r1', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + 'modified': [], + 'removed': [], + }, + }, + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + }, + }, + } + + self.assertFalse(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision=UNKNOWN)) + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r2', + path='foo', + revision=UNKNOWN)) + + def test_modified_in_parent_with_revision(self): + """Testing get_file_exists_in_history for a file modified in a + parent revision for an SCM that uses file revisions + """ + repository = self.create_repository() + self.spy_on( + repository.get_file_exists, + call_fake=self._make_get_file_exists_in_history('foo', 'a' * 40)) + + validation_info = { + 'r2': { + 'parent_id': 'r1', + 'tree': { + 'added': [], + 'modified': [{ + 'filename': 'foo', + 'revision': 'c' * 40, + }], + 'removed': [], + }, + }, + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [{ + 'filename': 'foo', + 'revision': 'b' * 40, + }], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r2', + path='foo', + revision='c' * 40)) + self.assertFalse(repository.get_file_exists.spy.called) + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision='b' * 40)) + self.assertFalse(repository.get_file_exists.spy.called) + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r0', + path='foo', + revision='a' * 40)) + self.assertTrue(repository.get_file_exists.spy.called_with( + 'foo', 'a' * 40, + )) + + def test_modified_in_parent_unknown_revision(self): + """Testing get_file_exists_in_history for a file modified in a + parent revision for an SCM that does not use file revision + """ + repository = self.create_repository(tool_name='Mercurial') + self.spy_on( + repository.get_file_exists, + call_fake=self._make_get_file_exists_in_history('foo', UNKNOWN)) + + validation_info = { + 'r2': { + 'parent_id': 'r1', + 'tree': { + 'added': [], + 'modified': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + 'removed': [], + }, + }, + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [{ + 'filename': 'foo', + 'revision': UNKNOWN, + }], + 'removed': [], + }, + }, + } + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r2', + path='foo', + revision=UNKNOWN)) + self.assertFalse(repository.get_file_exists.spy.called) + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r1', + path='foo', + revision=UNKNOWN)) + self.assertFalse(repository.get_file_exists.spy.called) + + self.assertTrue(get_file_exists_in_history( + validation_info=validation_info, + repository=repository, + parent_id='r0', + path='foo', + revision=UNKNOWN)) + self.assertTrue(repository.get_file_exists.spy.called_with( + 'foo', UNKNOWN)) + + def _make_get_file_exists_in_history(self, target_path, target_revision): + """Return a fake get_file_exists_in_history method for a repository. + + Args: + target_path (unicode): + The path that should report as existing in the repository. + + target_revision (unicode): + The revision of the file. + + Returns: + callable: + A function that only returns True when called with the given + ``target_path`` and ``target_revision``. + """ + def get_file_exists_in_history(repository, path, revision, *args, + **kwargs): + return path == target_path and revision == target_revision + + return get_file_exists_in_history diff --git a/reviewboard/diffviewer/tests/test_diffcommit_manager.py b/reviewboard/diffviewer/tests/test_diffcommit_manager.py index cb1bca194f204bc3782c2b3d2b537a4ba17af71a..941ccf52112f4839068a7994155cc5c0c944f861 100644 --- a/reviewboard/diffviewer/tests/test_diffcommit_manager.py +++ b/reviewboard/diffviewer/tests/test_diffcommit_manager.py @@ -53,7 +53,8 @@ class DiffCommitManagerTests(SpyAgency, TestCase): committer_email='committer@example.com', committer_date=parsed_date, commit_message='Description', - diffset=diffset) + diffset=diffset, + validation_info={}) self.assertEqual(commit.files.count(), 1) self.assertEqual(diffset.files.count(), commit.files.count()) diff --git a/reviewboard/diffviewer/tests/test_forms.py b/reviewboard/diffviewer/tests/test_forms.py index 99b376d2c1d77ec08637a2fa7d75a73399d56cdd..bc904035ee6b4c190862712436770637bef97a4b 100644 --- a/reviewboard/diffviewer/tests/test_forms.py +++ b/reviewboard/diffviewer/tests/test_forms.py @@ -1,15 +1,24 @@ from __future__ import unicode_literals +import base64 +import json + import nose from django.core.files.uploadedfile import SimpleUploadedFile +from django.test.client import RequestFactory +from djblets.siteconfig.models import SiteConfiguration from kgb import SpyAgency from reviewboard.admin.import_utils import has_module from reviewboard.diffviewer.diffutils import (get_original_file, get_patched_file, patch) -from reviewboard.diffviewer.forms import UploadCommitForm, UploadDiffForm +from reviewboard.diffviewer.errors import (DiffParserError, DiffTooBigError, + EmptyDiffError) +from reviewboard.diffviewer.forms import (UploadCommitForm, UploadDiffForm, + ValidateCommitForm) from reviewboard.diffviewer.models import DiffSet, DiffSetHistory +from reviewboard.scmtools.errors import FileNotFoundError from reviewboard.scmtools.models import Repository, Tool from reviewboard.testing import TestCase @@ -502,3 +511,368 @@ class UploadDiffFormTests(SpyAgency, TestCase): self.assertIn('basedir', form.errors) self.assertIn('This field is required.', form.errors['basedir']) + + +class ValidateCommitFormTests(SpyAgency, TestCase): + """Unit tests for ValidateCommitForm.""" + + fixtures = ['test_scmtools'] + + _PARENT_DIFF_DATA = ( + b'diff --git a/README b/README\n' + b'new file mode 100644\n' + b'index 0000000..94bdd3e\n' + b'--- /dev/null\n' + b'+++ b/README\n' + b'@@ -0,0 +2 @@\n' + b'+blah blah\n' + b'+blah blah\n' + ) + + @classmethod + def setUpClass(cls): + super(ValidateCommitFormTests, cls).setUpClass() + + cls.request_factory = RequestFactory() + + def setUp(self): + super(ValidateCommitFormTests, self).setUp() + + self.repository = self.create_repository(tool_name='Git') + self.request = self.request_factory.get('/') + self.diff = SimpleUploadedFile('diff', self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch') + + def test_clean_already_validated(self): + """Testing ValidateCommitForm.clean for a commit that has already been + validated + """ + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'removed': [], + 'modified': [], + }, + }, + })) + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r1', + 'parent_id': 'r0', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + }) + + self.assertFalse(form.is_valid()) + self.assertEqual(form.errors, { + 'validation_info': ['This commit was already validated.'], + }) + + def test_clean_parent_not_validated(self): + """Testing ValidateCommitForm.clean for a commit whose parent has not + been validated + """ + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'removed': [], + 'modified': [], + }, + }, + })) + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r3', + 'parent_id': 'r2', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + }) + + self.assertFalse(form.is_valid()) + self.assertEqual(form.errors, { + 'validation_info': ['The parent commit was not validated.'], + }) + + def test_clean_parent_diff_subsequent_commit(self): + """Testing ValidateCommitForm.clean with a non-empty parent diff for + a subsequent commit + """ + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'removed': [], + 'modified': [], + }, + }, + })) + + parent_diff = SimpleUploadedFile('diff', + self._PARENT_DIFF_DATA, + content_type='text/x-patch') + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + 'parent_diff': parent_diff, + }) + + self.assertTrue(form.is_valid()) + + def test_clean_validation_info(self): + """Testing ValidateCommitForm.clean_validation_info""" + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'removed': [], + 'modified': [], + }, + }, + })) + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + }) + + self.assertTrue(form.is_valid()) + + def test_clean_validation_info_invalid_base64(self): + """Testing ValidateCommitForm.clean_validation_info with + non-base64-encoded data""" + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': 'This is not base64!', + }, + files={ + 'diff': self.diff, + }) + + self.assertFalse(form.is_valid()) + self.assertEqual(form.errors, { + 'validation_info': [ + 'Could not parse validation info "This is not base64!": ' + 'Incorrect padding', + ], + }) + + def test_clean_validation_info_invalid_json(self): + """Testing ValidateCommitForm.clean_validation_info with base64-encoded + non-json data + """ + validation_info = base64.b64encode('Not valid json.') + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + }) + + self.assertFalse(form.is_valid()) + self.assertEqual(form.errors, { + 'validation_info': [ + 'Could not parse validation info "%s": No JSON object could ' + 'be decoded' + % validation_info, + ], + }) + + def test_validate_diff(self): + """Testing ValidateCommitForm.validate_diff""" + self.spy_on(self.repository.get_file_exists, + call_fake=lambda *args, **kwargs: True) + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r1', + 'parent_id': 'r2', + }, + files={ + 'diff': self.diff, + }) + + self.assertTrue(form.is_valid()) + form.validate_diff() + + def test_validate_diff_subsequent_commit(self): + """Testing ValidateCommitForm.validate_diff for a subsequent commit""" + diff_content = ( + b'diff --git a/foo b/foo\n' + b'index %s..%s 1000644\n' + b'--- a/foo\n' + b'+++ b/foo\n' + b'@@ -0,0 +1,2 @@\n' + b'+This is not a new file.\n' + % (b'a' * 40, b'b' * 40) + ) + diff = SimpleUploadedFile('diff', diff_content, + content_type='text/x-patch') + + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'foo', + 'revision': 'a' * 40, + }], + 'removed': [], + 'modified': [], + }, + }, + })) + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': validation_info, + }, + files={ + 'diff': diff, + }) + + self.assertTrue(form.is_valid()) + form.validate_diff() + + def test_validate_diff_missing_files(self): + """Testing ValidateCommitForm.validate_diff for a subsequent commit + with missing files + """ + validation_info = base64.b64encode(json.dumps({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'removed': [], + 'modified': [], + }, + }, + })) + + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'validation_info': validation_info, + }, + files={ + 'diff': self.diff, + }) + + self.assertTrue(form.is_valid()) + + with self.assertRaises(FileNotFoundError): + form.validate_diff() + + def test_validate_diff_empty(self): + """Testing ValidateCommitForm.validate_diff for an empty diff""" + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r1', + 'parent_id': 'r0', + }, + files={ + 'diff': SimpleUploadedFile('diff', b' ', + content_type='text/x-patch'), + }) + + self.assertTrue(form.is_valid()) + + with self.assertRaises(EmptyDiffError): + form.validate_diff() + + def test_validate_diff_too_big(self): + """Testing ValidateCommitForm.validate_diff for a diff that is too + large + """ + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r1', + 'parent_id': 'r0', + }, + files={ + 'diff': self.diff, + }) + + self.assertTrue(form.is_valid()) + + siteconfig = SiteConfiguration.objects.get_current() + max_diff_size = siteconfig.get('diffviewer_max_diff_size') + siteconfig.set('diffviewer_max_diff_size', 1) + siteconfig.save() + + with self.assertRaises(DiffTooBigError): + try: + form.validate_diff() + finally: + siteconfig.set('diffviewer_max_diff_size', max_diff_size) + siteconfig.save() + + def test_validate_diff_parser_error(self): + """Testing ValidateCommitForm.validate_diff for an invalid diff""" + form = ValidateCommitForm( + repository=self.repository, + request=self.request, + data={ + 'commit_id': 'r1', + 'parent_id': 'r0', + }, + files={ + 'diff': SimpleUploadedFile('diff', b'asdf', + content_type='text/x-patch'), + }) + + self.assertTrue(form.is_valid()) + + with self.assertRaises(DiffParserError): + form.validate_diff() diff --git a/reviewboard/webapi/resources/draft_diffcommit.py b/reviewboard/webapi/resources/draft_diffcommit.py index 2e7421bb03035df601431a458cf0dfc3e5f50021..205fb11ae56c9beeff35102dfb99db2687f35db6 100644 --- a/reviewboard/webapi/resources/draft_diffcommit.py +++ b/reviewboard/webapi/resources/draft_diffcommit.py @@ -229,6 +229,15 @@ class DraftDiffCommitResource(DiffCommitResource): 'type': FileFieldType, 'description': 'The optional parent diff to upload.', }, + 'validation_info': { + 'type': StringFieldType, + 'description': ( + 'Validation metadata from the :ref:`DiffCommit validation ' + 'resource `.' + '\n\n' + 'This is required for all but the first commit.' + ), + }, }, allow_unknown=True ) diff --git a/reviewboard/webapi/resources/validate_diffcommit.py b/reviewboard/webapi/resources/validate_diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..c5286fa4f88fafdab69e924f8c9f458a8510d686 --- /dev/null +++ b/reviewboard/webapi/resources/validate_diffcommit.py @@ -0,0 +1,315 @@ +"""The DiffCommit validation resource.""" + +from __future__ import unicode_literals + +import base64 +import json +import logging + +from django.db.models import Q +from django.utils import six +from djblets.webapi.decorators import (webapi_login_required, + webapi_request_fields, + webapi_response_errors) +from djblets.webapi.errors import (DOES_NOT_EXIST, INVALID_ATTRIBUTE, + INVALID_FORM_DATA, NOT_LOGGED_IN, + PERMISSION_DENIED) +from djblets.webapi.fields import FileFieldType, StringFieldType + +from reviewboard.diffviewer.errors import (DiffParserError, + DiffTooBigError, + EmptyDiffError) +from reviewboard.diffviewer.features import dvcs_feature +from reviewboard.diffviewer.forms import ValidateCommitForm +from reviewboard.diffviewer.models import FileDiff +from reviewboard.scmtools.core import PRE_CREATION +from reviewboard.scmtools.errors import FileNotFoundError, SCMError +from reviewboard.scmtools.git import ShortSHA1Error +from reviewboard.scmtools.models import Repository +from reviewboard.webapi.base import WebAPIResource +from reviewboard.webapi.decorators import (webapi_check_local_site, + webapi_check_login_required) +from reviewboard.webapi.errors import (DIFF_EMPTY, DIFF_PARSE_ERROR, + DIFF_TOO_BIG, INVALID_REPOSITORY, + REPO_FILE_NOT_FOUND) + + +logger = logging.getLogger(__name__) + + +class ValidateDiffCommitResource(WebAPIResource): + """Verifies whether or not a diff file for a commit will work. + + This allows clients to validate whether or not diff files for commits can + be parsed and displayed without actually creating a review request first. + """ + + added_in = '4.0' + + singleton = True + name = 'commit_validation' + uri_name = 'commits' + uri_object_key = None + model = None + + allowed_methods = ('GET', 'POST') + + required_features = [dvcs_feature] + + item_child_resources = [] + list_child_resources = [] + + fields = { + 'validation_info': { + 'type': StringFieldType, + 'description': ( + 'Validation metdata to pass to this resource to help validate ' + 'the next commit.' + ), + }, + } + + def serialize_validation_info(self, validation_info): + """Serialize the validation_info field. + + Returns: + unicode: + The ``validation_info`` field serialized as base64-encoded JSON. + """ + return base64.b64encode(json.dumps(validation_info)) + + @webapi_check_local_site + @webapi_check_login_required + def get(self, request, *args, **kwargs): + """Return links for using this resource.""" + return 200, { + 'links': self.get_links(request=request, *args, **kwargs), + } + + @webapi_login_required + @webapi_check_local_site + @webapi_response_errors( + DIFF_EMPTY, + DIFF_PARSE_ERROR, + DIFF_TOO_BIG, + DOES_NOT_EXIST, + INVALID_ATTRIBUTE, + INVALID_FORM_DATA, + INVALID_REPOSITORY, + NOT_LOGGED_IN, + REPO_FILE_NOT_FOUND, + PERMISSION_DENIED + ) + @webapi_request_fields( + required={ + 'repository': { + 'type': StringFieldType, + 'description': 'The path or ID of the repository.', + }, + 'diff': { + 'type': FileFieldType, + 'description': 'The diff file to validate.', + }, + 'commit_id': { + 'type': StringFieldType, + 'description': 'The ID of the commit being validated.', + }, + 'parent_id': { + 'type': StringFieldType, + 'description': 'The ID of the parent commit.', + }, + }, + optional={ + 'base_commit_id': { + 'type': StringFieldType, + 'description': 'The base commit ID.', + }, + 'parent_diff': { + 'type': FileFieldType, + 'description': ( + 'The parent diff of the commit being validated.\n' + ), + }, + 'validation_info': { + 'type': StringFieldType, + 'description': ( + 'Validation metadata from a previous call to this API.\n' + '\n' + 'This field is required for all but the first commit in a ' + 'series.' + ), + }, + } + ) + def create(self, request, repository, commit_id, parent_id, + base_commit_id=None, local_site_name=None, *args, **kwargs): + """Validate a diff for a commit. + + This API has a similar signature to the :ref:`Draft DiffCommit resource + ` POST API, but instead of + actually creating commits, it will return a result representing whether + or not the included diff file parsed and validated correctly. + + This API must be called before posting to the :ref:`Draft DiffCommit + resource ` because the + ``validation_info`` field returned by this resource is required for + posting to that resource. + """ + local_site = self._get_local_site(local_site_name) + + try: + q = Q(pk=int(repository)) + except ValueError: + q = (Q(path=repository) | + Q(mirror_path=repository) | + Q(name=repository)) + + repository_qs = ( + Repository.objects + .accessible(request.user, local_site=local_site) + .filter(q) + ) + repository_count = len(repository_qs) + + if repository_count == 0: + return INVALID_REPOSITORY, { + 'repository': repository, + } + elif repository_count > 1: + msg = ( + 'Too many repositories matched "%s". Try specifying the ' + 'repository by name instead.' + % repository + ) + + return INVALID_REPOSITORY.with_message(msg), { + 'repository': repository, + } + + repository = repository_qs.first() + + if not repository.scmtool_class.supports_history: + return INVALID_ATTRIBUTE, { + 'reason': ( + 'The "%s" repository does not support review requests ' + 'created with history.' + % repository.name + ), + } + + form = ValidateCommitForm(repository=repository, + request=request, + data=request.POST, + files=request.FILES) + + if not form.is_valid(): + return INVALID_FORM_DATA, { + 'fields': self._get_form_errors(form), + } + + try: + filediffs = form.validate_diff() + except FileNotFoundError as e: + return REPO_FILE_NOT_FOUND, { + 'file': e.path, + 'revision': six.text_type(e.revision), + } + except EmptyDiffError: + return DIFF_EMPTY + except DiffTooBigError as e: + return DIFF_TOO_BIG, { + 'reason': six.text_type(e), + 'max_size': e.max_diff_size, + } + except DiffParserError as e: + return DIFF_PARSE_ERROR, { + 'reason': six.text_type(e), + 'linenum': e.linenum, + } + except ShortSHA1Error as e: + return REPO_FILE_NOT_FOUND, { + 'reason': six.text_type(e), + 'file': e.path, + 'revision': six.text_type(e.revision), + } + except SCMError as e: + return DIFF_PARSE_ERROR.with_message(six.text_type(e)) + except Exception as e: + logger.exception( + 'Unexpected exception occurred while validating commit "%s" ' + 'in repository "%s" (id %d) with base_commit_id="%s"', + commit_id, + repository.name, + repository.pk, + base_commit_id, + request=request) + return DIFF_PARSE_ERROR.with_message( + 'Unexpected error while validating the diff: %s' % e) + + validation_info = form.cleaned_data.get('validation_info', {}) + validation_info[commit_id] = { + 'parent_id': parent_id, + 'tree': self._generate_validation_info_tree(filediffs), + } + + return 200, { + self.item_result_key: { + 'validation_info': self.serialize_validation_info( + validation_info), + } + } + + def _generate_validation_info_tree(self, parsed_filediffs): + """Generate tree information for the parsed filediffs. + + Args: + parsed_filediffs (list of reviewboard.diffviewer.models.filediff. + FileDiff): + The parsed filediffs from :py:func:`~reviewboard.diffviewer. + filediff_creator.create_filediffs`. + + Returns: + dict: + A dictionary containing the following keys: + + ``added`` (:py:class:`list` of :py:class:`dict`): + The names of added files and their revisions. + + ``modified`` (:py:class:`list` of :py:class:`dict`): + The names of modified files and their new revisions. + + ``removed`` (:py:class:`list` of :py:class:`dict`): + The names of removed files and their source revisions. + """ + added = [] + removed = [] + modified = [] + + for f in parsed_filediffs: + if f.status in (FileDiff.DELETED, FileDiff.MOVED): + removed.append({ + 'filename': f.source_file, + 'revision': f.source_revision, + }) + + if (f.status in (FileDiff.COPIED, FileDiff.MOVED) or + (f.status == FileDiff.MODIFIED and + f.source_revision == PRE_CREATION)): + added.append({ + 'filename': f.dest_file, + 'revision': f.dest_detail, + }) + elif f.status == FileDiff.MODIFIED: + modified.append({ + 'filename': f.dest_file, + 'revision': f.dest_detail, + }) + + return { + 'added': added, + 'removed': removed, + 'modified': modified, + } + + +validate_diffcommit_resource = ValidateDiffCommitResource() diff --git a/reviewboard/webapi/resources/validation.py b/reviewboard/webapi/resources/validation.py index 91c878e32d47d993c01c97666519a4a64859b8f6..10ea9ac3d957b949a3628f2d96c5ee7bb26f8c1c 100644 --- a/reviewboard/webapi/resources/validation.py +++ b/reviewboard/webapi/resources/validation.py @@ -18,6 +18,7 @@ class ValidationResource(RBResourceMixin, DjbletsRootResource): def __init__(self, *args, **kwargs): super(ValidationResource, self).__init__([ resources.validate_diff, + resources.validate_diffcommit, ], include_uri_templates=False, *args, **kwargs) @webapi_check_login_required diff --git a/reviewboard/webapi/tests/mimetypes.py b/reviewboard/webapi/tests/mimetypes.py index 34a0779f257fd7903bc3bab9a2601e37e012492b..51b40c7bbcb0cb988af886fe62e2145ea13e4e86 100644 --- a/reviewboard/webapi/tests/mimetypes.py +++ b/reviewboard/webapi/tests/mimetypes.py @@ -200,6 +200,9 @@ user_file_attachment_item_mimetype = _build_mimetype('user-file-attachment') validate_diff_mimetype = _build_mimetype('diff-validation') +validate_diffcommit_mimetype = _build_mimetype('commit-validation') + + watched_review_group_list_mimetype = _build_mimetype('watched-review-groups') watched_review_group_item_mimetype = _build_mimetype('watched-review-group') diff --git a/reviewboard/webapi/tests/mixins.py b/reviewboard/webapi/tests/mixins.py index 75e326239b6c4c7cd5c4d5ff69b6bfb8fa974557..4970e6f0940e520cbfb02991487e3a797b42ff90 100644 --- a/reviewboard/webapi/tests/mixins.py +++ b/reviewboard/webapi/tests/mixins.py @@ -986,6 +986,7 @@ class BasicPostTestsMixin(BasicTestsMixin): """ basic_post_fixtures = [] basic_post_use_admin = False + basic_post_success_status = 201 def setup_basic_post_test(self, user, with_local_site, local_site_name, post_valid_data): @@ -1007,7 +1008,8 @@ class BasicPostTestsMixin(BasicTestsMixin): self.assertFalse(url.startswith('/s/' + self.local_site_name)) with override_feature_checks(self.override_features): - rsp = self.api_post(url, post_data, expected_mimetype=mimetype) + rsp = self.api_post(url, post_data, expected_mimetype=mimetype, + expected_status=self.basic_post_success_status) self._close_file_handles(post_data) self.assertEqual(rsp['stat'], 'ok') @@ -1028,7 +1030,8 @@ class BasicPostTestsWithLocalSiteMixin(BasicPostTestsMixin): self._setup_test_post_with_site() with override_feature_checks(self.override_features): - rsp = self.api_post(url, post_data, expected_mimetype=mimetype) + rsp = self.api_post(url, post_data, expected_mimetype=mimetype, + expected_status=self.basic_post_success_status) self._close_file_handles(post_data) self.assertEqual(rsp['stat'], 'ok') @@ -1090,7 +1093,8 @@ class BasicPostTestsWithLocalSiteAndAPITokenMixin(object): webapi_token_local_site_id=self.local_site_id) with override_feature_checks(self.override_features): - rsp = self.api_post(url, post_data, expected_mimetype=mimetype) + rsp = self.api_post(url, post_data, expected_mimetype=mimetype, + expected_status=self.basic_post_success_status) self._close_file_handles(post_data) self.assertEqual(rsp['stat'], 'ok') @@ -1131,7 +1135,8 @@ class BasicPostTestsWithLocalSiteAndOAuthTokenMixin(object): ) with override_feature_checks(self.override_features): - rsp = self.api_post(url, post_data, expected_mimetype=mimetype) + rsp = self.api_post(url, post_data, expected_mimetype=mimetype, + expected_status=self.basic_post_success_status) self._close_file_handles(post_data) self.assertEqual(rsp['stat'], 'ok') diff --git a/reviewboard/webapi/tests/test_draft_diffcommit.py b/reviewboard/webapi/tests/test_draft_diffcommit.py index d90f34420c134f97141d99441d9170755976ea17..1444b69d1c6c07303cbd6d62c0c062a0ae438159 100644 --- a/reviewboard/webapi/tests/test_draft_diffcommit.py +++ b/reviewboard/webapi/tests/test_draft_diffcommit.py @@ -364,6 +364,65 @@ class ResourceListTests(BaseWebAPITestCase): self.assertEqual(err_fields['committer_date'], ['This date must be in ISO 8601 format.']) + @webapi_test_template + def test_post_subsequent(self): + """Testing the POST API with a subsequent commit""" + repository = self.create_repository(tool_name='Git') + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request, draft=True) + + commit = self.create_diffcommit( + repository, + diffset, + diff_contents=self._DEFAULT_DIFF_CONTENTS) + + validation_info = \ + resources.validate_diffcommit.serialize_validation_info({ + commit.commit_id: { + 'parent_id': commit.parent_id, + 'tree': { + 'added': [], + 'modified': [{ + 'filename': 'readme', + 'revision': '5b50866', + }], + 'removed': [], + }, + }, + }) + + diff = SimpleUploadedFile( + 'diff', + (b'diff --git a/readme b/readme\n' + b'index 5b50866..f00f00f 100644\n' + b'--- a/readme\n' + b'+++ a/readme\n' + b'@@ -1 +1,4 @@\n' + b' Hello there\n' + b' \n' + b' Oh hi!\n' + b'+Goodbye!\n'), + content_type='text/x-patch') + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'commit_id': 'r2', + 'parent_id': 'r1', + 'diff': diff, + 'validation_info': validation_info, + }), + expected_mimetype=draft_diffcommit_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('draft_commit', rsp) + + item_rsp = rsp['draft_commit'] + self.compare_item(item_rsp, DiffCommit.objects.get(pk=item_rsp['id'])) + @six.add_metaclass(BasicTestsMetaclass) class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): diff --git a/reviewboard/webapi/tests/test_validate_diffcommit.py b/reviewboard/webapi/tests/test_validate_diffcommit.py new file mode 100644 index 0000000000000000000000000000000000000000..3648ba3c60580d48377fbe5ea9be6d5cfcff103b --- /dev/null +++ b/reviewboard/webapi/tests/test_validate_diffcommit.py @@ -0,0 +1,384 @@ +"""Unit tests for reviewboard.webapi.resources.validate_diffcommit.""" + +from __future__ import unicode_literals + +import base64 +import json + +from django.core.files.uploadedfile import SimpleUploadedFile +from django.utils import six +from djblets.siteconfig.models import SiteConfiguration +from djblets.webapi.errors import INVALID_ATTRIBUTE +from djblets.webapi.testing.decorators import webapi_test_template +from kgb import SpyAgency + +from reviewboard.scmtools.models import Repository +from reviewboard.webapi.errors import (DIFF_EMPTY, DIFF_PARSE_ERROR, + DIFF_TOO_BIG, INVALID_REPOSITORY) +from reviewboard.webapi.resources import resources +from reviewboard.webapi.tests.base import BaseWebAPITestCase +from reviewboard.webapi.tests.mimetypes import validate_diffcommit_mimetype +from reviewboard.webapi.tests.mixins import BasicTestsMetaclass +from reviewboard.webapi.tests.urls import get_validate_diffcommit_url + + +@six.add_metaclass(BasicTestsMetaclass) +class ResourceTests(SpyAgency, BaseWebAPITestCase): + """Testing ValidateDiffCommitResource API.""" + + resource = resources.validate_diffcommit + sample_api_url = 'validation/validate-commit/' + + fixtures = ['test_scmtools', 'test_users'] + + # The basic GET request *does* return JSON, but it is a singleton resource + # whose item_result_key is only present in successful POST responses. + basic_get_returns_json = False + basic_post_success_status = 200 + + def compare_item(self, item_rsp, item): + """Compare a response to the item. + + This is intentionally a no-op because there will be no created model to + compare against. + + Args: + item_rsp (dict): + The serialized response. + + item (object): + The item to compare to. This is always ``None``. + """ + pass + + def setup_http_not_allowed_item_test(self, user): + return get_validate_diffcommit_url() + + # + # HTTP GET tests + # + + def setup_basic_get_test(self, user, with_local_site, local_site_name): + return (get_validate_diffcommit_url(local_site_name=local_site_name), + validate_diffcommit_mimetype, + None) + + # + # HTTP POST tests + # + + def setup_basic_post_test(self, user, with_local_site, local_site_name, + post_valid_data): + repository = self.create_repository(tool_name='Git', + with_local_site=with_local_site) + + post_data = {} + + if post_valid_data: + self.spy_on(Repository.get_file_exists, + call_fake=lambda *args, **kwargs: True) + + validation_info = self.resource.serialize_validation_info({ + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [{ + 'filename': 'README', + 'revision': '94bdd3e', + }], + 'modified': [], + 'removed': [], + }, + }, + }) + diff = SimpleUploadedFile('diff', self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch') + post_data = { + 'commit_id': 'r2', + 'parent_id': 'r1', + 'diff': diff, + 'validation_info': validation_info, + 'repository': repository.pk, + } + + return (get_validate_diffcommit_url(local_site_name=local_site_name), + validate_diffcommit_mimetype, + post_data, + []) + + def check_post_result(self, user, rsp): + self.assertIn('commit_validation', rsp) + self.assertIn('validation_info', rsp['commit_validation']) + + @webapi_test_template + def test_post_diff_too_big(self): + """Testing the POST API with a diff that is too big""" + repo = self.create_repository(tool_name='Git') + + siteconfig = SiteConfiguration.objects.get_current() + max_diff_size = siteconfig.get('diffviewer_max_diff_size') + siteconfig.set('diffviewer_max_diff_size', 1) + siteconfig.save() + + try: + rsp = self.api_post( + get_validate_diffcommit_url(), + { + + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + finally: + siteconfig.set('diffviewer_max_diff_size', max_diff_size) + siteconfig.save() + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], DIFF_TOO_BIG.code) + self.assertEqual(rsp['max_size'], 1) + + @webapi_test_template + def test_post_diff_empty(self): + """Testing the POST API with an empty diff""" + repo = self.create_repository(tool_name='Git') + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + b' ', + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], DIFF_EMPTY.code) + + @webapi_test_template + def test_post_diff_parser_error(self): + """Testing the POST API with a diff that does not parse""" + repo = self.create_repository(tool_name='Git') + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + b'not a valid diff at all.', + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], DIFF_PARSE_ERROR.code) + self.assertEqual(rsp['linenum'], 0) + + @webapi_test_template + def test_post_repo_no_history_support(self): + """Testing the POST API with a repository that does not support + history + """ + repo = self.create_repository(tool_name='Test') + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_ATTRIBUTE.code) + self.assertEqual( + rsp['reason'], + 'The "%s" repository does not support review requests created ' + 'with history.' + % repo.name) + + @webapi_test_template + def test_post_repo_does_not_exist(self): + """Testing the POST API with a repository that does not exist""" + rsp = self.api_post( + get_validate_diffcommit_url(), + { + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch'), + 'repository': 'nope', + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_REPOSITORY.code) + + @webapi_test_template + def test_post_repo_no_access(self): + """Testing the POST API with a repository the user does not have + access to + """ + repo = self.create_repository(public=False) + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_REPOSITORY.code) + + @webapi_test_template + def test_post_repo_multiple(self): + """Testing the POST API with multiple matching repositories""" + repo = self.create_repository(name='repo') + self.create_repository(name=repo.name) + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': SimpleUploadedFile('diff', + self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch'), + 'repository': repo.name, + }, + expected_status=400) + + self.assertEqual(rsp['stat'], 'fail') + self.assertEqual(rsp['err']['code'], INVALID_REPOSITORY.code) + self.assertEqual(rsp['err']['msg'], + 'Too many repositories matched "%s". Try specifying ' + 'the repository by name instead.' + % repo.name) + + @webapi_test_template + def test_post_parent_diff(self): + """Testing the POST API with a parent diff""" + def _exists(repository, filename, revision, *args, **kwargs): + return filename == 'README' and revision == '94bdd3e' + + repo = self.create_repository(name='repo') + self.spy_on(Repository.get_file_exists, call_fake=_exists) + + parent_diff_contents = ( + b'diff --git a/README b/README\n' + b'index 94bdd3e..f00f00 100644\n' + b'--- README\n' + b'+++ README\n' + b'@@ -2 +2 @@\n' + b'-blah blah\n' + b'+foo bar\n' + ) + parent_diff = SimpleUploadedFile('parent_diff', parent_diff_contents, + content_type='text/x-patch') + + diff_contents = ( + b'diff --git a/README b/README\n' + b'index f00f00..197009f 100644\n' + b'--- README\n' + b'+++ README\n' + b'@@ -2 +2 @@\n' + b'-foo bar\n' + b'+blah!\n' + ) + diff = SimpleUploadedFile('diff', diff_contents, + content_type='text/x-patch') + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + 'commit_id': 'r1', + 'parent_id': 'r0', + 'diff': diff, + 'parent_diff': parent_diff, + 'repository': repo.name, + }, + expected_mimetype=validate_diffcommit_mimetype, + expected_status=200) + + self.assertEqual(rsp['stat'], 'ok') + + validation_info = json.loads(base64.b64decode( + rsp['commit_validation']['validation_info'])) + self.assertEqual(validation_info, { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [ + { + 'filename': 'README', + 'revision': '197009f', + }, + ], + 'removed': [], + }, + }, + }) + + @webapi_test_template + def test_post_added_in_parent(self): + """Testing the POST API with a subsequent commit that contains a + file added in the parent diff + """ + def _exists(repository, filename, revision, *args, **kwargs): + return filename == 'README' and revision == '94bdd3e' + + initial_validation_info = { + 'r1': { + 'parent_id': 'r0', + 'tree': { + 'added': [], + 'modified': [], + 'removed': [], + }, + }, + } + + repo = self.create_repository(name='repo') + + self.spy_on(Repository.get_file_exists, call_fake=_exists) + + diff = SimpleUploadedFile('diff', self.DEFAULT_GIT_FILEDIFF_DATA, + content_type='text/x-patch') + + rsp = self.api_post( + get_validate_diffcommit_url(), + { + 'commit_id': 'r2', + 'parent_id': 'r1', + 'diff': diff, + 'repository': repo.name, + 'validation_info': self.resource.serialize_validation_info( + initial_validation_info), + }, + expected_mimetype=validate_diffcommit_mimetype, + expected_status=200) + + self.assertEqual(rsp['stat'], 'ok') diff --git a/reviewboard/webapi/tests/urls.py b/reviewboard/webapi/tests/urls.py index 53cda4a434922b06fe8954b92e95cec7b62cbf02..a38655a4bc6baf9ff360b43c15edcd3faec004fe 100644 --- a/reviewboard/webapi/tests/urls.py +++ b/reviewboard/webapi/tests/urls.py @@ -875,6 +875,14 @@ def get_validate_diff_url(local_site_name=None): local_site_name=local_site_name) +# +# ValidateDiffCommitResource +# +def get_validate_diffcommit_url(local_site_name=None): + return resources.validate_diffcommit.get_item_url( + local_site_name=local_site_name) + + # # WatchedReviewGroupResource #