Merge lp:~milo/linaro-patchmetrics/bug1013577 into lp:linaro-patchmetrics

[Milo Casagrande]

Hello,

I would like to propose the following changes to linaro-patchmetrics in order to fix the problems we are facing with gerrit and its transition to a new REST interface from the JSON-RPC one.

The merge proposal does not fix all the problems, it is still a work in progress, since the JSON output provided by the new REST interface is different than the old one, and we need to make further changes in the code base (I need to retrieve some JSON output from an old version of Gerrit, probably using Linaro instance on Gerrit, and compare it with the new one).

Since there are already substantial changes, I'm asking for a preliminary review.

What is included in this merge proposal is:
- PEP8 fixes.
- A fix to the call invoked by the script 'sync-gerrit-changes.py' (the script itself did not change): this fix introduces new functions in the 'patchmetrics/gerrit.py' file, adding new functionalities in order to address the construction of the REST query string. What I tried to do has been to keep the new functions as small as possible, in order for them to be easier to maintain and to be as "testable" as possible. I tried to keep the function also as reusable as possible, in order to use them in the future to fix the other JSON-RPC deprecation we might encouter (and I guess there will be more). The new functions basically create the query strings from the parameters passed: there is a specific function to create the correct query string, since it has a particular construct.
- I also wrote new tests for the new functions added.

[lp:~milo/linaro-patchmetrics/bug1013577]

363. By Milo Casagrande on 2012-06-21

Fixed error on test, fixed indentation.

[Deepti B. Kalakeri (deeptik)]

The changes looks good. One typo needs to be fixed

"more complex queries need to be though of, since" , should be "more complex queries need to be thought of, since"

Also, I was not able to find any information on how to run the tests.
Can you point me where this information is present OR If it does not exist then could you add a README somewhere ?

You can make these changes while merging. I will approve this so that we can go ahead with trying to see if it fixes our initial problems.

review: Approve

[lp:~milo/linaro-patchmetrics/bug1013577]

364. By Milo Casagrande on 2012-06-22

Use the new JSON format to create the necessary objects.

[Milo Casagrande (milo)]

Hello,

first, thanks Deepti for the first review!
I updated the code again, pushing also the final fixes for the gerrit problem.

With the latest push:
- I introduced a new python module, 'gerrit_json.py', that only holds variables that are the names used inside the Gerrit JSON output: these variables are used to retrieve the values from the output, and might be used also somewhere else in the code, that is why I created a separate module.
- I fixed all the old calls in the code that were used to access the JSON values: before we had a list of elements, now we have a list of dictionaries whose values we need.
- Cleaned up also other parts of the code, in particular:
 * Use "COMPATC_JSON" as the default JSON output, so that it should make a little bit smaller and hopefully faster.
 * Use "gzip" compression on the output from the server: probably needs more testing to see if it really is beneficial, but if we get back a lot of results from the server, we can save a little bit on space and gain some peed.
 * Cleaned up a little bit the tests.
 * Added some links as a reference to Gerrit REST API.

For the tests part, they should be called directly from the command line from django, but I have problem with my django setup: for my own tests, I created a small unittest class that calls all the necessary parts to be tested.

[Milo Casagrande (milo)]

Oh, I forgot, I removed also one entire function that was not needed anymore: before, some of the necessary information, were retrieved via two calls to the Java Servlet: one to get the actual changes, and another one to get the details of the change. Now all the information is stored in the same output.

For a comparison of the two JSON output, look at the bug report from the merge proposal, I attached there the output we get from android-review.googlesource.com with the new REST API, and the output we get from the Linaro gerrit instance using the old methods. Both the request are made using a user that has changes in both instances.

The difference from the old approach, is that now if a user is not in the Gerrit database, we receive back an error 400, that we catch and deal with.

[lp:~milo/linaro-patchmetrics/bug1013577]

365. By Milo Casagrande on 2012-06-25

Fixed indentation; re-added return statement, problem with tests.

[Deepti B. Kalakeri (deeptik)]

