launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/better-bzr-mp-diffs into lp:launchpad with lp:~cjwatson/launchpad/branch-hosting-client as a prerequisite.

Commit message:
Proxy loggerhead branch diffs through the webapp, allowing AJAX MP revision diffs to work for private branches.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #904070 in Launchpad itself: "broken inline incremental ajax diff on private branches"
  https://bugs.launchpad.net/launchpad/+bug/904070

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/better-bzr-mp-diffs/+merge/345705

This is the same approach as we already use for Git, built on top of my recent work to expose a suitable loggerhead API on a private port.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/better-bzr-mp-diffs into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2018-03-16 21:20:00 +0000
+++ lib/lp/code/browser/branch.py	2018-05-16 19:08:06 +0000
@@ -46,6 +46,7 @@
     providedBy,
     )
 from zope.publisher.interfaces import NotFound
+from zope.publisher.interfaces.browser import IBrowserPublisher
 from zope.schema import (
     Bool,
     Choice,
@@ -203,6 +204,22 @@
             return None
         return self.context.getMergeProposalByID(id)
 
+    @stepto("+diff")
+    def traverse_diff(self):
+        segments = list(self.request.getTraversalStack())
+        if len(segments) == 1:
+            new = segments.pop()
+            old = None
+            self.request.stepstogo.consume()
+        elif len(segments) == 2:
+            new = segments.pop()
+            old = segments.pop()
+            self.request.stepstogo.consume()
+            self.request.stepstogo.consume()
+        else:
+            return None
+        return BranchDiffView(self.context, self.request, new, old=old)
+
     @stepto("+code-import")
     def traverse_code_import(self):
         """Traverses to the `ICodeImport` for the branch."""
@@ -1280,3 +1297,28 @@
                     'target_branch',
                     "This branch is not mergeable into %s." %
                     target_branch.bzr_identity)
+
+
+@implementer(IBrowserPublisher)
+class BranchDiffView:
+
+    def __init__(self, context, request, new, old=None):
+        self.context = context
+        self.request = request
+        self.new = new
+        self.old = old
+
+    def __call__(self):
+        content = self.context.getDiff(self.new, old=self.old)
+        if self.old is None:
+            filename = "%s.diff" % self.new
+        else:
+            filename = "%s_%s.diff" % (self.old, self.new)
+        self.request.response.setHeader("Content-Type", "text/x-patch")
+        self.request.response.setHeader("Content-Length", str(len(content)))
+        self.request.response.setHeader(
+            "Content-Disposition", "attachment; filename=%s" % filename)
+        return content
+
+    def browserDefault(self, request):
+        return self, ()

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2018-05-16 19:08:06 +0000
@@ -617,19 +617,8 @@
         super(BranchMergeProposalView, self).initialize()
         cache = IJSONRequestCache(self.request)
         cache.objects['branch_name'] = self.context.merge_source.name
-        if IBranch.providedBy(self.context.merge_source):
-            cache.objects.update({
-                'branch_diff_link':
-                    'https://%s/+loggerhead/%s/diff/' % (
-                        config.launchpad.code_domain,
-                        self.context.source_branch.unique_name),
-                })
-        else:
-            cache.objects.update({
-                'branch_diff_link':
-                    canonical_url(self.context.source_git_repository) +
-                    '/+diff/',
-                })
+        cache.objects['branch_diff_link'] = (
+            canonical_url(self.context.parent) + '/+diff/')
         if getFeatureFlag("longpoll.merge_proposals.enabled"):
             cache.objects['merge_proposal_event_key'] = subscribe(
                 self.context).event_key

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2017-09-27 13:50:02 +0000
+++ lib/lp/code/browser/configure.zcml	2018-05-16 19:08:06 +0000
@@ -442,6 +442,10 @@
         class="lp.code.browser.codeimport.CodeImportEditView"
         permission="launchpad.Edit"
         template="../../app/templates/generic-edit.pt"/>
+    <class class="lp.code.browser.branch.BranchDiffView">
+        <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher"
+               attributes="__call__"/>
+    </class>
     <browser:page
         name="+delete"
         for="lp.code.interfaces.branch.IBranch"

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2018-02-01 18:38:24 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2018-05-16 19:08:06 +0000
@@ -16,6 +16,7 @@
 from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
@@ -32,6 +33,7 @@
     RepositoryFormat,
     )
 from lp.code.enums import BranchType
+from lp.code.tests.helpers import BranchHostingFixture
 from lp.registry.enums import BranchSharingPolicy
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import PersonVisibility
@@ -1220,3 +1222,45 @@
             InformationType.USERDATA.description, description.renderContents())
         self.assertIsNotNone(
             soup.find('a', id='privacy-link', attrs={'href': edit_url}))
