From 570c122dc569fa15788280f2c1810d3862db36b8 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 19 2020 15:56:28 +0000 Subject: [PATCH 1/4] Handle attempts to create an existing git tag If someone attempts to create a git tag that already exists, pygit2 will raise an error that we are now catching. To distinguish if the git tag that was asked to be created was created or not, we also include in the JSON returned a ``tag_created`` field that is a boolean indicating if the tag was created or not (not created in this case means that it already existed). Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/project.py b/pagure/api/project.py index 8f2bb72..768a3f9 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -17,6 +17,11 @@ from sqlalchemy.exc import SQLAlchemyError from six import string_types from pygit2 import GitError, Repository +try: + from pygit2 import AlreadyExistsError +except ImportError: + AlreadyExistsError = ValueError + import pagure import pagure.forms import pagure.exceptions @@ -356,6 +361,9 @@ def api_new_git_tags(repo, username=None, namespace=None): Create new git tags ------------------- Create a new tag on the project Git repository. + If the request tried to create a git tag that already existed, the JSON + returned will include ``"tag_created": false``, otherwise, this field will + be ``true``. :: @@ -397,6 +405,7 @@ def api_new_git_tags(repo, username=None, namespace=None): { "total_tags": 2, "tags": ["0.0.1", "0.0.2"], + "tag_created": true, } @@ -406,6 +415,7 @@ def api_new_git_tags(repo, username=None, namespace=None): "0.0.1": "bb8fa2aa199da08d6085e1c9badc3d83d188d38c", "0.0.2": "d16fe107eca31a1bdd66fb32c6a5c568e45b627e" }, + "tag_created": false, } """ @@ -417,6 +427,7 @@ def api_new_git_tags(repo, username=None, namespace=None): ) form = pagure.forms.AddGitTagForm(csrf_enabled=False) + created = None if form.validate_on_submit(): user_obj = pagure.lib.query.get_user( flask.g.session, flask.g.fas_user.username @@ -429,6 +440,9 @@ def api_new_git_tags(repo, username=None, namespace=None): user=user_obj, message=form.message.data, ) + created = True + except AlreadyExistsError: + created = False except GitError as err: _log.exception(err) raise pagure.exceptions.APIError( @@ -442,7 +456,9 @@ def api_new_git_tags(repo, username=None, namespace=None): tags = pagure.lib.git.get_git_tags(repo, with_commits=with_commits) - jsonout = flask.jsonify({"total_tags": len(tags), "tags": tags}) + jsonout = flask.jsonify( + {"total_tags": len(tags), "tags": tags, "tag_created": created} + ) return jsonout diff --git a/tests/test_pagure_flask_api_project_git_tags.py b/tests/test_pagure_flask_api_project_git_tags.py index bfd90db..13ea6f0 100644 --- a/tests/test_pagure_flask_api_project_git_tags.py +++ b/tests/test_pagure_flask_api_project_git_tags.py @@ -96,9 +96,12 @@ class PagureFlaskApiProjectGitTagstests(tests.Modeltests): ) self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ["tags", "total_tags"]) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) self.assertEqual(data["tags"], ["test-tag-no-message"]) self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) output = self.app.get("/api/0/test/git/tags?with_commits=t") self.assertEqual(output.status_code, 200) @@ -135,11 +138,14 @@ class PagureFlaskApiProjectGitTagstests(tests.Modeltests): ) self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ["tags", "total_tags"]) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) self.assertEqual( data["tags"], {"test-tag-no-message": latest_commit.oid.hex} ) self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) def test_api_new_git_tag_with_message(self): """ Test the api_new_git_tags function. """ @@ -166,9 +172,63 @@ class PagureFlaskApiProjectGitTagstests(tests.Modeltests): ) self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) + self.assertEqual(data["tags"], ["test-tag-no-message"]) + self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) + + def test_api_new_git_tag_with_message_twice(self): + """ Test the api_new_git_tags function. """ + + # Before + output = self.app.get("/api/0/test/git/tags") + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) self.assertEqual(sorted(data.keys()), ["tags", "total_tags"]) + self.assertEqual(data["tags"], []) + self.assertEqual(data["total_tags"], 0) + + # Add a tag so that we can list it + repo = pygit2.Repository(os.path.join(self.path, "repos", "test.git")) + latest_commit = repo.revparse_single("HEAD") + data = { + "tagname": "test-tag-no-message", + "commit_hash": latest_commit.oid.hex, + "message": "This is a long annotation\nover multiple lines\n for testing", + } + + output = self.app.post( + "/api/0/test/git/tags", headers=self.headers, data=data + ) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) self.assertEqual(data["tags"], ["test-tag-no-message"]) self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) + + # Submit the same request/tag a second time to the same commit + data = { + "tagname": "test-tag-no-message", + "commit_hash": latest_commit.oid.hex, + "message": "This is a long annotation\nover multiple lines\n for testing", + } + + output = self.app.post( + "/api/0/test/git/tags", headers=self.headers, data=data + ) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) + self.assertEqual(data["tags"], ["test-tag-no-message"]) + self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], False) def test_api_new_git_tag_user_no_access(self): """ Test the api_new_git_tags function. """ @@ -240,6 +300,9 @@ class PagureFlaskApiProjectGitTagstests(tests.Modeltests): ) self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ["tags", "total_tags"]) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) self.assertEqual(data["tags"], ["test-tag-no-message"]) self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) From 28bce03ba33b6f020179c59f84aeca52b31bb383 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 19 2020 15:56:28 +0000 Subject: [PATCH 2/4] Allow to force the creation of a git tag Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/project.py b/pagure/api/project.py index 768a3f9..2fc9fc4 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -396,6 +396,13 @@ def api_new_git_tags(repo, username=None, namespace=None): | | | | tags found in the repo | | | | | in the data returned | +-----------------+----------+---------------+--------------------------+ + | ``force`` | boolean | Optional | | If a similar git tag | + | | | | already exists, remove | + | | | | it from the repo and | + | | | | create the specified | + | | | | one, thus forcing it | + +-----------------+----------+---------------+--------------------------+ + Sample response ^^^^^^^^^^^^^^^ @@ -439,6 +446,7 @@ def api_new_git_tags(repo, username=None, namespace=None): target=form.commit_hash.data, user=user_obj, message=form.message.data, + force=form.force.data, ) created = True except AlreadyExistsError: diff --git a/pagure/forms.py b/pagure/forms.py index a41a278..228a020 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -946,3 +946,8 @@ class AddGitTagForm(PagureForm): message = wtforms.TextAreaField( "Annotation message", [wtforms.validators.Optional()] ) + force = wtforms.BooleanField( + "Force the creation of the git tag", + [wtforms.validators.optional()], + false_values=FALSE_VALUES, + ) diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 4a5a400..4cae87d 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -2352,7 +2352,7 @@ def get_git_tags(project, with_commits=False): return tags -def new_git_tag(project, tagname, target, user, message=None): +def new_git_tag(project, tagname, target, user, message=None, force=False): """ Create a new git tag in the git repositorie of the specified project. :arg project: the project in which we want to create a git tag @@ -2363,6 +2363,9 @@ def new_git_tag(project, tagname, target, user, message=None): :type user: pagure.lib.model.User :kwarg message: the message to include in the annotation of the tag :type message: str or None + :kwarg force: a boolean specifying wether to force the creation of + the git tag or not + :type message: bool """ repopath = pagure.utils.get_repo_path(project) repo_obj = PagureRepo(repopath) @@ -2371,6 +2374,11 @@ def new_git_tag(project, tagname, target, user, message=None): if not target_obj: raise pygit2.GitError("Unknown target: %s" % target) + if force: + existing_tag = repo_obj.lookup_reference("refs/tags/%s" % tagname) + if existing_tag: + existing_tag.delete() + tag = repo_obj.create_tag( tagname, target, diff --git a/tests/test_pagure_flask_api_project_git_tags.py b/tests/test_pagure_flask_api_project_git_tags.py index 13ea6f0..53c0c68 100644 --- a/tests/test_pagure_flask_api_project_git_tags.py +++ b/tests/test_pagure_flask_api_project_git_tags.py @@ -306,3 +306,60 @@ class PagureFlaskApiProjectGitTagstests(tests.Modeltests): self.assertEqual(data["tags"], ["test-tag-no-message"]) self.assertEqual(data["total_tags"], 1) self.assertEqual(data["tag_created"], True) + + def test_api_new_git_tag_forced(self): + """ Test the api_new_git_tags function. """ + + # Before + output = self.app.get("/api/0/test/git/tags") + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(sorted(data.keys()), ["tags", "total_tags"]) + self.assertEqual(data["tags"], []) + self.assertEqual(data["total_tags"], 0) + + # Add a tag so that we can list it + repo = pygit2.Repository(os.path.join(self.path, "repos", "test.git")) + latest_commit = repo.revparse_single("HEAD") + prev_commit = latest_commit.parents[0].oid.hex + data = { + "tagname": "test-tag-no-message", + "commit_hash": prev_commit, + "message": "This is a long annotation\nover multiple lines\n for testing", + "with_commits": True, + } + + output = self.app.post( + "/api/0/test/git/tags", headers=self.headers, data=data + ) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) + self.assertEqual(data["tags"], {"test-tag-no-message": prev_commit}) + self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) + + # Submit the same request/tag a second time to the same commit + data = { + "tagname": "test-tag-no-message", + "commit_hash": latest_commit.oid.hex, + "message": "This is a long annotation\nover multiple lines\n for testing", + "with_commits": True, + "force": True, + } + + output = self.app.post( + "/api/0/test/git/tags", headers=self.headers, data=data + ) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + sorted(data.keys()), ["tag_created", "tags", "total_tags"] + ) + self.assertEqual( + data["tags"], {"test-tag-no-message": latest_commit.oid.hex} + ) + self.assertEqual(data["total_tags"], 1) + self.assertEqual(data["tag_created"], True) From 1e8197841a3826651823a62b27e59e5c5081f054 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 19 2020 15:56:28 +0000 Subject: [PATCH 3/4] Allow blocking updating git tags via the API For example, in the case of src.fedoraproject.org we do not want people to update/move git tags that were created, so for this pagure instance this feature will be turned off. Signed-off-by: Pierre-Yves Chibon --- diff --git a/doc/configuration.rst b/doc/configuration.rst index 23118f2..a561b88 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -1714,6 +1714,18 @@ the highlighting language/category to use as values. Defaults to: ``{".spec": "specfile", ".patch": "diff"}`` +ALLOW_API_UPDATE_GIT_TAGS +~~~~~~~~~~~~~~~~~~~~~~~~~ + +This configuration key determines whether users are allowed to update +existing git tags via the API. +When set to ``False``, this essentially makes the API ignore whether the +``force`` argument is set or not. + + +Default to: ``True`` + + RepoSpanner Options ------------------- diff --git a/pagure/api/project.py b/pagure/api/project.py index 2fc9fc4..31919a0 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -439,6 +439,10 @@ def api_new_git_tags(repo, username=None, namespace=None): user_obj = pagure.lib.query.get_user( flask.g.session, flask.g.fas_user.username ) + force = form.force.data + if not pagure_config.get("ALLOW_API_UPDATE_GIT_TAGS", True): + force = False + try: pagure.lib.git.new_git_tag( project=repo, @@ -446,7 +450,7 @@ def api_new_git_tags(repo, username=None, namespace=None): target=form.commit_hash.data, user=user_obj, message=form.message.data, - force=form.force.data, + force=force, ) created = True except AlreadyExistsError: From 6e9a29c33e59ee3283aa7bad92055cf20d7efc1e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 19 2020 15:56:59 +0000 Subject: [PATCH 4/4] Adjust the unit-tests for markdown 3.2.1 It looks like markdown 3.2.1 changed its behavior again, now some of the characters are escaped where they were not before. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/project.py b/pagure/api/project.py index 31919a0..c1f0988 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -20,6 +20,7 @@ from pygit2 import GitError, Repository try: from pygit2 import AlreadyExistsError except ImportError: + # Older version of pygit2 do not have the AlreadyExistsError defined AlreadyExistsError = ValueError import pagure diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index b575a6d..a675fc5 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -4204,6 +4204,7 @@ class PagureLibtests(tests.Modeltests): markdown_v = markdown.__version_info__ # python-markdown >= 3.2.0 returns to the old behavior on img tag trailing slash old_markdown = markdown_v < (2, 6, 0) or markdown_v >= (3, 2, 0) + mk_321 = markdown_v >= (3, 2, 0) texts = [ "foo bar test#1 see?", @@ -4334,7 +4335,22 @@ class PagureLibtests(tests.Modeltests): '

but not someone@pingou.com

', ] - if old_markdown: + if mk_321: + print("**** Markdown 3.2.1+ behavior") + expected.append( + # '[![Fedora_infinity_small.png]' + # '(/test/issue/raw/Fedora_infinity_small.png)]' + # '(/test/issue/raw/Fedora_infinity_small.png)', + '

' + 'Fedora_infinity_small.png' + "

" + ) + elif old_markdown: + print("**** Old markdown behavior") expected.append( # '[![Fedora_infinity_small.png]' # '(/test/issue/raw/Fedora_infinity_small.png)]' @@ -4348,6 +4364,7 @@ class PagureLibtests(tests.Modeltests): "

" ) else: + print("**** Not old but no longer new markdown") expected.append( # '[![Fedora_infinity_small.png]' # '(/test/issue/raw/Fedora_infinity_small.png)]'