> Hello,
>
> first, thanks Deepti for the first review!
> I updated the code again, pushing also the final fixes for the gerrit problem.
>
> With the latest push:
> - I introduced a new python module, 'gerrit_json.py', that only holds
> variables that are the names used inside the Gerrit JSON output: these
> variables are used to retrieve the values from the output, and might be used
> also somewhere else in the code, that is why I created a separate module.
> - I fixed all the old calls in the code that were used to access the JSON
> values: before we had a list of elements, now we have a list of dictionaries
> whose values we need.
> - Cleaned up also other parts of the code, in particular:
> * Use "COMPATC_JSON" as the default JSON output, so that it should make a
> little bit smaller and hopefully faster.
> * Use "gzip" compression on the output from the server: probably needs more
> testing to see if it really is beneficial, but if we get back a lot of results
> from the server, we can save a little bit on space and gain some peed.
> * Cleaned up a little bit the tests.
> * Added some links as a reference to Gerrit REST API.
>
> For the tests part, they should be called directly from the command line from
> django, but I have problem with my django setup: for my own tests, I created a
> small unittest class that calls all the necessary parts to be tested.

It would be good if you added your test script here as well.

[Milo Casagrande (milo)]

Hi Deepti,

I attached it to the bug report, since here it is not possible to insert files, and I would prefer not to upload it in the bzr branch, as it is not really related to the test infrastructure there. I have that file in the 'apps/patchmetrics/test/ directory'.

You might need to tweak your PYTHONPATH to make it work, since it needs to pick-up other files from the project while it imports the other statements.
Otherwise, I have another file that is a copy of the modified functions in 'gerrit.py' and that is possible to test directly without importing all the rest of the project.

[Deepti B. Kalakeri (deeptik)]

Thanks for the fixes:

161 # The request in this case needs to be a GET.
162 json_string = sanitize_json_response(urllib2.urlopen(request).read())
163 return json_string
It is probably good to initialize json_string in gerrit.py to None here before we called sanitize_json_response.

175 def sanitize_json_response(json_string):
176 """Cleans up the JSON response got from the REST interface, in order to be
177 valid JSON.
178
179 :param json_string: the JSON string to clean up.
180 """
181 # The JSON response is not valid JSON, there is a magic sequence at the
182 # beginning of the result. We are paranoid, and remove also other white
183 # spaces.
184 if json_string.startswith(GERRIT_SANITIZABLE_VALUE):
185 return json_string.lstrip(GERRIT_SANITIZABLE_VALUE).strip()

Are we sure we that the json_string should always start with GERRIT_SANITIZABLE_VALUE ??
If not, then we need to return json_string even when json_string does not start with GERRIT_SANITIZABLE_VALUE.

review: Needs Fixing

[Milo Casagrande (milo)]

> Thanks for the fixes:
>
> 161 # The request in this case needs to be a GET.
> 162 json_string =
> sanitize_json_response(urllib2.urlopen(request).read())
> 163 return json_string
> It is probably good to initialize json_string in gerrit.py to None here
> before we called sanitize_json_response.

Sure, fixed.

> 175 def sanitize_json_response(json_string):
> 176 """Cleans up the JSON response got from the REST interface, in order
> to be
> 177 valid JSON.
> 178
> 179 :param json_string: the JSON string to clean up.
> 180 """
> 181 # The JSON response is not valid JSON, there is a magic sequence at
> the
> 182 # beginning of the result. We are paranoid, and remove also other
> white
> 183 # spaces.
> 184 if json_string.startswith(GERRIT_SANITIZABLE_VALUE):
> 185 return json_string.lstrip(GERRIT_SANITIZABLE_VALUE).strip()
>
> Are we sure we that the json_string should always start with
> GERRIT_SANITIZABLE_VALUE ??
> If not, then we need to return json_string even when json_string does not
> start with GERRIT_SANITIZABLE_VALUE.

As for Gerrit documentation, it should be. Link where it is written is here:
https://gerrit-review.googlesource.com/Documentation/rest-api.html

But better handle that case too.
Thanks.

[lp:~milo/linaro-patchmetrics/bug1013577]

366. By Milo Casagrande on 2012-06-25

Fixed review comments.

367. By Milo Casagrande on 2012-06-25

Return the json object, not the string.

[Deepti B. Kalakeri (deeptik)]

+1 Looks good.

review: Approve

