launchpad-reviewers team mailing list archive

[Bowen Fan <mp+452540@xxxxxxxxxxxxxxxxxx>]

Bowen Fan has proposed merging ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master.

Commit message:
Add ref creation POST endpoint to turnip-api

The "collection_post" endpoint is mounted at "/repo/{name}/refs".
It requires the ref name, e.g. "/heads/new-feature-branch" and 
the target commit SHA1. It accepts an optional "force" flag to
force overwrite an existing ref (if any).

Requested reviews:
  Gravity Yang (gravi1984)
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~bowenfan/turnip/+git/turnip/+merge/452540

Please see jira ticket and its parent epic for more context: https://warthogs.atlassian.net/browse/SN-2052

Thanks!
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~bowenfan/turnip:SN2052-add_ref_creation_endpoint_to_turnip_api into turnip:master.

diff --git a/turnip/api/store.py b/turnip/api/store.py
index ae897c2..aa52860 100644
--- a/turnip/api/store.py
+++ b/turnip/api/store.py
@@ -17,8 +17,16 @@ from pygit2 import (
     GIT_OBJ_TAG,
     GIT_REF_OID,
     GIT_SORT_TOPOLOGICAL,
+)
+
+# Some irony in that this error name already exists and is used to signify
+# an existing repo. It inherits from GitError. pygit2's AlreadyExistsError
+# is more appropriate for objects (e.g. references) and we rename it thus.
+from pygit2 import AlreadyExistsError as ObjectAlreadyExistsError
+from pygit2 import (
     GitError,
     IndexEntry,
+    InvalidSpecError,
     Oid,
     Repository,
     init_repository,
@@ -555,6 +563,45 @@ def get_ref(repo_store, repo_name, ref):
         return ref_obj
 
 
+def create_references(repo_store, repo_name, refs_to_create):
+    """Create a git reference against a given commit sha1.
+
+    :param refs_to_create: List of ref creation requests. Each request is a
+        dict containing the ref name "ref" and the "commit_sha1" against
+        which to create the ref. An optional a "force" key is accepted;
+        if passed, an existing ref will be overwritten on conflict.
+    :return: 2 dicts: created and errors. Created takes the form
+        {ref:commit_sha1} and errors take the form {ref:err_msg}.
+    """
+    created = {}
+    errors = {}
+    with open_repo(repo_store, repo_name) as repo:
+        for ref_to_create in refs_to_create:
+            # Validated in the API layer
+            # Ref names are validated to be unique so we can use them as keys
+            ref = ref_to_create["ref"]
+            commit_sha1 = ref_to_create["commit_sha1"]
+            force = ref_to_create.get("force", False)
+
+            try:
+                repo.create_reference(ref, commit_sha1, force)
+            # The payload might contain multiple errors but pygit2 will only
+            # raise the last one. For such edge cases the client may need to
+            # retry more than once.
+            except InvalidSpecError:
+                errors[ref] = f"Invalid ref name '{ref}'"
+            except GitError:
+                errors[ref] = f"Commit '{commit_sha1}' not found"
+            except ObjectAlreadyExistsError:
+                errors[ref] = (
+                    f"Ref '{ref}' already exists, "
+                    "retry with force to overwrite"
+                )
+            else:
+                created[ref] = commit_sha1
+        return created, errors
+
+
 def get_common_ancestor_diff(
     repo_store, repo_name, sha1_target, sha1_source, context_lines=3
 ):
diff --git a/turnip/api/tests/test_api.py b/turnip/api/tests/test_api.py
index 1513618..68b56d9 100644
--- a/turnip/api/tests/test_api.py
+++ b/turnip/api/tests/test_api.py
@@ -241,6 +241,240 @@ class ApiTestCase(TestCase, ApiRepoStoreMixin):
         resp = self.app.get("/repo/1/refs", expect_errors=True)
         self.assertEqual(404, resp.status_code)
 
+    def test_repo_create_ref_request_body_validation(self):
+        factory = RepoFactory(self.repo_store)
+        commit_oid = factory.add_commit("foo", "foobar.txt")
+
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs", [], expect_errors=True
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "Empty request",
+            resp.text,
+        )
+
+        missing_sha_1 = [{"ref": "1701"}]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs", missing_sha_1, expect_errors=True
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "Missing commit_sha1 for ref target",
+            resp.text,
+        )
+
+        missing_ref_name = [
+            {
+                "commit_sha1": commit_oid.hex,
+            }
+        ]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs",
+            missing_ref_name,
+            expect_errors=True,
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "Missing ref name",
+            resp.text,
+        )
+
+        duplicate_ref_name = [
+            {
+                "commit_sha1": commit_oid.hex,
+                "ref": "refs/tags/1701",
+            },
+            {
+                "commit_sha1": commit_oid.hex,
+                "ref": "refs/tags/1701",
+            },
+        ]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs",
+            duplicate_ref_name,
+            expect_errors=True,
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "Duplicate ref 'refs/tags/1701' in request",
+            resp.text,
+        )
+
+        wrong_ref_name_format = [
+            {
+                "commit_sha1": commit_oid.hex,
+                "ref": "tags/1701",
+            }
+        ]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs",
+            wrong_ref_name_format,
+            expect_errors=True,
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "Invalid ref format. Ref must start with 'refs/'",
+            resp.text,
+        )
+
+        bad_force_optiom = [
+            {
+                "commit_sha1": commit_oid.hex,
+                "ref": "refs/tags/1701",
+                "force": "y",
+            }
+        ]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs",
+            bad_force_optiom,
+            expect_errors=True,
+        )
+        self.assertEqual(400, resp.status_code)
+        self.assertIn(
+            "'force' must be a json boolean",
+            resp.text,
+        )
+
+    def test_repo_create_single_ref(self):
+        factory = RepoFactory(self.repo_store)
+        commit_sha1 = factory.add_commit("foo", "foobar.txt").hex
+        tag_name = "refs/tags/1701"
+        branch_name = "refs/heads/new-feature"
+
+        for ref in [tag_name, branch_name]:
+            body = [
+                {
+                    "ref": ref,
+                    "commit_sha1": commit_sha1,
+                }
+            ]
+            resp = self.app.post_json(f"/repo/{self.repo_path}/refs", body)
+            self.assertEqual(201, resp.status_code)
+            self.assertEqual({ref: commit_sha1}, resp.json["created"])
+            self.assertEqual({}, resp.json["errors"])
+
+    def test_repo_force_overwrite_ref(self):
+        factory = RepoFactory(self.repo_store)
+        commit_oid = factory.add_commit("foo", "foobar.txt")
+        factory.add_tag("1701", "", commit_oid)
+        factory.add_branch("new-feature", commit_oid)
+        tag_name = "refs/tags/1701"
+        branch_name = "refs/heads/branch-new-feature"
+
+        for ref in [tag_name, branch_name]:
+            body = [
+                {
+                    "commit_sha1": commit_oid.hex,
+                    "ref": ref,
+                    "force": True,
+                }
+            ]
+            resp = self.app.post_json(f"/repo/{self.repo_path}/refs", body)
+            self.assertEqual(201, resp.status_code)
+            self.assertEqual({ref: commit_oid.hex}, resp.json["created"])
+            self.assertEqual({}, resp.json["errors"])
+
+    def test_repo_create_multiple_mixed_success_and_errors(self):
+        factory = RepoFactory(self.repo_store)
+
+        expected_created = []
+        refs_to_create = []
+        commits = []
+        # Expected successful cases: 5 commits with a tag and a branch ref
+        # created against each commit.
+        for i in range(5):
+            commit = factory.add_commit(f"foo-{i}", f"foobar-{i}.txt")
+            commits.append(commit)
+            commit = commit.hex
+
+            tag = f"refs/tags/{i}"
+            refs_to_create.append({"ref": tag, "commit_sha1": commit})
+            expected_created.append((tag, commit))
+
+            branch = f"refs/heads/new-feature-{i}"
+            refs_to_create.append({"ref": branch, "commit_sha1": commit})
+            expected_created.append((branch, commit))
+
+        expected_errors = []
+        # Invalid ref name
+        invalid_name = "refs/tags/?*"
+        refs_to_create.append(
+            {
+                "ref": invalid_name,
+                "commit_sha1": commits[0].hex,
+            }
+        )
+        expected_errors.append((invalid_name, commits[0].hex))
+
+        # Commit does not exist
+        nonexistent_commit = factory.nonexistent_oid()
+        nonexistent_commit_ref = "refs/tags/nonexistent-commit"
+        refs_to_create.append(
+            {
+                "ref": nonexistent_commit_ref,
+                "commit_sha1": nonexistent_commit,
+            }
+        )
+        expected_errors.append((nonexistent_commit_ref, nonexistent_commit))
+
+        # Ref already exists and force is not passed
+        factory.add_branch("existing", commits[0])
+        existing_ref = "refs/heads/branch-existing"
+        refs_to_create.append(
+            {
+                "ref": existing_ref,
+                "commit_sha1": commits[0].hex,
+            }
+        )
+        expected_errors.append((existing_ref, commits[0].hex))
+
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs", refs_to_create
+        )
+        created = resp.json["created"]
+        errors = resp.json["errors"]
+
+        for entry in expected_created:
+            # Assert {ref:commit} key value pair for successful cases
+            self.assertEqual(entry[1], created[entry[0]])
+
+        self.assertEqual(
+            f"Invalid ref name '{invalid_name}'", errors[invalid_name]
+        )
+        self.assertEqual(
+            f"Commit '{nonexistent_commit}' not found",
+            errors[nonexistent_commit_ref],
+        )
+        self.assertEqual(
+            (
+                f"Ref '{existing_ref}' already exists, "
+                "retry with force to overwrite"
+            ),
+            errors[existing_ref],
+        )
+
+    def test_repo_create_refs_only_errors(self):
+        factory = RepoFactory(self.repo_store)
+        tag_name = "refs/tags/1701"
+        nonexistent_commit = factory.nonexistent_oid()
+        body = [
+            {
+                "ref": tag_name,
+                "commit_sha1": factory.nonexistent_oid(),
+            }
+        ]
+        resp = self.app.post_json(
+            f"/repo/{self.repo_path}/refs", body, expect_errors=True
+        )
+        # 400 response code if nothing is created and we have only errors
+        self.assertEqual(400, resp.status_code)
+        self.assertEqual(
+            {tag_name: f"Commit '{nonexistent_commit}' not found"},
+            resp.json["errors"],
+        )
+        self.assertEqual({}, resp.json["created"])
+
     def test_ignore_non_unicode_refs(self):
         """Ensure non-unicode refs are dropped from ref collection."""
         factory = RepoFactory(self.repo_store)
