From c13fcafe1c50efc5eab88fd246f0c69b171dbe87 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 06 2018 20:12:25 +0000 Subject: [PATCH 1/2] Move parse_path into pagure.utils so it can be re-used elsewhere Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure-ev/pagure_stream_server.py b/pagure-ev/pagure_stream_server.py index a114deb..45b00fe 100644 --- a/pagure-ev/pagure_stream_server.py +++ b/pagure-ev/pagure_stream_server.py @@ -110,60 +110,11 @@ OBJECTS = { } -def _parse_path(path): - """Get the repo name, object type, object ID, and (if present) - username and/or namespace from a URL path component. Will only - handle the known object types from the OBJECTS dict. Assumes: - * Project name comes immediately before object type - * Object ID comes immediately after object type - * If a fork, path starts with /fork/(username) - * Namespace, if present, comes after fork username (if present) or at start - * No other components come before the project name - * None of the parsed items can contain a / - """ - username = None - namespace = None - # path always starts with / so split and throw away first item - items = path.split('/')[1:] - # find the *last* match for any object type - try: - objtype = [item for item in items if item in OBJECTS][-1] - except IndexError: - raise PagureEvException( - "No known object type found in path: %s" % path) - try: - # objid is the item after objtype, we need all items up to it - items = items[:items.index(objtype) + 2] - # now strip the repo, objtype and objid off the end - (repo, objtype, objid) = items[-3:] - items = items[:-3] - except (IndexError, ValueError): - raise PagureEvException( - "No project or object ID found in path: %s" % path) - # now check for a fork - if items and items[0] == 'fork': - try: - # get the username and strip it and 'fork' - username = items[1] - items = items[2:] - except IndexError: - raise PagureEvException( - "Path starts with /fork but no user found! Path: %s" % path) - # if we still have an item left, it must be the namespace - if items: - namespace = items.pop(0) - # if we have any items left at this point, we've no idea - if items: - raise PagureEvException( - "More path components than expected! Path: %s" % path) - - return username, namespace, repo, objtype, objid - - def get_obj_from_path(path): """ Return the Ticket or Request object based on the path provided. """ - (username, namespace, reponame, objtype, objid) = _parse_path(path) + (username, namespace, reponame, objtype, objid) = pagure.utils.parse_path( + path) session = _get_session() repo = pagure.lib.query.get_authorized_project( session, reponame, user=username, namespace=namespace) @@ -212,7 +163,7 @@ def handle_client(client_reader, client_writer): try: obj = get_obj_from_path(url.path) - except PagureEvException as err: + except PagureException as err: log.warning(err.message) return diff --git a/pagure/utils.py b/pagure/utils.py index 8ef3d35..719831f 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -22,6 +22,7 @@ import pygit2 import six import werkzeug +from pagure.exceptions import PagureException from pagure.config import config as pagure_config @@ -742,3 +743,57 @@ def project_has_hook_attr_value(project, hook, attr, value): retval = True return retval + + +def parse_path(path): + """Get the repo name, object type, object ID, and (if present) + username and/or namespace from a URL path component. Will only + handle the known object types from the OBJECTS dict. Assumes: + * Project name comes immediately before object type + * Object ID comes immediately after object type + * If a fork, path starts with /fork/(username) + * Namespace, if present, comes after fork username (if present) or at start + * No other components come before the project name + * None of the parsed items can contain a / + """ + username = None + namespace = None + # path always starts with / so split and throw away first item + items = path.split("/")[1:] + # find the *last* match for any object type + try: + objtype = [ + item for item in items if item in ["issue", "pull-request"] + ][-1] + except IndexError: + raise PagureException("No known object type found in path: %s" % path) + try: + # objid is the item after objtype, we need all items up to it + items = items[: items.index(objtype) + 2] + # now strip the repo, objtype and objid off the end + (repo, objtype, objid) = items[-3:] + items = items[:-3] + except (IndexError, ValueError): + raise PagureException( + "No project or object ID found in path: %s" % path + ) + # now check for a fork + if items and items[0] == "fork": + try: + # get the username and strip it and 'fork' + username = items[1] + items = items[2:] + except IndexError: + raise PagureException( + "Path starts with /fork but no user found! Path: %s" % path + ) + # if we still have an item left, it must be the namespace + if items: + namespace = items.pop(0) + # if we have any items left at this point, we've no idea + if items: + raise PagureException( + "More path components than expected! Path: %s" % path + ) + + return username, namespace, repo, objtype, objid diff --git a/tests/test_stream_server.py b/tests/test_stream_server.py index 0c84e4e..5c4a37a 100644 --- a/tests/test_stream_server.py +++ b/tests/test_stream_server.py @@ -33,7 +33,7 @@ if six.PY3: raise unittest.case.SkipTest('Skipping on python3') import pagure.lib.query # pylint: disable=wrong-import-position -from pagure.exceptions import PagureEvException # pylint: disable=wrong-import-position +from pagure.exceptions import PagureException, PagureEvException # pylint: disable=wrong-import-position import tests # pylint: disable=wrong-import-position # comes from ev-server/ import pagure_stream_server as pss # pylint: disable=wrong-import-position, import-error @@ -110,70 +110,76 @@ class StreamingServerTests(tests.Modeltests): """Tests for _parse_path.""" # Result format is: (username, namespace, repo, objtype, objid) # Simple case: issue for non-namespaced, non-forked repo. - result = pss._parse_path('/pagure/issue/1') + result = pagure.utils.parse_path('/pagure/issue/1') self.assertEqual(result, (None, None, 'pagure', 'issue', '1')) # Pull request for namespaced repo. - result = pss._parse_path('/fedora-qa/fedfind/pull-request/2') + result = pagure.utils.parse_path('/fedora-qa/fedfind/pull-request/2') self.assertEqual(result, (None, 'fedora-qa', 'fedfind', 'pull-request', '2')) # Issue for forked repo. - result = pss._parse_path('/fork/adamwill/pagure/issue/3') + result = pagure.utils.parse_path('/fork/adamwill/pagure/issue/3') self.assertEqual(result, ('adamwill', None, 'pagure', 'issue', '3')) # Issue for forked, namespaced repo. - result = pss._parse_path('/fork/pingou/fedora-qa/fedfind/issue/4') + result = pagure.utils.parse_path('/fork/pingou/fedora-qa/fedfind/issue/4') self.assertEqual(result, ('pingou', 'fedora-qa', 'fedfind', 'issue', '4')) # Issue for repo named 'pull-request' (yeah, now we're getting tricksy). - result = pss._parse_path('/pull-request/issue/5') + result = pagure.utils.parse_path('/pull-request/issue/5') self.assertEqual(result, (None, None, 'pull-request', 'issue', '5')) # Unknown object type. six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"No known object", - pss._parse_path, '/pagure/unexpected/1' + pagure.utils.parse_path, + '/pagure/unexpected/1' ) # No object ID. six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"No project or object ID", - pss._parse_path, '/pagure/issue' + pagure.utils.parse_path, + '/pagure/issue' ) # No repo name. Note: we cannot catch 'namespace but no repo name', # but that should fail later in pagure.lib.query.get_project six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"No project or object ID", - pss._parse_path, '/issue/1' + pagure.utils.parse_path, + '/issue/1' ) # /fork but no user name. six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"no user found!", - pss._parse_path, '/fork/pagure/issue/1' + pagure.utils.parse_path, + '/fork/pagure/issue/1' ) # Too many path components before object type. six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"More path components", - pss._parse_path, '/fork/adamwill/fedora-qa/fedfind/unexpected/issue/1' + pagure.utils.parse_path, + '/fork/adamwill/fedora-qa/fedfind/unexpected/issue/1' ) six.assertRaisesRegex( self, - PagureEvException, + PagureException, r"More path components", - pss._parse_path, '/fedora-qa/fedfind/unexpected/issue/1' + pagure.utils.parse_path, + '/fedora-qa/fedfind/unexpected/issue/1' ) def test_get_issue(self): From 7c5e53fe9a67623ccd627817935bf99722897dc1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 06 2018 20:12:25 +0000 Subject: [PATCH 2/2] Match the entire URL so we can act on the right project Before this we were just matching the project name, so we missed namespaces, fork information and all. This with the fact that if even when we matched the project named we did not update the project object on which we were working, so we ended up working on the wrong ticket/PR. This can be seen in the ticket reported upstream: https://pagure.io/pagure/issue/4052 Fixes https://pagure.io/pagure/issue/4052 --- diff --git a/pagure/lib/link.py b/pagure/lib/link.py index e86df26..dc89116 100644 --- a/pagure/lib/link.py +++ b/pagure/lib/link.py @@ -15,37 +15,38 @@ from __future__ import unicode_literals import re import pagure.lib.query import pagure.exceptions +import pagure.utils +from pagure.config import config as pagure_config FIXES = [ - re.compile(r"(?:.*\s+)?fixe?[sd]?:?\s*?#(\d+)", re.I), - re.compile( - r"(?:.*\s+)?fixe?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", - re.I, - ), - re.compile(r"(?:.*\s+)?merge?[sd]?:?\s*?#(\d+)", re.I), - re.compile( - r"(?:.*\s+)?merge?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", - re.I, - ), - re.compile(r"(?:.*\s+)?close?[sd]?:?\s*?#(\d+)", re.I), + re.compile(r"(?:.*\s+)?{0}?[sd]?:?\s*?#(\d+)".format(kw), re.I) + for kw in ["fixe", "merge", "close"] +] +FIXES += [ re.compile( - r"(?:.*\s+)?close?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", + r"(?:.*\s+)?{0}?[sd]?:?\s*?{1}" + r"(/.*?/(?:issue|pull-request)/\d+)".format( + kw, pagure_config["APP_URL"].rstrip("/") + ), re.I, - ), + ) + for kw in ["fixe", "merge", "close"] ] + RELATES = [ - re.compile(r"(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?#(\d+)", re.I), - re.compile(r"(?:.*\s+)?relate[sd]?:?\s?#(\d+)", re.I), + re.compile(r"(?:.*\s+)?{0}?[sd]?:?\s*?(?:to)?\s*?#(\d+)".format(kw), re.I) + for kw in ["relate"] +] +RELATES += [ re.compile( - r"(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?" - r"https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)/issue/(\d+)", + r"(?:.*\s+)?{0}?[sd]?:?\s*?(?:to)?\s*?{1}(/.*?/issue/\d+)".format( + kw, pagure_config["APP_URL"].rstrip("/") + ), re.I, - ), + ) + for kw in ["relate"] ] @@ -87,12 +88,22 @@ def get_relation( for motif in regex: relid = None project = None - if motif.match(text): - if len(motif.match(text).groups()) >= 2: - relid = motif.match(text).group(2) - project = motif.match(text).group(1) - else: - relid = motif.match(text).group(1) + got_match = motif.match(text) + if got_match: + relid = got_match.group(1) + if not relid.isdigit(): + ( + username, + namespace, + reponame, + objtype, + relid, + ) = pagure.utils.parse_path(relid) + repo = pagure.lib.query.get_authorized_project( + session, reponame, user=username, namespace=namespace + ) + if not repo: + continue if relid: relation = pagure.lib.query.search_issues( diff --git a/tests/test_pagure_hooks_pagure_hook.py b/tests/test_pagure_hooks_pagure_hook.py new file mode 100644 index 0000000..aa768ef --- /dev/null +++ b/tests/test_pagure_hooks_pagure_hook.py @@ -0,0 +1,174 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import unicode_literals + +import unittest +import sys +import os + +import mock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.hooks.pagure_hook +import tests + + +class PagureHooksPagureHooktests(tests.SimplePagureTest): + """ Tests for pagure.hooks.pagure_hook """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureHooksPagureHooktests, self).setUp() + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + # Add one issue to each projects otherwise we won't be able to find + project = pagure.lib.query.get_authorized_project(self.session, 'test') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue', + content='We should work on this', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + project = pagure.lib.query.get_authorized_project( + self.session, 'test2') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue on test2', + content='We should work on this, really', + user='foo', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue on test2') + + # Create a fork of test for foo with its own ticket + item = pagure.lib.model.Project( + user_id=2, # foo + name='test', + is_fork=True, + parent_id=1, + description='test project #1', + hook_token='aaabbbccc_foo', + ) + item.close_status = [ + 'Invalid', 'Insufficient data', 'Fixed', 'Duplicate'] + self.session.add(item) + self.session.commit() + project = pagure.lib.query.get_authorized_project( + self.session, 'test', user="foo") + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue on fork/foo/test', + content='We should work on this, really', + user='foo', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue on fork/foo/test') + + self.folder = os.path.join(self.path, 'repos', 'test.git') + + # Add a README to the git repo - First commit + tests.add_readme_git_repo(self.folder) + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_short_url(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a short link to the same project. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\nFixes #1' + ) + + project = pagure.lib.query.get_authorized_project(self.session, 'test') + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project.issues[0], + 'http://localhost.localdomain/') + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_full_url(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a full link to another project. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\n' + 'Fixes http://localhost.localdomain/test2/issue/1' + ) + + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + project2 = pagure.lib.query.get_authorized_project( + self.session, 'test2') + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project2.issues[0], + 'http://localhost.localdomain/') + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_full_url_fork(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a full link to a fork. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\n' + 'Fixes http://localhost.localdomain/fork/foo/test/issue/1' + ) + + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + project_fork = pagure.lib.query.get_authorized_project( + self.session, 'test', user="foo") + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project_fork.issues[0], + 'http://localhost.localdomain/') + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/tests/test_pagure_lib_link.py b/tests/test_pagure_lib_link.py index d5e7b35..cb7df6e 100644 --- a/tests/test_pagure_lib_link.py +++ b/tests/test_pagure_lib_link.py @@ -33,9 +33,9 @@ COMMENTS = [ 'Did you see #1?', 'This is a duplicate of #2', 'This is a fixes #3', - 'Might be worth looking at https://fedorahosted.org/pagure/tests2/issue/4', + 'Might be worth looking at http://localhost.localdomain/pagure/tests2/issue/4', 'This relates to #5', - 'Could this be related to https://fedorahosted.org/pagure/tests2/issue/6', + 'Could this be related to http://localhost.localdomain/test/issue/6', ] @@ -186,21 +186,23 @@ class PagureLibLinktests(tests.Modeltests): self.assertEqual( str(link), '[Issue(3, project:test, user:pingou, title:issue 3)]') + self.assertEqual(len(link), 1) + self.assertEqual(link[0].project.fullname, 'test') else: self.assertEqual(link, []) def test_relates_regex(self): ''' Test the relates regex present in pagure.lib.link. ''' - text = 'relates to http://localhost/fork/pingou/test/issue/1' + text = 'relates to http://localhost.localdomain/fork/pingou/test/issue/1' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) - text = 'relates http://209.132.184.222/fork/pingou/test/issue/1' + text = 'relates http://localhost.localdomain/fork/pingou/test/issue/1' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) @@ -213,16 +215,16 @@ class PagureLibLinktests(tests.Modeltests): self.assertEqual(regex.match(text), None) text = 'Could this be related to '\ - ' https://fedorahosted.org/pagure/tests2/issue/6' + ' http://localhost.localdomain/pagure/tests2/issue/6' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) - text = 'relates https://localhost/SSSD/ding-libs/issue/31' + text = 'relates http://localhost.localdomain/SSSD/ding-libs/issue/31' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) @@ -238,34 +240,44 @@ class PagureLibLinktests(tests.Modeltests): if match: break self.assertNotEqual(match, None) - self.assertEqual(len(match.groups()), 2) + self.assertEqual(len(match.groups()), 1) self.assertEqual(match.groups(), groups) data = [ # [string, groups] ] - project_match('fixes http://localhost/fork/pingou/test/issue/1', - ('test', '1')) - project_match('fix http://209.132.184.222/fork/pingou/test/issue/1', - ('test', '1')) - project_match('Could this be fixes ' - ' https://fedorahosted.org/pagure/tests2/issue/6', - ('tests2', '6')) - project_match('merged https://pagure.io/myproject/pull-request/70', - ('myproject', '70')) - project_match('Now we merge https://pagure.io/myproject/pull-request/99', - ('myproject', '99')) - project_match('Merges http://localhost/fork/pingou/test/issue/1', - ('test', '1')) - project_match('Merges: http://localhost/fork/pingou/test/issue/12', - ('test', '12')) - project_match('Merged http://localhost/fork/pingou/test/issue/123#', - ('test', '123')) - project_match('Merge: http://localhost/fork/pingou/test/issue/1234#foo', - ('test', '1234')) - project_match('Merges: https://localhost/SSSD/ding-libs/pull-request/3188', - ('ding-libs', '3188')) + project_match( + 'fixes ' + 'http://localhost.localdomain/fork/pingou/test/issue/1', + ('/fork/pingou/test/issue/1',)) + project_match( + 'Could this be fixes ' + ' http://localhost.localdomain/pagure/tests2/issue/6', + ('/pagure/tests2/issue/6',)) + project_match( + 'merged http://localhost.localdomain/myproject/pull-request/70', + ('/myproject/pull-request/70',)) + project_match( + 'Now we merge http://localhost.localdomain/myproject/pull-request/99', + ('/myproject/pull-request/99',)) + project_match( + 'Merges http://localhost.localdomain/fork/pingou/test/issue/1', + ('/fork/pingou/test/issue/1',)) + project_match( + 'Merges: http://localhost.localdomain/fork/pingou/test/issue/12', + ('/fork/pingou/test/issue/12',)) + project_match( + 'Merged http://localhost.localdomain/fork/pingou/test/issue/123#', + ('/fork/pingou/test/issue/123',)) + project_match('Merge: http://localhost.localdomain/fork/pingou/test/issue/1234#foo', + ('/fork/pingou/test/issue/1234',)) + project_match( + 'Merges: http://localhost.localdomain/SSSD/ding-libs/pull-request/3188', + ('/SSSD/ding-libs/pull-request/3188',)) + project_match( + 'Fixes: http://localhost.localdomain/fedpkg/issue/220', + ('/fedpkg/issue/220',)) # issue matches def issue_match(text, issue):