[lp:~milo/linaro-patchmetrics/bug1013577]

368. By Milo Casagrande on 2012-06-25

Removed unused function.

=== modified file 'apps/patchmetrics/bin/sync-gerrit-changes.py'
--- apps/patchmetrics/bin/sync-gerrit-changes.py	2012-01-23 18:41:51 +0000
+++ apps/patchmetrics/bin/sync-gerrit-changes.py	2012-06-25 12:32:18 +0000
@@ -6,9 +6,14 @@
 
 from patchmetrics.models import TeamMembership
 from patchmetrics.gerrit import (
+    GerritError,
+    create_or_update_patches_from_aosp_changes,
     get_aosp_changes_from,
-    create_or_update_patches_from_aosp_changes,
-    GerritError)
+    )
+from patchmetrics.gerrit_json import (
+    GERRIT_MORE_CHANGES,
+    GERRIT_SORT_KEY,
+)
 
 
 linaro_engineers = Person.objects.filter(
@@ -17,8 +22,8 @@
     start_after = None
     while True:
         try:
-            result = get_aosp_changes_from(
-                person.email, start_after=start_after)
+            results = get_aosp_changes_from(person.email,
+                                            start_after=start_after)
         except GerritError, e:
             if '%s not found' % person.email in str(e):
                 # Gerrit returns an error when you search for changes owned by
@@ -26,15 +31,17 @@
                 # just skip to the next email.
                 break
             raise
-        at_end = result['atEnd']
-        changes = result['changes']
+        # If we get all the changes in a single batch, the GERRIT_MORE_CHANGES
+        # key will not be in the JSON output, so we set it to False.
+        more_changes = results[-1].get(GERRIT_MORE_CHANGES) or False
         created_or_updated_patches = (
-            create_or_update_patches_from_aosp_changes(changes, person))
+            create_or_update_patches_from_aosp_changes(results, person))
 
         # If there are less created/updated changes than the number of changes
         # returned by Gerrit we know we can stop as they're sorted most
         # recently updated first.
-        if len(changes) != len(created_or_updated_patches) or at_end:
+        if len(results) != len(created_or_updated_patches) or \
+        (not more_changes):
             break
 
-        start_after = changes[-1]['sortKey']
+        start_after = results[-1].get(GERRIT_SORT_KEY)

=== modified file 'apps/patchmetrics/gerrit.py'
--- apps/patchmetrics/gerrit.py	2012-01-31 18:32:44 +0000
+++ apps/patchmetrics/gerrit.py	2012-06-25 12:32:18 +0000
@@ -1,14 +1,37 @@
 from datetime import datetime
 import email
 import urllib2
+from urllib import urlencode
 
 import json
 
 from patchwork.models import Patch, Project, State
 from patchmetrics.models import GerritChange
+from patchmetrics.gerrit_json import (
+    GERRIT_CREATED_KEY,
+    GERRIT_ID_KEY,
+    GERRIT_NUMBER_KEY,
+    GERRIT_PROJECT_KEY,
+    GERRIT_SANITIZABLE_VALUE,
+    GERRIT_STATUS_KEY,
+    GERRIT_SUBJECT_KEY,
+    GERRIT_UPDATED_KEY,
+)
 
 
 ROOT_URL = 'https://android-review.googlesource.com/gerrit/rpc'
+# This is the URL for the REST interface.
+ROOT_REST_URL = 'https://android-review.googlesource.com'
+# The default path for the REST interface.
+CHANGES_QUERY_PATH = '/changes/'
+# This is the query prefix used for gerrit REST interface.
+GERRIT_QUERY_PREFIX = 'q='
+# Separator for gerrit query parameters.
+GERRIT_QUERY_SEPARATOR = '+'
+# Default separator for URL query string.
+QUERY_STRING_SEP = '&'
+# Default character for starting a query string.
+QUERY_STRING_START = '?'
 # A mapping from AOSP states to our internal ones.
 STATE_MAP = {'MERGED': 'Accepted', 'NEW': 'New', 'ABANDONED': 'Rejected'}
 
@@ -36,32 +59,141 @@
     return response.get('result')
 
 
-def get_aosp_change_details(change_id):
-    """Get the details of the Gerrit change with the given ID."""
-    return jsonrpc_request(
-        '/ChangeDetailService',
-        'changeDetail', [dict(id=change_id)])['change']
-
-
-def get_aosp_changes_from(email, start_after=None, page_size=100):
+def get_aosp_changes_from(email, start_after=None, page_size=25,
+                          return_format='JSON_COMPACT'):
     """Get the list of changes authored by the given email address.
 
-    :param start_after: Only include changes whose sortKey is lower than
-                        this. Use None if you want to start from the beginning.
-    :param page_size: Number of items to include in the list.
-    """
-    if start_after is None:
-        start_after = 'z'
-    query = "owner: %s" % email
-    return jsonrpc_request(
-        '/ChangeListService',
-        'allQueryNext', [query, start_after, page_size])
-
-
-def create_or_update_patches_from_aosp_changes(changes, author):
-    """Create or update Patch objects from the given list of changes.
-
-    For each of the changes in the list, make sure there's a matching Patch
+    :param email: The email of the user to get the changes of.
+    :param start_after: Only include changes whose 'sortkey_after' is lower
+                        than this. Use None if you want to start from the
+                        beginning.
+    :param page_size: Number of items to include in the list, defaults to 25.
+    :param return_format: The format of the response, defaults to
+                          'JSON_COMPACT' for JSON format without spaces, also
+                          possible to specify 'JSON'.
+    """
+    params = {'format': return_format, 'n': page_size}
+    query_params = {'owner': email, 'sortkey_after': start_after}
+    return rest_api_request(ROOT_REST_URL, CHANGES_QUERY_PATH, params,
+                            query_params)
+
+
+def rest_api_request(url, path, params, query_params):
+    """Calls the gerrit REST interface and return the JSON response.
+
+    :param url: the root URL of the web service.
+    :param path: the path of the REST interface, after the URL.
+    :param params: the parameters for the query, can be a dictionary or tuple.
+    :param query_params: the parameters for the gerrit query interface, must
+                         be a dictionary.
+    """
+    request_url = create_request_query_url(url, path, params, query_params)
+    return gerrit_request(request_url)
+
+
+def create_request_query_url(url, path, params, query_params):
+    """Creates the full query URL from the various elements.
+
+    :param url: the root URL of the web service, without path to the REST
+                interface.
+    :param path: the path of the REST interface, after the URL.
+    :param params: the parameters for the query, can be a dictionary or tuple.
+    :param query_params: the parameters for the gerrit query interface, must
+                         be a dictionary.
+    """
+    if not url.endswith('/') and not path.startswith('/'):
+        url += '/'
+    if not path.endswith('/'):
+        path += '/'
+    request_url = url + path + QUERY_STRING_START
+
+    request_url += urlencode(params)
+    request_url += create_gerrit_rest_query(query_params)
+
+    return request_url
+
+
+def create_gerrit_rest_query(query_params):
+    """Creates the gerrit query based on the passed parametes.
+
+    The query result should be in the form of:
+    &q=key:value+key:value[+key:value ...]
+
+    :param query_params: a dictionary containing the (key,value) pairs that
+                         will be used for the query.
+    """
+    if not isinstance(query_params, dict):
+        raise TypeError("'query_params' must be of type 'dict', got '%s'" %
+                        type(query_params).__name__)
+
+    params_list = []
+    # Where we will store the query.
+    gerrit_query = ''
+    for key, value in query_params.iteritems():
+        # We cannot have None values here, we just skip them.
+        if value is not None:
+            params_list.append(":".join((key, value)))
+    # The resulting query string, without separators or prefixes, should be in
+    # the form of:
+    # key:value+key:value
+    # This is a normal query, more complex queries need to be thought of, since
+    # we can express also OR, AND and NOT operators. By default it is an AND.
+    if params_list:
+        gerrit_query = QUERY_STRING_SEP + GERRIT_QUERY_PREFIX + \
+        GERRIT_QUERY_SEPARATOR.join(params_list)
+
+    return gerrit_query
+
+
+def gerrit_request(request_url):
+    """Performs the REST call, opening the connection to the web service.
+
+    :param request_url: the full URL of the request to be performed.
+    """
+    json_object = None
+    try:
+        request = urllib2.Request(request_url)
+        # Looks like, starting with gerrit version 2.5, we need to do this to
+        # talk with it. More info here:
+        # https://groups.google.com/group/repo-discuss/msg/78e70b07ad1aacce
+        # https://gerrit-review.googlesource.com/Documentation/rest-api.html
+        request.add_header("Accept", "application/json")
+        request.add_header("Accept-Encoding", "gzip")
+        # The request in this case needs to be a GET.
+        json_string = sanitize_json_response(urllib2.urlopen(request).read())
+        json_object = json.loads(json_string)
+    except urllib2.HTTPError, ex:
+        # If we get back an error code 400, it probably means that what we are
+        # looking for does not exist. We need to make sure of that and handle
+        # it. As it is now, we raise the same expected exception with the same
+        # message as before.
+        if ex.code == 400:
+            raise GerritError(ex.read())
+        else:
+            raise(ex)
+
+    return json_object
+
+
+def sanitize_json_response(json_string):
+    """Cleans up the JSON response got from the REST interface, in order to be
+    valid JSON.
+
+    :param json_string: the JSON string to clean up.
+    """
+    # The JSON response is not valid JSON, there is a magic sequence at the
+    # beginning of the result. We are paranoid, and remove also other white
+    # spaces.
+    valid_json = json_string
+    if json_string.startswith(GERRIT_SANITIZABLE_VALUE):
+        valid_json = json_string.lstrip(GERRIT_SANITIZABLE_VALUE)
+    return valid_json.strip()
+
+
+def create_or_update_patches_from_aosp_changes(results, author):
+    """Create or update Patch objects from the given list of results.
+
+    For each of the result in the list, make sure there's a matching Patch
     (and GerritChange) object in our DB matching it and update it to be in
     sync with the remote change.
 
@@ -69,23 +201,22 @@
     """
     print "Checking changes submitted by %s ..." % author.email
     updated_changes = []
-    for change in changes:
-        gerrit_id = change['id']['id']
-        last_updated_on = datetime.strptime(
-            change['lastUpdatedOn'], '%Y-%m-%d %H:%M:%S.%f000')
+    for result in results:
+        gerrit_id = result.get(GERRIT_NUMBER_KEY)
+        last_updated_on = datetime.strptime(result.get(GERRIT_UPDATED_KEY),
+                                            '%Y-%m-%d %H:%M:%S.%f000')
         try:
             existing = GerritChange.objects.get(gerrit_id=gerrit_id)
         except GerritChange.DoesNotExist:
             existing = None
-        # Must get the change's details because we need their creation date.
-        details = get_aosp_change_details(gerrit_id)
-        date_created = datetime.strptime(
-            details['createdOn'], '%Y-%m-%d %H:%M:%S.%f000')
+        date_created = datetime.strptime(result.get(GERRIT_CREATED_KEY),
+                                         '%Y-%m-%d %H:%M:%S.%f000')
         change_dict = dict(
-            id=gerrit_id, project=change['project']['key']['name'],
-            last_updated_on=last_updated_on, status=change['status'],
-            subject=change['subject'], date_created=date_created,
-            change_id=change['key']['id'])
+            id=gerrit_id, project=result.get(GERRIT_PROJECT_KEY),
+            last_updated_on=last_updated_on,
+            status=result.get(GERRIT_STATUS_KEY),
+            subject=result.get(GERRIT_SUBJECT_KEY), date_created=date_created,
+            change_id=result.get(GERRIT_ID_KEY))
         if existing is None:
             create_patch_from_aosp_change(change_dict, author)
             updated_changes.append(gerrit_id)

=== added file 'apps/patchmetrics/gerrit_json.py'
--- apps/patchmetrics/gerrit_json.py	1970-01-01 00:00:00 +0000
+++ apps/patchmetrics/gerrit_json.py	2012-06-25 12:32:18 +0000
@@ -0,0 +1,20 @@
+# File with all the gerrit JSON valid keys.
+# For more information about the values defined here, look:
+# https://gerrit-review.googlesource.com/Documentation/json.html
+# Also, on bug https://bugs.launchpad.net/linaro-patchmetrics/+bug/1013577,
+# there is a sample JSON output from Gerrit new REST API.
+GERRIT_CREATED_KEY = 'created'
+GERRIT_ID_KEY = 'id'
+GERRIT_MORE_CHANGES = '_more_changes'
+GERRIT_NUMBER_KEY = '_number'
+GERRIT_OWNER_KEY = 'owner'
+GERRIT_OWNER_NAME_KEY = 'name'
+GERRIT_SORT_KEY = '_sortkey'
+GERRIT_UPDATED_KEY = 'updated'
+GERRIT_STATUS_KEY = 'status'
+GERRIT_PROJECT_KEY = 'project'
+GERRIT_SUBJECT_KEY = 'subject'
+# Gerrit JSON output starts with a magic prefix, making it not valid JSON.
+# For more info:
+# https://gerrit-review.googlesource.com/Documentation/rest-api.html
+GERRIT_SANITIZABLE_VALUE = ")]}'"

=== modified file 'apps/patchmetrics/tests/test_gerrit.py'
--- apps/patchmetrics/tests/test_gerrit.py	2011-07-19 11:56:25 +0000
+++ apps/patchmetrics/tests/test_gerrit.py	2012-06-25 12:32:18 +0000
@@ -4,10 +4,16 @@
 from django.test import TestCase
 
 from patchmetrics.gerrit import (
+    CHANGES_QUERY_PATH,
+    QUERY_STRING_START,
+    ROOT_REST_URL,
+    create_gerrit_rest_query,
     create_patch_from_aosp_change,
+    create_request_query_url,
     get_project_for_aosp_name,
     get_project_name_for_aosp_name,
-    update_local_gerrit_change_from_aosp_change)
+    update_local_gerrit_change_from_aosp_change,
+    )
 from patchmetrics.tests.factory import factory
 
 
@@ -50,7 +56,6 @@
         # date_last_state_change set to last_updated_on.
         change_dict = copy(self.change_dict)
         change_dict['status'] = 'MERGED'
-        author = factory.makePerson()
         patch = create_patch_from_aosp_change(
             change_dict, factory.makePerson())
         self.assertEqual('Accepted', patch.state.name)
@@ -67,3 +72,41 @@
         self.assertEqual('MERGED', gerrit_change.status)
         self.assertEqual('Accepted', gerrit_change.patch.state.name)
         self.assertEqual(change_dict['subject'], gerrit_change.patch.name)
+
+    def test_create_gerrit_rest_query(self):
+        """Tests the creation of a valid gerrit query."""
+        query_params = {'key1': 'value1', 'key2': 'value2'}
+        self.assertEqual('&q=key2:value2+key1:value1',
+                         create_gerrit_rest_query(query_params))
+
+    def test_create_gerrit_rest_query_empty(self):
+        """Tests gerrit query creation with empty dictionary."""
+        self.assertEqual('', create_gerrit_rest_query({}))
+
+    def test_create_gerrit_rest_query_with_none(self):
+        """Tests that if a value is None it is not added."""
+        query_params = {'key1': 'value1', 'key2': None}
+        self.assertEqual('&q=key1:value1',
+                         create_gerrit_rest_query(query_params))
+
+    def test_create_gerrit_request_query_with_list(self):
+        """Tests that the function accepts only a dictionary, passing a list.
+        """
+        self.assertRaises(TypeError, create_gerrit_rest_query, [])
+
+    def test_create_gerrit_request_query_with_tuple(self):
+        """Tests that the function accepts only a dictionary, passing a tuple.
+        """
+        self.assertRaises(TypeError, create_gerrit_rest_query, ())
+
+    def test_create_request_query_url(self):
+        """Tests the creation of a valid request URL."""
+        params = {'format': 'JSON', 'n': 100}
+        query_params = {'owner': '[e-mail]', 'sortkey_after': 'abcd'}
+        expected_result = ROOT_REST_URL + CHANGES_QUERY_PATH + \
+        QUERY_STRING_START + \
+        "n=100&format=JSON&q=owner:[e-mail]+sortkey_after:abcd"
+        self.assertEquals(expected_result,
+                          create_request_query_url(ROOT_REST_URL,
+                                                   CHANGES_QUERY_PATH,
+                                                   params, query_params))