Merge ~pappacena/launchpad:git-fork-backend into launchpad:master

[Thiago F. Pappacena]

Fork method on GitRepository to allow users to asynchronously create a copy of a repository

[Thiago F. Pappacena (pappacena)]

This MP depends on https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/386461 being approved and landed in production.

[Colin Watson (cjwatson)]

I haven't completely gone through this yet, but here are a few high-level/structural comments.

review: Needs Information

4774c21... by Thiago F. Pappacena on 2020-07-15

Merge branch 'master' into git-fork-backend

249af52... by Thiago F. Pappacena on 2020-07-15

Running confirmation job with existing db user

c313a77... by Thiago F. Pappacena on 2020-07-15

separating user that requested a fork and the new repository owner

[Thiago F. Pappacena (pappacena)]

I'm pushing some of the requested changes, and I could use your opinion on one of the comments, cjwatson.

f56198c... by Thiago F. Pappacena on 2020-07-16

Remove polling job to confirm repo creation

2eff6c9... by Thiago F. Pappacena on 2020-07-16

Loose permission check for confirm/abort repo creation

730a9dc... by Thiago F. Pappacena on 2020-07-17

Avoiding race condition on create/confirm API calls

6fdbd8c... by Thiago F. Pappacena on 2020-07-18

Fixing test

[Colin Watson (cjwatson)]

review: Approve

ded71d0... by Thiago F. Pappacena on 2020-09-22

Merge branch 'master' into git-fork-backend

9867039... by Thiago F. Pappacena on 2020-09-22

Fixing repository name clash when forking

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes. This should be good to go now.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index 378930a..d854174 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for communication with the Git hosting service."""
@@ -14,13 +14,15 @@ from zope.interface import Interface
 class IGitHostingClient(Interface):
     """Interface for the internal API provided by the Git hosting service."""
 
-    def create(path, clone_from=None):
+    def create(path, clone_from=None, async_create=False):
         """Create a Git repository.
 
         :param path: Physical path of the new repository on the hosting
             service.
         :param clone_from: If not None, clone the new repository from this
             other physical path.
+        :param async_create: Do not block the call until the repository is
+            actually created.
         """
 
     def getProperties(path):
diff --git a/lib/lp/code/interfaces/gitnamespace.py b/lib/lp/code/interfaces/gitnamespace.py
index 9c6886b..3a6e600 100644
--- a/lib/lp/code/interfaces/gitnamespace.py
+++ b/lib/lp/code/interfaces/gitnamespace.py
@@ -40,8 +40,16 @@ class IGitNamespace(Interface):
     def createRepository(repository_type, registrant, name,
                          information_type=None, date_created=None,
                          target_default=False, owner_default=False,
-                         with_hosting=False, status=None):
-        """Create and return an `IGitRepository` in this namespace."""
+                         with_hosting=False, async_hosting=False, status=None):
+        """Create and return an `IGitRepository` in this namespace.
+
+        :param with_hosting: If True, also creates the repository on git
+            hosting service.
+        :param async_hosting: If with_hosting is True, this controls if the
+            call to create repository on hosting service will be done
+            asynchronously, or it will block until the service creates the
+            repository.
+        """
 
     def isNameUsed(name):
         """Is 'name' already used in this namespace?"""
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 344bc43..201c9a4 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -968,6 +968,14 @@ class IGitRepositorySet(Interface):
         :param with_hosting: Create the repository on the hosting service.
         """
 