+
+
+class TestBranchDiffView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_render(self):
+        diff = b"A fake diff\n"
+        hosting_fixture = self.useFixture(BranchHostingFixture(diff=diff))
+        person = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=person)
+        browser = self.getUserBrowser(
+            canonical_url(branch) + "/+diff/2/1")
+        with person_logged_in(person):
+            self.assertEqual(
+                [((branch.unique_name, "2"), {"old": "1"})],
+                hosting_fixture.getDiff.calls)
+        self.assertEqual("text/x-patch", browser.headers["Content-Type"])
+        self.assertEqual(str(len(diff)), browser.headers["Content-Length"])
+        self.assertEqual(
+            "attachment; filename=1_2.diff",
+            browser.headers["Content-Disposition"])
+        self.assertEqual(diff, browser.contents)
+
+    def test_security(self):
+        # A user who can see a private branch can fetch diffs from it, but
+        # other users cannot.
+        diff = b"A fake diff\n"
+        self.useFixture(BranchHostingFixture(diff=diff))
+        person = self.factory.makePerson()
+        project = self.factory.makeProduct(
+            owner=person, information_type=InformationType.PROPRIETARY)
+        with person_logged_in(person):
+            branch = self.factory.makeBranch(
+                owner=person, product=project,
+                information_type=InformationType.PROPRIETARY)
+            branch_url = canonical_url(branch)
+        browser = self.getUserBrowser(branch_url + "/+diff/2/1", user=person)
+        self.assertEqual(diff, browser.contents)
+        self.useFixture(FakeLogger())
+        self.assertRaises(
+            Unauthorized, self.getUserBrowser, branch_url + "/+diff/2/1")

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-05-16 19:08:06 +0000
@@ -1635,14 +1635,14 @@
 
     def test_client_cache_bzr(self):
         # For Bazaar, the client cache contains the branch name and a
-        # loggerhead-based diff link.
+        # webapp-based diff link.
         bmp = self.factory.makeBranchMergeProposal()
         view = create_initialized_view(bmp, '+index')
         cache = IJSONRequestCache(view.request)
         self.assertThat(cache.objects, ContainsDict({
             'branch_name': Equals(bmp.source_branch.name),
             'branch_diff_link': Equals(
-                'https://code.launchpad.dev/+loggerhead/%s/diff/' %
+                'http://code.launchpad.dev/%s/+diff/' %
                 bmp.source_branch.unique_name),
             }))
 

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2018-03-06 00:59:06 +0000
+++ lib/lp/code/interfaces/branch.py	2018-05-16 19:08:06 +0000
@@ -765,6 +765,15 @@
         :param launchbag: `ILaunchBag`.
         """
 
+    def getDiff(new, old):
+        """Get the diff between two revisions in this branch.
+
+        :param new: The new revno or revision ID.
+        :param old: The old revno or revision ID.  Defaults to the parent
+            revision of `new`.
+        :return: The diff as a byte string.
+        """
+
     @export_read_operation()
     @operation_for_version('beta')
     def canBeDeleted():

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2018-03-06 00:59:06 +0000
+++ lib/lp/code/model/branch.py	2018-05-16 19:08:06 +0000
@@ -105,6 +105,7 @@
     WrongNumberOfReviewTypeArguments,
     )
 from lp.code.interfaces.branchcollection import IAllBranches
+from lp.code.interfaces.branchhosting import IBranchHostingClient
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES,
@@ -782,6 +783,12 @@
                 RevisionAuthor, revisions, ['revision_author_id'])
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
+    def getDiff(self, new, old=None):
+        """See `IBranch`."""
+        hosting_client = getUtility(IBranchHostingClient)
+        diff = hosting_client.getDiff(self.unique_name, new, old=old)
+        return diff
+
     def canBeDeleted(self):
         """See `IBranch`."""
         if ((len(self.deletionRequirements()) != 0) or not

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2017-12-19 17:16:38 +0000
+++ lib/lp/code/tests/helpers.py	2018-05-16 19:08:06 +0000
@@ -8,6 +8,7 @@
 __metaclass__ = type
 __all__ = [
     'add_revision_to_branch',
+    'BranchHostingFixture',
     'get_non_existant_source_package_branch_unique_name',
     'GitHostingFixture',
     'make_erics_fooix_project',
@@ -35,6 +36,7 @@
     )
 
 from lp.app.enums import InformationType
+from lp.code.interfaces.branchhosting import IBranchHostingClient
 from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposalJobSource,
     )
@@ -301,6 +303,19 @@
     c.execute('delete from branch')
 
 
+class BranchHostingFixture(fixtures.Fixture):
+    """A fixture that temporarily registers a fake Bazaar hosting client."""
+
+    def __init__(self, diff=None, inventory=None, blob=None):
+        self.create = FakeMethod()
+        self.getDiff = FakeMethod(result=diff or {})
+        self.getInventory = FakeMethod(result=inventory or {})
+        self.getBlob = FakeMethod(result=blob)
+
+    def _setUp(self):
+        self.useFixture(ZopeUtilityFixture(self, IBranchHostingClient))
+
+
 class GitHostingFixture(fixtures.Fixture):
     """A fixture that temporarily registers a fake Git hosting client."""