diff --git a/turnip/api/tests/test_store.py b/turnip/api/tests/test_store.py
index 47a97da..4eb09a0 100644
--- a/turnip/api/tests/test_store.py
+++ b/turnip/api/tests/test_store.py
@@ -8,6 +8,7 @@ import uuid
 
 import pygit2
 import yaml
+
 from fixtures import EnvironmentVariable, MonkeyPatch, TempDir
 from testtools import TestCase
 
@@ -391,6 +392,131 @@ class InitTestCase(TestCase):
             packed_refs, os.path.join(too_path, "turnip-subordinate")
         )
 
+    def test_create_single_ref(self):
+        repo_path = os.path.join(self.repo_store, uuid.uuid1().hex)
+        factory = RepoFactory(repo_path)
+        commit_sha1 = factory.add_commit("foo", "foobar.txt").hex
+        tag_name = "refs/tags/1701"
+        branch_name = "refs/heads/new-feature"
+
+        for ref in [tag_name, branch_name]:
+            refs_to_create = [{"ref": ref, "commit_sha1": commit_sha1}]
+            created, _ = store.create_references(
+                self.repo_store, repo_path, refs_to_create
+            )
+            assert created[ref] == commit_sha1
+            self.assertAdvertisedRefs([(ref, commit_sha1)], [], repo_path)
+
+    def test_create_multiple_mixed_success_and_errors(self):
+        repo_path = os.path.join(self.repo_store, uuid.uuid1().hex)
+        factory = RepoFactory(repo_path)
+
+        expected_created = []
+        refs_to_create = []
+        # Expected successful cases: 5 commits with a tag and a branch ref
+        # created against each commit.
+        for i in range(5):
+            commit = factory.add_commit(f"foo-{i}", f"foobar-{i}.txt").hex
+
+            tag = f"refs/tags/{i}"
+            refs_to_create.append({"ref": tag, "commit_sha1": commit})
+            expected_created.append((tag, commit))
+
+            branch = f"refs/heads/new-feature-{i}"
+            refs_to_create.append({"ref": branch, "commit_sha1": commit})
+            expected_created.append((branch, commit))
+
+        expected_errors = []
+        # Invalid ref name
+        invalid_name = "refs/tags/?*"
+        refs_to_create.append(
+            {
+                "ref": invalid_name,
+                "commit_sha1": refs_to_create[0]["commit_sha1"],
+            }
+        )
+        expected_errors.append(
+            (invalid_name, refs_to_create[0]["commit_sha1"])
+        )
+
+        # Commit does not exist
+        nonexistent_commit = factory.nonexistent_oid()
+        nonexistent_commit_ref = "refs/tags/nonexistent-commit"
+        refs_to_create.append(
+            {
+                "ref": nonexistent_commit_ref,
+                "commit_sha1": nonexistent_commit,
+            }
+        )
+        expected_errors.append((nonexistent_commit_ref, nonexistent_commit))
+
+        # Ref already exists and force is not passed
+        existing_ref = refs_to_create[0]["ref"]
+        refs_to_create.append(
+            {
+                "ref": existing_ref,
+                "commit_sha1": refs_to_create[0]["commit_sha1"],
+            }
+        )
+        expected_errors.append(
+            (existing_ref, refs_to_create[0]["commit_sha1"])
+        )
+
+        created, errors = store.create_references(
+            self.repo_store, repo_path, refs_to_create
+        )
+
+        for entry in expected_created:
+            # Assert {ref:commit} key value pair for successful cases
+            self.assertEqual(entry[1], created[entry[0]])
+
+        self.assertEqual(
+            f"Invalid ref name '{invalid_name}'", errors[invalid_name]
+        )
+        self.assertEqual(
+            f"Commit '{nonexistent_commit}' not found",
+            errors[nonexistent_commit_ref],
+        )
+        self.assertEqual(
+            (
+                f"Ref '{existing_ref}' already exists, "
+                "retry with force to overwrite"
+            ),
+            errors[existing_ref],
+        )
+
+        # Verify successful cases are created and errorneous ones are not
+        # on the git level
+        self.assertAdvertisedRefs(expected_created, expected_errors, repo_path)
+
+    def test_force_overwrite_ref(self):
+        repo_path = os.path.join(self.repo_store, uuid.uuid1().hex)
+        factory = RepoFactory(repo_path)
+        first_commit_sha1 = factory.add_commit("foo", "foobar.txt").hex
+        tag_name = "refs/tags/1701"
+        branch_name = "refs/heads/new-feature"
+
+        for ref in [tag_name, branch_name]:
+            refs_to_create = [
+                {"ref": ref, "commit_sha1": first_commit_sha1, "force": False}
+            ]
+            store.create_references(self.repo_store, repo_path, refs_to_create)
+            second_commit_oid = factory.add_commit("bar", "barbaz.txt")
+            second_commit_sha1 = second_commit_oid.hex
+            refs_to_create = [
+                {"ref": ref, "commit_sha1": second_commit_sha1, "force": True}
+            ]
+            created, _ = store.create_references(
+                self.repo_store, repo_path, refs_to_create
+            )
+
+            assert created[ref] == second_commit_sha1
+            self.assertAdvertisedRefs(
+                [(ref, second_commit_sha1)],  # in refs
+                [(ref, first_commit_sha1)],  # not in refs
+                repo_path,
+            )
+
     def test_fetch_refs(self):
         celery_fixture = CeleryWorkerFixture()
         self.useFixture(celery_fixture)
