diff --git a/reviewboard/diffviewer/diffutils.py b/reviewboard/diffviewer/diffutils.py index f2715810c3202c1fa5a4af25b8b5d02cd7b9c1b9..54d1269299c01b65988f159cc36c910116d11bfe 100644 --- a/reviewboard/diffviewer/diffutils.py +++ b/reviewboard/diffviewer/diffutils.py @@ -678,6 +678,10 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, A list of dictionaries containing information on the files to show in the diff, in the order in which they would be shown. """ + # It is presently not supported to do an interdiff with commit spans. It + # would require base/tip commits for the interdiffset as well. + assert not interdiffset or (base_commit is None and tip_commit is None) + if (diffset.commit_count > 0 and base_commit and tip_commit and @@ -686,7 +690,7 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, # **must** be empty. return [] - all_filediffs = None + per_commit_filediffs = None if filediff: filediffs = [filediff] @@ -708,9 +712,34 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, # The requested FileDiff is outside the requested commit range. return [] else: - # Even if we have base_commit, we need to query for all FileDiffs so - # that we can do ancestor computations. - filediffs = all_filediffs = list(diffset.files.select_related().all()) + if (diffset.commit_count > 0 and + (base_commit is not None or tip_commit is not None)): + # Even if we have base_commit, we need to query for all FileDiffs + # so that we can do ancestor computations. + filediffs = per_commit_filediffs = diffset.per_commit_files + + if base_commit: + base_commit_id = base_commit.pk + else: + base_commit_id = 0 + + if tip_commit: + tip_commit_id = tip_commit.pk + else: + tip_commit_id = None + + filediffs = [ + f + for f in filediffs + if (f.commit_id > base_commit_id and + (not tip_commit_id or + f.commit_id <= tip_commit_id)) + ] + + filediffs = exclude_ancestor_filediffs(filediffs, + per_commit_filediffs) + else: + filediffs = diffset.cumulative_files if interdiffset: log_timer = log_timed("Generating diff file info for " @@ -722,29 +751,6 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, "diffset id %s" % diffset.id, request=request) - if diffset.commit_count > 0: - if base_commit or tip_commit: - if base_commit: - base_commit_id = base_commit.pk - else: - base_commit_id = 0 - - if tip_commit: - tip_commit_id = tip_commit.pk - else: - tip_commit_id = None - - filediffs = [ - f - for f in filediffs - if (f.commit_id > base_commit_id and - (not tip_commit_id or - f.commit_id <= tip_commit_id)) - ] - - filediffs = exclude_ancestor_filediffs(filediffs, - all_filediffs) - # Filediffs that were created with leading slashes stripped won't match # those created with them present, so we need to compare them without in # order for the filenames to match up properly. @@ -752,8 +758,12 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, if interdiffset: if not filediff: - interfilediffs = exclude_ancestor_filediffs( - list(interdiffset.files.all())) + if interdiffset.commit_count > 0: + # Currently, only interdiffing between cumulative diffs is + # supported. + interfilediffs = interdiffset.cumulative_files + else: + interfilediffs = list(interdiffset.files.all()) elif interfilediff: interfilediffs = [interfilediff] @@ -856,10 +866,10 @@ def get_diff_files(diffset, filediff=None, interdiffset=None, # If we pre-computed this above (or before) and we have all # FileDiffs, this will cost no additional queries. # - # Otherwise this will cost up to ``1 + len(diffset.files.count())`` - # queries. + # Otherwise this will cost up to + # ``1 + len(diffset.per_commit_files.count())`` queries. ancestors = filediff.get_ancestors(minimal=False, - filediffs=all_filediffs) + filediffs=per_commit_filediffs) if ancestors: if base_commit: diff --git a/reviewboard/diffviewer/models/diffset.py b/reviewboard/diffviewer/models/diffset.py index ffb9a0d2625890717e677ed49c825e3d18a7b50d..256222dbfd691b7a31be9f8ab698224b92746f54 100644 --- a/reviewboard/diffviewer/models/diffset.py +++ b/reviewboard/diffviewer/models/diffset.py @@ -2,12 +2,14 @@ from __future__ import unicode_literals +from django.core.exceptions import ValidationError from django.db import models -from django.utils import timezone +from django.utils import six, timezone from django.utils.encoding import python_2_unicode_compatible -from django.utils.translation import ugettext_lazy as _ +from django.utils.translation import ugettext, ugettext_lazy as _ from djblets.db.fields import JSONField, RelationCounterField +from reviewboard.diffviewer.filediff_creator import create_filediffs from reviewboard.diffviewer.diffutils import get_total_line_counts from reviewboard.diffviewer.managers import DiffSetManager from reviewboard.scmtools.models import Repository @@ -17,6 +19,8 @@ from reviewboard.scmtools.models import Repository class DiffSet(models.Model): """A revisioned collection of FileDiffs.""" + _FINALIZED_COMMIT_SERIES_KEY = '__finalized_commit_series' + name = models.CharField(_('name'), max_length=256) revision = models.IntegerField(_("revision")) timestamp = models.DateTimeField(_("timestamp"), default=timezone.now) @@ -43,6 +47,120 @@ class DiffSet(models.Model): objects = DiffSetManager() + @property + def is_commit_series_finalized(self): + """Whether the commit series represented by this DiffSet is finalized. + + When a commit series is finalized, no more :py:class:`DiffCommits + ` can be added to + it. + """ + return (self.extra_data and + self.extra_data.get(self._FINALIZED_COMMIT_SERIES_KEY, False)) + + def finalize_commit_series(self, cumulative_diff, validation_info, + parent_diff=None, request=None, validate=True, + save=False): + """Finalize the commit series represented by this DiffSet. + + Args: + cumulative_diff (bytes): + The cumulative diff of the entire commit series. + + validation_info (dict): + The parsed validation information. + + parent_diff (bytes, optional): + The parent diff of the cumulative diff, if any. + + request (django.http.HttpRequest, optional): + The HTTP request from the client, if any. + + validate (bool, optional): + Whether or not the cumulative diff (and optional parent diff) + should be validated, up to and including file existence checks. + + save (bool, optional): + Whether to save the model after finalization. Defaults to + ``False``. + + If ``True``, only the :py:attr:`extra_data` field will be + updated. + + If ``False``, the caller must save this model. + + Returns: + list of reviewboard.diffviewer.models.filediff.FileDiff: + The list of created FileDiffs. + + Raises: + django.core.exceptions.ValidationError: + The commit series failed validation. + """ + if validate: + if self.is_commit_series_finalized: + raise ValidationError( + ugettext('This diff is already finalized.'), + code='invalid') + + if not self.files.exists(): + raise ValidationError( + ugettext('Cannot finalize an empty commit series.'), + code='invalid') + + commits = { + commit.commit_id: commit + for commit in self.commits.all() + } + + missing_commit_ids = set() + + for commit_id, info in six.iteritems(validation_info): + if (commit_id not in commits or + commits[commit_id].parent_id != info['parent_id']): + missing_commit_ids.add(commit_id) + + if missing_commit_ids: + raise ValidationError( + ugettext('The following commits are specified in ' + 'validation_info but do not exist: %s') + % ', '.join(missing_commit_ids), + code='validation_info') + + for commit_id, commit in six.iteritems(commits): + if (commit_id not in validation_info or + validation_info[commit_id]['parent_id'] != + commit.parent_id): + missing_commit_ids.add(commit_id) + + if missing_commit_ids: + raise ValidationError( + ugettext('The following commits exist but are not ' + 'present in validation_info: %s') + % ', '.join(missing_commit_ids), + code='validation_info') + + filediffs = create_filediffs( + get_file_exists=self.repository.get_file_exists, + diff_file_contents=cumulative_diff, + parent_diff_file_contents=parent_diff, + repository=self.repository, + request=request, + basedir=self.basedir, + base_commit_id=self.base_commit_id, + diffset=self, + check_existence=validate) + + if self.extra_data is None: + self.extra_data = {} + + self.extra_data[self._FINALIZED_COMMIT_SERIES_KEY] = True + + if save: + self.save(update_fields=('extra_data',)) + + return filediffs + def get_total_line_counts(self): """Return the total line counts of all child FileDiffs. @@ -64,6 +182,69 @@ class DiffSet(models.Model): """ return get_total_line_counts(self.files.all()) + @property + def per_commit_files(self): + """The files limited to per-commit diffs. + + This will cache the results for future lookups. If the set of all files + has already been fetched with :py:meth:`~django.db.models.query. + QuerySet.prefetch_related`, no queries will be performed. + """ + if not hasattr(self, '_per_commit_files'): + # This is a giant hack because Django 1.6.x does not support + # Prefetch() statements. In Django 1.8+ we can replace any use of: + # + # # Django == 1.6 + # ds = DiffSet.objects.prefetch_related('files') + # for d in ds: + # # Do something with d.per_commit_files + # + # with: + # + # # Django >= 1.8 + # ds = DiffSet.objects.prefetch_related( + # Prefetch('files', + # queryset=File.objects.filter( + # commit_id__isnull=False), + # to_attr='per_commit_files') + # for d in ds: + # # Do something with d.per_commit_files + if (hasattr(self, '_prefetched_objects_cache') and + 'files' in self._prefetched_objects_cache): + self._per_commit_files = [ + f + for f in self.files.all() + if f.commit_id is not None + ] + else: + self._per_commit_files = list(self.files.filter( + commit_id__isnull=False)) + + return self._per_commit_files + + @property + def cumulative_files(self): + """The files limited to the cumulative diff. + + This will cache the results for future lookups. If the set of all files + has been already been fetched with :py:meth:`~django.db.models.query. + QuerySet.prefetch_related`, no queries will be incurred. + """ + # See per_commit_files for why we are doing this hack. + if not hasattr(self, '_cumulative_files'): + if (hasattr(self, '_prefetched_objects_cache') and + 'files' in self._prefetched_objects_cache): + self._cumulative_files = [ + f + for f in self.files.all() + if f.commit_id is None + ] + else: + self._cumulative_files = list(self.files.filter( + commit_id__isnull=True)) + + return self._cumulative_files + def update_revision_from_history(self, diffset_history): """Update the revision of this diffset based on a diffset history. diff --git a/reviewboard/diffviewer/tests/test_diffset.py b/reviewboard/diffviewer/tests/test_diffset.py index ce2761d0ab6240882b541f5440b11e9f5d308c08..50dc2b75decb14877b11f9cc1dc9fac9d269288d 100644 --- a/reviewboard/diffviewer/tests/test_diffset.py +++ b/reviewboard/diffviewer/tests/test_diffset.py @@ -43,3 +43,76 @@ class DiffSetTests(TestCase): with self.assertRaises(ValueError): diffset.update_revision_from_history(diffset_history) + def test_per_commit_files_unpopulated(self): + """Testing DiffSet.per_commit_files without pre-fetching files""" + repository = self.create_repository() + diffset = self.create_diffset(repository=repository) + commit = self.create_diffcommit(diffset=diffset) + + expected = list(commit.files.all()) + + self.create_filediff(diffset=diffset) + + diffset = DiffSet.objects.get(pk=diffset.pk) + + with self.assertNumQueries(1): + result = diffset.per_commit_files + + self.assertEqual(result, expected) + + with self.assertNumQueries(0): + result = diffset.per_commit_files + + self.assertEqual(result, expected) + + def test_per_commit_files_prefetch_files(self): + """Testing DiffSet.per_commit_files with pre-fetching files""" + repository = self.create_repository() + diffset = self.create_diffset(repository=repository) + commit = self.create_diffcommit(diffset=diffset) + + expected = list(commit.files.all()) + + self.create_filediff(diffset=diffset) + + diffset = DiffSet.objects.prefetch_related('files').get(pk=diffset.pk) + + with self.assertNumQueries(0): + result = diffset.per_commit_files + + self.assertEqual(result, expected) + + def test_cumulative_files_unpopulated(self): + """Testing DiffSet.cumulative_files without pre-fetching files""" + repository = self.create_repository() + diffset = self.create_diffset(repository=repository) + self.create_diffcommit(diffset=diffset) + + expected = [self.create_filediff(diffset=diffset)] + + diffset = DiffSet.objects.get(pk=diffset.pk) + + with self.assertNumQueries(1): + result = diffset.cumulative_files + + self.assertEqual(result, expected) + + with self.assertNumQueries(0): + result = diffset.cumulative_files + + self.assertEqual(result, expected) + + def test_cumulative_files_prefetch_files(self): + """Testing DiffSet.cumulative_files with pre-fetching files""" + repository = self.create_repository() + diffset = self.create_diffset(repository=repository) + self.create_diffcommit(diffset=diffset) + + expected = [self.create_filediff(diffset=diffset)] + + diffset = DiffSet.objects.prefetch_related('files').get(pk=diffset.pk) + + with self.assertNumQueries(0): + result = diffset.cumulative_files + + self.assertEqual(result, expected) diff --git a/reviewboard/diffviewer/tests/test_diffutils.py b/reviewboard/diffviewer/tests/test_diffutils.py index 87af4b0aa7ded1746f5b5172b87115ceeb2927dc..c9147405c98473d449666f95d51fc5b95d1e03c1 100644 --- a/reviewboard/diffviewer/tests/test_diffutils.py +++ b/reviewboard/diffviewer/tests/test_diffutils.py @@ -139,6 +139,48 @@ class BaseFileDiffAncestorTests(SpyAgency, TestCase): }, ] + _CUMULATIVE_DIFF = { + 'parent': b''.join( + parent_diff + for parent_diff in ( + entry['parent'] + for entry in _COMMITS + ) + if parent_diff is not None + ), + 'diff': ( + b'diff --git a/qux b/qux\n' + b'new file mode 100644\n' + b'index 000000..03b37a0\n' + b'--- /dev/null\n' + b'+++ /b/qux\n' + b'@@ -0,0 +1 @@\n' + b'foo bar baz qux\n' + + b'diff --git a/bar b/quux\n' + b'index 5716ca5..e69de29 100644\n' + b'--- a/bar\n' + b'+++ b/quux\n' + b'@@ -1 +0,0 @@\n' + b'-bar\n' + + b'diff --git a/baz b/baz\n' + b'index 7601807..280beb2 100644\n' + b'--- a/baz\n' + b'+++ b/baz\n' + b'@@ -1 +1 @@\n' + b'-baz\n' + b'+baz baz baz\n' + + b'diff --git a/corge b/corge\n' + b'index e69de29..f248ba3 100644\n' + b'--- a/corge\n' + b'+++ b/corge\n' + b'@@ -0,0 +1 @@\n' + b'+corge\n' + ), + } + _FILES = { ('bar', 'e69de29'): b'', } @@ -220,12 +262,18 @@ class BaseFileDiffAncestorTests(SpyAgency, TestCase): diff_contents=diff['diff'], parent_diff_contents=diff['parent']) + self.filediffs = list(FileDiff.objects.all()) + self.diffset.finalize_commit_series( + cumulative_diff=self._CUMULATIVE_DIFF['diff'], + parent_diff=self._CUMULATIVE_DIFF['parent'], + validation_info=None, + validate=False, + save=True) + # This was only necessary so that we could side step diff validation # during creation. Repository.get_file_exists.unspy() - self.filediffs = list(FileDiff.objects.all()) - def get_filediffs_by_details(self): """Return a mapping of FileDiff details to the FileDiffs. @@ -1021,72 +1069,40 @@ class GetDiffFilesTests(BaseFileDiffAncestorTests): self.assertTrue(diff_file['force_interdiff']) def test_get_diff_files_history(self): - """Testing get_diff_files for a whole diffset with history""" + """Testing get_diff_files for a diffset with history""" self.set_up_filediffs() review_request = self.create_review_request(repository=self.repository, create_with_history=True) review_request.diffset_history.diffsets = [self.diffset] - by_details = self.get_filediffs_by_details() + result = get_diff_files(diffset=self.diffset) - diff_files = get_diff_files(diffset=self.diffset) - leaf_filediffs = { - by_details[details] - for details in ( - (2, 'baz', 'PRE-CREATION', 'baz', '280beb2'), - (3, 'corge', 'PRE-CREATION', 'corge', 'f248ba3'), - (3, 'foo', '257cc56', 'qux', '03b37a0'), - (4, 'bar', '5716ca5', 'quux', 'e69de29'), - ) - } + self.assertEqual(len(result), len(self.diffset.cumulative_files)) - self.assertEqual(len(diff_files), len(leaf_filediffs)) self.assertEqual( - [diff_file['filediff'].pk for diff_file in diff_files], - [filediff.pk for filediff in get_sorted_filediffs(leaf_filediffs)]) + [diff_file['filediff'].pk for diff_file in result], + [ + filediff.pk + for filediff in get_sorted_filediffs( + self.diffset.cumulative_files) + ]) - for diff_file in diff_files: + for diff_file in result: filediff = diff_file['filediff'] print('Current filediff is: ', filediff) - history = self._HISTORY[( - filediff.commit_id, - filediff.source_file, - filediff.source_revision, - filediff.dest_file, - filediff.dest_detail, - )] - - if history[0]: - oldest_ancestor = by_details[history[0][0]] - elif history[1]: - oldest_ancestor = by_details[history[1][0]] - else: - oldest_ancestor = None - - self.assertEqual(diff_file['base_filediff'], - oldest_ancestor) - base = oldest_ancestor - - if not base: - base = filediff - - self.assertEqual(diff_file['revision'], - get_revision_str(base.source_revision)) - self.assertEqual(diff_file['depot_filename'], - base.source_file) + self.assertIsNone(diff_file['base_filediff']) def test_with_diff_files_history_query_count(self): - """Testing get_diff_files query count for a whole diffset with history - """ + """Testing get_diff_files query count for a diffset with history""" self.set_up_filediffs() review_request = self.create_review_request(repository=self.repository, create_with_history=True) review_request.diffset_history.diffsets = [self.diffset] - with self.assertNumQueries(3 + len(self.filediffs)): + with self.assertNumQueries(3): get_diff_files(diffset=self.diffset) def test_get_diff_files_history_query_count_ancestors_precomputed(self): diff --git a/reviewboard/reviews/builtin_fields.py b/reviewboard/reviews/builtin_fields.py index ba668eea5f744c16d04921bec731cbdf4caa7acd..79944981a36ea702f38436b663c0f09866239517 100644 --- a/reviewboard/reviews/builtin_fields.py +++ b/reviewboard/reviews/builtin_fields.py @@ -912,7 +912,7 @@ class DiffField(ReviewRequestPageDataMixin, BuiltinFieldMixin, '