+    def fork(origin, requester, new_owner):
+        """Fork a repository to the given user's account.
+
+        :param origin: The original GitRepository.
+        :param requester: The IPerson performing this fork.
+        :param new_owner: The IPerson that will own the forked repository.
+        :return: The newly created GitRepository."""
+
     # Marker for references to Git URL layouts: ##GITNAMESPACE##
     @call_with(user=REQUEST_USER)
     @operation_parameters(
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index 94d6538..60dff43 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Communication with the Git hosting service."""
@@ -90,13 +90,20 @@ class GitHostingClient:
     def _delete(self, path, **kwargs):
         return self._request("delete", path, **kwargs)
 
-    def create(self, path, clone_from=None):
+    def create(self, path, clone_from=None, async_create=False):
         """See `IGitHostingClient`."""
         try:
             if clone_from:
                 request = {"repo_path": path, "clone_from": clone_from}
             else:
                 request = {"repo_path": path}
+            if async_create:
+                # XXX pappacena 2020-07-02: async forces to clone_refs
+                # because it's only used in situations where this is
+                # desirable for now. We might need to add "clone_refs" as
+                # parameter in the future.
+                request['async'] = True
+                request['clone_refs'] = clone_from is not None
             self._post("/repo", json=request)
         except requests.RequestException as e:
             raise GitRepositoryCreationFault(
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index 3c041da..ab37d2b 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -12,6 +12,7 @@ __all__ = [
     'ReclaimGitRepositorySpaceJob',
     ]
 
+
 from lazr.delegates import delegate_to
 from lazr.enum import (
     DBEnumeratedType,
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index f9ac2f5..22e9d8f 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -14,6 +14,7 @@ __all__ = [
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 from storm.locals import And
+import transaction
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implementer
@@ -33,6 +34,7 @@ from lp.code.enums import (
     BranchSubscriptionDiffSize,
     BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel,
+    GitRepositoryStatus,
     )
 from lp.code.errors import (
     GitDefaultConflict,
@@ -75,7 +77,8 @@ class _BaseGitNamespace:
                          reviewer=None, information_type=None,
                          date_created=DEFAULT, description=None,
                          target_default=False, owner_default=False,
-                         with_hosting=False, status=None):
+                         with_hosting=False, async_hosting=False,
+                         status=GitRepositoryStatus.AVAILABLE):
         """See `IGitNamespace`."""
         repository_set = getUtility(IGitRepositorySet)
 
@@ -119,11 +122,20 @@ class _BaseGitNamespace:
 
             # Flush to make sure that repository.id is populated.
             IStore(repository).flush()
+            if async_hosting:
+                # If we are going to run async creation, we need to be sure
+                # the transaction is committed.
+                # Async creation will run a callback on Launchpad, and if
+                # the creation is quick enough, it might try to confirm on
+                # Launchpad (in another transaction) the creation of this
+                # repo before this transaction is actually committed.
+                transaction.commit()
             assert repository.id is not None
 
             clone_from_repository = repository.getClonedFrom()
             repository._createOnHostingService(
-                clone_from_repository=clone_from_repository)
+                clone_from_repository=clone_from_repository,
+                async_create=async_hosting)
 
         return repository
 
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 3d2408e..726e9f1 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -359,7 +359,8 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         self.owner_default = False
         self.target_default = False
 
-    def _createOnHostingService(self, clone_from_repository=None):
+    def _createOnHostingService(
+            self, clone_from_repository=None, async_create=False):
         """Create this repository on the hosting service."""
         hosting_path = self.getInternalPath()
         if clone_from_repository is not None:
@@ -367,7 +368,8 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         else:
             clone_from_path = None
         getUtility(IGitHostingClient).create(
-            hosting_path, clone_from=clone_from_path)
+            hosting_path, clone_from=clone_from_path,
+            async_create=async_create)
 
     def getClonedFrom(self):
         """See `IGitRepository`"""
@@ -1729,13 +1731,27 @@ class GitRepositorySet:
 
     def new(self, repository_type, registrant, owner, target, name,
             information_type=None, date_created=DEFAULT, description=None,
-            with_hosting=False):
+            with_hosting=False, async_hosting=False,
+            status=GitRepositoryStatus.AVAILABLE):
         """See `IGitRepositorySet`."""
         namespace = get_git_namespace(target, owner)
         return namespace.createRepository(
             repository_type, registrant, name,
             information_type=information_type, date_created=date_created,
-            description=description, with_hosting=with_hosting)
+            description=description, with_hosting=with_hosting,
+            async_hosting=async_hosting, status=status)
+
+    def fork(self, origin, requester, new_owner):
+        namespace = get_git_namespace(origin.target, new_owner)
+        name = namespace.findUnusedName(origin.name)
+        repository = self.new(
+            repository_type=GitRepositoryType.HOSTED,
+            registrant=requester, owner=new_owner, target=origin.target,
+            name=name, information_type=origin.information_type,
+            date_created=UTC_NOW, description=origin.description,
+            with_hosting=True, async_hosting=True,
+            status=GitRepositoryStatus.CREATING)
+        return repository
 
     def getByPath(self, user, path):
         """See `IGitRepositorySet`."""
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index d966dbe..b35fdd7 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for `GitHostingClient`.
@@ -23,7 +23,6 @@ from lazr.restful.utils import get_current_browser_request
 import responses
 from six.moves.urllib.parse import (
     parse_qsl,
-    urljoin,
     urlsplit,
     )
 from testtools.matchers import (
@@ -130,6 +129,15 @@ class TestGitHostingClient(TestCase):
             "repo", method="POST",
             json_data={"repo_path": "123", "clone_from": "122"})
 
+    def test_create_async(self):
+        with self.mockRequests("POST"):
+            self.client.create("123", clone_from="122", async_create=True)
+        self.assertRequest(
+            "repo", method="POST",
+            json_data={
+                "repo_path": "123", "clone_from": "122", "async": True,
+                "clone_refs": True})
+
     def test_create_failure(self):
         with self.mockRequests("POST", status=400):
             self.assertRaisesWithContent(
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index 5964944..d4bfe20 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `GitJob`s."""
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 62bf865..7888d2f 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -2729,6 +2729,38 @@ class TestGitRepositoryMarkSnapsStale(TestCaseWithFactory):
         self.assertFalse(snap.is_stale)
 
 
+class TestGitRepositoryFork(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositoryFork, self).setUp()
+        self.hosting_fixture = self.useFixture(GitHostingFixture())
+
+    def test_fork(self):
+        repo = self.factory.makeGitRepository()
+        another_person = self.factory.makePerson()
+        another_team = self.factory.makeTeam(members=[another_person])
+
+        forked_repo = getUtility(IGitRepositorySet).fork(
+            repo, another_person, another_team)
+        self.assertEqual(another_team, forked_repo.owner)
+        self.assertEqual(another_person, forked_repo.registrant)
+        self.assertEqual(1, self.hosting_fixture.create.call_count)
+
+    def test_fork_same_name(self):
+        repo = self.factory.makeGitRepository()
+
+        person = self.factory.makePerson()
+        same_name_repo = self.factory.makeGitRepository(
+            owner=person, registrant=person,
+            name=repo.name, target=repo.target)
+
+        forked_repo = getUtility(IGitRepositorySet).fork(repo, person, person)
+        self.assertEqual(forked_repo.target, repo.target)
+        self.assertEqual(forked_repo.name, "%s-1" % same_name_repo.name)
+
+
 class TestGitRepositoryDetectMerges(TestCaseWithFactory):
 
     layer = ZopelessDatabaseLayer
@@ -3271,7 +3303,8 @@ class TestGitRepositorySet(TestCaseWithFactory):
         self.assertThat(repository, MatchesStructure.byEquality(
             registrant=owner, owner=owner, target=target, name=name))
         self.assertEqual(
-            [((repository.getInternalPath(),), {"clone_from": None})],
+            [((repository.getInternalPath(),),
+              {'async_create': False, "clone_from": None})],
             hosting_fixture.create.calls)
 
     def test_provides_IGitRepositorySet(self):
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index a4f8416..4ef843d 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -355,7 +355,7 @@ class GitHostingFixture(fixtures.Fixture):
                  merges=None, blob=None, disable_memcache=True):
         self.create = FakeMethod()
         self.getProperties = FakeMethod(
-            result={"default_branch": default_branch})
+            result={"default_branch": default_branch, "is_available": True})
         self.setProperties = FakeMethod()
         self.getRefs = FakeMethod(result=({} if refs is None else refs))
         self.getCommits = FakeMethod(
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 0c3a2f1..c148acc 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -627,6 +627,15 @@ class GitAPI(LaunchpadXMLRPCView):
         if requester == LAUNCHPAD_ANONYMOUS:
             requester = None
 
+        if naked_repo.status != GitRepositoryStatus.CREATING:
+            raise faults.Unauthorized()
+
+        if requester == LAUNCHPAD_SERVICES and "macaroon" not in auth_params:
+            # For repo creation management operations, we trust
+            # LAUNCHPAD_SERVICES, since it should be just an internal call
+            # to confirm/abort repository creation.
+            return
+
         verified = self._verifyAuthParams(requester, repository, auth_params)
         if verified is not None and verified.user is NO_USER:
             # For internal-services authentication, we check if its using a
@@ -642,9 +651,6 @@ class GitAPI(LaunchpadXMLRPCView):
             if requester != naked_repo.registrant:
                 raise faults.Unauthorized()
 
-        if naked_repo.status != GitRepositoryStatus.CREATING:
-            raise faults.Unauthorized()
-
     def _confirmRepoCreation(self, requester, translated_path, auth_params):
         naked_repo = removeSecurityProxy(
             getUtility(IGitLookup).getByHostingPath(translated_path))
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index c3b8126..9fb9b2e 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -401,9 +401,10 @@ class TestGitAPIMixin:
             expected_status = GitRepositoryStatus.AVAILABLE
             expected_hosting_calls = 1
             expected_hosting_call_args = [(repository.getInternalPath(),)]
-            expected_hosting_call_kwargs = [
-                {"clone_from": (cloned_from.getInternalPath()
-                                if cloned_from else None)}]
+            expected_hosting_call_kwargs = [{
+                "clone_from": (cloned_from.getInternalPath()
+                                if cloned_from else None),
+                "async_create": False}]
 
         self.assertEqual(GitRepositoryType.HOSTED, repository.repository_type)
         self.assertEqual(expected_translation, translation)
@@ -780,6 +781,12 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         repo.status = GitRepositoryStatus.CREATING
         self.assertConfirmsRepoCreation(owner, repo)
 
+    def test_launchpad_service_confirm_git_repository_creation(self):
+        owner = self.factory.makePerson()
+        repo = removeSecurityProxy(self.factory.makeGitRepository(owner=owner))
+        repo.status = GitRepositoryStatus.CREATING
+        self.assertConfirmsRepoCreation(LAUNCHPAD_SERVICES, repo)
+
     def test_only_requester_can_confirm_git_repository_creation(self):
         requester = self.factory.makePerson()
         repo = removeSecurityProxy(self.factory.makeGitRepository())
@@ -961,6 +968,12 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         repo.status = GitRepositoryStatus.CREATING
         self.assertAbortsRepoCreation(requester, repo)
 
+    def test_launchpad_service_abort_git_repository_creation(self):
+        owner = self.factory.makePerson()
+        repo = removeSecurityProxy(self.factory.makeGitRepository(owner=owner))
+        repo.status = GitRepositoryStatus.CREATING
+        self.assertAbortsRepoCreation(LAUNCHPAD_SERVICES, repo)
+
     def test_only_requester_can_abort_git_repository_creation(self):
         requester = self.factory.makePerson()
         repo = removeSecurityProxy(self.factory.makeGitRepository())