diff --git a/turnip/api/views.py b/turnip/api/views.py
index 5014634..9602df0 100644
--- a/turnip/api/views.py
+++ b/turnip/api/views.py
@@ -1,6 +1,7 @@
 # Copyright 2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import json
 import os
 import re
 
@@ -264,6 +265,84 @@ class RefAPI(BaseAPI):
         except (KeyError, GitError):
             return exc.HTTPNotFound()  # 404
 
+    def _validate_refs_api_payload(self, refs_to_create):
+        ref_set = set()
+        if not refs_to_create:
+            self.request.errors.add(
+                "body",
+                description="Empty request",
+            )
+            return
+
+        for ref_to_create in refs_to_create:
+            ref = ref_to_create.get("ref")
+            if not ref:
+                self.request.errors.add(
+                    "body",
+                    "ref",
+                    "Missing ref name",
+                )
+                return False
+            if ref in ref_set:
+                self.request.errors.add(
+                    "body",
+                    "ref",
+                    f"Duplicate ref '{ref}' in request",
+                )
+                return False
+            ref_set.add(ref)
+            commit_sha1 = ref_to_create.get("commit_sha1")
+            if not commit_sha1:
+                self.request.errors.add(
+                    "body", "commit_sha1", "Missing commit_sha1 for ref target"
+                )
+                return False
+            if not (isinstance(ref, str) and ref.startswith("refs/")):
+                self.request.errors.add(
+                    "body",
+                    "ref",
+                    "Invalid ref format. Ref must start with 'refs/'",
+                )
+                return False
+            force = ref_to_create.get("force", False)
+            if force and not isinstance(force, bool):
+                self.request.errors.add(
+                    "body", "force", "'force' must be a json boolean"
+                )
+                return False
+        return True
+
+    @validate_path
+    def collection_post(self, repo_store, repo_name):
+        """Bulk create git refs against corresponding commits.
+
+        The JSON request body should be a list of creation request dicts.
+        Each dict should contain the ref name "ref" and the "commit_sha1"
+        against which to create the ref. An optional a "force" key is
+        accepted; if passed, an existing ref will be overwritten on conflict.
+
+        The JSON response body contains 2 dicts: resp.json["created"] is a dict
+        of successful requests in the form {ref:commit_sha1} and
+        respo.json["errors"] is a dict of error messages in the form
+        {ref:err_msg}.
+        """
+        refs_to_create = extract_cstruct(self.request)["body"]
+
+        if not self._validate_refs_api_payload(refs_to_create):
+            return
+
+        created, errors = store.create_references(
+            repo_store, repo_name, refs_to_create
+        )
+        resp = {"created": created, "errors": errors}
+        resp = json.dumps(resp).encode()
+
+        return (
+            Response(resp, status=201, content_type="application/json")
+            if created
+            else Response(resp, status=400, content_type="application/json")
+        )
+
     @validate_path
     def get(self, repo_store, repo_name):
         ref = "refs/" + self.request.matchdict["ref"]