Handle namespaced projects in SSE server

The SSE server's code for parsing the URL to get to the project
does not handle namespacing. With this, it does. I tried to
figure out the 'rules' for the path as best I could from other
code in the project.

Also refactor a bit to split out a path parsing function and
separate object finding functions, and be more explicit about
the specific object types we support.

Also add a test suite for the streaming server, and update all
docs, Ansible play, spec file etc. for the rename from
'pagure-stream-server' to 'pagure_stream_server'.

Fixes https://pagure.io/pagure/issue/1532

Signed-off-by: Adam Williamson <awilliam@redhat.com>

ansible/roles/pagure-dev/files/pagure_ev.service

@@ -6,7 +6,7 @@ Documentation=https://pagure.io/pagure
 [Service]
 Environment="PAGURE_CONFIG=/home/vagrant/pagure.cfg"
 ExecStart=/home/vagrant/.virtualenvs/python2-pagure/bin/python \
-          /home/vagrant/devel/ev-server/pagure-stream-server.py
+          /home/vagrant/devel/ev-server/pagure_stream_server.py
 Type=simple
 
 [Install]

doc/install_evs.rst

@@ -28,7 +28,7 @@ The eventsource server is easy to set-up.
 +----------------------------------------+-----------------------------------------------------+
 |              Source                    |                   Destination                       |
 +========================================+=====================================================+
-| ``ev-server/pagure-stream-server.py``  | ``/usr/libexec/pagure-ev/pagure-stream-server.py``  |
+| ``ev-server/pagure_stream_server.py``  | ``/usr/libexec/pagure-ev/pagure_stream_server.py``  |
 +----------------------------------------+-----------------------------------------------------+
 | ``ev-server/pagure_ev.service``        | ``/etc/systemd/system/pagure_ev.service``           |
 +----------------------------------------+-----------------------------------------------------+

ev-server/pagure_ev.service

@@ -4,7 +4,7 @@ After=redis.target
 Documentation=https://pagure.io/pagure
 
 [Service]
-ExecStart=/usr/libexec/pagure-ev/pagure-stream-server.py
+ExecStart=/usr/libexec/pagure-ev/pagure_stream_server.py
 Type=simple
 User=git
 Group=git

ev-server/pagure-stream-server.py → ev-server/pagure_stream_server.py

@@ -42,53 +42,116 @@ from pagure.exceptions import PagureEvException
 
 SERVER = None
 
-def get_obj_from_path(path):
-    """ Return the Ticket or Request object based on the path provided.
+
+def _get_issue(repo, objid):
+    """Get a Ticket (issue) instance for a given repo (Project) and
+    objid (issue number).
     """
-    username = None
-    try:
-        if path.startswith('/fork'):
-            username, repo, obj, objid = path.split('/')[2:6]
-        else:
-            repo, obj, objid = path.split('/')[1:4]
-    except:
-        raise PagureEvException("Invalid URL: %s" % path)
+    issue = None
+    if not repo.settings.get('issue_tracker', True):
+        raise PagureEvException("No issue tracker found for this project")
 
-    repo = pagure.lib.get_project(pagure.SESSION, repo, user=username)
+    issue = pagure.lib.search_issues(
+        pagure.SESSION, repo, issueid=objid)
 
-    if repo is None:
-        raise PagureEvException("Project '%s' not found" % repo)
+    if issue is None or issue.project != repo:
+        raise PagureEvException("Issue '%s' not found" % objid)
 
-    output = None
-    if obj == 'issue':
-        if not repo.settings.get('issue_tracker', True):
-            raise PagureEvException("No issue tracker found for this project")
+    if issue.private:
+        # TODO: find a way to do auth
+        raise PagureEvException(
+            "This issue is private and you are not allowed to view it")
 
-        output = pagure.lib.search_issues(
-            pagure.SESSION, repo, issueid=objid)
+    return issue
 
-        if output is None or output.project != repo:
-            raise PagureEvException("Issue '%s' not found" % objid)
 
-        if output.private:
-            # TODO: find a way to do auth
-            raise PagureEvException(
-                "This issue is private and you are not allowed to view it")
-    elif obj == 'pull-request':
-        if not repo.settings.get('pull_requests', True):
-            raise PagureEvException(
-                "No pull-request tracker found for this project")
+def _get_pull_request(repo, objid):
+    """Get a PullRequest instance for a given repo (Project) and objid
+    (request number).
+    """
+    if not repo.settings.get('pull_requests', True):
+        raise PagureEvException(
+            "No pull-request tracker found for this project")
+
+    request = pagure.lib.search_pull_requests(
+        pagure.SESSION, project_id=repo.id, requestid=objid)
+
+    if request is None or request.project != repo:
+        raise PagureEvException("Pull-Request '%s' not found" % objid)
+
+    return request
+
+
+# Dict representing known object types that we handle requests for,
+# and the bound functions for getting an object instance from the
+# parsed path data. Has to come after the functions it binds
+OBJECTS = {
+    'issue': _get_issue,
+    'pull-request': _get_pull_request
+}
+
+
+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)
 
-        output = pagure.lib.search_pull_requests(
-            pagure.SESSION, project_id=repo.id, requestid=objid)
 
-        if output is None or output.project != repo:
-            raise PagureEvException("Pull-Request '%s' not found" % 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)
+    repo = pagure.lib.get_project(pagure.SESSION, reponame, user=username, namespace=namespace)
+    if repo is None:
+        raise PagureEvException("Project '%s' not found" % reponame)
 
-    else:
-        raise PagureEvException("Invalid object provided: '%s'" % obj)
+    # find the appropriate object getter function from OBJECTS
+    try:
+        getfunc = OBJECTS[objtype]
+    except KeyError:
+        raise PagureEvException("Invalid object provided: '%s'" % objtype)
 
-    return output
+    return getfunc(repo, objid)
 
 
 @trollius.coroutine

files/pagure.spec

@@ -207,8 +207,8 @@ install -m 644 milters/comment_email_milter.py \
 
 # Install the eventsource
 mkdir -p $RPM_BUILD_ROOT/%{_libexecdir}/pagure-ev
-install -m 755 ev-server/pagure-stream-server.py \
-    $RPM_BUILD_ROOT/%{_libexecdir}/pagure-ev/pagure-stream-server.py
+install -m 755 ev-server/pagure_stream_server.py \
+    $RPM_BUILD_ROOT/%{_libexecdir}/pagure-ev/pagure_stream_server.py
 install -m 644 ev-server/pagure_ev.service \
     $RPM_BUILD_ROOT/%{_unitdir}/pagure_ev.service
 

pagure/exceptions.py

@@ -47,7 +47,7 @@ class BranchNotFoundException(PagureException):
 
 
 class PagureEvException(PagureException):
-    ''' Exceptions used in the pagure-stream-server.
+    ''' Exceptions used in the pagure_stream_server.
     '''
     pass
 

tests/test_stream_server.py

@@ -0,0 +1,237 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+"""
+ (c) 2016 - Copyright Red Hat Inc
+
+ Authors:
+   Adam Williamson <awilliam@redhat.com>
+
+Tests for the Pagure streaming server.
+
+"""
+
+# obviously this is fine for testing.
+# pylint: disable=locally-disabled, protected-access
+
+import logging
+import os
+import sys
+import unittest
+
+import mock
+
+sys.path.insert(0, os.path.join(os.path.dirname(
+    os.path.abspath(__file__)), '..'))
+sys.path.insert(0, os.path.join(os.path.dirname(
+    os.path.abspath(__file__)), '../ev-server'))
+
+import pagure                                       # pylint: disable=wrong-import-position
+from pagure.exceptions import 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
+
+logging.basicConfig(stream=sys.stderr)
+
+
+class StreamingServerTests(tests.Modeltests):
+    """Tests for the streaming server."""
+
+    def setUp(self):
+        """Set up the environnment, run before every test."""
+        super(StreamingServerTests, self).setUp()
+        pagure.SESSION = self.session
+
+        # Mock send_email, we never want to send or see emails here.
+        self.mailpatcher = mock.patch('pagure.lib.notify.send_email')
+        self.mailpatcher.start()
+
+        # Setup projects
+        tests.create_projects(self.session)
+        self.repo = pagure.lib.get_project(self.session, 'test')
+        self.repo2 = pagure.lib.get_project(self.session, 'test2')
+
+        # Disable repo 2's issue tracker and PR tracker
+        pagure.lib.update_project_settings(
+            session=self.session,
+            repo=self.repo2,
+            user='pingou',
+            settings={
+                'issue_tracker': False,
+                'pull_requests': False,
+            }
+        )
+
+        # Create a public issue
+        pagure.lib.new_issue(
+            session=self.session,
+            repo=self.repo,
+            title='Test issue',
+            content='We should work on this',
+            user='pingou',
+            ticketfolder=None
+        )
+
+        # Create a private issue
+        pagure.lib.new_issue(
+            session=self.session,
+            repo=self.repo,
+            title='Private issue #2',
+            content='The world can see my porn folder',
+            user='pingou',
+            private=True,
+            ticketfolder=None
+        )
+
+        # Create a PR
+        pagure.lib.new_pull_request(
+            session=self.session,
+            repo_from=self.repo,
+            repo_to=self.repo,
+            branch_from='feature',
+            branch_to='master',
+            title='Test PR',
+            user='pingou',
+            requestfolder=None
+        )
+
+    def tearDown(self):
+        "Stop the patchers, as well as calling super."""
+        super(StreamingServerTests, self).tearDown()
+        self.mailpatcher.stop()
+
+    def test_parse_path(self):
+        """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')
+        self.assertEqual(result, (None, None, 'pagure', 'issue', '1'))
+
+        # Pull request for namespaced repo.
+        result = pss._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')
+        self.assertEqual(result, ('adamwill', None, 'pagure', 'issue', '3'))
+
+        # Issue for forked, namespaced repo.
+        result = pss._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')
+        self.assertEqual(result, (None, None, 'pull-request', 'issue', '5'))
+
+        # Unknown object type.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"No known object",
+            pss._parse_path, '/pagure/unexpected/1'
+        )
+
+        # No object ID.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"No project or object ID",
+            pss._parse_path, '/pagure/issue'
+        )
+
+        # No repo name. Note: we cannot catch 'namespace but no repo name',
+        # but that should fail later in pagure.lib.get_project
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"No project or object ID",
+            pss._parse_path, '/issue/1'
+        )
+
+        # /fork but no user name.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"no user found!",
+            pss._parse_path, '/fork/pagure/issue/1'
+        )
+
+        # Too many path components before object type.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"More path components",
+            pss._parse_path, '/fork/adamwill/fedora-qa/fedfind/unexpected/issue/1'
+        )
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"More path components",
+            pss._parse_path, '/fedora-qa/fedfind/unexpected/issue/1'
+        )
+
+    def test_get_issue(self):
+        """Tests for _get_issue."""
+        # Simple case: get the existing issue from the existing repo.
+        result = pss._get_issue(self.repo, '1')
+        self.assertEqual(result.id, 1)
+
+        # Issue that doesn't exist.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"Issue '3' not found",
+            pss._get_issue, self.repo, '3'
+        )
+
+        # Private issue (for now we don't handle auth).
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"issue is private",
+            pss._get_issue, self.repo, '2'
+        )
+
+        # Issue from a project with no issue tracker.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"No issue tracker found",
+            pss._get_issue, self.repo2, '1'
+        )
+
+    def test_get_pull_request(self):
+        """Tests for _get_pull_request."""
+        # Simple case: get the existing PR from the existing repo.
+        result = pss._get_pull_request(self.repo, '3')
+        self.assertEqual(result.id, 3)
+
+        # PR that doesn't exist.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"Pull-Request '2' not found",
+            pss._get_pull_request, self.repo, '2'
+        )
+
+        # PR from a project with no PR tracker.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"No pull-request tracker found",
+            pss._get_pull_request, self.repo2, '1'
+        )
+
+    def test_get_obj_from_path(self):
+        """Tests for get_obj_from_path."""
+        # Simple issue case.
+        result = pss.get_obj_from_path('/test/issue/1')
+        self.assertEqual(result.id, 1)
+
+        # Simple PR case.
+        result = pss.get_obj_from_path('/test/pull-request/3')
+        self.assertEqual(result.id, 3)
+
+        # Non-existent repo.
+        self.assertRaisesRegexp(
+            PagureEvException,
+            r"Project 'foo' not found",
+            pss.get_obj_from_path, '/foo/issue/1'
+        )
+
+        # NOTE: we cannot test the 'Invalid object provided' exception
+        # as it's a backup (current code will never hit it)
+
+if __name__ == '__main__':
+    SUITE = unittest.TestLoader().loadTestsFromTestCase(StreamingServerTests)
+    unittest.TextTestRunner(verbosity=2).run(SUITE)