launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/turnip:paginated-check-refs-permissions into turnip:master.

Commit message:
Adding better error reporting and paginating the requests to check_ref_permissions on hooks, to avoid connection timeout when a user git pushes dozens of refs at once.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Ideally, when a user pushes a lot of branches at once, we should parallelize (in a thread pool executor, for example) the permissions checks in batches (instead of checking one batch after another). 

But such change would require a bit more refactoring to create different sockets, so this goes in a future MP.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/turnip:paginated-check-refs-permissions into turnip:master.

diff --git a/requirements.txt b/requirements.txt
index a298c0a..cb86dfe 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -26,6 +26,7 @@ linecache2==1.0.0
 m2r==0.1.14
 mccabe==0.3
 mistune==0.8.3
+mock==3.0.5
 Paste==2.0.2
 PasteDeploy==2.1.0
 pbr==5.4.4
diff --git a/setup.py b/setup.py
index cd8f4fb..b5821f7 100755
--- a/setup.py
+++ b/setup.py
@@ -34,6 +34,7 @@ test_requires = [
     'docutils',
     'fixtures',
     'flake8',
+    'mock',
     'testtools',
     'webtest',
     ]
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index e135395..b1348fa 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -113,14 +113,20 @@ def netstring_recv(sock):
     c = sock.recv(1)
     lengthstr = b''
     while c != b':':
-        assert c.isdigit()
+        if not c.isdigit():
+            raise ValueError(
+                "Invalid response: %s" % (six.ensure_text(c + sock.recv(256))))
         lengthstr += c
         c = sock.recv(1)
     length = int(lengthstr)
     s = bytearray()
     while len(s) < length:
         s += sock.recv(length - len(s))
-    assert sock.recv(1) == b','
+    ending = sock.recv(1)
+    if ending != b',':
+        raise ValueError(
+            "Length error for message '%s': ending='%s'" %
+            (six.ensure_text(bytes(s)), six.ensure_text(ending)))
     return bytes(s)
 
 
@@ -135,14 +141,24 @@ def rpc_invoke(sock, method, args):
     return res['result']
 
 
-def check_ref_permissions(sock, rpc_key, ref_paths):
+def split_list(lst, n):
+    """Splits the given list into chunks of size n."""
+    lst = list(lst)
+    return [lst[i:i + n] for i in range(0, len(lst), n)]
+
+
+def check_ref_permissions(sock, rpc_key, ref_paths, page_size=25):
     ref_paths = [base64.b64encode(path).decode('UTF-8') for path in ref_paths]
-    rule_lines = rpc_invoke(
-        sock, 'check_ref_permissions',
-        {'key': rpc_key, 'paths': ref_paths})
-    return {
-        base64.b64decode(path.encode('UTF-8')): permissions
-        for path, permissions in rule_lines.items()}
+    permissions = {}
+    # Paginate the rpc calls to avoid timeouts.
+    for ref_paths_chunk in split_list(ref_paths, page_size):
+        rule_lines = rpc_invoke(
+            sock, 'check_ref_permissions',
+            {'key': rpc_key, 'paths': ref_paths_chunk})
+        permissions.update({
+            base64.b64decode(path.encode('UTF-8')): permissions
+            for path, permissions in rule_lines.items()})
+    return permissions
 
 
 def send_mp_url(received_line):
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index 449b9f9..b383d6b 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -17,7 +17,12 @@ from fixtures import (
     MonkeyPatch,
     TempDir,
     )
+try:
+    from unittest import mock
+except ImportError:
+    import mock
 import pygit2
+import six
 from testtools import TestCase
 from testtools.deferredruntest import AsynchronousDeferredRunTest
 from twisted.internet import (
@@ -29,6 +34,7 @@ from twisted.internet import (
 from turnip.pack import hookrpc
 from turnip.pack.helpers import ensure_hooks
 from turnip.pack.hooks import hook
+from turnip.pack.hooks.hook import split_list, check_ref_permissions
 
 
 class HookProcessProtocol(protocol.ProcessProtocol):
@@ -108,15 +114,21 @@ class TestNetstringRecv(TestCase):
 
     def test_nondigit(self):
         sock = MockSocket([b'zzz:abc,'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Invalid response: zzz:abc,",
+            hook.netstring_recv, sock)
 
     def test_short(self):
         sock = MockSocket([b'4:abc,'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Length error for message 'abc,': ending=''",
+            hook.netstring_recv, sock)
 
     def test_unterminated(self):
         sock = MockSocket([b'4:abcd'])
-        self.assertRaises(AssertionError, hook.netstring_recv, sock)
+        self.assertRaisesRegex(
+            ValueError, "Length error for message 'abcd': ending=''",
+            hook.netstring_recv, sock)
 
     def test_split_data(self):
         sock = MockSocket([b'12:abcd', b'efgh', b'ijkl,'])
@@ -446,3 +458,38 @@ class TestDeterminePermissions(TestCase):
         output = hook.determine_permissions_outcome(
             pygit2.GIT_OID_HEX_ZERO, 'ref', {'ref': ['force_push']})
         self.assertEqual(b"You do not have permission to create ref.", output)
+
+
+class TestSplitRefPathsCalls(TestCase):
+    def test_split_list(self):
+        self.assertEqual(
+            split_list(range(10), 3), [[0, 1, 2], [3, 4, 5], [6, 7, 8], [9]])
+
+    @mock.patch('turnip.pack.hooks.hook.rpc_invoke')
+    def test_rcp_calls_are_paginated(self, rpc_invoke):
+        encode = lambda x: six.ensure_text(base64.b64encode(x))
+        rpc_invoke.side_effect = [
+            {encode(b'master'): [], encode(b'develop'): []},
+            {encode(b'head'): []}
+        ]
+        sock = mock.Mock()
+        # Call it with page size = 2
+        result = check_ref_permissions(
+            sock, "rpc-key", [b"master", b"develop", b"head"], 2)
+
+        # The final result should have been joined into.
+        self.assertEqual(
+            result, {b"master": [], b"develop": [], b"head": []})
+
+        # Check that it called correctly the rpc_invoke method.
+        self.assertEqual(rpc_invoke.call_args_list, [
+            mock.call(
+                sock, 'check_ref_permissions', {
+                    'key': 'rpc-key', 'paths': [
+                        encode(b"master"), encode(b"develop")]
+                }),
+            mock.call(
+                sock, 'check_ref_permissions', {
+                    'key': 'rpc-key', 'paths': [encode(b"head")]
+                }),
+        ])