', url=diff_url, label=_('Revision %s') % diff_revision, - count=_('%d files') % diffset.file_count, + count=_('%d files') % len(diffset.cumulative_files), line_counts=mark_safe(' '.join(line_counts)))) if past_revision > 0: @@ -931,7 +931,9 @@ class DiffField(ReviewRequestPageDataMixin, BuiltinFieldMixin, url=interdiff_url, text=_('Show changes'))) - if diffset.file_count > 0: + file_count = len(diffset.cumulative_files) + + if file_count > 0: # Begin displaying the list of files modified in this diff. # It will be capped at a fixed number (MAX_FILES_PREVIEW). s += [ @@ -942,7 +944,7 @@ class DiffField(ReviewRequestPageDataMixin, BuiltinFieldMixin, # We want a sorted list of filediffs, but tagged with the order in # which they come from the database, so that we can properly link # to the respective files in the diff viewer. - files = get_sorted_filediffs(enumerate(diffset.files.all()), + files = get_sorted_filediffs(enumerate(diffset.cumulative_files), key=lambda i: i[1]) for i, filediff in files[:self.MAX_FILES_PREVIEW]: @@ -966,7 +968,7 @@ class DiffField(ReviewRequestPageDataMixin, BuiltinFieldMixin, url=diff_url + '#%d' % i, filename=filediff.source_file)) - num_remaining = diffset.file_count - self.MAX_FILES_PREVIEW + num_remaining = file_count - self.MAX_FILES_PREVIEW if num_remaining > 0: # There are more files remaining than we've shown, so show diff --git a/reviewboard/reviews/models/review_request.py b/reviewboard/reviews/models/review_request.py index f5873f22b689ba370bf191c69289e0970807db8e..973ad0fd849f7590cdabc6b1348687d7c2744011 100644 --- a/reviewboard/reviews/models/review_request.py +++ b/reviewboard/reviews/models/review_request.py @@ -838,7 +838,6 @@ class ReviewRequest(BaseReviewRequestDetails): self._diffsets = list( DiffSet.objects .filter(history__pk=self.diffset_history_id) - .annotate(file_count=Count('files')) .prefetch_related('files')) return self._diffsets diff --git a/reviewboard/reviews/models/review_request_draft.py b/reviewboard/reviews/models/review_request_draft.py index abf3db7731ef3d7058716084cd1cbe2aa7db0587..2c86f5922cd969872d3deb19333aad814150aa9b 100644 --- a/reviewboard/reviews/models/review_request_draft.py +++ b/reviewboard/reviews/models/review_request_draft.py @@ -321,6 +321,11 @@ class ReviewRequestDraft(BaseReviewRequestDetails): ugettext('There are no commits attached to the diff.')) if self.diffset: + if (review_request.created_with_history and not + self.diffset.is_commit_series_finalized): + raise PublishError(ugettext( + 'This commit series is not finalized.')) + self.diffset.history = review_request.diffset_history self.diffset.timestamp = timestamp self.diffset.save(update_fields=('history', 'timestamp')) diff --git a/reviewboard/reviews/tests/test_builtin_fields.py b/reviewboard/reviews/tests/test_builtin_fields.py index ab9b1534e51013826c26134fc25e4166453d8a56..bec3b0ec7dc6328866229e5005dc2d81609abaa4 100644 --- a/reviewboard/reviews/tests/test_builtin_fields.py +++ b/reviewboard/reviews/tests/test_builtin_fields.py @@ -323,6 +323,12 @@ class CommitListFieldTests(TestCase): commit_message='New commit message 2', author_name=author_name) + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() @@ -400,6 +406,12 @@ class CommitListFieldTests(TestCase): 'So very long of a message.\n', author_name=author_name) + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() @@ -498,6 +510,12 @@ class CommitListFieldTests(TestCase): 'So very long of a message.\n', author_name=submitter_name) + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() @@ -600,6 +618,12 @@ class CommitListFieldTests(TestCase): commit_message='New commit message 2', author_name=submitter_name) + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() @@ -681,6 +705,12 @@ class CommitListFieldTests(TestCase): commit_message='New commit message 2', author_name='Example Author') + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() @@ -760,6 +790,12 @@ class CommitListFieldTests(TestCase): commit_message='New commit message 2', author_name='Example Author') + draft_diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + review_request.publish(user=review_request.submitter) changedesc = review_request.changedescs.latest() field = self._make_field(review_request) diff --git a/reviewboard/reviews/tests/test_review_request_draft.py b/reviewboard/reviews/tests/test_review_request_draft.py index ce67fc149965beb36d280693a40e2b4c9993e8a5..c35917726291e96cc29ed50da91fb9655ea6e31c 100644 --- a/reviewboard/reviews/tests/test_review_request_draft.py +++ b/reviewboard/reviews/tests/test_review_request_draft.py @@ -3,10 +3,12 @@ from __future__ import unicode_literals import os from django.contrib.auth.models import User +from djblets.features.testing import override_feature_check from kgb import SpyAgency from reviewboard.accounts.models import Profile from reviewboard.attachments.models import FileAttachment +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.reviews.errors import PublishError from reviewboard.reviews.fields import (BaseEditableField, BaseTextAreaField, @@ -490,6 +492,68 @@ class ReviewRequestDraftTests(TestCase): with self.assertRaisesMessage(PublishError, error_msg): draft.publish() + def test_publish_with_history_diffset_not_finalized(self): + """Testing ReviewRequestDraft.publish for a review request created with + commit history support when the diffset has not been finalized + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_with_history=True, + create_repository=True) + self.create_diffset(review_request, draft=True) + draft = review_request.get_draft() + + draft.target_people = [review_request.submitter] + + error_msg = \ + 'Error publishing: There are no commits attached to the diff' + + with self.assertRaisesMessage(PublishError, error_msg): + draft.publish() + + def test_publish_with_history_diffset_finalized(self): + """Testing ReviewRequestDraft.publish for a review request created with + commit history support when the diffset has been finalized + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_with_history=True, + create_repository=True) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + draft = review_request.get_draft() + draft.target_people = [review_request.submitter] + draft.publish() + + review_request = ReviewRequest.objects.get(pk=review_request.pk) + self.assertEqual(review_request.status, + ReviewRequest.PENDING_REVIEW) + + def test_publish_without_history_not_finalized(self): + """Testing ReviewRequestDraft.publish for a review request created + without commit history support when the diffset has not been finalized + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True) + diffset = self.create_diffset(review_request, draft=True) + draft = review_request.get_draft() + draft.target_people = [review_request.submitter] + self.create_filediff(diffset=diffset) + + draft.publish() + + review_request = ReviewRequest.objects.get(pk=review_request.pk) + self.assertEqual(review_request.status, + ReviewRequest.PENDING_REVIEW) + def _get_draft(self): """Convenience function for getting a new draft to work with.""" review_request = self.create_review_request(publish=True) diff --git a/reviewboard/testing/testcase.py b/reviewboard/testing/testcase.py index 9da3f6b2ad594e590b0d9e249c8f64b5729f1872..d1451a5afcf6ca6ab0a5ca38ba5c50ee368b7600 100644 --- a/reviewboard/testing/testcase.py +++ b/reviewboard/testing/testcase.py @@ -196,6 +196,10 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): **kwargs): """Create a DiffCommit for testing. + This also creates a + :py:class:`reviewboard.diffviewer.models.filediff.FileDiff` attached to + the commit. + Args: repository (reviewboard.scmtools.models.Repository, optional): The repository the commit is associated with. diff --git a/reviewboard/webapi/resources/diff.py b/reviewboard/webapi/resources/diff.py index a7e0d34c026a7e0e9dce92db40362dbb287a8378..53f3652ec860f417c4d529ea22c38256bcf2555c 100644 --- a/reviewboard/webapi/resources/diff.py +++ b/reviewboard/webapi/resources/diff.py @@ -387,9 +387,7 @@ class DiffResource(WebAPIResource): @webapi_login_required @webapi_check_local_site @webapi_response_errors(DOES_NOT_EXIST, NOT_LOGGED_IN, PERMISSION_DENIED) - @webapi_request_fields( - allow_unknown=True - ) + @webapi_request_fields(allow_unknown=True) def update(self, request, extra_fields={}, *args, **kwargs): """Updates a diff. diff --git a/reviewboard/webapi/resources/draft_diff.py b/reviewboard/webapi/resources/draft_diff.py index f70ae0dbdff47aefbaa5fb7d365097f5c4abc0f0..0885da8a88ca3d052f14bf320150571ec4d6f506 100644 --- a/reviewboard/webapi/resources/draft_diff.py +++ b/reviewboard/webapi/resources/draft_diff.py @@ -1,9 +1,23 @@ from __future__ import unicode_literals -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, ValidationError from djblets.util.decorators import augment_method_from -from djblets.webapi.decorators import webapi_login_required +from djblets.webapi.decorators import (webapi_login_required, + webapi_response_errors, + webapi_request_fields) +from djblets.webapi.errors import (DOES_NOT_EXIST, + INVALID_ATTRIBUTE, + INVALID_FORM_DATA, + NOT_LOGGED_IN, + PERMISSION_DENIED) +from djblets.webapi.fields import (BooleanFieldType, + FileFieldType, + StringFieldType) +from reviewboard.diffviewer.commit_utils import deserialize_validation_info +from reviewboard.diffviewer.features import dvcs_feature +from reviewboard.webapi.base import ImportExtraDataError +from reviewboard.webapi.decorators import webapi_check_local_site from reviewboard.webapi.resources import resources from reviewboard.webapi.resources.diff import DiffResource @@ -59,5 +73,194 @@ class DraftDiffResource(DiffResource): """ pass + @webapi_login_required + @webapi_check_local_site + @webapi_response_errors(DOES_NOT_EXIST, INVALID_ATTRIBUTE, + INVALID_FORM_DATA, NOT_LOGGED_IN, + PERMISSION_DENIED) + @webapi_request_fields( + optional={ + 'cumulative_diff': { + 'type': FileFieldType, + 'description': ( + 'The cumulative diff of the entire commit series this ' + 'resource represents.' + ), + }, + 'finalize_commit_series': { + 'type': BooleanFieldType, + 'description': ( + 'Whether or not this is a request to finalize ' + 'the commit series represented by this resource.\n' + '\n' + 'If this is set to ``true``, then both the ' + '``cumulative_diff`` and ``validation_info`` fields are ' + 'required.' + ), + }, + 'parent_diff': { + 'type': FileFieldType, + 'description': ( + 'The parent diff of the cumulative diff of the entire ' + 'commit series this resource represents.' + ), + }, + 'validation_info': { + 'type': StringFieldType, + 'description': ( + 'Validation information returned when validating the last ' + 'commit in the series with the :ref:`DiffCommit ' + 'validation resource ' + '`.' + ), + }, + }, + allow_unknown=True + ) + def update(self, request, finalize_commit_series=False, + validation_info=None, extra_fields={}, *args, **kwargs): + """Update a diff. + + This is used for two purposes: + + 1. For updating extra data on a draft diff. + + Extra data can be stored later lookup. See + :ref:`webapi2.0-extra-data` for more information. + + 2. For finalization of a draft diff on a review request created with + commit history. + """ + try: + review_request = resources.review_request.get_object( + request, *args, **kwargs) + diffset = self.get_object(request, *args, **kwargs) + except ObjectDoesNotExist: + return DOES_NOT_EXIST + + if not review_request.is_mutable_by(request.user): + return self.get_no_access_error(request) + + if extra_fields: + try: + self.import_extra_data(diffset, diffset.extra_data, + extra_fields) + except ImportExtraDataError as e: + return e.error_payload + + if finalize_commit_series: + if review_request.created_with_history: + cumulative_diff = request.FILES.get('cumulative_diff') + parent_diff = request.FILES.get('parent_diff') + + error_rsp = self._finalize_commit_series(request, + diffset, + cumulative_diff, + parent_diff, + validation_info) + + if error_rsp is not None: + return error_rsp + elif dvcs_feature.is_enabled(request=request): + return INVALID_ATTRIBUTE, { + 'reason': 'This review request was not created with ' + 'commit history support.', + } + else: + # Otherwise we silently ignore this option. + finalize_commit_series = False + + if extra_fields or finalize_commit_series: + diffset.save(update_fields=('extra_data',)) + + return 200, { + self.item_result_key: diffset, + } + + def _finalize_commit_series(self, request, diffset, cumulative_diff, + parent_diff, validation_info): + + """Finalize the commit series represented by the given diffset. + + Args: + request (django.http.HttpRequest): + The HTTP request from the client. + + diffset (reviewboard.diffviewer.models.diffset.DiffSet): + The diffset representing a commit series. + + cumulative_diff (django.core.files.uploadedfile.UploadedFile): + The cumulative diff of the entire commit series. + + parent_diff (django.core.files.uploadedfile.UploadedFile): + The parent diff, if any. + + validation_info (unicode): + Validation information from the :py:class:`~reviewboard.webapi. + resources.validate_diffcommit.ValidateDiffCommitResource`. + + Returns: + tuple: + If the finalization process is successful, ``None`` will be + returned. Otherwise, a 2-tuple of the following will be returned: + + * The WebAPI error (:py:class:`~djblets.webapi.errors. + WebAPIError`). + * A response to serialize with the error (:py:class:`dict`). + """ + field_errors = {} + + if cumulative_diff is None: + field_errors['cumulative_diff'] = [ + 'This field is required when finalize_commit_series is set.', + ] + + if validation_info is None or not validation_info.strip(): + field_errors['validation_info'] = [ + 'This field is required when finalize_commit_series is set.', + ] + else: + try: + validation_info = deserialize_validation_info(validation_info) + except (TypeError, ValueError) as e: + field_errors['validation_info'] = [ + 'Could not parse field: %s' % e, + ] + + if diffset.is_commit_series_finalized: + return INVALID_ATTRIBUTE, { + 'reason': 'This diff is already finalized.', + } + + if field_errors: + return INVALID_FORM_DATA, { + 'fields': field_errors, + } + + if parent_diff: + parent_diff_file_contents = parent_diff.read() + else: + parent_diff_file_contents = None + + diff_file_contents = cumulative_diff.read() + + try: + diffset.finalize_commit_series( + cumulative_diff=diff_file_contents, + validation_info=validation_info, + parent_diff=parent_diff_file_contents, + request=request, + save=False) + except ValidationError as e: + if e.code == 'invalid': + return INVALID_ATTRIBUTE, { + 'reason': e.message, + } + elif e.code == 'validation_info': + return INVALID_FORM_DATA, { + 'fields': { + 'validation_info': [e.message], + }, + } draft_diff_resource = DraftDiffResource() diff --git a/reviewboard/webapi/resources/draft_diffcommit.py b/reviewboard/webapi/resources/draft_diffcommit.py index 205fb11ae56c9beeff35102dfb99db2687f35db6..ed6911eec103f74d45098d88cae40b31811a0955 100644 --- a/reviewboard/webapi/resources/draft_diffcommit.py +++ b/reviewboard/webapi/resources/draft_diffcommit.py @@ -296,6 +296,10 @@ class DraftDiffCommitResource(DiffCommitResource): return DOES_NOT_EXIST, { 'reason': 'An empty diff must be created first.', } + elif diffset.is_commit_series_finalized: + return INVALID_ATTRIBUTE, { + 'reason': 'The diff has already been finalized.', + } form = UploadCommitForm( review_request=review_request, diff --git a/reviewboard/webapi/resources/filediff.py b/reviewboard/webapi/resources/filediff.py index fae229d17a72e212a07386965aad50a1b1262aa7..2b65313bb7c5a5f20c708a723befd008607f2c45 100644 --- a/reviewboard/webapi/resources/filediff.py +++ b/reviewboard/webapi/resources/filediff.py @@ -190,6 +190,8 @@ class FileDiffResource(WebAPIResource): if commit_id: qs = qs.filter(commit__commit_id=commit_id) + else: + qs = qs.filter(commit_id__isnull=True) return qs diff --git a/reviewboard/webapi/tests/test_draft_diff.py b/reviewboard/webapi/tests/test_draft_diff.py index ef0632c11ea981dd009fc2c2fb304348d4594c8a..26a67e5f170aa548ac277bd4c080ca26d74a07d5 100644 --- a/reviewboard/webapi/tests/test_draft_diff.py +++ b/reviewboard/webapi/tests/test_draft_diff.py @@ -1,16 +1,21 @@ from __future__ import unicode_literals +import base64 import os from django.core.files.uploadedfile import SimpleUploadedFile from django.utils import six from djblets.features.testing import override_feature_check -from djblets.webapi.errors import INVALID_FORM_DATA +from djblets.webapi.errors import INVALID_ATTRIBUTE, INVALID_FORM_DATA from djblets.webapi.testing.decorators import webapi_test_template +from kgb import SpyAgency from reviewboard import scmtools +from reviewboard.diffviewer.commit_utils import (serialize_validation_info, + update_validation_info) from reviewboard.diffviewer.features import dvcs_feature -from reviewboard.diffviewer.models import DiffSet +from reviewboard.diffviewer.models import DiffSet, FileDiff +from reviewboard.scmtools.models import Repository from reviewboard.webapi.errors import DIFF_TOO_BIG from reviewboard.webapi.resources import resources from reviewboard.webapi.tests.base import BaseWebAPITestCase @@ -243,7 +248,7 @@ class ResourceListTests(ExtraDataListMixin, BaseWebAPITestCase): @six.add_metaclass(BasicTestsMetaclass) -class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): +class ResourceItemTests(SpyAgency, ExtraDataItemMixin, BaseWebAPITestCase): """Testing the DraftDiffResource item APIs.""" fixtures = ['test_users', 'test_scmtools'] sample_api_url = 'review-requests//draft/diffs//' @@ -324,3 +329,510 @@ class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): def check_put_result(self, user, item_rsp, diffset): diffset = DiffSet.objects.get(pk=diffset.pk) self.compare_item(item_rsp, diffset) + + @webapi_test_template + def test_put_finalize(self): + """Testing the PUT API with finalize_commit_series=1""" + def _get_file_exists(repository, path, revision, + base_commit_id=None, request=None): + self.assertEqual(path, 'README') + self.assertEqual(revision, '94bdd3e') + + return True + + self.spy_on(Repository.get_file_exists, call_fake=_get_file_exists) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + commit = self.create_diffcommit(diffset=diffset) + + filediff = FileDiff.objects.get() + + cumulative_diff = SimpleUploadedFile('diff', filediff.diff, + content_type='text/x-patch') + + validation_info = update_validation_info( + {}, + commit_id=commit.commit_id, + parent_id=commit.parent_id, + filediffs=[filediff]) + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info( + validation_info), + }, + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.compare_item(rsp['diff'], diffset) + + @webapi_test_template + def test_put_finalized_with_parent(self): + """Testing the PUT API with finalize_commit_series=1 and a parent + diff + """ + def _get_file_exists(repository, path, revision, + base_commit_id=None, request=None): + self.assertEqual(path, 'README') + self.assertEqual(revision, 'f00f00') + + return True + + self.spy_on(Repository.get_file_exists, call_fake=_get_file_exists) + + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + commit = self.create_diffcommit(diffset=diffset) + + filediff = FileDiff.objects.get() + filediff.parent_diff = ( + b'diff --git a/README b/README\n' + b'index f00f00..94bdd3e\n' + b'--- a/README\n' + b'+++ b/README\n' + ) + filediff.extra_data[FileDiff._IS_PARENT_EMPTY_KEY] = True + filediff.save(update_fields=('extra_data',)) + + cumulative_diff = SimpleUploadedFile('diff', filediff.diff, + content_type='text/x-patch') + + parent_diff = SimpleUploadedFile('parent_diff', + filediff.parent_diff, + content_type='text/x-patch') + + validation_info = update_validation_info( + {}, + commit_id=commit.commit_id, + parent_id=commit.parent_id, + filediffs=[filediff]) + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'parent_diff': parent_diff, + 'validation_info': serialize_validation_info( + validation_info), + }, + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.compare_item(rsp['diff'], diffset) + + @webapi_test_template + def test_put_finalize_again(self): + """Testing the PUT API with finalize_commit_series=1 when the + diff is already finalized + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info({}), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_ATTRIBUTE.code, + 'msg': INVALID_ATTRIBUTE.msg, + }, + 'reason': 'This diff is already finalized.', + }) + + @webapi_test_template + def test_put_finalize_missing_fields(self): + """Testing the PUT API with finalize_commit_series=1 with missing + request fields + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'cumulative_diff': [ + 'This field is required when finalize_commit_series is ' + 'set.', + ], + 'validation_info': [ + 'This field is required when finalize_commit_series is ' + 'set.', + ], + }, + }) + + @webapi_test_template + def test_put_finalize_review_request_without_history(self): + """Testing the PUT API with finalize_commit_series=1 when the + review request was created without commit history support + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info({}), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_ATTRIBUTE.code, + 'msg': INVALID_ATTRIBUTE.msg, + }, + 'reason': 'This review request was not created with commit ' + 'history support.', + }) + + @webapi_test_template + def test_put_finalize_dvcs_feature_disabled(self): + """Testing the PUT API with finalize_commit_series=1 when the + DVCS feature is disabled + """ + with override_feature_check(dvcs_feature.feature_id, enabled=False): + review_request = self.create_review_request( + create_repository=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info({}), + }, + expected_mimetype=diff_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + self.compare_item(rsp['diff'], diffset) + + @webapi_test_template + def test_put_finalize_empty_commit_series(self): + """Testing the PUT API with finalize_commit_series=1 for an empty + commit series + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info({}), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_ATTRIBUTE.code, + 'msg': INVALID_ATTRIBUTE.msg, + }, + 'reason': 'Cannot finalize an empty commit series.', + }) + + @webapi_test_template + def test_put_finalize_invalid_validation_info_base64(self): + """Testing the PUT API with finalize_commit_series=1 when + validation_info is invalid base64 + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': 'foo', + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'validation_info': [ + 'Could not parse field: Incorrect padding', + ], + }, + }) + + @webapi_test_template + def test_put_finalize_invalid_validation_info_json_format(self): + """Testing the PUT API with finalize_commit_series=1 when + validation_info is invalid json + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info('foo'), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'validation_info': [ + 'Could not parse field: Invalid format.', + ], + }, + }) + + @webapi_test_template + def test_put_finalize_invalid_validation_info_not_json(self): + """Testing the PUT API with finalize_commit_series=1 when + validation_info is JSON in the incorrect format + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + self.create_diffcommit(diffset=diffset) + + cumulative_diff = SimpleUploadedFile('diff', b'', + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': base64.b64encode('AAAAAAA'), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'validation_info': [ + 'Could not parse field: No JSON object could be decoded', + ], + }, + }) + + @webapi_test_template + def test_put_finalize_validation_info_extra_commits(self): + """Testing the PUT API with finalize_commit_series=1 when + validation_info contains commits that do not exist + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + commit = self.create_diffcommit(diffset=diffset) + filediff = commit.files.first() + + validation_info = update_validation_info( + {}, + commit_id=commit.commit_id, + parent_id=commit.parent_id, + filediffs=[filediff]) + + validation_info = update_validation_info( + validation_info, + commit_id='f00', + parent_id=commit.commit_id, + filediffs=[]) + + cumulative_diff = SimpleUploadedFile('diff', filediff.diff, + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info( + validation_info), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'validation_info': [ + 'The following commits are specified in validation_info ' + 'but do not exist: f00' + ], + }, + }) + + @webapi_test_template + def test_put_finalize_validation_info_missing_commits(self): + """Testing the PUT API with finalize_commit_series=1 when + validation_info does not contain all commits + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_repository=True, + create_with_history=True, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + draft=True) + + commits = [ + self.create_diffcommit(diffset=diffset, **kwargs) + for kwargs in ( + {'commit_id': 'r1', 'parent_id': 'r0'}, + {'commit_id': 'r2', 'parent_id': 'r1'}, + ) + ] + + filediff = commits[0].files.first() + + validation_info = update_validation_info( + {}, + commit_id=commits[0].commit_id, + parent_id=commits[0].parent_id, + filediffs=[filediff]) + + cumulative_diff = SimpleUploadedFile('diff', filediff.diff, + content_type='text/x-patch') + + rsp = self.api_put( + get_draft_diff_item_url(review_request, diffset.revision), + { + 'finalize_commit_series': True, + 'cumulative_diff': cumulative_diff, + 'validation_info': serialize_validation_info( + validation_info), + }, + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_FORM_DATA.code, + 'msg': INVALID_FORM_DATA.msg, + }, + 'fields': { + 'validation_info': [ + 'The following commits exist but are not present in ' + 'validation_info: r2', + ], + } + }) diff --git a/reviewboard/webapi/tests/test_draft_diffcommit.py b/reviewboard/webapi/tests/test_draft_diffcommit.py index ff0d8cfbe1bc76e9e6e80f00d8854586410caa3c..c3a42d5c4b58d5c29390494339e4ed084b3bc62f 100644 --- a/reviewboard/webapi/tests/test_draft_diffcommit.py +++ b/reviewboard/webapi/tests/test_draft_diffcommit.py @@ -438,6 +438,47 @@ class ResourceListTests(BaseWebAPITestCase): item_rsp = rsp['draft_commit'] self.compare_item(item_rsp, DiffCommit.objects.get(pk=item_rsp['id'])) + @webapi_test_template + def test_post_finalized(self): + """Testing the POST API after the parent DiffSet has been + finalized + """ + with override_feature_checks(self.override_features): + review_request = self.create_review_request( + create_repository=True, + submitter=self.user, + create_with_history=True) + + diffset = self.create_diffset(review_request, draft=True) + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + diff = SimpleUploadedFile( + 'diff', + self._DEFAULT_DIFF_CONTENTS, + content_type='text/x-patch') + + rsp = self.api_post( + get_draft_diffcommit_list_url(review_request, + diffset.revision), + dict(self._DEFAULT_POST_DATA, **{ + 'validation_info': serialize_validation_info({}), + 'diff': diff, + }), + expected_status=400) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': INVALID_ATTRIBUTE.code, + 'msg': INVALID_ATTRIBUTE.msg, + }, + 'reason': 'The diff has already been finalized.', + }) + @six.add_metaclass(BasicTestsMetaclass) class ResourceItemTests(ExtraDataItemMixin, BaseWebAPITestCase): diff --git a/reviewboard/webapi/tests/test_filediff.py b/reviewboard/webapi/tests/test_filediff.py index 8a034255a0c4324406d4561038d9bb60fd44dd14..7a6e5c9d71dde8badd4c9f858b89680c6de5309b 100644 --- a/reviewboard/webapi/tests/test_filediff.py +++ b/reviewboard/webapi/tests/test_filediff.py @@ -1,8 +1,10 @@ from __future__ import unicode_literals from django.utils import six +from djblets.features.testing import override_feature_check from djblets.webapi.testing.decorators import webapi_test_template +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.diffviewer.models import FileDiff from reviewboard.webapi.resources import resources from reviewboard.webapi.tests.base import BaseWebAPITestCase @@ -83,53 +85,107 @@ class ResourceListTests(ReviewRequestChildListMixin, BaseWebAPITestCase): """Testing the GET ?commit-id= API filters FileDiffs to the requested commit """ - repository = self.create_repository() - review_request = self.create_review_request(repository=repository, - submitter=self.user) - diffset = self.create_diffset(review_request=review_request, - repository=repository) - commit = self.create_diffcommit(diffset=diffset, - repository=repository) - - rsp = self.api_get( - '%s?commit-id=%s' - % (get_filediff_list_url(diffset, review_request), - commit.commit_id), - expected_status=200, - expected_mimetype=filediff_list_mimetype) - - self.assertIn('stat', rsp) - self.assertEqual(rsp['stat'], 'ok') - self.assertIn('files', rsp) - self.assertEqual(rsp['total_results'], 1) - - item_rsp = rsp['files'][0] - filediff = FileDiff.objects.get(pk=item_rsp['id']) - self.compare_item(item_rsp, filediff) + with override_feature_check(dvcs_feature.feature_id, enabled=True): + repository = self.create_repository() + review_request = self.create_review_request(repository=repository, + submitter=self.user) + diffset = self.create_diffset(review_request=review_request, + repository=repository) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + rsp = self.api_get( + '%s?commit-id=%s' + % (get_filediff_list_url(diffset, review_request), + commit.commit_id), + expected_status=200, + expected_mimetype=filediff_list_mimetype) + + self.assertIn('stat', rsp) + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('files', rsp) + self.assertEqual(rsp['total_results'], 1) + + item_rsp = rsp['files'][0] + filediff = FileDiff.objects.get(pk=item_rsp['id']) + self.compare_item(item_rsp, filediff) @webapi_test_template def test_commit_filter_no_results(self): """Testing the GET ?commit-id= API with no results""" - repository = self.create_repository() - review_request = self.create_review_request(repository=repository, - submitter=self.user) - diffset = self.create_diffset(review_request=review_request, - repository=repository) - commit = self.create_diffcommit(diffset=diffset, - repository=repository) - - rsp = self.api_get( - '%s?commit-id=%s' - % (get_filediff_list_url(diffset, review_request), - commit.parent_id), - expected_status=200, - expected_mimetype=filediff_list_mimetype) - - self.assertIn('stat', rsp) - self.assertEqual(rsp['stat'], 'ok') - self.assertIn('files', rsp) - self.assertEqual(rsp['files'], []) - self.assertEqual(rsp['total_results'], 0) + with override_feature_check(dvcs_feature.feature_id, enabled=True): + repository = self.create_repository() + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request=review_request, + repository=repository) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + rsp = self.api_get( + '%s?commit-id=%s' + % (get_filediff_list_url(diffset, review_request), + commit.parent_id), + expected_status=200, + expected_mimetype=filediff_list_mimetype) + + self.assertIn('stat', rsp) + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('files', rsp) + self.assertEqual(rsp['files'], []) + self.assertEqual(rsp['total_results'], 0) + + @webapi_test_template + def test_history_no_commit_filter(self): + """Testing the GET API for a diffset with commits only returns + cumulative files + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + repository = self.create_repository() + review_request = self.create_review_request( + repository=repository, + submitter=self.user, + create_with_history=True) + diffset = self.create_diffset(review_request=review_request, + repository=repository) + commit = self.create_diffcommit(diffset=diffset, + repository=repository) + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + cumulative_filediff = diffset.cumulative_files[0] + + rsp = self.api_get( + get_filediff_list_url(diffset, review_request), + expected_mimetype=filediff_list_mimetype) + + self.assertIn('stat', rsp) + self.assertEqual(rsp['stat'], 'ok') + self.assertIn('files', rsp) + self.assertEqual(rsp['total_results'], 1) + self.assertEqual(rsp['files'][0]['id'], + cumulative_filediff.pk) + + self.assertNotEqual(commit.files.get().pk, + cumulative_filediff.pk) @six.add_metaclass(BasicTestsMetaclass) diff --git a/reviewboard/webapi/tests/test_review_request_draft.py b/reviewboard/webapi/tests/test_review_request_draft.py index c3eb1bcae7b865361a9b50684ce37c6c247bbe13..b3e86fcddac22b46c6166f1b9164041730fbfb11 100644 --- a/reviewboard/webapi/tests/test_review_request_draft.py +++ b/reviewboard/webapi/tests/test_review_request_draft.py @@ -4,12 +4,15 @@ from django.contrib import auth from django.contrib.auth.models import Permission, User from django.core import mail from django.utils import six +from djblets.features.testing import override_feature_check from djblets.testing.decorators import add_fixtures from djblets.webapi.errors import INVALID_FORM_DATA, PERMISSION_DENIED +from djblets.webapi.testing.decorators import webapi_test_template from kgb import SpyAgency from reviewboard.accounts.backends import AuthBackend from reviewboard.accounts.models import LocalSiteProfile +from reviewboard.diffviewer.features import dvcs_feature from reviewboard.reviews.fields import (BaseEditableField, BaseTextAreaField, BaseReviewRequestField, @@ -17,7 +20,7 @@ from reviewboard.reviews.fields import (BaseEditableField, from reviewboard.reviews.models import ReviewRequest, ReviewRequestDraft from reviewboard.reviews.signals import (review_request_published, review_request_publishing) -from reviewboard.webapi.errors import NOTHING_TO_PUBLISH +from reviewboard.webapi.errors import NOTHING_TO_PUBLISH, PUBLISH_ERROR from reviewboard.webapi.resources import resources from reviewboard.webapi.tests.base import BaseWebAPITestCase from reviewboard.webapi.tests.mimetypes import \ @@ -1289,6 +1292,77 @@ class ResourceTests(SpyAgency, ExtraDataListMixin, ExtraDataItemMixin, self.assertEqual(rsp['stat'], 'fail') self.assertTrue(backend.get_or_create_user.called) + @add_fixtures(['test_scmtools']) + @webapi_test_template + def test_put_created_with_history_public_unfinalized_series(self): + """Testing the PUT API with public=1 for a review request + created with commit history support that has an unfinalized diffset + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_with_history=True, + create_repository=True, + submitter=self.user) + diffset = self.create_diffset(review_request, draft=True) + draft = review_request.get_draft() + + self.create_diffcommit(diffset=diffset) + + draft.target_people = [review_request.submitter] + draft.save() + + rsp = self.api_put( + get_review_request_draft_url(review_request), + {'public': True}, + expected_status=500) + + self.assertEqual(rsp, { + 'stat': 'fail', + 'err': { + 'code': PUBLISH_ERROR.code, + 'msg': 'Error publishing: This commit series is not ' + 'finalized.', + }, + }) + + # If the draft still exists we indeed did not publish! + self.assertTrue( + ReviewRequestDraft.objects.filter(pk=draft.pk).exists()) + + @add_fixtures(['test_scmtools']) + @webapi_test_template + def test_put_created_with_history_public_finalized_series(self): + """Testing the PUT API with public=1 for a review request + created with commit history support that has a finalized diffset + """ + with override_feature_check(dvcs_feature.feature_id, enabled=True): + review_request = self.create_review_request( + create_with_history=True, + create_repository=True, + submitter=self.user) + diffset = self.create_diffset(review_request, draft=True) + draft = review_request.get_draft() + + self.create_diffcommit(diffset=diffset) + + draft.target_people = [review_request.submitter] + draft.save() + + diffset.finalize_commit_series( + cumulative_diff=self.DEFAULT_GIT_FILEDIFF_DATA, + validation_info=None, + validate=False, + save=True) + + rsp = self.api_put( + get_review_request_draft_url(review_request), + {'public': True}, + expected_mimetype=review_request_draft_item_mimetype) + + self.assertEqual(rsp['stat'], 'ok') + + self.assertFalse(ReviewRequestDraft.objects.exists()) + def _create_update_review_request(self, api_func, expected_status, review_request=None, local_site_name=None):