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"]
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp